Address comments

Signed-off-by: Lakshya Taragi <lakshya.taragi@gmail.com>

Author: ltaragi

server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java

@@ -50,7 +50,7 @@ public class RemoteStoreMigrationAllocationIT extends MigrationBaseTestCase {
 
     // tests for primary shard copy allocation with MIXED mode and REMOTE_STORE direction
 
-    public void testDontAllocateNewPrimaryShardOnNonRemoteNodeForMixedModeAndRemoteStoreDirection() throws Exception {
+    public void testAllocateNewPrimaryShardForMixedModeAndRemoteStoreDirection() throws Exception {
         logger.info(" --> initialize cluster");
         initializeCluster(false);
 
@@ -71,59 +71,29 @@ public void testDontAllocateNewPrimaryShardOnNonRemoteNodeForMixedModeAndRemoteS
         setDirection(REMOTE_STORE.direction);
 
         Decision decision = getDecisionForTargetNode(nonRemoteNode, true, true, false);
-        Decision.Type type = Decision.Type.NO;
-        assertEquals(type, decision.type());
+        assertEquals(Decision.Type.NO, decision.type());
         assertEquals(
             "[remote_store migration_direction]: primary shard copy can not be allocated to a non-remote node",
             decision.getExplanation().toLowerCase(Locale.ROOT)
         );
 
-        logger.info(" --> attempt allocation");
+        logger.info(" --> attempt allocation on non-remote node");
         attemptAllocation(nonRemoteNodeName);
 
-        logger.info(" --> verify non-allocation of primary shard");
+        logger.info(" --> verify non-allocation of primary shard on non-remote node");
         assertNonAllocation(true);
-    }
-
-    public void testAllocateNewPrimaryShardOnRemoteNodeForMixedModeAndRemoteStoreDirection() throws Exception {
-        logger.info(" --> initialize cluster");
-        initializeCluster(false);
-
-        logger.info(" --> add remote and non-remote nodes");
-        setClusterMode(MIXED.mode);
-        addRemote = true;
-        String remoteNodeName = internalCluster().startNode();
-        addRemote = false;
-        String nonRemoteNodeName = internalCluster().startNode();
-        internalCluster().validateClusterFormed();
-        DiscoveryNode remoteNode = assertNodeInCluster(remoteNodeName);
-        DiscoveryNode nonRemoteNode = assertNodeInCluster(nonRemoteNodeName);
 
         logger.info(" --> verify expected decision for allocating a new primary shard on a remote node");
-        prepareIndexWithoutReplica(Optional.empty());
-
-        logger.info(" --> set remote_store direction");
-        setDirection(REMOTE_STORE.direction);
-
-        Decision decision = getDecisionForTargetNode(remoteNode, true, true, false);
+        prepareDecisions();
+        decision = getDecisionForTargetNode(remoteNode, true, true, false);
         assertEquals(Decision.Type.YES, decision.type());
         assertEquals(
             "[remote_store migration_direction]: primary shard copy can be allocated to a remote node",
             decision.getExplanation().toLowerCase(Locale.ROOT)
         );
 
-        logger.info(" --> attempt allocation");
-        client.admin()
-            .indices()
-            .prepareUpdateSettings(TEST_INDEX)
-            .setSettings(
-                Settings.builder()
-                    .put("index.routing.allocation.include._name", allNodesExcept(null))
-                    .put("index.routing.allocation.exclude._name", "")
-            )
-            .execute()
-            .actionGet();
-
+        logger.info(" --> attempt allocation on remote node");
+        attemptAllocation(remoteNodeName);
         ensureGreen(TEST_INDEX);
 
         logger.info(" --> verify allocation of primary shard");
@@ -236,11 +206,11 @@ public void testAllocateNewReplicaShardOnNonRemoteNodeIfPrimaryShardOnNonRemoteN
         logger.info(" --> verify expected decision for replica shard");
         prepareDecisions();
         Decision decision = getDecisionForTargetNode(nonRemoteNode2, false, true, false);
-        Decision.Type type = Decision.Type.YES;
-        String reason = "[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node";
+        Decision.Type expectedType = Decision.Type.YES;
+        String expectedReason = "[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node";
 
-        assertEquals(type, decision.type());
-        assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
+        assertEquals(expectedType, decision.type());
+        assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));
 
         logger.info(" --> allocate replica shard on the other non-remote node");
         attemptAllocation(nonRemoteNodeName2);
@@ -276,8 +246,8 @@ public void testAllocateNewReplicaShardOnNonRemoteNodeIfPrimaryShardOnRemoteNode
         prepareDecisions();
         Decision decision = getDecisionForTargetNode(nonRemoteNode, false, true, false);
 
-        Decision.Type type = Decision.Type.YES;
-        assertEquals(type, decision.type());
+        Decision.Type expectedType = Decision.Type.YES;
+        assertEquals(expectedType, decision.type());
         assertEquals(
             "[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node",
             decision.getExplanation().toLowerCase(Locale.ROOT)
@@ -352,13 +322,13 @@ public void testAlwaysAllocateNewShardForStrictMode() throws Exception {
         prepareDecisions();
         Decision decision = getDecisionForTargetNode(targetNode, !isReplicaAllocation, true, false);
         assertEquals(Decision.Type.YES, decision.type());
-        String reason = String.format(
+        String expectedReason = String.format(
             Locale.ROOT,
             "[remote_store migration_direction]: %s shard copy can be allocated to a %s node for strict compatibility mode",
             (isReplicaAllocation ? "replica" : "primary"),
             (isRemoteCluster ? "remote" : "non-remote")
         );
-        assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
+        assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));
 
         logger.info(" --> attempt allocation");
         attemptAllocation(targetNode.getName());
@@ -410,12 +380,12 @@ public void testDontAllocateToNonRemoteNodeForRemoteStoreBackedIndex() throws Ex
         prepareDecisions();
         Decision decision = getDecisionForTargetNode(nonRemoteNode, !isReplicaAllocation, false, false);
         assertEquals(Decision.Type.NO, decision.type());
-        String reason = String.format(
+        String expectedReason = String.format(
             Locale.ROOT,
             "[remote_store migration_direction]: %s shard copy can not be allocated to a non-remote node because a remote store backed index's shard copy can only be allocated to a remote node",
             (isReplicaAllocation ? "replica" : "primary")
         );
-        assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
+        assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));
 
         logger.info(" --> attempt allocation of shard on non-remote node");
         attemptAllocation(nonRemoteNodeName);

server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java

@@ -10,18 +10,14 @@
 
 import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
 import org.opensearch.client.Client;
-import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.settings.SettingsException;
 import org.opensearch.core.rest.RestStatus;
 import org.opensearch.index.IndexSettings;
 import org.opensearch.indices.replication.common.ReplicationType;
-import org.opensearch.snapshots.SnapshotInfo;
-import org.opensearch.snapshots.SnapshotState;
 import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
-import java.nio.file.Path;
 import java.util.Optional;
 
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY;
@@ -38,7 +34,6 @@
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.prepareIndexWithoutReplica;
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.setClusterMode;
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.setDirection;
-import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
 
 @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, autoManageMasterNodes = false)
 public class RemoteStoreMigrationSettingsUpdateIT extends MigrationBaseTestCase {
@@ -56,9 +51,9 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
 
         logger.info(" --> add non-remote node");
         addRemote = false;
-        String remoteNodeName = internalCluster().startNode();
+        String nonRemoteNodeName = internalCluster().startNode();
         internalCluster().validateClusterFormed();
-        assertNodeInCluster(remoteNodeName);
+        assertNodeInCluster(nonRemoteNodeName);
 
         logger.info(" --> create an index");
         prepareIndexWithoutReplica(Optional.of(indexName1));
@@ -72,9 +67,9 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
 
         logger.info(" --> add remote node");
         addRemote = true;
-        String nonRemoteNodeName = internalCluster().startNode();
+        String remoteNodeName = internalCluster().startNode();
         internalCluster().validateClusterFormed();
-        assertNodeInCluster(nonRemoteNodeName);
+        assertNodeInCluster(remoteNodeName);
 
         logger.info(" --> create another index");
         prepareIndexWithoutReplica(Optional.of(indexName2));
@@ -83,72 +78,6 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
         assertRemoteStoreBackedIndex(indexName2);
     }
 
