From 2013a2f5db1f197ca3a2f182948da18c19b122ce Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Mon, 31 Jul 2023 17:22:05 +0530 Subject: [PATCH 01/11] Add support to clear archived index setting Signed-off-by: Ankit Kala --- CHANGELOG.md | 1 + .../indices/settings/UpdateSettingsIT.java | 17 +++++++++++++++++ .../metadata/MetadataUpdateSettingsService.java | 6 +++++- .../common/settings/AbstractScopedSettings.java | 2 +- .../common/settings/IndexScopedSettings.java | 2 ++ .../opensearch/common/settings/Settings.java | 5 +++-- .../org/opensearch/index/IndexSettings.java | 4 +++- .../common/settings/SettingsTests.java | 14 ++++++++++++++ 8 files changed, 46 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e4a26caa2b10..da7914807ae22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Fixed - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827)) - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944)) +- Add support to clear archived index setting ([#9019](https://github.com/opensearch-project/OpenSearch/pull/9019)) ### Security diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index beb0ea797bbec..72a4c206cb60c 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -56,6 +56,7 @@ import java.util.Collections; import java.util.List; +import static org.hamcrest.Matchers.startsWith; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_METADATA; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_READ; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_WRITE; @@ -99,6 +100,22 @@ public void testInvalidDynamicUpdate() { assertNotEquals(indexMetadata.getSettings().get("index.dummy"), "invalid dynamic value"); } + public void testArchivedSettingUpdateOpenIndex() { + createIndex("test"); + + // Archived setting update should fail on open index. + IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().put("archived.*", "null")) + .execute() + .actionGet() + ); + assertThat(exception.getMessage(), startsWith("Can't update non dynamic settings [[archived.*]] for open indices [[test")); + } + @Override protected Collection> nodePlugins() { return Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java index a5caf3269ef26..1e1e7f6d24660 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -73,6 +73,7 @@ import java.util.Set; import static org.opensearch.action.support.ContextPreservingActionListener.wrapPreservingContext; +import static org.opensearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; import static org.opensearch.index.IndexSettings.same; /** @@ -137,7 +138,9 @@ public void updateSettings( for (String key : normalizedSettings.keySet()) { Setting setting = indexScopedSettings.get(key); boolean isWildcard = setting == null && Regex.isSimpleMatchPattern(key); + boolean isArchived = key.startsWith(ARCHIVED_SETTINGS_PREFIX); assert setting != null // we already validated the normalized settings + || isArchived || (isWildcard && normalizedSettings.hasValue(key) == false) : "unknown setting: " + key + " isWildcard: " @@ -145,7 +148,8 @@ public void updateSettings( + " hasValue: " + normalizedSettings.hasValue(key); settingsForClosedIndices.copy(key, normalizedSettings); - if (isWildcard || setting.isDynamic()) { + // Only allow dynamic settings and wildcards for open indices. Skip archived settings. + if (isArchived == false && (isWildcard || setting.isDynamic())) { settingsForOpenIndices.copy(key, normalizedSettings); } else { skippedSettings.add(key); diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java index 8b7a2a82e5cb1..9dfecc986783a 100644 --- a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java @@ -496,7 +496,7 @@ public final void validate(final Settings settings, final boolean validateDepend * @see Setting#getSettingsDependencies(String) */ public final void validate(final Settings settings, final boolean validateDependencies, final boolean validateInternalOrPrivateIndex) { - validate(settings, validateDependencies, false, false, validateInternalOrPrivateIndex); + validate(settings, validateDependencies, false, true, validateInternalOrPrivateIndex); } /** diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 3cc7c351fe1bf..f02400f6f5b3d 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -76,6 +76,8 @@ public final class IndexScopedSettings extends AbstractScopedSettings { public static final Predicate INDEX_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(IndexMetadata.INDEX_SETTING_PREFIX); + public static final Predicate ARCHIVED_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(ARCHIVED_SETTINGS_PREFIX); + public static final Set> BUILT_IN_INDEX_SETTINGS = Collections.unmodifiableSet( new HashSet<>( Arrays.asList( diff --git a/server/src/main/java/org/opensearch/common/settings/Settings.java b/server/src/main/java/org/opensearch/common/settings/Settings.java index cd80e9727e0df..3f21aee9741bc 100644 --- a/server/src/main/java/org/opensearch/common/settings/Settings.java +++ b/server/src/main/java/org/opensearch/common/settings/Settings.java @@ -89,6 +89,7 @@ import java.util.stream.Stream; import static org.opensearch.core.common.unit.ByteSizeValue.parseBytesSizeValue; +import static org.opensearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; import static org.opensearch.common.unit.TimeValue.parseTimeValue; /** @@ -1217,7 +1218,7 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) { } /** - * Checks that all settings in the builder start with the specified prefix. + * Checks that all settings(except archived settings and wildcards) in the builder start with the specified prefix. * * If a setting doesn't start with the prefix, the builder appends the prefix to such setting. */ @@ -1227,7 +1228,7 @@ public Builder normalizePrefix(String prefix) { while (iterator.hasNext()) { Map.Entry entry = iterator.next(); String key = entry.getKey(); - if (key.startsWith(prefix) == false && key.endsWith("*") == false) { + if (key.startsWith(prefix) == false && key.endsWith("*") == false && key.startsWith(ARCHIVED_SETTINGS_PREFIX) == false) { replacements.put(prefix + key, entry.getValue()); iterator.remove(); } diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index d0fdbd9ac4e03..ffd2e1d66b4ce 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -1146,7 +1146,9 @@ public synchronized boolean updateIndexMetadata(IndexMetadata indexMetadata) { */ public static boolean same(final Settings left, final Settings right) { return left.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE) - .equals(right.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE)); + .equals(right.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE)) + && left.filter(IndexScopedSettings.ARCHIVED_SETTINGS_KEY_PREDICATE) + .equals(right.filter(IndexScopedSettings.ARCHIVED_SETTINGS_KEY_PREDICATE)); } /** diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsTests.java index 1fd8658a09e67..106c83071fdc4 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsTests.java @@ -301,6 +301,20 @@ public void testPrefixNormalization() { assertThat(settings.get("foo.test"), equalTo("test")); } + public void testPrefixNormalizationArchived() { + Settings settings = Settings.builder().put("archived.foo.bar", "baz").normalizePrefix("foo.").build(); + + assertThat(settings.size(), equalTo(1)); + assertThat(settings.get("foo.archived.foo.bar"), nullValue()); + assertThat(settings.get("archived.foo.bar"), equalTo("baz")); + + settings = Settings.builder().put("archived.foo.*", "baz").normalizePrefix("foo.").build(); + + assertThat(settings.size(), equalTo(1)); + assertThat(settings.get("foo.archived.foo.*"), nullValue()); + assertThat(settings.get("archived.foo.*"), equalTo("baz")); + } + public void testFilteredMap() { Settings.Builder builder = Settings.builder(); builder.put("a", "a1"); From 3376536f1298c6d1d29a5c62cbc2a26736e602b8 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Mon, 7 Aug 2023 12:08:06 +0530 Subject: [PATCH 02/11] Retry gradle Signed-off-by: Ankit Kala From 3bd5c960e878919b2a8cc2f1b7a2934071919410 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Thu, 10 Aug 2023 12:29:41 +0530 Subject: [PATCH 03/11] Updated the tests Signed-off-by: Ankit Kala --- .../indices/settings/UpdateSettingsIT.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index 72a4c206cb60c..07d46fce48aae 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -100,7 +100,7 @@ public void testInvalidDynamicUpdate() { assertNotEquals(indexMetadata.getSettings().get("index.dummy"), "invalid dynamic value"); } - public void testArchivedSettingUpdateOpenIndex() { + public void testArchivedSettingUpdate() { createIndex("test"); // Archived setting update should fail on open index. @@ -114,6 +114,22 @@ public void testArchivedSettingUpdateOpenIndex() { .actionGet() ); assertThat(exception.getMessage(), startsWith("Can't update non dynamic settings [[archived.*]] for open indices [[test")); + + // close the index. + client().admin().indices().prepareClose("test").get(); + + // Setting update on closed index shouldn't fail during validation. + // It'll still fail with SettingsException as the setting doesn't exist though. + SettingsException settingsException = expectThrows( + SettingsException.class, + () -> client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().put("archived.*", "null")) + .execute() + .actionGet() + ); + assertTrue(settingsException.getMessage().startsWith("test setting [archived.*], not recognized")); } @Override From 47142bc8bbce0e9ac7d47cac88ac06b63c2f4578 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Fri, 11 Aug 2023 13:09:37 +0530 Subject: [PATCH 04/11] Add IT to create archived index setting and its removal Signed-off-by: Ankit Kala --- .../settings/ArchivedIndexSettingsIT.java | 125 ++++++++++++++++++ .../indices/settings/UpdateSettingsIT.java | 35 +---- .../MetadataUpdateSettingsService.java | 2 + .../opensearch/test/InternalTestCluster.java | 16 ++- 4 files changed, 144 insertions(+), 34 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java new file mode 100644 index 0000000000000..9a70f372dd584 --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java @@ -0,0 +1,125 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.indices.settings; + +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; +import org.opensearch.plugins.Plugin; +import org.opensearch.test.InternalTestCluster; +import org.opensearch.test.OpenSearchIntegTestCase; + +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +import static org.hamcrest.Matchers.startsWith; + +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, supportsDedicatedMasters = false) +public class ArchivedIndexSettingsIT extends OpenSearchIntegTestCase { + public void testArchiveSettings() throws Exception { + // Setup the cluster with an index containing dummy setting(owned by dummy plugin) + String oldClusterManagerNode = internalCluster().startClusterManagerOnlyNode(); + String oldDataNode = internalCluster().startDataOnlyNode(); + assertEquals(2, internalCluster().numDataAndClusterManagerNodes()); + createIndex("test"); + ensureYellow(); + // Add a dummy setting + client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().put("index.dummy", "foobar").put("index.dummy2", "foobar")) + .execute() + .actionGet(); + + // Remove dummy plugin and replace the cluster manager node so that the stale plugin setting moves to "archived". + internalCluster().removePlugin(DummySettingPlugin.class); + String newClusterManagerNode = internalCluster().startClusterManagerOnlyNode(); + internalCluster().stopRandomNode(InternalTestCluster.nameFilter(oldClusterManagerNode)); + internalCluster().restartNode(newClusterManagerNode); + + // Verify that archived settings exists. + assertTrue( + client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy") + ); + assertTrue( + client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy2") + ); + + // Archived setting update should fail on open index. + IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().putNull("archived.index.dummy")) + .execute() + .actionGet() + ); + assertThat( + exception.getMessage(), + startsWith("Can't update non dynamic settings [[archived.index.dummy]] for open indices [[test") + ); + + // close the index. + client().admin().indices().prepareClose("test").get(); + + // Remove archived.index.dummy explicitly. + assertTrue( + client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().putNull("archived.index.dummy")) + .execute() + .actionGet() + .isAcknowledged() + ); + + // Remove archived.index.dummy2 using wildcard. + assertTrue( + client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().putNull("archived.*")) + .execute() + .actionGet() + .isAcknowledged() + ); + + // Verify that archived settings are cleaned up successfully. + assertFalse( + client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy") + ); + assertFalse( + client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy2") + ); + } + + @Override + protected Collection> nodePlugins() { + return Arrays.asList(DummySettingPlugin.class); + } + + public static class DummySettingPlugin extends Plugin { + public static final Setting DUMMY_SETTING = Setting.simpleString( + "index.dummy", + Setting.Property.IndexScope, + Setting.Property.Dynamic + ); + public static final Setting DUMMY_SETTING2 = Setting.simpleString( + "index.dummy2", + Setting.Property.IndexScope, + Setting.Property.Dynamic + ); + + @Override + public List> getSettings() { + return Arrays.asList(DUMMY_SETTING, DUMMY_SETTING2); + } + } +} diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index 07d46fce48aae..e486fa818ec9d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -56,7 +56,6 @@ import java.util.Collections; import java.util.List; -import static org.hamcrest.Matchers.startsWith; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_METADATA; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_READ; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_BLOCKS_WRITE; @@ -70,6 +69,8 @@ import static org.hamcrest.Matchers.nullValue; public class UpdateSettingsIT extends OpenSearchIntegTestCase { + List> nodePlugins = Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); + public void testInvalidUpdateOnClosedIndex() { createIndex("test"); assertAcked(client().admin().indices().prepareClose("test").get()); @@ -100,38 +101,6 @@ public void testInvalidDynamicUpdate() { assertNotEquals(indexMetadata.getSettings().get("index.dummy"), "invalid dynamic value"); } - public void testArchivedSettingUpdate() { - createIndex("test"); - - // Archived setting update should fail on open index. - IllegalArgumentException exception = expectThrows( - IllegalArgumentException.class, - () -> client().admin() - .indices() - .prepareUpdateSettings("test") - .setSettings(Settings.builder().put("archived.*", "null")) - .execute() - .actionGet() - ); - assertThat(exception.getMessage(), startsWith("Can't update non dynamic settings [[archived.*]] for open indices [[test")); - - // close the index. - client().admin().indices().prepareClose("test").get(); - - // Setting update on closed index shouldn't fail during validation. - // It'll still fail with SettingsException as the setting doesn't exist though. - SettingsException settingsException = expectThrows( - SettingsException.class, - () -> client().admin() - .indices() - .prepareUpdateSettings("test") - .setSettings(Settings.builder().put("archived.*", "null")) - .execute() - .actionGet() - ); - assertTrue(settingsException.getMessage().startsWith("test setting [archived.*], not recognized")); - } - @Override protected Collection> nodePlugins() { return Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java index 1e1e7f6d24660..5f5561decae43 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -309,6 +309,8 @@ public ClusterState execute(ClusterState currentState) { Settings finalSettings = indexSettings.build(); indexScopedSettings.validate( finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false), + true, + false, true ); metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(finalSettings)); diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index 6d9767843400b..a3689f1a6974c 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -233,6 +233,8 @@ public final class InternalTestCluster extends TestCluster { static final int DEFAULT_MIN_NUM_CLIENT_NODES = 0; static final int DEFAULT_MAX_NUM_CLIENT_NODES = 1; + private final Set> nodePlugins; + /* Sorted map to make traverse order reproducible. * The map of nodes is never mutated so individual reads are safe without synchronization. * Updates are intended to follow a copy-on-write approach. */ @@ -398,6 +400,8 @@ public InternalTestCluster( autoManageClusterManagerNodes ? "auto-managed" : "manual" ); this.nodeConfigurationSource = nodeConfigurationSource; + this.nodePlugins = new HashSet<>(nodeConfigurationSource.nodePlugins()); + numDataPaths = random.nextInt(5) == 0 ? 2 + random.nextInt(3) : 1; Builder builder = Settings.builder(); builder.put(Environment.PATH_HOME_SETTING.getKey(), baseDir); @@ -509,11 +513,21 @@ private Settings getSettings(int nodeOrdinal, long nodeSeed, Settings others) { } public Collection> getPlugins() { - Set> plugins = new HashSet<>(nodeConfigurationSource.nodePlugins()); + Set> plugins = new HashSet<>(nodePlugins); plugins.addAll(mockPlugins); return plugins; } + /** + * Remove any plugin that was added in test. This doesn't affect the existing nodes in the cluster + * and only the newer nodes which are started after the plugin removal. + * @param plugin to be removed + * @return + */ + public boolean removePlugin(Class plugin) { + return nodePlugins.remove(plugin); + } + private static Settings getRandomNodeSettings(long seed) { Random random = new Random(seed); Builder builder = Settings.builder(); From 3e0851bbdf10fd798fe42e936ac06de81693b4ba Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Sun, 13 Aug 2023 13:45:29 +0530 Subject: [PATCH 05/11] Fixed Javadoc Signed-off-by: Ankit Kala --- .../src/main/java/org/opensearch/test/InternalTestCluster.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index a3689f1a6974c..ea06548908505 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -522,7 +522,7 @@ public Collection> getPlugins() { * Remove any plugin that was added in test. This doesn't affect the existing nodes in the cluster * and only the newer nodes which are started after the plugin removal. * @param plugin to be removed - * @return + * @return true if the plugin was found and removed. */ public boolean removePlugin(Class plugin) { return nodePlugins.remove(plugin); From c68842455ec9db96de2169fcab470e40bb200d8a Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Tue, 15 Aug 2023 09:38:52 +0530 Subject: [PATCH 06/11] Fix failing tests Signed-off-by: Ankit Kala --- .../indices/settings/ArchivedIndexSettingsIT.java | 10 +++++++--- .../org/opensearch/test/InternalTestCluster.java | 15 +-------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java index 9a70f372dd584..20b0a6175c562 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java @@ -16,14 +16,18 @@ import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import static org.hamcrest.Matchers.startsWith; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, supportsDedicatedMasters = false) public class ArchivedIndexSettingsIT extends OpenSearchIntegTestCase { + private volatile boolean installPlugin; + public void testArchiveSettings() throws Exception { - // Setup the cluster with an index containing dummy setting(owned by dummy plugin) + installPlugin = true; + // Set up the cluster with an index containing dummy setting(owned by dummy plugin) String oldClusterManagerNode = internalCluster().startClusterManagerOnlyNode(); String oldDataNode = internalCluster().startDataOnlyNode(); assertEquals(2, internalCluster().numDataAndClusterManagerNodes()); @@ -38,7 +42,7 @@ public void testArchiveSettings() throws Exception { .actionGet(); // Remove dummy plugin and replace the cluster manager node so that the stale plugin setting moves to "archived". - internalCluster().removePlugin(DummySettingPlugin.class); + installPlugin = false; String newClusterManagerNode = internalCluster().startClusterManagerOnlyNode(); internalCluster().stopRandomNode(InternalTestCluster.nameFilter(oldClusterManagerNode)); internalCluster().restartNode(newClusterManagerNode); @@ -102,7 +106,7 @@ public void testArchiveSettings() throws Exception { @Override protected Collection> nodePlugins() { - return Arrays.asList(DummySettingPlugin.class); + return installPlugin ? Arrays.asList(DummySettingPlugin.class) : Collections.emptyList(); } public static class DummySettingPlugin extends Plugin { diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index ea06548908505..03bc0351be807 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -233,8 +233,6 @@ public final class InternalTestCluster extends TestCluster { static final int DEFAULT_MIN_NUM_CLIENT_NODES = 0; static final int DEFAULT_MAX_NUM_CLIENT_NODES = 1; - private final Set> nodePlugins; - /* Sorted map to make traverse order reproducible. * The map of nodes is never mutated so individual reads are safe without synchronization. * Updates are intended to follow a copy-on-write approach. */ @@ -400,7 +398,6 @@ public InternalTestCluster( autoManageClusterManagerNodes ? "auto-managed" : "manual" ); this.nodeConfigurationSource = nodeConfigurationSource; - this.nodePlugins = new HashSet<>(nodeConfigurationSource.nodePlugins()); numDataPaths = random.nextInt(5) == 0 ? 2 + random.nextInt(3) : 1; Builder builder = Settings.builder(); @@ -513,21 +510,11 @@ private Settings getSettings(int nodeOrdinal, long nodeSeed, Settings others) { } public Collection> getPlugins() { - Set> plugins = new HashSet<>(nodePlugins); + Set> plugins = new HashSet<>(nodeConfigurationSource.nodePlugins()); plugins.addAll(mockPlugins); return plugins; } - /** - * Remove any plugin that was added in test. This doesn't affect the existing nodes in the cluster - * and only the newer nodes which are started after the plugin removal. - * @param plugin to be removed - * @return true if the plugin was found and removed. - */ - public boolean removePlugin(Class plugin) { - return nodePlugins.remove(plugin); - } - private static Settings getRandomNodeSettings(long seed) { Random random = new Random(seed); Builder builder = Settings.builder(); From 41b2ecd1f73923d9510b7a3875767575a3b7a3ae Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Tue, 15 Aug 2023 09:44:28 +0530 Subject: [PATCH 07/11] Remove unused variable Signed-off-by: Ankit Kala --- .../java/org/opensearch/indices/settings/UpdateSettingsIT.java | 2 -- .../src/main/java/org/opensearch/test/InternalTestCluster.java | 1 - 2 files changed, 3 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index e486fa818ec9d..beb0ea797bbec 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -69,8 +69,6 @@ import static org.hamcrest.Matchers.nullValue; public class UpdateSettingsIT extends OpenSearchIntegTestCase { - List> nodePlugins = Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); - public void testInvalidUpdateOnClosedIndex() { createIndex("test"); assertAcked(client().admin().indices().prepareClose("test").get()); diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index 03bc0351be807..6d9767843400b 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -398,7 +398,6 @@ public InternalTestCluster( autoManageClusterManagerNodes ? "auto-managed" : "manual" ); this.nodeConfigurationSource = nodeConfigurationSource; - numDataPaths = random.nextInt(5) == 0 ? 2 + random.nextInt(3) : 1; Builder builder = Settings.builder(); builder.put(Environment.PATH_HOME_SETTING.getKey(), baseDir); From 16f8b0fc9cf73a8fc9d4470d3db73579ddc4dfb6 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Tue, 15 Aug 2023 23:43:21 +0530 Subject: [PATCH 08/11] Retry gradle Signed-off-by: Ankit Kala From b78a4c893ed3a252ef0883a8355875bb1445f314 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Wed, 16 Aug 2023 20:57:04 +0530 Subject: [PATCH 09/11] Retry gradle Signed-off-by: Ankit Kala From e732c64de4104f125198210b2d12b5c9a9ce6ee5 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Mon, 21 Aug 2023 17:16:52 +0530 Subject: [PATCH 10/11] Addressed comments Signed-off-by: Ankit Kala --- .../cluster/metadata/MetadataUpdateSettingsService.java | 2 ++ .../org/opensearch/common/settings/AbstractScopedSettings.java | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java index 7c32a905bcbd8..16ad3f6690e53 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -133,6 +133,8 @@ public void updateSettings( indexScopedSettings.validate( normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false), // don't validate wildcards false, // don't validate dependencies here we check it below never allow to change the number of shards + false, + true, // Ignore archived setting. true ); // validate internal or private index settings for (String key : normalizedSettings.keySet()) { diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java index 9dfecc986783a..8b7a2a82e5cb1 100644 --- a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java @@ -496,7 +496,7 @@ public final void validate(final Settings settings, final boolean validateDepend * @see Setting#getSettingsDependencies(String) */ public final void validate(final Settings settings, final boolean validateDependencies, final boolean validateInternalOrPrivateIndex) { - validate(settings, validateDependencies, false, true, validateInternalOrPrivateIndex); + validate(settings, validateDependencies, false, false, validateInternalOrPrivateIndex); } /** From 37051509a8597f66d62c6a0ae9497577a371e6b2 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Tue, 22 Aug 2023 10:22:30 +0530 Subject: [PATCH 11/11] Retry gradle Signed-off-by: Ankit Kala