From 6fa0fac67f97340f26e34d3a58125c255835d48a Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Tue, 22 May 2018 15:48:35 +0200 Subject: [PATCH] Use original settings on full-cluster restart --- .../java/org/elasticsearch/node/Node.java | 11 +++++++- .../test/InternalTestCluster.java | 2 +- .../test/test/InternalTestClusterTests.java | 28 +++++++++++++++++-- .../xpack/ml/MachineLearning.java | 8 ++---- 4 files changed, 38 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 054b91dc51101..44ecb6b04d627 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -230,6 +230,7 @@ public static final Settings addNodeNameIfNeeded(Settings settings, final String private final Lifecycle lifecycle = new Lifecycle(); private final Injector injector; private final Settings settings; + private final Settings originalSettings; private final Environment environment; private final NodeEnvironment nodeEnvironment; private final PluginsService pluginsService; @@ -260,6 +261,7 @@ protected Node(final Environment environment, Collection logger.info("initializing ..."); } try { + originalSettings = environment.settings(); Settings tmpSettings = Settings.builder().put(environment.settings()) .put(Client.CLIENT_TYPE_SETTING_S.getKey(), CLIENT_TYPE).build(); @@ -563,7 +565,14 @@ protected void processRecoverySettings(ClusterSettings clusterSettings, Recovery } /** - * The settings that were used to create the node. + * The original settings that were used to create the node + */ + public Settings originalSettings() { + return originalSettings; + } + + /** + * The settings that are used by this node. Contains original settings as well as additional settings provided by plugins. */ public Settings settings() { return this.settings; 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 5099fc0540de2..de4b5a5ae0582 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -909,7 +909,7 @@ private void clearDataIfNeeded(RestartCallback callback) throws IOException { private void createNewNode(final Settings newSettings) { final long newIdSeed = NodeEnvironment.NODE_ID_SEED_SETTING.get(node.settings()) + 1; // use a new seed to make sure we have new node id - Settings finalSettings = Settings.builder().put(node.settings()).put(newSettings).put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), newIdSeed).build(); + Settings finalSettings = Settings.builder().put(node.originalSettings()).put(newSettings).put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), newIdSeed).build(); if (DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.exists(finalSettings) == false) { throw new IllegalStateException(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey() + " is not configured after restart of [" + name + "]"); 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 c70708c73acbf..6ecc07a3ab990 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 @@ -19,7 +19,6 @@ */ package org.elasticsearch.test.test; -import org.elasticsearch.common.settings.Setting; import org.elasticsearch.core.internal.io.IOUtils; import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.client.Client; @@ -63,7 +62,6 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFileExists; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFileNotExists; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.not; @@ -476,9 +474,11 @@ public Settings transportClientSettings() { boolean enableHttpPipelining = randomBoolean(); String nodePrefix = "test"; Path baseDir = createTempDir(); + List> plugins = new ArrayList<>(mockPlugins()); + plugins.add(NodeAttrCheckPlugin.class); InternalTestCluster cluster = new InternalTestCluster(randomLong(), baseDir, false, true, 2, 2, "test", nodeConfigurationSource, 0, enableHttpPipelining, nodePrefix, - mockPlugins(), Function.identity()); + plugins, Function.identity()); try { cluster.beforeTest(random(), 0.0); assertMMNinNodeSetting(cluster, 2); @@ -509,4 +509,26 @@ public Settings onNodeStopped(String nodeName) throws Exception { cluster.close(); } } + + /** + * Plugin that adds a simple node attribute as setting and checks if that node attribute is not already defined. + * Allows to check that the full-cluster restart logic does not copy over plugin-derived settings. + */ + public static class NodeAttrCheckPlugin extends Plugin { + + private final Settings settings; + + public NodeAttrCheckPlugin(Settings settings) { + this.settings = settings; + } + + @Override + public Settings additionalSettings() { + if (settings.get("node.attr.dummy") != null) { + fail("dummy setting already exists"); + } + return Settings.builder().put("node.attr.dummy", true).build(); + } + + } } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java index bdefabdb294e5..a1714a8e3f5db 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java @@ -316,12 +316,8 @@ public Settings additionalSettings() { } private void addMlNodeAttribute(Settings.Builder additionalSettings, String attrName, String value) { - // Unfortunately we cannot simply disallow any value, because the internal cluster integration - // test framework will restart nodes with settings copied from the node immediately before it - // was stopped. The best we can do is reject inconsistencies, and report this in a way that - // makes clear that setting the node attribute directly is not allowed. String oldValue = settings.get(attrName); - if (oldValue == null || oldValue.equals(value)) { + if (oldValue == null) { additionalSettings.put(attrName, value); } else { reportClashingNodeAttribute(attrName); @@ -487,7 +483,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestStartDatafeedAction(settings, restController), new RestStopDatafeedAction(settings, restController), new RestDeleteModelSnapshotAction(settings, restController), - new RestDeleteExpiredDataAction(settings, restController), + new RestDeleteExpiredDataAction(settings, restController), new RestForecastJobAction(settings, restController), new RestGetCalendarsAction(settings, restController), new RestPutCalendarAction(settings, restController),