-    public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() throws Exception {
-        logger.info(" --> initialize cluster: gives non remote cluster manager");
-        initializeCluster(false);
-
-        logger.info(" --> add remote and non-remote nodes");
-        setClusterMode(MIXED.mode);
-        addRemote = false;
-        String nonRemoteNodeName = internalCluster().startNode();
-        addRemote = true;
-        String remoteNodeName = internalCluster().startNode();
-        internalCluster().validateClusterFormed();
-        assertNodeInCluster(nonRemoteNodeName);
-        assertNodeInCluster(remoteNodeName);
-
-        logger.info(" --> create a non remote-backed index");
-        client.admin()
-            .indices()
-            .prepareCreate(TEST_INDEX)
-            .setSettings(
-                Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0).build()
-            )
-            .get();
-
-        logger.info(" --> verify that non remote stored backed index is created");
-        assertNonRemoteStoreBackedIndex(TEST_INDEX);
-
-        logger.info(" --> create repository");
-        String snapshotName = "test-snapshot";
-        String snapshotRepoName = "test-restore-snapshot-repo";
-        Path snapshotRepoNameAbsolutePath = randomRepoPath().toAbsolutePath();
-        assertAcked(
-            clusterAdmin().preparePutRepository(snapshotRepoName)
-                .setType("fs")
-                .setSettings(Settings.builder().put("location", snapshotRepoNameAbsolutePath))
-        );
-
-        logger.info(" --> create snapshot of non remote stored backed index");
-
-        SnapshotInfo snapshotInfo = client().admin()
-            .cluster()
-            .prepareCreateSnapshot(snapshotRepoName, snapshotName)
-            .setIndices(TEST_INDEX)
-            .setWaitForCompletion(true)
-            .get()
-            .getSnapshotInfo();
-
-        assertEquals(SnapshotState.SUCCESS, snapshotInfo.state());
-        assertTrue(snapshotInfo.successfulShards() > 0);
-        assertEquals(0, snapshotInfo.failedShards());
-
-        logger.info(" --> restore index from snapshot under NONE direction");
-        String restoredIndexName1 = TEST_INDEX + "-restored1";
-        restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName1);
-
-        logger.info(" --> verify that restored index is non remote-backed");
-        assertNonRemoteStoreBackedIndex(restoredIndexName1);
-
-        logger.info(" --> restore index from snapshot under REMOTE_STORE direction");
-        setDirection(REMOTE_STORE.direction);
-        String restoredIndexName2 = TEST_INDEX + "-restored2";
-        restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName2);
-
-        logger.info(" --> verify that restored index is non remote-backed");
-        assertRemoteStoreBackedIndex(restoredIndexName2);
-    }
-
     // compatibility mode setting test
 
     public void testSwitchToStrictMode() throws Exception {

server/src/main/java/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java

@@ -61,7 +61,9 @@
 import org.opensearch.transport.TransportService;
 
 import java.io.IOException;
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
 
 /**
  * Transport action for updating cluster settings
@@ -268,21 +270,22 @@ public ClusterState execute(final ClusterState currentState) {
         );
     }
 
-    private void validateCompatibilityModeSettingRequest(ClusterUpdateSettingsRequest request, ClusterState state) {
+    /**
+     * Verifies that while trying to switch to STRICT compatibility mode, all nodes must be of the
+     * same type (all remote or all non-remote). If not, it throws SettingsException error
+     * @param request cluster settings update request, for settings to be updated and new values
+     * @param clusterState current state of cluster, for information on nodes
+     */
+    private void validateCompatibilityModeSettingRequest(ClusterUpdateSettingsRequest request, ClusterState clusterState) {
         if (RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.exists(request.persistentSettings())) {
             String value = request.persistentSettings().get(RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey());
             if (value.equals(RemoteStoreNodeService.CompatibilityMode.STRICT.mode)) {
-                boolean hasRemoteNode = false, hasNonRemoteNode = false;
-                Map<String, DiscoveryNode> nodes = state.nodes().getNodes();
-                for (Map.Entry<String, DiscoveryNode> entry : nodes.entrySet()) {
-                    DiscoveryNode node = entry.getValue();
-                    if (node.isRemoteStoreNode()) {
-                        hasRemoteNode = true;
-                        continue;
-                    }
-                    hasNonRemoteNode = true;
-                }
-                if (hasRemoteNode && hasNonRemoteNode) {
+                List<DiscoveryNode> discoveryNodeList = new ArrayList<>(clusterState.nodes().getNodes().values());
+                Optional<DiscoveryNode> remoteNode = discoveryNodeList.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst();
+                Optional<DiscoveryNode> nonRemoteNode = discoveryNodeList.stream()
+                    .filter(dn -> dn.isRemoteStoreNode() == false)
+                    .findFirst();
+                if (remoteNode.isPresent() && nonRemoteNode.isPresent()) {
                     throw new SettingsException(
                         "can not switch to STRICT compatibility mode when the cluster contains both remote and non-remote nodes"
                     );

server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java

@@ -319,6 +319,8 @@ public Iterator<Setting<?>> settings() {
         Property.Final
     );
 
+    public static final String SETTING_REMOTE_STORE_PREFIX = "index.remote_store.";
+
     public static final String SETTING_REMOTE_STORE_ENABLED = "index.remote_store.enabled";
 
     public static final String SETTING_REMOTE_SEGMENT_STORE_REPOSITORY = "index.remote_store.segment.repository";