From e44b3f1993d6e2094e51200a3da34c30818ddf5b Mon Sep 17 00:00:00 2001 From: Ashish Date: Mon, 15 May 2023 15:45:54 +0530 Subject: [PATCH] [Remote Store] Clear feature flag set during unit test runs (#7523) Signed-off-by: Ashish Singh Co-authored-by: dblock --- .../common/settings/SettingsModuleTests.java | 70 +++++++++---------- .../common/util/FeatureFlagTests.java | 9 ++- .../extensions/ExtensionsManagerTests.java | 7 +- .../opensearch/index/IndexSettingsTests.java | 23 +++--- .../index/engine/ReadOnlyEngineTests.java | 27 ++++--- .../opensearch/index/store/StoreTests.java | 3 +- .../opensearch/test/FeatureFlagSetter.java | 47 ++++++++++--- .../opensearch/test/OpenSearchTestCase.java | 7 +- 8 files changed, 107 insertions(+), 86 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java index cfc123e210a16..b15d3518e2f99 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java @@ -240,48 +240,46 @@ public void testOldMaxClauseCountSetting() { ); } - public void testDynamicNodeSettingsRegistration() throws Exception { - try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) { - Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); - SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); - assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // For unregistered setting the value is expected to be null - assertNull(module.getClusterSettings().get("some.custom.setting2")); - assertInstanceBinding(module, Settings.class, (s) -> s == settings); + public void testDynamicNodeSettingsRegistration() { + FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); + Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); + SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); + assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // For unregistered setting the value is expected to be null + assertNull(module.getClusterSettings().get("some.custom.setting2")); + assertInstanceBinding(module, Settings.class, (s) -> s == settings); - assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope))); - assertNotNull(module.getClusterSettings().get("some.custom.setting2")); - // verify if some.custom.setting still exists - assertNotNull(module.getClusterSettings().get("some.custom.setting")); + assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope))); + assertNotNull(module.getClusterSettings().get("some.custom.setting2")); + // verify if some.custom.setting still exists + assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // verify exception is thrown when setting registration fails - expectThrows( - SettingsException.class, - () -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)) - ); - } + // verify exception is thrown when setting registration fails + expectThrows( + SettingsException.class, + () -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)) + ); } - public void testDynamicIndexSettingsRegistration() throws Exception { - try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) { - Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); - SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); - assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // For unregistered setting the value is expected to be null - assertNull(module.getIndexScopedSettings().get("index.custom.setting2")); - assertInstanceBinding(module, Settings.class, (s) -> s == settings); + public void testDynamicIndexSettingsRegistration() { + FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); + Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); + SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); + assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // For unregistered setting the value is expected to be null + assertNull(module.getIndexScopedSettings().get("index.custom.setting2")); + assertInstanceBinding(module, Settings.class, (s) -> s == settings); - assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))); - assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2")); + assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))); + assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2")); - // verify if some.custom.setting still exists - assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // verify if some.custom.setting still exists + assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // verify exception is thrown when setting registration fails - expectThrows( - SettingsException.class, - () -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)) - ); - } + // verify exception is thrown when setting registration fails + expectThrows( + SettingsException.class, + () -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)) + ); } } diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index ca16efdf11d7d..f175308482b15 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -15,12 +15,11 @@ public class FeatureFlagTests extends OpenSearchTestCase { private final String FLAG_PREFIX = "opensearch.experimental.feature."; - public void testFeatureFlagSet() throws Exception { + public void testFeatureFlagSet() { final String testFlag = FLAG_PREFIX + "testFlag"; - try (FeatureFlagSetter f = FeatureFlagSetter.set(testFlag)) { - assertNotNull(System.getProperty(testFlag)); - assertTrue(FeatureFlags.isEnabled(testFlag)); - } + FeatureFlagSetter.set(testFlag); + assertNotNull(System.getProperty(testFlag)); + assertTrue(FeatureFlags.isEnabled(testFlag)); } public void testMissingFeatureFlag() { diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 65fb0322ffa2f..7bee57bb2beec 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -44,6 +44,7 @@ import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.ClusterSettingsResponse; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.env.EnvironmentSettingsResponse; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; @@ -62,7 +63,6 @@ import org.opensearch.common.settings.WriteableSetting.SettingType; import org.opensearch.common.settings.SettingsModule; import org.opensearch.common.transport.TransportAddress; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.PageCacheRecycler; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.env.Environment; @@ -94,8 +94,6 @@ import org.opensearch.usage.UsageService; public class ExtensionsManagerTests extends OpenSearchTestCase { - - private FeatureFlagSetter featureFlagSetter; private TransportService transportService; private ActionModule actionModule; private RestController restController; @@ -134,7 +132,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { @Before public void setup() throws Exception { - featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); + FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); Settings settings = Settings.builder().put("cluster.name", "test").build(); transport = new MockNioTransport( settings, @@ -195,7 +193,6 @@ public void tearDown() throws Exception { transportService.close(); client.close(); ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); - featureFlagSetter.close(); } public void testDiscover() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index 3d4d505147a04..8f65271b9fcc9 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -1045,18 +1045,17 @@ public void testSetRemoteTranslogBufferIntervalFailsWhenEmpty() { @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testExtendedCompatibilityVersionForRemoteSnapshot() throws Exception { - try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { - IndexMetadata metadata = newIndexMeta( - "index", - Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - .build() - ); - IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); - assertTrue(settings.isRemoteSnapshot()); - assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion()); - } + FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); + IndexMetadata metadata = newIndexMeta( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .build() + ); + IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); + assertTrue(settings.isRemoteSnapshot()); + assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion()); } public void testExtendedCompatibilityVersionForNonRemoteSnapshot() { diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index fed07e3805642..b23394d1edd9c 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -246,21 +246,18 @@ public void testReadOldIndices() throws Exception { final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); - try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { - final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( - "index", - Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - .build() - ); - try (Store store = createStore(newFSDirectory(tmp))) { - EngineConfig config = config(indexSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get); - try ( - ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true) - ) { - assertVisibleCount(readOnlyEngine, 1, false); - } + FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); + final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .build() + ); + try (Store store = createStore(newFSDirectory(tmp))) { + EngineConfig config = config(indexSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get); + try (ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true)) { + assertVisibleCount(readOnlyEngine, 1, false); } } } diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index b810aa7d197f2..b11e8554027b1 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -1273,7 +1273,8 @@ public void testReadSegmentsFromOldIndices() throws Exception { final ShardId shardId = new ShardId("index", "_na_", 1); Store store = null; - try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { + try { + FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( "index", Settings.builder() diff --git a/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java b/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java index 26e884e707964..eddcf9c738bb3 100644 --- a/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java +++ b/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java @@ -8,32 +8,57 @@ package org.opensearch.test; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.common.SuppressForbidden; +import org.opensearch.common.util.concurrent.ConcurrentCollections; import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.Set; /** * Helper class that wraps the lifecycle of setting and finally clearing of - * a {@link org.opensearch.common.util.FeatureFlags} string in an {@link AutoCloseable}. + * a {@link org.opensearch.common.util.FeatureFlags} string. */ -public class FeatureFlagSetter implements AutoCloseable { +public class FeatureFlagSetter { - private final String flag; + private static FeatureFlagSetter INSTANCE = null; - private FeatureFlagSetter(String flag) { - this.flag = flag; + private static synchronized FeatureFlagSetter getInstance() { + if (INSTANCE == null) { + INSTANCE = new FeatureFlagSetter(); + } + return INSTANCE; } + public static synchronized void set(String flag) { + getInstance().setFlag(flag); + } + + public static synchronized void clear() { + if (INSTANCE != null) { + INSTANCE.clearAll(); + INSTANCE = null; + } + } + + private static final Logger LOGGER = LogManager.getLogger(FeatureFlagSetter.class); + private final Set flags = ConcurrentCollections.newConcurrentSet(); + @SuppressForbidden(reason = "Enables setting of feature flags") - public static final FeatureFlagSetter set(String flag) { + private void setFlag(String flag) { + flags.add(flag); AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(flag, "true")); - return new FeatureFlagSetter(flag); + LOGGER.info("set feature_flag={}", flag); } - @SuppressForbidden(reason = "Clears the set feature flag on close") - @Override - public void close() throws Exception { - AccessController.doPrivileged((PrivilegedAction) () -> System.clearProperty(this.flag)); + @SuppressForbidden(reason = "Clears the set feature flags") + private void clearAll() { + for (String flag : flags) { + AccessController.doPrivileged((PrivilegedAction) () -> System.clearProperty(flag)); + } + LOGGER.info("unset feature_flags={}", flags); + flags.clear(); } } diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java index ca416cafe8ea4..7722b59313b5f 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java @@ -103,7 +103,6 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContent; import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.MediaType; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.core.xcontent.XContentParser.Token; import org.opensearch.env.Environment; @@ -221,6 +220,12 @@ public static void resetPortCounter() { portGenerator.set(0); } + @Override + public void tearDown() throws Exception { + FeatureFlagSetter.clear(); + super.tearDown(); + } + // Allows distinguishing between parallel test processes public static final String TEST_WORKER_VM_ID;