Skip to content

Commit

Permalink
[Remote Store] Clear feature flag set during unit test runs (opensear…
Browse files Browse the repository at this point in the history 
…ch-project#7523)

Signed-off-by: Ashish Singh <[email protected]>
Co-authored-by: dblock <[email protected]>
  • Loading branch information
@ashking94 @dblock
ashking94 and dblock authored May 15, 2023
1 parent ba35781 commit e44b3f1
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 86 deletions.
70 changes: 34 additions & 36 deletions server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
);
}
}
9 changes: 4 additions & 5 deletions server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
7 changes: 2 additions & 5 deletions server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
23 changes: 11 additions & 12 deletions server/src/test/java/org/opensearch/index/IndexSettingsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
27 changes: 12 additions & 15 deletions server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion server/src/test/java/org/opensearch/index/store/StoreTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
47 changes: 36 additions & 11 deletions test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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<String>) () -> 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<String>) () -> System.clearProperty(this.flag));
@SuppressForbidden(reason = "Clears the set feature flags")
private void clearAll() {
for (String flag : flags) {
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.clearProperty(flag));
}
LOGGER.info("unset feature_flags={}", flags);
flags.clear();
}
}
7 changes: 6 additions & 1 deletion test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down

0 comments on commit e44b3f1

Please sign in to comment.