From 6be7770b108d05c6dd05b8b60ef3196d80cfb9d8 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Wed, 23 May 2018 09:02:01 +0200 Subject: [PATCH] Use original settings on full-cluster restart (#30780) When doing a node restart using the test framework, the restarted node does not only use the settings provided to the original node, but also additional settings provided by plugin extensions, which does not correspond to the settings that a node would have on a true restart. --- .../java/org/elasticsearch/node/Node.java | 11 +++++++- .../test/InternalTestCluster.java | 2 +- .../test/test/InternalTestClusterTests.java | 26 ++++++++++++++++++- .../xpack/ml/MachineLearning.java | 8 ++---- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index ecf5dfc6b24f9..c514b135f6f0c 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(); @@ -577,7 +579,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 12acd21903ec4..12bb3448eb50d 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 23c665af30a30..e2c8a99cf8b79 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 @@ -484,9 +484,11 @@ public Settings transportClientSettings() { boolean enableHttpPipelining = randomBoolean(); String nodePrefix = "test"; Path baseDir = createTempDir(); + List> plugins = new ArrayList<>(Arrays.asList(getTestTransportPlugin(), TestZenDiscovery.TestPlugin.class)); + plugins.add(NodeAttrCheckPlugin.class); InternalTestCluster cluster = new InternalTestCluster(randomLong(), baseDir, false, true, 2, 2, "test", nodeConfigurationSource, 0, enableHttpPipelining, nodePrefix, - Arrays.asList(getTestTransportPlugin(), TestZenDiscovery.TestPlugin.class), Function.identity()); + plugins, Function.identity()); try { cluster.beforeTest(random(), 0.0); assertMMNinNodeSetting(cluster, 2); @@ -518,4 +520,26 @@ public Settings onNodeStopped(String nodeName) throws Exception { } assertSettingDeprecationsAndWarnings(new Setting[] { NetworkModule.HTTP_ENABLED }); } + + /** + * 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 a80f6255cac5d..3cb995d3570d1 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 @@ -320,12 +320,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); @@ -488,7 +484,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),