From ebebe19bc9cbda7428ce14c2fa422ccb4190aadf Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 2 Mar 2018 17:57:37 -0800 Subject: [PATCH 01/12] Archive unknown or invalid settings on updates Today we can end up in a situation where the cluster state contains unknown or invalid settings. This can happen easily during a rolling upgrade. For example, consider two nodes that are on a version that considers the setting foo.bar to be known and valid. Assume one of these nodes is restarted on a higher version that considers foo.bar to now be either unknown or invalid, and then the second node is restarted too. Now, both nodes will be on a version that consider foo.bar to be unknown or invalid yet this setting will still be contained in the cluster state. This means that if a cluster settings update is applied and we validate the settings update with the existing settings then validation will fail. In such a state, the offending setting can not even be removed. This commit helps out with this situation by archiving any settings that are unknown or invalid at the time that a settings update is applied. This allows the setting update to go through, and the archived settings can be removed at a later time. --- .../cluster/settings/SettingsUpdater.java | 81 +++++++++++++++++-- .../TransportClusterUpdateSettingsAction.java | 3 +- .../settings/SettingsUpdaterTests.java | 79 +++++++++++++++--- 3 files changed, 143 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index dc13913652a34..67f2aa97742f0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -19,13 +19,19 @@ package org.elasticsearch.action.admin.cluster.settings; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; +import java.util.Map; + import static org.elasticsearch.cluster.ClusterState.builder; +import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; /** * Updates transient and persistent cluster state settings if there are any changes @@ -48,15 +54,35 @@ synchronized Settings getPersistentUpdate() { return persistentUpdates.build(); } - synchronized ClusterState updateSettings(final ClusterState currentState, Settings transientToApply, Settings persistentToApply) { + synchronized ClusterState updateSettings( + final ClusterState currentState, + final Settings transientToApply, + final Settings persistentToApply, + final Logger logger) { boolean changed = false; - Settings.Builder transientSettings = Settings.builder(); - transientSettings.put(currentState.metaData().transientSettings()); - changed |= clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient"); + /* + * Our cluster state could have unknown or invalid settings that are known and valid in a previous version of Elasticsearch. We can + * end up in this situation during a rolling upgrade where the previous version will infect the current version of Elasticsearch + * with settings that the current version either no longer knows about or now considers to have invalid values. When the current + * version of Elasticsearch becomes infected with a cluster state containing such settings, we need to skip validating such settings + * and instead archive them. Consequently, for the current transient and persistent settings in the cluster state we do the + * following: + * - create a settings instance with that has archived all unknown or invalid settings + * - split this settings instance into two with the known and valid settings in one, and the unknown or invalid in another + * - validate the incoming settings update combined with the existing known and valid settings + * - merge in the archived unknown or invalid settings + */ + final Tuple partitionedTransientSettings = + partitionKnownAndValidSettings(currentState.metaData().transientSettings(), "transient", logger); + final Settings.Builder transientSettings = Settings.builder(); + transientSettings.put(partitionedTransientSettings.v1()); + changed |= clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient"); - Settings.Builder persistentSettings = Settings.builder(); - persistentSettings.put(currentState.metaData().persistentSettings()); + final Tuple partitionedPersistentSettings = + partitionKnownAndValidSettings(currentState.metaData().persistentSettings(), "persistent", logger); + final Settings.Builder persistentSettings = Settings.builder(); + persistentSettings.put(partitionedPersistentSettings.v1()); changed |= clusterSettings.updateDynamicSettings(persistentToApply, persistentSettings, persistentUpdates, "persistent"); final ClusterState clusterState; @@ -69,8 +95,8 @@ synchronized ClusterState updateSettings(final ClusterState currentState, Settin clusterSettings.validate(persistentFinalSettings, true); MetaData.Builder metaData = MetaData.builder(currentState.metaData()) - .persistentSettings(persistentFinalSettings) - .transientSettings(transientFinalSettings); + .persistentSettings(Settings.builder().put(persistentFinalSettings).put(partitionedPersistentSettings.v2()).build()) + .transientSettings(Settings.builder().put(transientFinalSettings).put(partitionedTransientSettings.v2()).build()); ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean updatedReadOnly = MetaData.SETTING_READ_ONLY_SETTING.get(metaData.persistentSettings()) @@ -102,5 +128,44 @@ synchronized ClusterState updateSettings(final ClusterState currentState, Settin return clusterState; } + /** + * Partitions the settings into those that are known and valid versus those that are unknown or invalid. The resulting tuple contains + * the known and valid settings in the first component and the unknown or invalid settings in the second component. + * + * @param settings the settings to partition + * @param settingsType a string to identify the settings (for logging) + * @param logger a logger to + * @return the partitioned settings + */ + private Tuple partitionKnownAndValidSettings( + final Settings settings, + final String settingsType, + final Logger logger) { + final Settings settingsWithUnknownOrInvalidArchived = clusterSettings.archiveUnknownOrInvalidSettings( + settings, + e -> logUnknownSetting(settingsType, e, logger), + (e, ex) -> logInvalidSetting(settingsType, e, ex, logger)); + return Tuple.tuple( + settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX) == false), + settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX))); + } + + private void logUnknownSetting(final String settingType, final Map.Entry e, final Logger logger) { + logger.warn("ignoring existing unknown {} setting: [{}] with value [{}]; archiving", settingType, e.getKey(), e.getValue()); + } + + private void logInvalidSetting( + final String settingType, + final Map.Entry e, + final IllegalArgumentException ex, + final Logger logger) { + logger.warn( + (org.apache.logging.log4j.util.Supplier) + () -> new ParameterizedMessage("ignoring existing invalid {} setting: [{}] with value [{}]; archiving", + settingType, + e.getKey(), + e.getValue()), + ex); + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java index dae55b2fc048a..edc30bd3c35fd 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java @@ -180,7 +180,8 @@ public void onFailure(String source, Exception e) { @Override public ClusterState execute(final ClusterState currentState) { - ClusterState clusterState = updater.updateSettings(currentState, request.transientSettings(), request.persistentSettings()); + ClusterState clusterState = + updater.updateSettings(currentState, request.transientSettings(), request.persistentSettings(), logger); changed = clusterState != currentState; return clusterState; } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 19dd64e6324ca..ee69ff7f865b5 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -30,9 +30,14 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; + public class SettingsUpdaterTests extends ESTestCase { @@ -51,7 +56,7 @@ public void testUpdateSetting() { .put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 4.5).build()); ClusterState build = builder.metaData(metaData).build(); ClusterState clusterState = updater.updateSettings(build, Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 0.5).build(), - Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 0.4).build()); + Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 0.4).build(), logger); assertNotSame(clusterState, build); assertEquals(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.get(clusterState.metaData().persistentSettings()), 0.4, 0.1); assertEquals(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.get(clusterState.metaData().persistentSettings()), 2.5, 0.1); @@ -59,14 +64,14 @@ public void testUpdateSetting() { assertEquals(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.get(clusterState.metaData().transientSettings()), 4.5, 0.1); clusterState = updater.updateSettings(clusterState, Settings.builder().putNull("cluster.routing.*").build(), - Settings.EMPTY); + Settings.EMPTY, logger); assertEquals(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.get(clusterState.metaData().persistentSettings()), 0.4, 0.1); assertEquals(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.get(clusterState.metaData().persistentSettings()), 2.5, 0.1); assertFalse(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.exists(clusterState.metaData().transientSettings())); assertFalse(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.exists(clusterState.metaData().transientSettings())); clusterState = updater.updateSettings(clusterState, - Settings.EMPTY, Settings.builder().putNull("cluster.routing.*").put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 10.0).build()); + Settings.EMPTY, Settings.builder().putNull("cluster.routing.*").put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 10.0).build(), logger); assertEquals(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.get(clusterState.metaData().persistentSettings()), 10.0, 0.1); assertFalse(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.exists(clusterState.metaData().persistentSettings())); @@ -93,7 +98,7 @@ public void testAllOrNothing() { try { updater.updateSettings(build, Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), "not a float").build(), - Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), "not a float").put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build()); + Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), "not a float").put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build(), logger); fail("all or nothing"); } catch (IllegalArgumentException ex) { logger.info("", ex); @@ -119,21 +124,21 @@ public void testClusterBlock() { ClusterState build = builder.metaData(metaData).build(); ClusterState clusterState = updater.updateSettings(build, Settings.builder().put(MetaData.SETTING_READ_ONLY_SETTING.getKey(), true).build(), - Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 1.6).put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build()); + Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 1.6).put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build(), logger); assertEquals(clusterState.blocks().global().size(), 1); assertEquals(clusterState.blocks().global().iterator().next(), MetaData.CLUSTER_READ_ONLY_BLOCK); clusterState = updater.updateSettings(build, Settings.EMPTY, - Settings.builder().put(MetaData.SETTING_READ_ONLY_SETTING.getKey(), false).build()); + Settings.builder().put(MetaData.SETTING_READ_ONLY_SETTING.getKey(), false).build(), logger); assertEquals(clusterState.blocks().global().size(), 0); clusterState = updater.updateSettings(build, Settings.builder().put(MetaData.SETTING_READ_ONLY_ALLOW_DELETE_SETTING.getKey(), true).build(), - Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 1.6).put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build()); + Settings.builder().put(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.getKey(), 1.6).put(BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING.getKey(), 1.0f).build(), logger); assertEquals(clusterState.blocks().global().size(), 1); assertEquals(clusterState.blocks().global().iterator().next(), MetaData.CLUSTER_READ_ONLY_ALLOW_DELETE_BLOCK); clusterState = updater.updateSettings(build, Settings.EMPTY, - Settings.builder().put(MetaData.SETTING_READ_ONLY_ALLOW_DELETE_SETTING.getKey(), false).build()); + Settings.builder().put(MetaData.SETTING_READ_ONLY_ALLOW_DELETE_SETTING.getKey(), false).build(), logger); assertEquals(clusterState.blocks().global().size(), 0); } @@ -151,16 +156,68 @@ public void testDeprecationLogging() { ClusterState.builder(new ClusterName("foo")).metaData(MetaData.builder().persistentSettings(settings).build()).build(); final Settings toApplyDebug = Settings.builder().put("logger.org.elasticsearch", "debug").build(); - final ClusterState afterDebug = settingsUpdater.updateSettings(clusterState, toApplyDebug, Settings.EMPTY); + final ClusterState afterDebug = settingsUpdater.updateSettings(clusterState, toApplyDebug, Settings.EMPTY, logger); assertSettingDeprecationsAndWarnings(new Setting[] { deprecatedSetting }); final Settings toApplyUnset = Settings.builder().putNull("logger.org.elasticsearch").build(); - final ClusterState afterUnset = settingsUpdater.updateSettings(afterDebug, toApplyUnset, Settings.EMPTY); + final ClusterState afterUnset = settingsUpdater.updateSettings(afterDebug, toApplyUnset, Settings.EMPTY, logger); assertSettingDeprecationsAndWarnings(new Setting[] { deprecatedSetting }); // we also check that if no settings are changed, deprecation logging still occurs - settingsUpdater.updateSettings(afterUnset, toApplyUnset, Settings.EMPTY); + settingsUpdater.updateSettings(afterUnset, toApplyUnset, Settings.EMPTY, logger); assertSettingDeprecationsAndWarnings(new Setting[] { deprecatedSetting }); } + public void testUpdateWithUnknownAndSettings() { + runUpdateWithUnknownAndInvalidSettingTest(builder -> builder::persistentSettings, MetaData::persistentSettings); + runUpdateWithUnknownAndInvalidSettingTest(builder -> builder::transientSettings, MetaData::transientSettings); + } + + private void runUpdateWithUnknownAndInvalidSettingTest( + final Function> function, + final Function settingsToTest) { + final Setting dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope); + final Setting invalidSetting = Setting.simpleString( + "invalid.setting", + (setting, settings) -> { + throw new IllegalArgumentException("invalid"); + }, + Property.NodeScope); + final Settings settings = Settings.builder().put("invalid.setting", "value").put("unknown.setting", "value").build(); + + final Set> knownSettings = + Stream.concat( + ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.stream(), + Stream.of(dynamicSetting, invalidSetting)) + .collect(Collectors.toSet()); + final ClusterSettings clusterSettings = new ClusterSettings(settings, knownSettings); + clusterSettings.addSettingsUpdateConsumer(dynamicSetting, s -> {}); + final SettingsUpdater settingsUpdater = new SettingsUpdater(clusterSettings); + final ClusterState clusterState = + ClusterState + .builder(new ClusterName("cluster")) + .metaData(function.apply(MetaData.builder()).apply(settings).build()) + .build(); + final Settings toApply = Settings.builder().put("dynamic.setting", "value").build(); + final boolean applyTransient = randomBoolean(); + final ClusterState clusterStateAfterUpdate; + if (applyTransient) { + clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, toApply, Settings.EMPTY, logger); + } else { + clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, Settings.EMPTY, toApply, logger); + } + + assertThat( + settingsToTest.apply(clusterStateAfterUpdate.metaData()).keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); + assertThat( + settingsToTest.apply(clusterStateAfterUpdate.metaData()).keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); + if (applyTransient) { + assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem("dynamic.setting")); + assertThat(clusterStateAfterUpdate.metaData().transientSettings().get("dynamic.setting"), equalTo("value")); + } else { + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem("dynamic.setting")); + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().get("dynamic.setting"), equalTo("value")); + } + } + } From 293895c0177012fa55cc475b840cafbdcc89521b Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 2 Mar 2018 18:16:44 -0800 Subject: [PATCH 02/12] Formatting --- .../action/admin/cluster/settings/SettingsUpdater.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index 67f2aa97742f0..a497ba8335a03 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -132,9 +132,9 @@ synchronized ClusterState updateSettings( * Partitions the settings into those that are known and valid versus those that are unknown or invalid. The resulting tuple contains * the known and valid settings in the first component and the unknown or invalid settings in the second component. * - * @param settings the settings to partition + * @param settings the settings to partition * @param settingsType a string to identify the settings (for logging) - * @param logger a logger to + * @param logger a logger to sending warnings to * @return the partitioned settings */ private Tuple partitionKnownAndValidSettings( From b3781b4306c395e141b039f13d3218b24c0ac76a Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 2 Mar 2018 18:28:03 -0800 Subject: [PATCH 03/12] Fix parameter name --- .../action/admin/cluster/settings/SettingsUpdaterTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index ee69ff7f865b5..e8663bba3a19f 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -179,7 +179,7 @@ private void runUpdateWithUnknownAndInvalidSettingTest( final Setting dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope); final Setting invalidSetting = Setting.simpleString( "invalid.setting", - (setting, settings) -> { + (value, settings) -> { throw new IllegalArgumentException("invalid"); }, Property.NodeScope); From d0cf14a73ba42e5a2791028c27bf5f63e84e6ff3 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 6 Mar 2018 08:37:32 -0800 Subject: [PATCH 04/12] Fix newlines --- .../cluster/settings/SettingsUpdater.java | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index a497ba8335a03..1e895d50c9cac 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -55,10 +55,7 @@ synchronized Settings getPersistentUpdate() { } synchronized ClusterState updateSettings( - final ClusterState currentState, - final Settings transientToApply, - final Settings persistentToApply, - final Logger logger) { + final ClusterState currentState, final Settings transientToApply, final Settings persistentToApply, final Logger logger) { boolean changed = false; /* @@ -68,21 +65,19 @@ synchronized ClusterState updateSettings( * version of Elasticsearch becomes infected with a cluster state containing such settings, we need to skip validating such settings * and instead archive them. Consequently, for the current transient and persistent settings in the cluster state we do the * following: - * - create a settings instance with that has archived all unknown or invalid settings - * - split this settings instance into two with the known and valid settings in one, and the unknown or invalid in another + * - split existing settings instance into two with the known and valid settings in one, and the unknown or invalid in another + * (note that existing archived settings are included in the known and valid settings) * - validate the incoming settings update combined with the existing known and valid settings * - merge in the archived unknown or invalid settings */ final Tuple partitionedTransientSettings = partitionKnownAndValidSettings(currentState.metaData().transientSettings(), "transient", logger); - final Settings.Builder transientSettings = Settings.builder(); - transientSettings.put(partitionedTransientSettings.v1()); + final Settings.Builder transientSettings = Settings.builder().put(partitionedTransientSettings.v1()); changed |= clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient"); final Tuple partitionedPersistentSettings = partitionKnownAndValidSettings(currentState.metaData().persistentSettings(), "persistent", logger); - final Settings.Builder persistentSettings = Settings.builder(); - persistentSettings.put(partitionedPersistentSettings.v1()); + final Settings.Builder persistentSettings = Settings.builder().put(partitionedPersistentSettings.v1()); changed |= clusterSettings.updateDynamicSettings(persistentToApply, persistentSettings, persistentUpdates, "persistent"); final ClusterState clusterState; @@ -130,7 +125,8 @@ synchronized ClusterState updateSettings( /** * Partitions the settings into those that are known and valid versus those that are unknown or invalid. The resulting tuple contains - * the known and valid settings in the first component and the unknown or invalid settings in the second component. + * the known and valid settings in the first component and the unknown or invalid settings in the second component. Note that archived + * settings contained in the settings to partition are included in the first component. * * @param settings the settings to partition * @param settingsType a string to identify the settings (for logging) @@ -138,15 +134,18 @@ synchronized ClusterState updateSettings( * @return the partitioned settings */ private Tuple partitionKnownAndValidSettings( - final Settings settings, - final String settingsType, - final Logger logger) { + final Settings settings, final String settingsType, final Logger logger) { + final Settings existingArchivedSettings = settings.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX)); + final Settings settingsExcludingExistingArchivedSettings = + settings.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX) == false); final Settings settingsWithUnknownOrInvalidArchived = clusterSettings.archiveUnknownOrInvalidSettings( - settings, + settingsExcludingExistingArchivedSettings, e -> logUnknownSetting(settingsType, e, logger), (e, ex) -> logInvalidSetting(settingsType, e, ex, logger)); return Tuple.tuple( - settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX) == false), + Settings.builder() + .put(settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX) == false)) + .put(existingArchivedSettings).build(), settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX))); } @@ -155,10 +154,7 @@ private void logUnknownSetting(final String settingType, final Map.Entry e, - final IllegalArgumentException ex, - final Logger logger) { + final String settingType, final Map.Entry e, final IllegalArgumentException ex, final Logger logger) { logger.warn( (org.apache.logging.log4j.util.Supplier) () -> new ParameterizedMessage("ignoring existing invalid {} setting: [{}] with value [{}]; archiving", From bf7f3564812e04a1142ccdb6ee43fd90b9d89a12 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 6 Mar 2018 08:47:34 -0800 Subject: [PATCH 05/12] Fix formatting --- .../action/admin/cluster/settings/SettingsUpdater.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index 1e895d50c9cac..d634142d2960d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -145,7 +145,8 @@ private Tuple partitionKnownAndValidSettings( return Tuple.tuple( Settings.builder() .put(settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX) == false)) - .put(existingArchivedSettings).build(), + .put(existingArchivedSettings) + .build(), settingsWithUnknownOrInvalidArchived.filter(k -> k.startsWith(ARCHIVED_SETTINGS_PREFIX))); } From 9e55facd9327de046861c890ebd510992a414266 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 6 Mar 2018 08:57:49 -0800 Subject: [PATCH 06/12] Clarify tuple --- .../admin/cluster/settings/SettingsUpdater.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index d634142d2960d..ec72dd949674c 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -21,6 +21,7 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; +import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.metadata.MetaData; @@ -72,12 +73,16 @@ synchronized ClusterState updateSettings( */ final Tuple partitionedTransientSettings = partitionKnownAndValidSettings(currentState.metaData().transientSettings(), "transient", logger); - final Settings.Builder transientSettings = Settings.builder().put(partitionedTransientSettings.v1()); + final Settings knownAndValidTransientSettings = partitionedTransientSettings.v1(); + final Settings unknownOrInvalidTransientSettings = partitionedTransientSettings.v2(); + final Settings.Builder transientSettings = Settings.builder().put(knownAndValidTransientSettings); changed |= clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient"); final Tuple partitionedPersistentSettings = partitionKnownAndValidSettings(currentState.metaData().persistentSettings(), "persistent", logger); - final Settings.Builder persistentSettings = Settings.builder().put(partitionedPersistentSettings.v1()); + final Settings knownAndValidPersistentSettings = partitionedPersistentSettings.v1(); + final Settings unknownOrInvalidPersistentSettings = partitionedPersistentSettings.v2(); + final Settings.Builder persistentSettings = Settings.builder().put(knownAndValidPersistentSettings); changed |= clusterSettings.updateDynamicSettings(persistentToApply, persistentSettings, persistentUpdates, "persistent"); final ClusterState clusterState; @@ -90,8 +95,8 @@ synchronized ClusterState updateSettings( clusterSettings.validate(persistentFinalSettings, true); MetaData.Builder metaData = MetaData.builder(currentState.metaData()) - .persistentSettings(Settings.builder().put(persistentFinalSettings).put(partitionedPersistentSettings.v2()).build()) - .transientSettings(Settings.builder().put(transientFinalSettings).put(partitionedTransientSettings.v2()).build()); + .transientSettings(Settings.builder().put(transientFinalSettings).put(unknownOrInvalidTransientSettings).build()) + .persistentSettings(Settings.builder().put(persistentFinalSettings).put(unknownOrInvalidPersistentSettings).build()); ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean updatedReadOnly = MetaData.SETTING_READ_ONLY_SETTING.get(metaData.persistentSettings()) @@ -157,7 +162,7 @@ private void logUnknownSetting(final String settingType, final Map.Entry e, final IllegalArgumentException ex, final Logger logger) { logger.warn( - (org.apache.logging.log4j.util.Supplier) + (Supplier) () -> new ParameterizedMessage("ignoring existing invalid {} setting: [{}] with value [{}]; archiving", settingType, e.getKey(), From 2a1dc17ce433f45dd139b8d33c811cb5a309f445 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 12 Mar 2018 06:08:34 -0400 Subject: [PATCH 07/12] Clarify function --- .../admin/cluster/settings/SettingsUpdaterTests.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index e8663bba3a19f..1674e1dbf5270 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -30,6 +30,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -169,12 +170,12 @@ public void testDeprecationLogging() { } public void testUpdateWithUnknownAndSettings() { - runUpdateWithUnknownAndInvalidSettingTest(builder -> builder::persistentSettings, MetaData::persistentSettings); - runUpdateWithUnknownAndInvalidSettingTest(builder -> builder::transientSettings, MetaData::transientSettings); + runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::persistentSettings, MetaData::persistentSettings); + runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::transientSettings, MetaData::transientSettings); } private void runUpdateWithUnknownAndInvalidSettingTest( - final Function> function, + final BiFunction metaDataSettingsBuilder, final Function settingsToTest) { final Setting dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope); final Setting invalidSetting = Setting.simpleString( @@ -196,7 +197,7 @@ private void runUpdateWithUnknownAndInvalidSettingTest( final ClusterState clusterState = ClusterState .builder(new ClusterName("cluster")) - .metaData(function.apply(MetaData.builder()).apply(settings).build()) + .metaData(metaDataSettingsBuilder.apply(MetaData.builder(), settings).build()) .build(); final Settings toApply = Settings.builder().put("dynamic.setting", "value").build(); final boolean applyTransient = randomBoolean(); From e5ceaac1d5bf3cc2136a263822e825151c7659e6 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 12 Mar 2018 06:11:05 -0400 Subject: [PATCH 08/12] Remove randomization --- .../admin/cluster/settings/SettingsUpdaterTests.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 1674e1dbf5270..2f996e72a575d 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -170,13 +170,14 @@ public void testDeprecationLogging() { } public void testUpdateWithUnknownAndSettings() { - runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::persistentSettings, MetaData::persistentSettings); - runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::transientSettings, MetaData::transientSettings); + runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::persistentSettings, MetaData::persistentSettings, false); + runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::transientSettings, MetaData::transientSettings, true); } private void runUpdateWithUnknownAndInvalidSettingTest( final BiFunction metaDataSettingsBuilder, - final Function settingsToTest) { + final Function settingsToTest, + final boolean applyTransient) { final Setting dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope); final Setting invalidSetting = Setting.simpleString( "invalid.setting", @@ -200,7 +201,6 @@ private void runUpdateWithUnknownAndInvalidSettingTest( .metaData(metaDataSettingsBuilder.apply(MetaData.builder(), settings).build()) .build(); final Settings toApply = Settings.builder().put("dynamic.setting", "value").build(); - final boolean applyTransient = randomBoolean(); final ClusterState clusterStateAfterUpdate; if (applyTransient) { clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, toApply, Settings.EMPTY, logger); From d2bc7fd016f4883a0363b6e28ebcb2639711cd66 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 12 Mar 2018 06:26:01 -0400 Subject: [PATCH 09/12] Simplify test --- .../settings/SettingsUpdaterTests.java | 98 +++++++++++++------ 1 file changed, 68 insertions(+), 30 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 2f996e72a575d..5f631a8f3468e 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -28,6 +28,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; +import java.util.ArrayList; +import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; @@ -170,55 +172,91 @@ public void testDeprecationLogging() { } public void testUpdateWithUnknownAndSettings() { - runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::persistentSettings, MetaData::persistentSettings, false); - runUpdateWithUnknownAndInvalidSettingTest(MetaData.Builder::transientSettings, MetaData::transientSettings, true); - } + final int numberOfDynamicSettings = randomIntBetween(2, 8); + final List> dynamicSettings = new ArrayList<>(); + for (int i = 0; i < numberOfDynamicSettings; i++) { + final Setting dynamicSetting = Setting.simpleString("dynamic.setting" + i, Property.Dynamic, Property.NodeScope); + dynamicSettings.add(dynamicSetting); + } - private void runUpdateWithUnknownAndInvalidSettingTest( - final BiFunction metaDataSettingsBuilder, - final Function settingsToTest, - final boolean applyTransient) { - final Setting dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope); final Setting invalidSetting = Setting.simpleString( "invalid.setting", (value, settings) -> { throw new IllegalArgumentException("invalid"); }, Property.NodeScope); - final Settings settings = Settings.builder().put("invalid.setting", "value").put("unknown.setting", "value").build(); + + final Settings.Builder existingPersistentSettings = Settings.builder(); + final Settings.Builder existingTransientSettings = Settings.builder(); + + if (randomBoolean()) { + existingPersistentSettings.put("invalid.setting", "value"); + } else { + existingTransientSettings.put("invalid.setting", "value"); + } + + if (randomBoolean()) { + existingPersistentSettings.put("unknown.setting", "value"); + } else { + existingTransientSettings.put("unknown.setting", "value"); + } final Set> knownSettings = Stream.concat( ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.stream(), - Stream.of(dynamicSetting, invalidSetting)) + Stream.concat(dynamicSettings.stream(), Stream.of(invalidSetting))) .collect(Collectors.toSet()); - final ClusterSettings clusterSettings = new ClusterSettings(settings, knownSettings); - clusterSettings.addSettingsUpdateConsumer(dynamicSetting, s -> {}); + final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, knownSettings); + for (final Setting dynamicSetting : dynamicSettings) { + clusterSettings.addSettingsUpdateConsumer(dynamicSetting, s -> {}); + } final SettingsUpdater settingsUpdater = new SettingsUpdater(clusterSettings); - final ClusterState clusterState = - ClusterState - .builder(new ClusterName("cluster")) - .metaData(metaDataSettingsBuilder.apply(MetaData.builder(), settings).build()) - .build(); - final Settings toApply = Settings.builder().put("dynamic.setting", "value").build(); + final MetaData.Builder metaDataBuilder = + MetaData.builder() + .persistentSettings(existingPersistentSettings.build()) + .transientSettings(existingTransientSettings.build()); + final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).metaData(metaDataBuilder).build(); + final Settings.Builder persistentToApply = Settings.builder(); + final Settings.Builder transientToApply = Settings.builder(); + for (final Setting dynamicSetting : dynamicSettings) { + if (randomBoolean()) { + persistentToApply.put(dynamicSetting.getKey(), "value"); + } else { + transientToApply.put(dynamicSetting.getKey(), "value"); + } + } final ClusterState clusterStateAfterUpdate; - if (applyTransient) { - clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, toApply, Settings.EMPTY, logger); + clusterStateAfterUpdate = + settingsUpdater.updateSettings(clusterState, transientToApply.build(), persistentToApply.build(), logger); + + if (existingPersistentSettings.keys().contains("invalid.setting")) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); } else { - clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, Settings.EMPTY, toApply, logger); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); } - assertThat( - settingsToTest.apply(clusterStateAfterUpdate.metaData()).keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); - assertThat( - settingsToTest.apply(clusterStateAfterUpdate.metaData()).keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); - if (applyTransient) { - assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem("dynamic.setting")); - assertThat(clusterStateAfterUpdate.metaData().transientSettings().get("dynamic.setting"), equalTo("value")); + if (existingPersistentSettings.keys().contains("unknown.setting")) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); } else { - assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem("dynamic.setting")); - assertThat(clusterStateAfterUpdate.metaData().persistentSettings().get("dynamic.setting"), equalTo("value")); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); + } + + for (final Setting dynamicSetting : dynamicSettings) { + if (persistentToApply.keys().contains(dynamicSetting.getKey())) { + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem(dynamicSetting.getKey())); + } else { + assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(dynamicSetting.getKey())); + } } + } } From 32986647b9e5829f73cfcea6e6edfbaf9784dcbd Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 12 Mar 2018 08:36:04 -0400 Subject: [PATCH 10/12] Add test --- .../settings/SettingsUpdaterTests.java | 215 +++++++++++++++--- 1 file changed, 180 insertions(+), 35 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 5f631a8f3468e..99839b9a279d2 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -40,6 +40,7 @@ import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.not; public class SettingsUpdaterTests extends ESTestCase { @@ -172,39 +173,59 @@ public void testDeprecationLogging() { } public void testUpdateWithUnknownAndSettings() { - final int numberOfDynamicSettings = randomIntBetween(2, 8); - final List> dynamicSettings = new ArrayList<>(); + // we will randomly apply some new dynamic persistent and transient settings + final int numberOfDynamicSettings = randomIntBetween(1, 8); + final List> dynamicSettings = new ArrayList<>(numberOfDynamicSettings); for (int i = 0; i < numberOfDynamicSettings; i++) { final Setting dynamicSetting = Setting.simpleString("dynamic.setting" + i, Property.Dynamic, Property.NodeScope); dynamicSettings.add(dynamicSetting); } - final Setting invalidSetting = Setting.simpleString( - "invalid.setting", - (value, settings) -> { - throw new IllegalArgumentException("invalid"); - }, - Property.NodeScope); + // these are invalid settings that exist as either persistent or transient settings + final int numberOfInvalidSettings = randomIntBetween(0, 7); + final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); + for (int i = 0; i < numberOfInvalidSettings; i++) { + final Setting invalidSetting = Setting.simpleString( + "invalid.setting" + i, + (value, settings) -> { + throw new IllegalArgumentException("invalid"); + }, + Property.NodeScope); + invalidSettings.add(invalidSetting); + } + + // these are unknown settings that exist as either persistent or transient settings + final int numberOfUnknownSettings = randomIntBetween(0, 7); + final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); + for (int i = 0; i < numberOfUnknownSettings; i++) { + final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); + unknownSettings.add(unknownSetting); + } final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); - if (randomBoolean()) { - existingPersistentSettings.put("invalid.setting", "value"); - } else { - existingTransientSettings.put("invalid.setting", "value"); + for (final Setting invalidSetting : invalidSettings) { + if (randomBoolean()) { + existingPersistentSettings.put(invalidSetting.getKey(), "value"); + } else { + existingTransientSettings.put(invalidSetting.getKey(), "value"); + } } - if (randomBoolean()) { - existingPersistentSettings.put("unknown.setting", "value"); - } else { - existingTransientSettings.put("unknown.setting", "value"); + for (final Setting unknownSetting : unknownSettings) { + if (randomBoolean()) { + existingPersistentSettings.put(unknownSetting.getKey(), "value"); + } else { + existingTransientSettings.put(unknownSetting.getKey(), "value"); + } } + // register all the known settings (not that we do not register the unknown settings) final Set> knownSettings = Stream.concat( ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.stream(), - Stream.concat(dynamicSettings.stream(), Stream.of(invalidSetting))) + Stream.concat(dynamicSettings.stream(), invalidSettings.stream())) .collect(Collectors.toSet()); final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, knownSettings); for (final Setting dynamicSetting : dynamicSettings) { @@ -216,6 +237,8 @@ public void testUpdateWithUnknownAndSettings() { .persistentSettings(existingPersistentSettings.build()) .transientSettings(existingTransientSettings.build()); final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).metaData(metaDataBuilder).build(); + + // prepare the dynamic settings update final Settings.Builder persistentToApply = Settings.builder(); final Settings.Builder transientToApply = Settings.builder(); for (final Setting dynamicSetting : dynamicSettings) { @@ -225,30 +248,36 @@ public void testUpdateWithUnknownAndSettings() { transientToApply.put(dynamicSetting.getKey(), "value"); } } - final ClusterState clusterStateAfterUpdate; - clusterStateAfterUpdate = + final ClusterState clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, transientToApply.build(), persistentToApply.build(), logger); - if (existingPersistentSettings.keys().contains("invalid.setting")) { - assertThat( - clusterStateAfterUpdate.metaData().persistentSettings().keySet(), - hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); - } else { - assertThat( - clusterStateAfterUpdate.metaData().transientSettings().keySet(), - hasItem(ARCHIVED_SETTINGS_PREFIX + "invalid.setting")); + // the invalid settings should be archived + for (final Setting invalidSetting : invalidSettings) { + if (existingPersistentSettings.keys().contains(invalidSetting.getKey())) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); + } else { + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); + } } - if (existingPersistentSettings.keys().contains("unknown.setting")) { - assertThat( - clusterStateAfterUpdate.metaData().persistentSettings().keySet(), - hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); - } else { - assertThat( - clusterStateAfterUpdate.metaData().transientSettings().keySet(), - hasItem(ARCHIVED_SETTINGS_PREFIX + "unknown.setting")); + // the unknown settings should be archived + for (final Setting unknownSetting : unknownSettings) { + if (existingPersistentSettings.keys().contains(unknownSetting.getKey())) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); + } else { + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); + } } + // the dynamic settings should be applied for (final Setting dynamicSetting : dynamicSettings) { if (persistentToApply.keys().contains(dynamicSetting.getKey())) { assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem(dynamicSetting.getKey())); @@ -256,7 +285,123 @@ public void testUpdateWithUnknownAndSettings() { assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(dynamicSetting.getKey())); } } + } + + public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknownSettings() { + // these are settings that are archived in the cluster state as either persistent or transient settings + final int numberOfArchivedSettings = randomIntBetween(1, 8); + final List> archivedSettings = new ArrayList<>(numberOfArchivedSettings); + for (int i = 0; i < numberOfArchivedSettings; i++) { + final Setting archivedSetting = Setting.simpleString("setting", Property.NodeScope); + archivedSettings.add(archivedSetting); + } + // these are invalid settings that exist as either persistent or transient settings + final int numberOfInvalidSettings = randomIntBetween(0, 7); + final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); + for (int i = 0; i < numberOfInvalidSettings; i++) { + final Setting invalidSetting = Setting.simpleString( + "invalid.setting" + i, + (value, settings) -> { + throw new IllegalArgumentException("invalid"); + }, + Property.NodeScope); + invalidSettings.add(invalidSetting); + } + + // these are unknown settings that exist as either persistent or transient settings + final int numberOfUnknownSettings = randomIntBetween(0, 7); + final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); + for (int i = 0; i < numberOfUnknownSettings; i++) { + final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); + unknownSettings.add(unknownSetting); + } + + final Settings.Builder existingPersistentSettings = Settings.builder(); + final Settings.Builder existingTransientSettings = Settings.builder(); + + for (final Setting archivedSetting : archivedSettings) { + if (randomBoolean()) { + existingPersistentSettings.put(ARCHIVED_SETTINGS_PREFIX + archivedSetting.getKey(), "value"); + } else { + existingTransientSettings.put(ARCHIVED_SETTINGS_PREFIX + archivedSetting.getKey(), "value"); + } + } + + for (final Setting invalidSetting : invalidSettings) { + if (randomBoolean()) { + existingPersistentSettings.put(invalidSetting.getKey(), "value"); + } else { + existingTransientSettings.put(invalidSetting.getKey(), "value"); + } + } + + for (final Setting unknownSetting : unknownSettings) { + if (randomBoolean()) { + existingPersistentSettings.put(unknownSetting.getKey(), "value"); + } else { + existingTransientSettings.put(unknownSetting.getKey(), "value"); + } + } + + // register all the known settings (not that we do not register the unknown settings) + final Set> knownSettings = + Stream.concat( + ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.stream(), + Stream.concat(archivedSettings.stream(), invalidSettings.stream())) + .collect(Collectors.toSet()); + final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, knownSettings); + final SettingsUpdater settingsUpdater = new SettingsUpdater(clusterSettings); + final MetaData.Builder metaDataBuilder = + MetaData.builder() + .persistentSettings(existingPersistentSettings.build()) + .transientSettings(existingTransientSettings.build()); + final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).metaData(metaDataBuilder).build(); + + final Settings.Builder persistentToApply = Settings.builder().put("archived.*", (String)null); + final Settings.Builder transientToApply = Settings.builder().put("archived.*", (String)null); + + final ClusterState clusterStateAfterUpdate = + settingsUpdater.updateSettings(clusterState, transientToApply.build(), persistentToApply.build(), logger); + + // existing archived settings are removed + for (final Setting archivedSetting : archivedSettings) { + if (existingPersistentSettings.keys().contains(ARCHIVED_SETTINGS_PREFIX + archivedSetting.getKey())) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + not(hasItem(ARCHIVED_SETTINGS_PREFIX + archivedSetting.getKey()))); + } else { + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + not(hasItem(ARCHIVED_SETTINGS_PREFIX + archivedSetting.getKey()))); + } + } + + // the invalid settings should be archived + for (final Setting invalidSetting : invalidSettings) { + if (existingPersistentSettings.keys().contains(invalidSetting.getKey())) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); + } else { + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); + } + } + + // the unknown settings should be archived + for (final Setting unknownSetting : unknownSettings) { + if (existingPersistentSettings.keys().contains(unknownSetting.getKey())) { + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); + } else { + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); + } + } } } From e42872839dc70834f3ed4391c97e82bb00f43145 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 13 Mar 2018 17:13:36 -0400 Subject: [PATCH 11/12] Fix typo --- .../action/admin/cluster/settings/SettingsUpdaterTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 99839b9a279d2..048a6073709c0 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -221,7 +221,7 @@ public void testUpdateWithUnknownAndSettings() { } } - // register all the known settings (not that we do not register the unknown settings) + // register all the known settings (note that we do not register the unknown settings) final Set> knownSettings = Stream.concat( ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.stream(), From e85ad011637a6c72bb480eb378aad3894d375fbe Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 13 Mar 2018 17:31:45 -0400 Subject: [PATCH 12/12] Even stronger testing --- .../settings/SettingsUpdaterTests.java | 83 +++++++++++++++++-- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 048a6073709c0..d582141898684 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -205,6 +205,19 @@ public void testUpdateWithUnknownAndSettings() { final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); + for (final Setting dynamicSetting : dynamicSettings) { + switch (randomIntBetween(0, 2)) { + case 0: + existingPersistentSettings.put(dynamicSetting.getKey(), "existing_value"); + break; + case 1: + existingTransientSettings.put(dynamicSetting.getKey(), "existing_value"); + break; + case 2: + break; + } + } + for (final Setting invalidSetting : invalidSettings) { if (randomBoolean()) { existingPersistentSettings.put(invalidSetting.getKey(), "value"); @@ -242,16 +255,31 @@ public void testUpdateWithUnknownAndSettings() { final Settings.Builder persistentToApply = Settings.builder(); final Settings.Builder transientToApply = Settings.builder(); for (final Setting dynamicSetting : dynamicSettings) { + switch (randomIntBetween(0, 2)) { + case 0: + persistentToApply.put(dynamicSetting.getKey(), "new_value"); + break; + case 1: + transientToApply.put(dynamicSetting.getKey(), "new_value"); + break; + case 2: + break; + } + } + + if (transientToApply.keys().isEmpty() && persistentToApply.keys().isEmpty()) { + // force a settings update otherwise our assertions below will fail if (randomBoolean()) { - persistentToApply.put(dynamicSetting.getKey(), "value"); + persistentToApply.put(dynamicSettings.get(0).getKey(), "new_value"); } else { - transientToApply.put(dynamicSetting.getKey(), "value"); + transientToApply.put(dynamicSettings.get(0).getKey(), "new_value"); } } + final ClusterState clusterStateAfterUpdate = settingsUpdater.updateSettings(clusterState, transientToApply.build(), persistentToApply.build(), logger); - // the invalid settings should be archived + // the invalid settings should be archived and not present in non-archived form for (final Setting invalidSetting : invalidSettings) { if (existingPersistentSettings.keys().contains(invalidSetting.getKey())) { assertThat( @@ -262,9 +290,15 @@ public void testUpdateWithUnknownAndSettings() { clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); } + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + not(hasItem(invalidSetting.getKey()))); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + not(hasItem(invalidSetting.getKey()))); } - // the unknown settings should be archived + // the unknown settings should be archived and not present in non-archived form for (final Setting unknownSetting : unknownSettings) { if (existingPersistentSettings.keys().contains(unknownSetting.getKey())) { assertThat( @@ -275,14 +309,37 @@ public void testUpdateWithUnknownAndSettings() { clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); } + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + not(hasItem(unknownSetting.getKey()))); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + not(hasItem(unknownSetting.getKey()))); } // the dynamic settings should be applied for (final Setting dynamicSetting : dynamicSettings) { if (persistentToApply.keys().contains(dynamicSetting.getKey())) { assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem(dynamicSetting.getKey())); - } else { + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().get(dynamicSetting.getKey()), equalTo("new_value")); + } else if (transientToApply.keys().contains(dynamicSetting.getKey())) { assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(dynamicSetting.getKey())); + assertThat(clusterStateAfterUpdate.metaData().transientSettings().get(dynamicSetting.getKey()), equalTo("new_value")); + } else { + if (existingPersistentSettings.keys().contains(dynamicSetting.getKey())) { + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), hasItem(dynamicSetting.getKey())); + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().get(dynamicSetting.getKey()), + equalTo("existing_value")); + } else if (existingTransientSettings.keys().contains(dynamicSetting.getKey())) { + assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(dynamicSetting.getKey())); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().get(dynamicSetting.getKey()), + equalTo("existing_value")); + } else { + assertThat(clusterStateAfterUpdate.metaData().persistentSettings().keySet(), not(hasItem(dynamicSetting.getKey()))); + assertThat(clusterStateAfterUpdate.metaData().transientSettings().keySet(), not(hasItem(dynamicSetting.getKey()))); + } } } } @@ -377,7 +434,7 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown } } - // the invalid settings should be archived + // the invalid settings should be archived and not present in non-archived form for (final Setting invalidSetting : invalidSettings) { if (existingPersistentSettings.keys().contains(invalidSetting.getKey())) { assertThat( @@ -388,9 +445,15 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + invalidSetting.getKey())); } + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + not(hasItem(invalidSetting.getKey()))); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + not(hasItem(invalidSetting.getKey()))); } - // the unknown settings should be archived + // the unknown settings should be archived and not present in non-archived form for (final Setting unknownSetting : unknownSettings) { if (existingPersistentSettings.keys().contains(unknownSetting.getKey())) { assertThat( @@ -401,6 +464,12 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown clusterStateAfterUpdate.metaData().transientSettings().keySet(), hasItem(ARCHIVED_SETTINGS_PREFIX + unknownSetting.getKey())); } + assertThat( + clusterStateAfterUpdate.metaData().persistentSettings().keySet(), + not(hasItem(unknownSetting.getKey()))); + assertThat( + clusterStateAfterUpdate.metaData().transientSettings().keySet(), + not(hasItem(unknownSetting.getKey()))); } }