From a227d20647eb4a791d6e6b86c5c12a9fd4102885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dharmesh=20=F0=9F=92=A4?= Date: Tue, 29 Aug 2023 17:02:58 +0530 Subject: [PATCH] Updating/Fixing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Dharmesh 💤 --- .../RemoteStoreBaseIntegTestCase.java | 41 +++++++++++-------- .../opensearch/remotestore/RemoteStoreIT.java | 21 ++++------ .../RemoteStoreRepositoryRegistrationIT.java | 29 ++++--------- .../remotestore/RemoteStoreService.java | 20 ++++----- .../coordination/JoinTaskExecutor.java | 12 ++++-- .../RepositoriesServiceTests.java | 3 ++ 6 files changed, 61 insertions(+), 65 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java index e021226fb3d18..9f1317d98049a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java @@ -16,6 +16,7 @@ import org.opensearch.action.index.IndexResponse; import org.opensearch.action.support.WriteRequest; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.cluster.metadata.RepositoriesMetadata; import org.opensearch.common.UUIDs; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; @@ -25,6 +26,7 @@ import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.test.OpenSearchIntegTestCase; import org.junit.After; +import org.junit.Before; import java.io.IOException; import java.nio.file.FileVisitResult; @@ -36,6 +38,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreNode.REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX; @@ -114,13 +117,10 @@ protected boolean addMockInternalEngine() { @Override protected Settings nodeSettings(int nodeOrdinal) { - if (nodeAttributesSettings == null) { - nodeAttributesSettings = remoteStoreNodeAttributes(REPOSITORY_NAME, REPOSITORY_2_NAME); - } return Settings.builder() .put(super.nodeSettings(nodeOrdinal)) .put(remoteStoreClusterSettings(REPOSITORY_NAME, REPOSITORY_2_NAME, true)) - .put(nodeAttributesSettings) + .put(remoteStoreNodeAttributes(REPOSITORY_NAME, REPOSITORY_2_NAME)) .build(); } @@ -192,12 +192,15 @@ public static Settings remoteStoreClusterSettings(String segmentRepoName, String } public Settings remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName) { + if (nodeAttributesSettings != null) { + return nodeAttributesSettings; + } absolutePath = randomRepoPath().toAbsolutePath(); absolutePath2 = randomRepoPath().toAbsolutePath(); if (segmentRepoName.equals(translogRepoName)) { absolutePath2 = absolutePath; } - return Settings.builder() + nodeAttributesSettings = Settings.builder() .put("node.attr." + REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, segmentRepoName) .put( String.format(Locale.getDefault(), "node.attr." + REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT, segmentRepoName), @@ -219,6 +222,7 @@ public Settings remoteStoreNodeAttributes(String segmentRepoName, String translo absolutePath2.toString() ) .build(); + return nodeAttributesSettings; } private Settings defaultIndexSettings() { @@ -260,18 +264,9 @@ protected void putRepository(Path path, String repoName) { assertAcked(clusterAdmin().preparePutRepository(repoName).setType("fs").setSettings(Settings.builder().put("location", path))); } - protected void setupRepo() { - setupRepo(true); - } - - protected void setupRepo(boolean startDedicatedClusterManager) { - if (startDedicatedClusterManager) { - internalCluster().startClusterManagerOnlyNode(); - } - absolutePath = randomRepoPath().toAbsolutePath(); - putRepository(absolutePath); - absolutePath2 = randomRepoPath().toAbsolutePath(); - putRepository(absolutePath2, REPOSITORY_2_NAME); + @Before + public void setup() throws Exception { + assertRepositoryMetadataPresentInClusterState(); } @After @@ -299,4 +294,16 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { return filesExisting.get(); } + void assertRepositoryMetadataPresentInClusterState() throws Exception { + assertBusy(() -> { + RepositoriesMetadata repositoriesMetadata = client().admin() + .cluster() + .prepareState() + .get() + .getState() + .metadata() + .custom(RepositoriesMetadata.TYPE); + assertTrue(repositoriesMetadata != null && !repositoriesMetadata.repositories().isEmpty()); + }, 30, TimeUnit.SECONDS); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java index 9a2948861e967..ee7604bea5a80 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java @@ -12,14 +12,16 @@ import org.opensearch.action.admin.indices.recovery.RecoveryResponse; import org.opensearch.action.index.IndexResponse; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.cluster.metadata.RepositoriesMetadata; +import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.cluster.routing.RecoverySource; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; import org.opensearch.indices.recovery.RecoveryState; import org.opensearch.plugins.Plugin; import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.transport.MockTransportService; import org.hamcrest.MatcherAssert; -import org.junit.Before; import java.nio.file.Path; import java.util.Arrays; @@ -35,7 +37,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.oneOf; -@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE, numDataNodes = 0) +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class RemoteStoreIT extends RemoteStoreBaseIntegTestCase { protected final String INDEX_NAME = "remote-store-test-idx-1"; @@ -45,18 +47,13 @@ protected Collection> nodePlugins() { return Arrays.asList(MockTransportService.TestPlugin.class); } - @Before - public void setup() { - setupRepo(); - } - @Override public Settings indexSettings() { return remoteStoreIndexSettings(0); } private void testPeerRecovery(int numberOfIterations, boolean invokeFlush) throws Exception { - internalCluster().startDataOnlyNodes(3); + internalCluster().startNodes(3); createIndex(INDEX_NAME, remoteStoreIndexSettings(0)); ensureYellowAndNoInitializingShards(INDEX_NAME); ensureGreen(INDEX_NAME); @@ -116,8 +113,8 @@ public void testPeerRecoveryWithRemoteStoreAndRemoteTranslogRefresh() throws Exc testPeerRecovery(randomIntBetween(2, 5), false); } - private void verifyRemoteStoreCleanup() throws Exception { - internalCluster().startDataOnlyNodes(3); + public void verifyRemoteStoreCleanup() throws Exception { + internalCluster().startNodes(3); createIndex(INDEX_NAME, remoteStoreIndexSettings(1)); indexData(5, randomBoolean(), INDEX_NAME); @@ -143,7 +140,7 @@ public void testRemoteTranslogCleanup() throws Exception { } public void testStaleCommitDeletionWithInvokeFlush() throws Exception { - internalCluster().startDataOnlyNodes(1); + internalCluster().startNode(); createIndex(INDEX_NAME, remoteStoreIndexSettings(1, 10000l, -1)); int numberOfIterations = randomIntBetween(5, 15); indexData(numberOfIterations, true, INDEX_NAME); @@ -170,7 +167,7 @@ public void testStaleCommitDeletionWithInvokeFlush() throws Exception { } public void testStaleCommitDeletionWithoutInvokeFlush() throws Exception { - internalCluster().startDataOnlyNodes(1); + internalCluster().startNode(); createIndex(INDEX_NAME, remoteStoreIndexSettings(1, 10000l, -1)); int numberOfIterations = randomIntBetween(5, 15); indexData(numberOfIterations, false, INDEX_NAME); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java index 87f1b83ae0dd1..8071c9328187f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java @@ -26,7 +26,7 @@ import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreNode.REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX; import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreNode.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT; -@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE, numDataNodes = 0) +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class RemoteStoreRepositoryRegistrationIT extends RemoteStoreBaseIntegTestCase { @Override @@ -51,7 +51,7 @@ private RepositoryMetadata buildRepositoryMetadata(DiscoveryNode node, String na return new RepositoryMetadata(name, type, settings.build()); } - private void assertRemoteStoreRepositoryOnAllNodes() { + private void assertRemoteStoreRepositoryOnAllNodes() throws Exception { RepositoriesMetadata repositories = internalCluster().getInstance(ClusterService.class, internalCluster().getNodeNames()[0]) .state() .metadata() @@ -69,34 +69,19 @@ private void assertRemoteStoreRepositoryOnAllNodes() { } } - public void testSingleNodeClusterRepositoryRegistration() { - internalCluster().startClusterManagerOnlyNode(remoteStoreNodeAttributes(REPOSITORY_NAME, REPOSITORY_2_NAME)); - ensureStableCluster(1); - + public void testSingleNodeClusterRepositoryRegistration() throws Exception { + internalCluster().startNode(); assertRemoteStoreRepositoryOnAllNodes(); } - public void testMultiNodeClusterRepositoryRegistration() { - Settings clusterSettings = remoteStoreNodeAttributes(REPOSITORY_NAME, REPOSITORY_2_NAME); - internalCluster().startClusterManagerOnlyNode(clusterSettings); - internalCluster().startNodes(3, clusterSettings); - ensureStableCluster(4); - - assertRemoteStoreRepositoryOnAllNodes(); - } - - public void testMultiNodeClusterOnlyDataRepositoryRegistration() { - Settings clusterSettings = remoteStoreNodeAttributes(REPOSITORY_NAME, REPOSITORY_2_NAME); - internalCluster().startNodes(3, clusterSettings); - ensureStableCluster(3); - + public void testMultiNodeClusterRepositoryRegistration() throws Exception { + internalCluster().startNodes(3); assertRemoteStoreRepositoryOnAllNodes(); } - public void testMultiNodeClusterRepositoryRegistrationWithMultipleMasters() { + public void testMultiNodeClusterRepositoryRegistrationWithMultipleMasters() throws Exception { internalCluster().startClusterManagerOnlyNodes(3); internalCluster().startNodes(3); - ensureStableCluster(6); assertRemoteStoreRepositoryOnAllNodes(); } diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java index 0fef8389063a1..9309771ed6fb9 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java @@ -35,10 +35,10 @@ public class RemoteStoreService { private static final Logger logger = LogManager.getLogger(RemoteStoreService.class); private final Supplier repositoriesService; private final ThreadPool threadPool; - public static final Setting REMOTE_STORE_COMPATIBILITY_MODE_SETTING = Setting.simpleString( + public static final Setting REMOTE_STORE_COMPATIBILITY_MODE_SETTING = new Setting<>( "remote_store.compatibility_mode", - CompatibilityMode.STRICT.value, - CompatibilityMode::validate, + CompatibilityMode.STRICT.name(), + CompatibilityMode::parseString, Setting.Property.Dynamic, Setting.Property.NodeScope ); @@ -52,7 +52,13 @@ public enum CompatibilityMode { STRICT("strict"), ALLOW_MIX("allow_mix"); - public static CompatibilityMode validate(String compatibilityMode) { + public final String mode; + + CompatibilityMode(String mode) { + this.mode = mode; + } + + public static CompatibilityMode parseString(String compatibilityMode) { try { return CompatibilityMode.valueOf(compatibilityMode.toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { @@ -66,12 +72,6 @@ public static CompatibilityMode validate(String compatibilityMode) { ); } } - - public final String value; - - CompatibilityMode(String value) { - this.value = value; - } } public RemoteStoreService(Supplier repositoriesService, ThreadPool threadPool) { diff --git a/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java b/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java index 11830772d662b..53b25970a1876 100644 --- a/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java +++ b/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java @@ -61,6 +61,7 @@ import java.util.function.BiConsumer; import java.util.stream.Collectors; +import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreService.CompatibilityMode; import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreService.CompatibilityMode.STRICT; import static org.opensearch.action.admin.cluster.remotestore.RemoteStoreService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING; import static org.opensearch.cluster.decommission.DecommissionHelper.nodeCommissioned; @@ -186,8 +187,11 @@ public ClusterTasksResult execute(ClusterState currentState, List jo final DiscoveryNode node = joinTask.node(); if (joinTask.isBecomeClusterManagerTask() || joinTask.isFinishElectionTask()) { // noop - } else if (currentNodes.nodeExistsWithSameRoles(node)) { - logger.debug("received a join request for an existing node [{}]", node); + } else if (currentNodes.nodeExists(node)) { + if (currentNodes.nodeExistsWithSameRoles(node)) { + logger.debug("received a join request for an existing node [{}]", node); + } + if (node.isRemoteStoreNode()) { /** cluster state is updated here as elect leader task can have same node present in join task as * well as current node. We want the repositories to be added in cluster state during first node @@ -477,8 +481,8 @@ public static void ensureRemoteStoreNodesCompatibility(DiscoveryNode joiningNode // TODO: The below check is valid till we support migration, once we start supporting migration a remote // store node will be able to join a non remote store cluster and vice versa. #7986 - String remoteStoreCompatibilityMode = REMOTE_STORE_COMPATIBILITY_MODE_SETTING.get(currentState.metadata().settings()); - if (STRICT.value.equals(remoteStoreCompatibilityMode)) { + CompatibilityMode remoteStoreCompatibilityMode = REMOTE_STORE_COMPATIBILITY_MODE_SETTING.get(currentState.metadata().settings()); + if (STRICT.equals(remoteStoreCompatibilityMode)) { DiscoveryNode existingNode = existingNodes.get(0); if (joiningNode.isRemoteStoreNode()) { if (existingNode.isRemoteStoreNode()) { diff --git a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java index 62bc4016d892d..b71bc0778ad47 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java @@ -306,6 +306,9 @@ public void verify(String verificationToken, DiscoveryNode localNode) { } + @Override + public void verifyLocally(DiscoveryNode localNode) {} + @Override public boolean isReadOnly() { return false;