Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Archive unknown or invalid settings on updates #28888

Merged
merged 14 commits into from
Mar 13, 2018
Merged

Archive unknown or invalid settings on updates #28888

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ebebe19
Archive unknown or invalid settings on updates
jasontedor Mar 3, 2018
293895c
Formatting
jasontedor Mar 3, 2018
b3781b4
Fix parameter name
jasontedor Mar 3, 2018
d0cf14a
Fix newlines
jasontedor Mar 6, 2018
bf7f356
Fix formatting
jasontedor Mar 6, 2018
9e55fac
Clarify tuple
jasontedor Mar 6, 2018
fe756a3
Merge branch 'master' into unknown-or-invalid-settings-updates
jasontedor Mar 7, 2018
98a1b6b
Merge branch 'master' into unknown-or-invalid-settings-updates
jasontedor Mar 12, 2018
2a1dc17
Clarify function
jasontedor Mar 12, 2018
e5ceaac
Remove randomization
jasontedor Mar 12, 2018
d2bc7fd
Simplify test
jasontedor Mar 12, 2018
3298664
Add test
jasontedor Mar 12, 2018
e428728
Fix typo
jasontedor Mar 13, 2018
e85ad01
Even stronger testing
jasontedor Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 73 additions & 8 deletions server/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -48,15 +54,35 @@ synchronized Settings getPersistentUpdate() {
return persistentUpdates.build();
}

synchronized ClusterState updateSettings(final ClusterState currentState, Settings transientToApply, Settings persistentToApply) {
synchronized ClusterState updateSettings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get your newlines under control my friend

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<Settings, Settings> partitionedTransientSettings =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have 2 methods Settings getValidSettings(Settings) and Settings getInvalidSetting(Settings) that would make it simpler to see what is valid and not instead of a tuple?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a change. I kept the tuple but clarified its usage. I hope this helps.

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<Settings, Settings> 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;
Expand All @@ -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())
Expand Down Expand Up @@ -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 sending warnings to
* @return the partitioned settings
*/
private Tuple<Settings, Settings> 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<String, String> 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<String, String> e,
final IllegalArgumentException ex,
final Logger logger) {
logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man newlines?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about painting it yellow. 😛

(org.apache.logging.log4j.util.Supplier<?>)
() -> new ParameterizedMessage("ignoring existing invalid {} setting: [{}] with value [{}]; archiving",
settingType,
e.getKey(),
e.getValue()),
ex);
}

}
3 changes: 2 additions & 1 deletion ...org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
79 changes: 68 additions & 11 deletions ...r/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {


Expand All @@ -51,22 +56,22 @@ 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);
assertEquals(BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING.get(clusterState.metaData().transientSettings()), 0.5, 0.1);
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()));
Expand All @@ -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);
Expand All @@ -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);

}
Expand All @@ -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<MetaData.Builder, Function<Settings, MetaData.Builder>> function,
final Function<MetaData, Settings> settingsToTest) {
final Setting<String> dynamicSetting = Setting.simpleString("dynamic.setting", Property.Dynamic, Property.NodeScope);
final Setting<String> 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<Setting<?>> 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took me quite a while to understand what's going on - on the one hand the persistent vs transient question is handled by parameters to this method (please rename the function parameter if we keep it ;)) and on the other hand we have a boolean here that deals with it directly which may be inconsistent. On top of it all, the rest API and the transport layer allows changing both at once - something we don't test. I think the functional programming got a bit out of hand here, probably due to multiple iterations. Can we have a straight forward test that works with both persistent and transient (randomly combining them)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed d2bc7fd.

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"));
}
}

}