From 3bd2efa177772fa67843555fefab3562e9fa9370 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 4 Apr 2017 03:01:00 -0400 Subject: [PATCH 1/8] Await termination after shutting down executors When terminating an executor service or a thread pool, we first shutdown. Then, we do a timed await termination. If the await termination fails because there are still tasks running, we then shutdown now. However, this method does not wait for actively executing tasks to terminate, so we should again wait for termination of these tasks before returning. This commit does that. Relates #23889 --- .../elasticsearch/threadpool/ThreadPool.java | 46 +++++++++++++------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index b68037b8dc6f2..f72956c4202e3 100644 --- a/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -679,14 +679,23 @@ static final class Fields { public static boolean terminate(ExecutorService service, long timeout, TimeUnit timeUnit) { if (service != null) { service.shutdown(); - try { - if (service.awaitTermination(timeout, timeUnit)) { - return true; - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } + if (awaitTermination(service, timeout, timeUnit)) return true; service.shutdownNow(); + return awaitTermination(service, timeout, timeUnit); + } + return false; + } + + private static boolean awaitTermination( + final ExecutorService service, + final long timeout, + final TimeUnit timeUnit) { + try { + if (service.awaitTermination(timeout, timeUnit)) { + return true; + } + } catch (final InterruptedException e) { + Thread.currentThread().interrupt(); } return false; } @@ -699,15 +708,10 @@ public static boolean terminate(ThreadPool pool, long timeout, TimeUnit timeUnit if (pool != null) { try { pool.shutdown(); - try { - if (pool.awaitTermination(timeout, timeUnit)) { - return true; - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } + if (awaitTermination(pool, timeout, timeUnit)) return true; // last resort pool.shutdownNow(); + return awaitTermination(pool, timeout, timeUnit); } finally { IOUtils.closeWhileHandlingException(pool); } @@ -715,6 +719,20 @@ public static boolean terminate(ThreadPool pool, long timeout, TimeUnit timeUnit return false; } + private static boolean awaitTermination( + final ThreadPool pool, + final long timeout, + final TimeUnit timeUnit) { + try { + if (pool.awaitTermination(timeout, timeUnit)) { + return true; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return false; + } + @Override public void close() throws IOException { threadContext.close(); From 71293a89bf8ba741ada9b3df5e0571080f5db992 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 4 Apr 2017 03:02:58 -0400 Subject: [PATCH 2/8] Introduce single-node discovery This commit adds a single node discovery type. With this discovery type, a node will elect itself as master and never form a cluster with another node. Relates #23595 --- .../discovery/DiscoveryModule.java | 26 +-- .../discovery/single/SingleNodeDiscovery.java | 144 ++++++++++++++ .../single/SingleNodeDiscoveryIT.java | 178 ++++++++++++++++++ .../single/SingleNodeDiscoveryTests.java | 101 ++++++++++ qa/smoke-test-client/build.gradle | 15 ++ .../test/InternalTestCluster.java | 8 +- 6 files changed, 457 insertions(+), 15 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java create mode 100644 core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryIT.java create mode 100644 core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryTests.java diff --git a/core/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java b/core/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java index ea3ae0c919b3c..2328b5a861675 100644 --- a/core/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java +++ b/core/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java @@ -19,30 +19,29 @@ package org.elasticsearch.discovery; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.function.Function; -import java.util.function.Supplier; - import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.inject.AbstractModule; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.network.NetworkService; -import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.discovery.single.SingleNodeDiscovery; import org.elasticsearch.discovery.zen.UnicastHostsProvider; import org.elasticsearch.discovery.zen.ZenDiscovery; -import org.elasticsearch.discovery.zen.ZenPing; import org.elasticsearch.plugins.DiscoveryPlugin; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + /** * A module for loading classes for node discovery. */ @@ -83,6 +82,7 @@ public DiscoveryModule(Settings settings, ThreadPool threadPool, TransportServic discoveryTypes.put("zen", () -> new ZenDiscovery(settings, threadPool, transportService, namedWriteableRegistry, clusterService, hostsProvider)); discoveryTypes.put("none", () -> new NoneDiscovery(settings, clusterService, clusterService.getClusterSettings())); + discoveryTypes.put("single-node", () -> new SingleNodeDiscovery(settings, clusterService)); for (DiscoveryPlugin plugin : plugins) { plugin.getDiscoveryTypes(threadPool, transportService, namedWriteableRegistry, clusterService, hostsProvider).entrySet().forEach(entry -> { @@ -96,10 +96,12 @@ public DiscoveryModule(Settings settings, ThreadPool threadPool, TransportServic if (discoverySupplier == null) { throw new IllegalArgumentException("Unknown discovery type [" + discoveryType + "]"); } + Loggers.getLogger(getClass(), settings).info("using discovery type [{}]", discoveryType); discovery = Objects.requireNonNull(discoverySupplier.get()); } public Discovery getDiscovery() { return discovery; } + } diff --git a/core/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java b/core/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java new file mode 100644 index 0000000000000..f4735c8bf3a0d --- /dev/null +++ b/core/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java @@ -0,0 +1,144 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.discovery.single; + +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateTaskConfig; +import org.elasticsearch.cluster.ClusterStateTaskExecutor; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.routing.allocation.AllocationService; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Priority; +import org.elasticsearch.common.component.AbstractLifecycleComponent; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.discovery.Discovery; +import org.elasticsearch.discovery.DiscoverySettings; +import org.elasticsearch.discovery.DiscoveryStats; +import org.elasticsearch.discovery.zen.PendingClusterStateStats; +import org.elasticsearch.discovery.zen.PendingClusterStatesQueue; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A discovery implementation where the only member of the cluster is the local node. + */ +public class SingleNodeDiscovery extends AbstractLifecycleComponent implements Discovery { + + private final ClusterService clusterService; + private final DiscoverySettings discoverySettings; + + public SingleNodeDiscovery(final Settings settings, final ClusterService clusterService) { + super(Objects.requireNonNull(settings)); + this.clusterService = Objects.requireNonNull(clusterService); + final ClusterSettings clusterSettings = + Objects.requireNonNull(clusterService.getClusterSettings()); + this.discoverySettings = new DiscoverySettings(settings, clusterSettings); + } + + @Override + public DiscoveryNode localNode() { + return clusterService.localNode(); + } + + @Override + public String nodeDescription() { + return clusterService.getClusterName().value() + "/" + clusterService.localNode().getId(); + } + + @Override + public void setAllocationService(final AllocationService allocationService) { + + } + + @Override + public void publish(final ClusterChangedEvent event, final AckListener listener) { + + } + + @Override + public DiscoveryStats stats() { + return new DiscoveryStats((PendingClusterStateStats) null); + } + + @Override + public DiscoverySettings getDiscoverySettings() { + return discoverySettings; + } + + @Override + public void startInitialJoin() { + final ClusterStateTaskExecutor executor = + new ClusterStateTaskExecutor() { + + @Override + public ClusterTasksResult execute( + final ClusterState current, + final List tasks) throws Exception { + assert tasks.size() == 1; + final DiscoveryNodes.Builder nodes = + DiscoveryNodes.builder(current.nodes()); + // always set the local node as master, there will not be other nodes + nodes.masterNodeId(localNode().getId()); + final ClusterState next = + ClusterState.builder(current).nodes(nodes).build(); + final ClusterTasksResult.Builder result = + ClusterTasksResult.builder(); + return result.successes(tasks).build(next); + } + + @Override + public boolean runOnlyOnMaster() { + return false; + } + + }; + final ClusterStateTaskConfig config = ClusterStateTaskConfig.build(Priority.URGENT); + clusterService.submitStateUpdateTasks( + "single-node-start-initial-join", + Collections.singletonMap(localNode(), (s, e) -> {}), config, executor); + } + + @Override + public int getMinimumMasterNodes() { + return 1; + } + + @Override + protected void doStart() { + + } + + @Override + protected void doStop() { + + } + + @Override + protected void doClose() throws IOException { + + } + +} diff --git a/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryIT.java b/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryIT.java new file mode 100644 index 0000000000000..25641e16fca9c --- /dev/null +++ b/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryIT.java @@ -0,0 +1,178 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.discovery.single; + +import org.apache.lucene.util.IOUtils; +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.discovery.zen.PingContextProvider; +import org.elasticsearch.discovery.zen.UnicastHostsProvider; +import org.elasticsearch.discovery.zen.UnicastZenPing; +import org.elasticsearch.discovery.zen.ZenPing; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.InternalTestCluster; +import org.elasticsearch.test.NodeConfigurationSource; +import org.elasticsearch.test.transport.MockTransportService; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.transport.MockTcpTransportPlugin; +import org.elasticsearch.transport.TransportService; + +import java.io.Closeable; +import java.io.IOException; +import java.util.Collections; +import java.util.Stack; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.function.Function; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; + +@ESIntegTestCase.ClusterScope( + scope = ESIntegTestCase.Scope.TEST, + numDataNodes = 1, + numClientNodes = 0, + supportsDedicatedMasters = false, + autoMinMasterNodes = false) +public class SingleNodeDiscoveryIT extends ESIntegTestCase { + + @Override + protected Settings nodeSettings(int nodeOrdinal) { + return Settings + .builder() + .put(super.nodeSettings(nodeOrdinal)) + .put("discovery.type", "single-node") + // TODO: do not use such a restrictive ephemeral port range + .put("transport.tcp.port", "49152-49156") + .build(); + } + + public void testDoesNotRespondToZenPings() throws Exception { + final Settings settings = + Settings.builder().put("cluster.name", internalCluster().getClusterName()).build(); + final Version version = Version.CURRENT; + final Stack closeables = new Stack<>(); + final TestThreadPool threadPool = new TestThreadPool(getClass().getName()); + try { + final MockTransportService pingTransport = + MockTransportService.createNewService(settings, version, threadPool, null); + pingTransport.start(); + closeables.push(pingTransport); + final TransportService nodeTransport = + internalCluster().getInstance(TransportService.class); + // try to ping the single node directly + final UnicastHostsProvider provider = + () -> Collections.singletonList(nodeTransport.getLocalNode()); + final CountDownLatch latch = new CountDownLatch(1); + final UnicastZenPing unicastZenPing = + new UnicastZenPing(settings, threadPool, pingTransport, provider) { + @Override + protected void finishPingingRound(PingingRound pingingRound) { + latch.countDown(); + super.finishPingingRound(pingingRound); + } + }; + final DiscoveryNodes nodes = + DiscoveryNodes.builder().add(pingTransport.getLocalNode()).build(); + final ClusterName clusterName = new ClusterName(internalCluster().getClusterName()); + final ClusterState state = ClusterState.builder(clusterName).nodes(nodes).build(); + unicastZenPing.start(new PingContextProvider() { + @Override + public ClusterState clusterState() { + return state; + } + + @Override + public DiscoveryNodes nodes() { + return DiscoveryNodes + .builder() + .add(nodeTransport.getLocalNode()) + .add(pingTransport.getLocalNode()) + .localNodeId(pingTransport.getLocalNode().getId()) + .build(); + } + }); + closeables.push(unicastZenPing); + final CompletableFuture responses = new CompletableFuture<>(); + unicastZenPing.ping(responses::complete, TimeValue.timeValueSeconds(3)); + latch.await(); + responses.get(); + assertThat(responses.get().size(), equalTo(0)); + } finally { + while (!closeables.isEmpty()) { + IOUtils.closeWhileHandlingException(closeables.pop()); + } + terminate(threadPool); + } + } + + public void testSingleNodesDoNotDiscoverEachOther() throws IOException, InterruptedException { + final NodeConfigurationSource configurationSource = new NodeConfigurationSource() { + @Override + public Settings nodeSettings(int nodeOrdinal) { + return Settings + .builder() + .put("discovery.type", "single-node") + .put("http.enabled", false) + .put("transport.type", "mock-socket-network") + /* + * We align the port ranges of the two as then with zen discovery these two + * nodes would find each other. + */ + // TODO: do not use such a restrictive ephemeral port range + .put("transport.tcp.port", "49152-49156") + .build(); + } + }; + try (InternalTestCluster other = + new InternalTestCluster( + randomLong(), + createTempDir(), + false, + false, + 1, + 1, + internalCluster().getClusterName(), + configurationSource, + 0, + false, + "other", + Collections.singletonList(MockTcpTransportPlugin.class), + Function.identity())) { + other.beforeTest(random(), 0); + final ClusterState first = internalCluster().getInstance(ClusterService.class).state(); + final ClusterState second = other.getInstance(ClusterService.class).state(); + assertThat(first.nodes().getSize(), equalTo(1)); + assertThat(second.nodes().getSize(), equalTo(1)); + assertThat( + first.nodes().getMasterNodeId(), + not(equalTo(second.nodes().getMasterNodeId()))); + assertThat( + first.metaData().clusterUUID(), + not(equalTo(second.metaData().clusterUUID()))); + } + } + +} diff --git a/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryTests.java b/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryTests.java new file mode 100644 index 0000000000000..a0e0b699d78db --- /dev/null +++ b/core/src/test/java/org/elasticsearch/discovery/single/SingleNodeDiscoveryTests.java @@ -0,0 +1,101 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.discovery.single; + +import org.apache.lucene.util.IOUtils; +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateObserver; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.transport.MockTransportService; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.threadpool.ThreadPool; + +import java.io.Closeable; +import java.util.Stack; +import java.util.concurrent.CountDownLatch; + +import static org.elasticsearch.test.ClusterServiceUtils.createClusterService; +import static org.hamcrest.Matchers.equalTo; + +public class SingleNodeDiscoveryTests extends ESTestCase { + + public void testInitialJoin() throws Exception { + final Settings settings = Settings.EMPTY; + final Version version = Version.CURRENT; + final ThreadPool threadPool = new TestThreadPool(getClass().getName()); + final Stack stack = new Stack<>(); + try { + final MockTransportService transportService = + MockTransportService.createNewService(settings, version, threadPool, null); + stack.push(transportService); + transportService.start(); + final DiscoveryNode node = transportService.getLocalNode(); + final ClusterService clusterService = createClusterService(threadPool, node); + stack.push(clusterService); + final SingleNodeDiscovery discovery = + new SingleNodeDiscovery(Settings.EMPTY, clusterService); + discovery.startInitialJoin(); + + // we are racing against the initial join which is asynchronous so we use an observer + final ClusterState state = clusterService.state(); + final ThreadContext threadContext = threadPool.getThreadContext(); + final ClusterStateObserver observer = + new ClusterStateObserver(state, clusterService, null, logger, threadContext); + if (state.nodes().getMasterNodeId() == null) { + final CountDownLatch latch = new CountDownLatch(1); + observer.waitForNextChange(new ClusterStateObserver.Listener() { + @Override + public void onNewClusterState(ClusterState state) { + latch.countDown(); + } + + @Override + public void onClusterServiceClose() { + latch.countDown(); + } + + @Override + public void onTimeout(TimeValue timeout) { + assert false; + } + }, s -> s.nodes().getMasterNodeId() != null); + + latch.await(); + } + + final DiscoveryNodes nodes = clusterService.state().nodes(); + assertThat(nodes.getSize(), equalTo(1)); + assertThat(nodes.getMasterNode().getId(), equalTo(node.getId())); + } finally { + while (!stack.isEmpty()) { + IOUtils.closeWhileHandlingException(stack.pop()); + } + terminate(threadPool); + } + } + +} diff --git a/qa/smoke-test-client/build.gradle b/qa/smoke-test-client/build.gradle index 888d932524220..e4d197e7e6a6d 100644 --- a/qa/smoke-test-client/build.gradle +++ b/qa/smoke-test-client/build.gradle @@ -1,3 +1,5 @@ +import org.elasticsearch.gradle.test.RestIntegTestTask + /* * Licensed to Elasticsearch under one or more contributor * license agreements. See the NOTICE file distributed with @@ -25,3 +27,16 @@ apply plugin: 'elasticsearch.rest-test' dependencies { testCompile project(path: ':client:transport', configuration: 'runtime') // randomly swapped in as a transport } + +task singleNodeIntegTest(type: RestIntegTestTask) { + mustRunAfter(precommit) +} + +singleNodeIntegTestCluster { + numNodes = 1 + setting 'discovery.type', 'single-node' +} + +integTest.dependsOn(singleNodeIntegTestRunner, 'singleNodeIntegTestCluster#stop') + +check.dependsOn(integTest) diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index d20a4e95e385f..cf9c2dc351510 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -64,6 +64,7 @@ import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.discovery.DiscoveryModule; import org.elasticsearch.discovery.zen.ElectMasterService; import org.elasticsearch.discovery.zen.ZenDiscovery; import org.elasticsearch.env.Environment; @@ -590,7 +591,8 @@ private NodeAndClient buildNode(int nodeId, long seed, Settings settings, .put("node.name", name) .put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), seed); - if (autoManageMinMasterNodes) { + final boolean usingSingleNodeDiscovery = DiscoveryModule.DISCOVERY_TYPE_SETTING.get(finalSettings.build()).equals("single-node"); + if (!usingSingleNodeDiscovery && autoManageMinMasterNodes) { assert finalSettings.get(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey()) == null : "min master nodes may not be set when auto managed"; assert finalSettings.get(INITIAL_STATE_TIMEOUT_SETTING.getKey()) == null : @@ -600,7 +602,7 @@ private NodeAndClient buildNode(int nodeId, long seed, Settings settings, // don't wait too long not to slow down tests .put(ZenDiscovery.MASTER_ELECTION_WAIT_FOR_JOINS_TIMEOUT_SETTING.getKey(), "5s") .put(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), defaultMinMasterNodes); - } else if (finalSettings.get(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey()) == null) { + } else if (!usingSingleNodeDiscovery && finalSettings.get(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey()) == null) { throw new IllegalArgumentException(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey() + " must be configured"); } MockNode node = new MockNode(finalSettings.build(), plugins); @@ -1083,7 +1085,7 @@ private void validateClusterFormed(String viaNode) { } return true; }, 30, TimeUnit.SECONDS) == false) { - throw new IllegalStateException("cluster failed to from with expected nodes " + expectedNodes + " and actual nodes " + + throw new IllegalStateException("cluster failed to form with expected nodes " + expectedNodes + " and actual nodes " + client.admin().cluster().prepareState().get().getState().nodes()); } } catch (InterruptedException e) { From 20b274d7b9d89be7e9e32aba0ebb06f7856757d8 Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Tue, 4 Apr 2017 10:36:09 +0200 Subject: [PATCH 3/8] testDifferentRolesMaintainPathOnRestart - lower join timeout as split elections are likely the test reduce the wait for initial cluster state to 0, causing multiple nodes to be start while elections are going on. This means there is a chance of a split election which shouldn't cause the test to time out. --- .../elasticsearch/test/test/InternalTestClusterTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java b/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java index 1e84610985d01..d7d5e09c1163a 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.network.NetworkModule; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.discovery.DiscoverySettings; +import org.elasticsearch.discovery.zen.ZenDiscovery; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESTestCase; @@ -356,7 +357,10 @@ public Settings nodeSettings(int nodeOrdinal) { .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), numNodes) .put(NetworkModule.HTTP_ENABLED.getKey(), false) .put(NetworkModule.TRANSPORT_TYPE_KEY, MockTcpTransportPlugin.MOCK_TCP_TRANSPORT_NAME) - .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0).build(); + .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0) + // timeout as no initial state timeout makes split elections more likely + .put(ZenDiscovery.JOIN_TIMEOUT_SETTING.getKey(), "3s") // speed up + .build(); } @Override From 2266947ac569f5fbc2f632508b24f298496de970 Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Tue, 4 Apr 2017 11:03:23 +0200 Subject: [PATCH 4/8] testDifferentRolesMaintainPathOnRestart - fix broken comment --- .../elasticsearch/test/test/InternalTestClusterTests.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java b/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java index d7d5e09c1163a..0284a59488385 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java @@ -358,8 +358,9 @@ public Settings nodeSettings(int nodeOrdinal) { .put(NetworkModule.HTTP_ENABLED.getKey(), false) .put(NetworkModule.TRANSPORT_TYPE_KEY, MockTcpTransportPlugin.MOCK_TCP_TRANSPORT_NAME) .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0) - // timeout as no initial state timeout makes split elections more likely - .put(ZenDiscovery.JOIN_TIMEOUT_SETTING.getKey(), "3s") // speed up + // speedup join timeout as setting initial state timeout to 0 makes split + // elections more likely + .put(ZenDiscovery.JOIN_TIMEOUT_SETTING.getKey(), "3s") .build(); } From a04350f0ddbabb97ea3d22e750357870924d9378 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 4 Apr 2017 12:35:48 +0200 Subject: [PATCH 5/8] Add a property to mark setting as final (#23872) This change adds a setting property that sets the value of a setting as final. Updating a final setting is prohibited in any context, for instance an index setting marked as final must be set at index creation and will refuse any update even if the index is closed. This change also marks the setting `index.number_of_shards` as Final and the special casing for refusing the updates on this setting has been removed. --- .../cluster/metadata/IndexMetaData.java | 2 +- .../MetaDataUpdateSettingsService.java | 4 -- .../settings/AbstractScopedSettings.java | 31 +++++--- .../common/settings/Setting.java | 16 +++++ .../cluster/ClusterModuleTests.java | 4 +- .../common/settings/ScopedSettingsTests.java | 72 +++++++++++++++++-- .../common/settings/SettingTests.java | 15 ++-- .../indices/settings/UpdateSettingsIT.java | 58 ++++++++++++--- 8 files changed, 165 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java index 1f7f3374ed41c..713fce2848f4a 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -183,7 +183,7 @@ static Setting buildNumberOfShardsSetting() { throw new IllegalArgumentException("es.index.max_number_of_shards must be > 0"); } return Setting.intSetting(SETTING_NUMBER_OF_SHARDS, Math.min(5, maxNumShards), 1, maxNumShards, - Property.IndexScope); + Property.IndexScope, Property.Final); } public static final String INDEX_SETTING_PREFIX = "index."; diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java index 82d2f5fc04b12..ac172ab661a18 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java @@ -165,10 +165,6 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request indexScopedSettings.validate(normalizedSettings); // never allow to change the number of shards for (Map.Entry entry : normalizedSettings.getAsMap().entrySet()) { - if (entry.getKey().equals(IndexMetaData.SETTING_NUMBER_OF_SHARDS)) { - listener.onFailure(new IllegalArgumentException("can't change the number of shards for an index")); - return; - } Setting setting = indexScopedSettings.get(entry.getKey()); assert setting != null; // we already validated the normalized settings settingsForClosedIndices.put(entry.getKey(), entry.getValue()); diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 65493b93cdace..05b7d96c8f6db 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -35,8 +35,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.SortedMap; -import java.util.TreeMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -382,11 +380,19 @@ private boolean assertMatcher(String key, int numComplexMatchers) { /** * Returns true if the setting for the given key is dynamically updateable. Otherwise false. */ - public boolean hasDynamicSetting(String key) { + public boolean isDynamicSetting(String key) { final Setting setting = get(key); return setting != null && setting.isDynamic(); } + /** + * Returns true if the setting for the given key is final. Otherwise false. + */ + public boolean isFinalSetting(String key) { + final Setting setting = get(key); + return setting != null && setting.isFinal(); + } + /** * Returns a settings object that contains all settings that are not * already set in the given source. The diff contains either the default value for each @@ -465,11 +471,14 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin boolean changed = false; final Set toRemove = new HashSet<>(); Settings.Builder settingsBuilder = Settings.builder(); - final Predicate canUpdate = (key) -> (onlyDynamic == false && get(key) != null) || hasDynamicSetting(key); - final Predicate canRemove = (key) ->( // we can delete if - onlyDynamic && hasDynamicSetting(key) // it's a dynamicSetting and we only do dynamic settings - || get(key) == null && key.startsWith(ARCHIVED_SETTINGS_PREFIX) // the setting is not registered AND it's been archived - || (onlyDynamic == false && get(key) != null)); // if it's not dynamic AND we have a key + final Predicate canUpdate = (key) -> ( + isFinalSetting(key) == false && // it's not a final setting + ((onlyDynamic == false && get(key) != null) || isDynamicSetting(key))); + final Predicate canRemove = (key) ->(// we can delete if + isFinalSetting(key) == false && // it's not a final setting + (onlyDynamic && isDynamicSetting(key) // it's a dynamicSetting and we only do dynamic settings + || get(key) == null && key.startsWith(ARCHIVED_SETTINGS_PREFIX) // the setting is not registered AND it's been archived + || (onlyDynamic == false && get(key) != null))); // if it's not dynamic AND we have a key for (Map.Entry entry : toApply.getAsMap().entrySet()) { if (entry.getValue() == null && (canRemove.test(entry.getKey()) || entry.getKey().endsWith("*"))) { // this either accepts null values that suffice the canUpdate test OR wildcard expressions (key ends with *) @@ -482,7 +491,11 @@ onlyDynamic && hasDynamicSetting(key) // it's a dynamicSetting and we only do d updates.put(entry.getKey(), entry.getValue()); changed = true; } else { - throw new IllegalArgumentException(type + " setting [" + entry.getKey() + "], not dynamically updateable"); + if (isFinalSetting(entry.getKey())) { + throw new IllegalArgumentException("final " + type + " setting [" + entry.getKey() + "], not updateable"); + } else { + throw new IllegalArgumentException(type + " setting [" + entry.getKey() + "], not dynamically updateable"); + } } } changed |= applyDeletes(toRemove, target, canRemove); diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index 864ceb487bf8f..633c861d1e2b4 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -95,6 +95,12 @@ public enum Property { */ Dynamic, + /** + * mark this setting as final, not updateable even when the context is not dynamic + * ie. Setting this property on an index scoped setting will fail update when the index is closed + */ + Final, + /** * mark this setting as deprecated */ @@ -135,6 +141,9 @@ private Setting(Key key, @Nullable Setting fallbackSetting, Functiontrue if this setting is final, otherwise false + */ + public final boolean isFinal() { + return properties.contains(Property.Final); + } + /** * Returns the setting properties * @see Property diff --git a/core/src/test/java/org/elasticsearch/cluster/ClusterModuleTests.java b/core/src/test/java/org/elasticsearch/cluster/ClusterModuleTests.java index c4241a2a989e1..51c66033ca9cf 100644 --- a/core/src/test/java/org/elasticsearch/cluster/ClusterModuleTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/ClusterModuleTests.java @@ -92,7 +92,7 @@ public void testRegisterClusterDynamicSettingDuplicate() { public void testRegisterClusterDynamicSetting() { SettingsModule module = new SettingsModule(Settings.EMPTY, Setting.boolSetting("foo.bar", false, Property.Dynamic, Property.NodeScope)); - assertInstanceBinding(module, ClusterSettings.class, service -> service.hasDynamicSetting("foo.bar")); + assertInstanceBinding(module, ClusterSettings.class, service -> service.isDynamicSetting("foo.bar")); } public void testRegisterIndexDynamicSettingDuplicate() { @@ -107,7 +107,7 @@ public void testRegisterIndexDynamicSettingDuplicate() { public void testRegisterIndexDynamicSetting() { SettingsModule module = new SettingsModule(Settings.EMPTY, Setting.boolSetting("index.foo.bar", false, Property.Dynamic, Property.IndexScope)); - assertInstanceBinding(module, IndexScopedSettings.class, service -> service.hasDynamicSetting("index.foo.bar")); + assertInstanceBinding(module, IndexScopedSettings.class, service -> service.isDynamicSetting("index.foo.bar")); } public void testRegisterAllocationDeciderDuplicate() { diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 7ec8f41034fe0..76905d43799f5 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -35,7 +35,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.IllegalFormatCodePointException; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -45,6 +44,7 @@ import java.util.function.BiConsumer; import java.util.function.Function; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.startsWith; @@ -252,13 +252,32 @@ public void testIsDynamic(){ new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope), Setting.intSetting("foo.bar.baz", 1, Property.NodeScope)))); - assertFalse(settings.hasDynamicSetting("foo.bar.baz")); - assertTrue(settings.hasDynamicSetting("foo.bar")); + assertFalse(settings.isDynamicSetting("foo.bar.baz")); + assertTrue(settings.isDynamicSetting("foo.bar")); assertNotNull(settings.get("foo.bar.baz")); settings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - assertTrue(settings.hasDynamicSetting("transport.tracer.include." + randomIntBetween(1, 100))); - assertFalse(settings.hasDynamicSetting("transport.tracer.include.BOOM")); - assertTrue(settings.hasDynamicSetting("cluster.routing.allocation.require.value")); + assertTrue(settings.isDynamicSetting("transport.tracer.include." + randomIntBetween(1, 100))); + assertFalse(settings.isDynamicSetting("transport.tracer.include.BOOM")); + assertTrue(settings.isDynamicSetting("cluster.routing.allocation.require.value")); + } + + public void testIsFinal() { + ClusterSettings settings = + new ClusterSettings(Settings.EMPTY, + new HashSet<>(Arrays.asList(Setting.intSetting("foo.int", 1, Property.Final, Property.NodeScope), + Setting.groupSetting("foo.group.", Property.Final, Property.NodeScope), + Setting.groupSetting("foo.list.", Property.Final, Property.NodeScope), + Setting.intSetting("foo.int.baz", 1, Property.NodeScope)))); + + assertFalse(settings.isFinalSetting("foo.int.baz")); + assertTrue(settings.isFinalSetting("foo.int")); + + assertFalse(settings.isFinalSetting("foo.list")); + assertTrue(settings.isFinalSetting("foo.list.0.key")); + assertTrue(settings.isFinalSetting("foo.list.key")); + + assertFalse(settings.isFinalSetting("foo.group")); + assertTrue(settings.isFinalSetting("foo.group.key")); } public void testDiff() throws IOException { @@ -581,4 +600,45 @@ public void testOverlappingComplexMatchSettings() { } } } + + public void testUpdateNumberOfShardsFail() { + IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, + IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> settings.updateSettings(Settings.builder().put("index.number_of_shards", 8).build(), + Settings.builder(), Settings.builder(), "index")); + assertThat(ex.getMessage(), + containsString("final index setting [index.number_of_shards], not updateable")); + } + + public void testFinalSettingUpdateFail() { + Setting finalSetting = Setting.intSetting("some.final.setting", 1, Property.Final, Property.NodeScope); + Setting finalGroupSetting = Setting.groupSetting("some.final.group.", Property.Final, Property.NodeScope); + Settings currentSettings = Settings.builder() + .put("some.final.setting", 9) + .put("some.final.group.foo", 7) + .build(); + ClusterSettings service = new ClusterSettings(currentSettings + , new HashSet<>(Arrays.asList(finalSetting, finalGroupSetting))); + + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> + service.updateDynamicSettings(Settings.builder().put("some.final.setting", 8).build(), + Settings.builder().put(currentSettings), Settings.builder(), "node")); + assertThat(exc.getMessage(), containsString("final node setting [some.final.setting]")); + + exc = expectThrows(IllegalArgumentException.class, () -> + service.updateDynamicSettings(Settings.builder().putNull("some.final.setting").build(), + Settings.builder().put(currentSettings), Settings.builder(), "node")); + assertThat(exc.getMessage(), containsString("final node setting [some.final.setting]")); + + exc = expectThrows(IllegalArgumentException.class, () -> + service.updateSettings(Settings.builder().put("some.final.group.new", 8).build(), + Settings.builder().put(currentSettings), Settings.builder(), "node")); + assertThat(exc.getMessage(), containsString("final node setting [some.final.group.new]")); + + exc = expectThrows(IllegalArgumentException.class, () -> + service.updateSettings(Settings.builder().put("some.final.group.foo", 5).build(), + Settings.builder().put(currentSettings), Settings.builder(), "node")); + assertThat(exc.getMessage(), containsString("final node setting [some.final.group.foo]")); + } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 24b48cbf3683c..0bb1abb37ada2 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -552,12 +552,15 @@ public void testMutuallyExclusiveScopes() { * We can't have Null properties */ public void testRejectNullProperties() { - try { - Setting.simpleString("foo.bar", (Property[]) null); - fail(); - } catch (IllegalArgumentException ex) { - assertThat(ex.getMessage(), containsString("properties cannot be null for setting")); - } + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> Setting.simpleString("foo.bar", (Property[]) null)); + assertThat(ex.getMessage(), containsString("properties cannot be null for setting")); + } + + public void testRejectConflictProperties() { + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> Setting.simpleString("foo.bar", Property.Final, Property.Dynamic)); + assertThat(ex.getMessage(), containsString("final setting [foo.bar] cannot be dynamic")); } public void testTimeValue() { diff --git a/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 762d409b6b75e..188498b56d026 100644 --- a/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -44,6 +44,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertBlocked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; @@ -66,7 +67,7 @@ public void testInvalidDynamicUpdate() { @Override protected Collection> nodePlugins() { - return Arrays.asList(DummySettingPlugin.class); + return Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); } public static class DummySettingPlugin extends Plugin { @@ -86,6 +87,19 @@ public List> getSettings() { } } + public static class FinalSettingPlugin extends Plugin { + public static final Setting FINAL_SETTING = Setting.simpleString("index.final", + Setting.Property.IndexScope, Setting.Property.Final); + @Override + public void onIndexModule(IndexModule indexModule) { + } + + @Override + public List> getSettings() { + return Collections.singletonList(FINAL_SETTING); + } + } + public void testResetDefault() { createIndex("test"); @@ -130,7 +144,7 @@ public void testResetDefault() { } public void testOpenCloseUpdateSettings() throws Exception { createIndex("test"); - try { + expectThrows(IllegalArgumentException.class, () -> client() .admin() .indices() @@ -139,20 +153,29 @@ public void testOpenCloseUpdateSettings() throws Exception { .put("index.refresh_interval", -1) // this one can change .put("index.fielddata.cache", "none")) // this one can't .execute() - .actionGet(); - fail(); - } catch (IllegalArgumentException e) { - // all is well - } - + .actionGet() + ); + expectThrows(IllegalArgumentException.class, () -> + client() + .admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder() + .put("index.refresh_interval", -1) // this one can change + .put("index.final", "no")) // this one can't + .execute() + .actionGet() + ); IndexMetaData indexMetaData = client().admin().cluster().prepareState().execute().actionGet().getState().metaData().index("test"); assertThat(indexMetaData.getSettings().get("index.refresh_interval"), nullValue()); assertThat(indexMetaData.getSettings().get("index.fielddata.cache"), nullValue()); + assertThat(indexMetaData.getSettings().get("index.final"), nullValue()); // Now verify via dedicated get settings api: GetSettingsResponse getSettingsResponse = client().admin().indices().prepareGetSettings("test").get(); assertThat(getSettingsResponse.getSetting("test", "index.refresh_interval"), nullValue()); assertThat(getSettingsResponse.getSetting("test", "index.fielddata.cache"), nullValue()); + assertThat(getSettingsResponse.getSetting("test", "index.final"), nullValue()); client() .admin() @@ -210,10 +233,27 @@ public void testOpenCloseUpdateSettings() throws Exception { assertThat(indexMetaData.getSettings().get("index.refresh_interval"), equalTo("1s")); assertThat(indexMetaData.getSettings().get("index.fielddata.cache"), equalTo("none")); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> + client() + .admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder() + .put("index.refresh_interval", -1) // this one can change + .put("index.final", "no")) // this one really can't + .execute() + .actionGet() + ); + assertThat(ex.getMessage(), containsString("final test setting [index.final], not updateable")); + indexMetaData = client().admin().cluster().prepareState().execute().actionGet().getState().metaData().index("test"); + assertThat(indexMetaData.getSettings().get("index.refresh_interval"), equalTo("1s")); + assertThat(indexMetaData.getSettings().get("index.final"), nullValue()); + + // Now verify via dedicated get settings api: getSettingsResponse = client().admin().indices().prepareGetSettings("test").get(); assertThat(getSettingsResponse.getSetting("test", "index.refresh_interval"), equalTo("1s")); - assertThat(getSettingsResponse.getSetting("test", "index.fielddata.cache"), equalTo("none")); + assertThat(getSettingsResponse.getSetting("test", "index.final"), nullValue()); } public void testEngineGCDeletesSetting() throws InterruptedException { From c14be2074472478c0abafca51631e22960281b44 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 4 Apr 2017 14:37:33 +0200 Subject: [PATCH 6/8] Add unit tests for the missing aggregator (#23895) * Add unit tests for the missing aggregator Relates #22278 --- .../aggregations/AggregatorTestCase.java | 13 +- .../InternalAggregationTestCase.java | 6 +- .../missing/MissingAggregatorTests.java | 145 ++++++++++++++++++ 3 files changed, 156 insertions(+), 8 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorTests.java diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 363e972456efb..a0f7bbfcba43b 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -238,13 +238,16 @@ protected A searchAndReduc if (aggs.isEmpty()) { return null; } else { - if (randomBoolean()) { + if (randomBoolean() && aggs.size() > 1) { // sometimes do an incremental reduce - List internalAggregations = randomSubsetOf(randomIntBetween(1, aggs.size()), aggs); - A internalAgg = (A) aggs.get(0).doReduce(internalAggregations, + int toReduceSize = aggs.size(); + Collections.shuffle(aggs, random()); + int r = randomIntBetween(1, toReduceSize); + List toReduce = aggs.subList(0, r); + A reduced = (A) aggs.get(0).doReduce(toReduce, new InternalAggregation.ReduceContext(root.context().bigArrays(), null, false)); - aggs.removeAll(internalAggregations); - aggs.add(internalAgg); + aggs = new ArrayList<>(aggs.subList(r, toReduceSize)); + aggs.add(reduced); } // now do the final reduce @SuppressWarnings("unchecked") diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java index 05d75d9af77f1..48f48951c474a 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java @@ -63,15 +63,15 @@ public void testReduceRandom() { ScriptService mockScriptService = mockScriptService(); MockBigArrays bigArrays = new MockBigArrays(Settings.EMPTY, new NoneCircuitBreakerService()); if (randomBoolean() && toReduce.size() > 1) { + // sometimes do an incremental reduce Collections.shuffle(toReduce, random()); - // we leave at least one element in the list - int r = Math.max(1, randomIntBetween(0, toReduceSize - 2)); + int r = randomIntBetween(1, toReduceSize); List internalAggregations = toReduce.subList(0, r); InternalAggregation.ReduceContext context = new InternalAggregation.ReduceContext(bigArrays, mockScriptService, false); @SuppressWarnings("unchecked") T reduced = (T) inputs.get(0).reduce(internalAggregations, context); - toReduce = toReduce.subList(r, toReduceSize); + toReduce = new ArrayList<>(toReduce.subList(r, toReduceSize)); toReduce.add(reduced); } InternalAggregation.ReduceContext context = diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorTests.java new file mode 100644 index 0000000000000..424c3aed2105d --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorTests.java @@ -0,0 +1,145 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.bucket.missing; + +import org.apache.lucene.document.Document; +import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.store.Directory; +import org.elasticsearch.common.lucene.search.Queries; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.support.ValueType; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Consumer; + + +public class MissingAggregatorTests extends AggregatorTestCase { + public void testMatchNoDocs() throws IOException { + int numDocs = randomIntBetween(10, 200); + testBothCases(numDocs, + "field", + Queries.newMatchAllQuery(), + doc -> doc.add(new SortedNumericDocValuesField("field", randomLong())), + internalMissing -> assertEquals(internalMissing.getDocCount(), 0)); + } + + public void testMatchAllDocs() throws IOException { + int numDocs = randomIntBetween(10, 200); + testBothCases(numDocs, + "field", + Queries.newMatchAllQuery(), + doc -> doc.add(new SortedNumericDocValuesField("another_field", randomLong())), + internalMissing -> assertEquals(internalMissing.getDocCount(), numDocs)); + } + + public void testMatchSparse() throws IOException { + int numDocs = randomIntBetween(100, 200); + final AtomicInteger count = new AtomicInteger(); + testBothCases(numDocs, + "field", + Queries.newMatchAllQuery(), + doc -> { + if (randomBoolean()) { + doc.add(new SortedNumericDocValuesField("another_field", randomLong())); + count.incrementAndGet(); + } else { + doc.add(new SortedNumericDocValuesField("field", randomLong())); + } + }, + internalMissing -> { + assertEquals(internalMissing.getDocCount(), count.get()); + count.set(0); + }); + } + + public void testMissingField() throws IOException { + int numDocs = randomIntBetween(10, 20); + testBothCases(numDocs, + "unknown_field", + Queries.newMatchAllQuery(), + doc -> { + doc.add(new SortedNumericDocValuesField("field", randomLong())); + }, + internalMissing -> { + assertEquals(internalMissing.getDocCount(), numDocs); + }); + } + + private void testBothCases(int numDocs, + String fieldName, + Query query, + Consumer consumer, + Consumer verify) throws IOException { + executeTestCase(numDocs, fieldName, query, consumer, verify, false); + executeTestCase(numDocs, fieldName, query, consumer, verify, true); + + } + + private void executeTestCase(int numDocs, + String fieldName, + Query query, + Consumer consumer, + Consumer verify, + boolean reduced) throws IOException { + try (Directory directory = newDirectory()) { + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { + Document document = new Document(); + for (int i = 0; i < numDocs; i++) { + if (frequently()) { + indexWriter.commit(); + } + consumer.accept(document); + indexWriter.addDocument(document); + document.clear(); + } + } + + try (IndexReader indexReader = DirectoryReader.open(directory)) { + IndexSearcher indexSearcher = + newSearcher(indexReader, true, true); + MissingAggregationBuilder builder = + new MissingAggregationBuilder("_name", ValueType.LONG); + builder.field(fieldName); + + NumberFieldMapper.Builder mapperBuilder = new NumberFieldMapper.Builder("_name", + NumberFieldMapper.NumberType.LONG); + MappedFieldType fieldType = mapperBuilder.fieldType(); + fieldType.setHasDocValues(true); + fieldType.setName(builder.field()); + + InternalMissing missing; + if (reduced) { + missing = searchAndReduce(indexSearcher, query, builder, fieldType); + } else { + missing = search(indexSearcher, query, builder, fieldType); + } + verify.accept(missing); + } + } + } +} From 51b5dbffb7de37f19c0bffb9f84126f34490bcbe Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 4 Apr 2017 09:39:04 -0400 Subject: [PATCH 7/8] Disable bootstrap checks for single-node discovery While there are use-cases where a single-node is in production, there are also use-cases for starting a single-node that binds transport to an external interface where the node is not in production (for example, for testing the transport client against a node started in a Docker container). It's tricky to balance the desire to always enforce the bootstrap checks when a node might be in production with the need for the community to perform testing in situations that would trip the bootstrap checks. This commit enables some flexibility for these users. By setting the discovery type to "single-node", we disable the bootstrap checks independently of how transport is bound. While this sounds like a hole in the bootstrap checks, the bootstrap checks can already be avoided in the single-node use-case by binding only HTTP but not transport. For users that are genuinely in production on a single-node use-case with transport bound to an external use-case, they can set the system property "es.enable.bootstrap.checks" to force running the bootstrap checks. It would be a mistake for them not to do this. Relates #23598 --- .../bootstrap/BootstrapChecks.java | 11 +- .../bootstrap/BootstrapChecksTests.java | 9 +- .../reference/setup/bootstrap-checks.asciidoc | 108 ++++++++++-------- 3 files changed, 76 insertions(+), 52 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java index a9758267ff3f1..1adf4ac7c7635 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.BoundTransportAddress; import org.elasticsearch.common.transport.TransportAddress; +import org.elasticsearch.discovery.DiscoveryModule; import org.elasticsearch.monitor.jvm.JvmInfo; import org.elasticsearch.monitor.process.ProcessProbe; import org.elasticsearch.node.Node; @@ -73,7 +74,7 @@ static void check(final Settings settings, final BoundTransportAddress boundTran final List combinedChecks = new ArrayList<>(builtInChecks); combinedChecks.addAll(additionalChecks); check( - enforceLimits(boundTransportAddress), + enforceLimits(boundTransportAddress, DiscoveryModule.DISCOVERY_TYPE_SETTING.get(settings)), Collections.unmodifiableList(combinedChecks), Node.NODE_NAME_SETTING.get(settings)); } @@ -166,11 +167,13 @@ static void log(final Logger logger, final String error) { * @param boundTransportAddress the node network bindings * @return {@code true} if the checks should be enforced */ - static boolean enforceLimits(final BoundTransportAddress boundTransportAddress) { - Predicate isLoopbackOrLinkLocalAddress = + static boolean enforceLimits(final BoundTransportAddress boundTransportAddress, final String discoveryType) { + final Predicate isLoopbackOrLinkLocalAddress = t -> t.address().getAddress().isLinkLocalAddress() || t.address().getAddress().isLoopbackAddress(); - return !(Arrays.stream(boundTransportAddress.boundAddresses()).allMatch(isLoopbackOrLinkLocalAddress) && + final boolean bound = + !(Arrays.stream(boundTransportAddress.boundAddresses()).allMatch(isLoopbackOrLinkLocalAddress) && isLoopbackOrLinkLocalAddress.test(boundTransportAddress.publishAddress())); + return bound && !"single-node".equals(discoveryType); } // the list of checks to execute diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java index c3e08b81d6c3e..677309531c94e 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java @@ -44,6 +44,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.hasToString; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -98,7 +99,9 @@ public void testEnforceLimitsWhenBoundToNonLocalAddress() { when(boundTransportAddress.boundAddresses()).thenReturn(transportAddresses.toArray(new TransportAddress[0])); when(boundTransportAddress.publishAddress()).thenReturn(publishAddress); - assertTrue(BootstrapChecks.enforceLimits(boundTransportAddress)); + final String discoveryType = randomFrom("zen", "single-node"); + + assertEquals(BootstrapChecks.enforceLimits(boundTransportAddress, discoveryType), !"single-node".equals(discoveryType)); } public void testEnforceLimitsWhenPublishingToNonLocalAddress() { @@ -114,7 +117,9 @@ public void testEnforceLimitsWhenPublishingToNonLocalAddress() { when(boundTransportAddress.boundAddresses()).thenReturn(transportAddresses.toArray(new TransportAddress[0])); when(boundTransportAddress.publishAddress()).thenReturn(publishAddress); - assertTrue(BootstrapChecks.enforceLimits(boundTransportAddress)); + final String discoveryType = randomFrom("zen", "single-node"); + + assertEquals(BootstrapChecks.enforceLimits(boundTransportAddress, discoveryType), !"single-node".equals(discoveryType)); } public void testExceptionAggregation() { diff --git a/docs/reference/setup/bootstrap-checks.asciidoc b/docs/reference/setup/bootstrap-checks.asciidoc index 2d18911beb67e..6f32d5054fbdc 100644 --- a/docs/reference/setup/bootstrap-checks.asciidoc +++ b/docs/reference/setup/bootstrap-checks.asciidoc @@ -31,16 +31,31 @@ Elasticsearch instances must be reachable via transport communication so they must bind transport to an external interface. Thus, we consider an Elasticsearch instance to be in development mode if it does not bind transport to an external interface (the default), and is otherwise in -production mode if it does bind transport to an external interface. Note -that HTTP can be configured independently of transport via +production mode if it does bind transport to an external interface. + +Note that HTTP can be configured independently of transport via <> and <>; this can be useful for configuring a single instance to be reachable via -HTTP for testing purposes without triggering production mode. If you do -want to force enforcement of the bootstrap checks independent of the -binding of the transport protocal, you can set the system property -`es.enforce.bootstrap.checks` to `true` (this can be useful on a -single-node production system that does not bind transport to an external -interface). +HTTP for testing purposes without triggering production mode. + +We recognize that some users need to bind transport to an external +interface for testing their usage of the transport client. For this +situation, we provide the discovery type `single-node` (configure it by +setting `discovery.type` to `single-node`); in this situation, a node +will elect itself master and will not form a cluster with any other +node. + +If you are running a single node in production, it is possible to evade +the bootstrap checks (either by not binding transport to an external +interface, or by binding transport to an external interface and setting +the discovery type to `single-node`). For this situation, you can force +execution of the bootstrap checks by setting the system property +`es.enforce.bootstrap.checks` to `true` (set this in <>, or +by adding `-Des.enforce.bootstrap.checks=true` to the environment +variable `ES_JAVA_OPTS`). We strongly encourage you to do this if you +are in this specific situation. This system property can be used to +force execution of the bootstrap checks independent of the node +configuration. === Heap size check @@ -48,11 +63,11 @@ If a JVM is started with unequal initial and max heap size, it can be prone to pauses as the JVM heap is resized during system usage. To avoid these resize pauses, it's best to start the JVM with the initial heap size equal to the maximum heap size. Additionally, if -<> is enabled, the JVM will -lock the initial size of the heap on startup. If the initial heap size -is not equal to the maximum heap size, after a resize it will not be the -case that all of the JVM heap is locked in memory. To pass the heap size -check, you must configure the <>. +<> is enabled, the JVM +will lock the initial size of the heap on startup. If the initial heap +size is not equal to the maximum heap size, after a resize it will not +be the case that all of the JVM heap is locked in memory. To pass the +heap size check, you must configure the <>. === File descriptor check @@ -76,13 +91,13 @@ Elasticsearch would much rather use to service requests. There are several ways to configure a system to disallow swapping. One way is by requesting the JVM to lock the heap in memory through `mlockall` (Unix) or virtual lock (Windows). This is done via the Elasticsearch setting -<>. However, there are cases -where this setting can be passed to Elasticsearch but Elasticsearch is -not able to lock the heap (e.g., if the `elasticsearch` user does not -have `memlock unlimited`). The memory lock check verifies that *if* the -`bootstrap.memory_lock` setting is enabled, that the JVM was successfully -able to lock the heap. To pass the memory lock check, you might have to -configure <>. +<>. However, there are +cases where this setting can be passed to Elasticsearch but +Elasticsearch is not able to lock the heap (e.g., if the `elasticsearch` +user does not have `memlock unlimited`). The memory lock check verifies +that *if* the `bootstrap.memory_lock` setting is enabled, that the JVM +was successfully able to lock the heap. To pass the memory lock check, +you might have to configure <>. === Maximum number of threads check @@ -139,29 +154,30 @@ the server VM. === Use serial collector check -There are various garbage collectors for the OpenJDK-derived JVMs targeting -different workloads. The serial collector in particular is best suited for -single logical CPU machines or extremely small heaps, neither of which are -suitable for running Elasticsearch. Using the serial collector with -Elasticsearch can be devastating for performance. The serial collector check -ensures that Elasticsearch is not configured to run with the serial -collector. To pass the serial collector check, you must not start Elasticsearch -with the serial collector (whether it's from the defaults for the JVM that -you're using, or you've explicitly specified it with `-XX:+UseSerialGC`). Note -that the default JVM configuration that ship with Elasticsearch configures -Elasticsearch to use the CMS collector. +There are various garbage collectors for the OpenJDK-derived JVMs +targeting different workloads. The serial collector in particular is +best suited for single logical CPU machines or extremely small heaps, +neither of which are suitable for running Elasticsearch. Using the +serial collector with Elasticsearch can be devastating for performance. +The serial collector check ensures that Elasticsearch is not configured +to run with the serial collector. To pass the serial collector check, +you must not start Elasticsearch with the serial collector (whether it's +from the defaults for the JVM that you're using, or you've explicitly +specified it with `-XX:+UseSerialGC`). Note that the default JVM +configuration that ship with Elasticsearch configures Elasticsearch to +use the CMS collector. === System call filter check - -Elasticsearch installs system call filters of various flavors depending on the -operating system (e.g., seccomp on Linux). These system call filters are -installed to prevent the ability to execute system calls related to forking as -a defense mechanism against arbitrary code execution attacks on Elasticsearch -The system call filter check ensures that if system call filters are enabled, -then they were successfully installed. To pass the system call filter check you -must either fix any configuration errors on your system that prevented system -call filters from installing (check your logs), or *at your own risk* disable -system call filters by setting `bootstrap.system_call_filter` to `false`. +Elasticsearch installs system call filters of various flavors depending +on the operating system (e.g., seccomp on Linux). These system call +filters are installed to prevent the ability to execute system calls +related to forking as a defense mechanism against arbitrary code +execution attacks on Elasticsearch The system call filter check ensures +that if system call filters are enabled, then they were successfully +installed. To pass the system call filter check you must either fix any +configuration errors on your system that prevented system call filters +from installing (check your logs), or *at your own risk* disable system +call filters by setting `bootstrap.system_call_filter` to `false`. === OnError and OnOutOfMemoryError checks @@ -188,8 +204,8 @@ release build of the JVM. === G1GC check -Early versions of the HotSpot JVM that shipped with JDK 8 are known to have -issues that can lead to index corruption when the G1GC collector is enabled. -The versions impacted are those earlier than the version of HotSpot that -shipped with JDK 8u40. The G1GC check detects these early versions of the -HotSpot JVM. +Early versions of the HotSpot JVM that shipped with JDK 8 are known to +have issues that can lead to index corruption when the G1GC collector is +enabled. The versions impacted are those earlier than the version of +HotSpot that shipped with JDK 8u40. The G1GC check detects these early +versions of the HotSpot JVM. From a01f77210acc50e818ae3e1e9e3c48cb2498fee7 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 4 Apr 2017 09:42:19 -0400 Subject: [PATCH 8/8] Fix Javadocs for BootstrapChecks#enforceLimits This commit adds a description for a parameter that was added to BootstrapChecks#enforceLimits(BoundTransportAddress, String) without the Javadocs having been updated. --- .../main/java/org/elasticsearch/bootstrap/BootstrapChecks.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java index 1adf4ac7c7635..3d371f0f23fc5 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java @@ -165,6 +165,7 @@ static void log(final Logger logger, final String error) { * Tests if the checks should be enforced. * * @param boundTransportAddress the node network bindings + * @param discoveryType the discovery type * @return {@code true} if the checks should be enforced */ static boolean enforceLimits(final BoundTransportAddress boundTransportAddress, final String discoveryType) {