Address comments

Signed-off-by: Lakshya Taragi <[email protected]>

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

@@ -8,10 +8,8 @@
 
 package org.opensearch.remotemigration;
 
-import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
 import org.opensearch.client.Client;
 import org.opensearch.common.settings.Settings;
-import org.opensearch.core.rest.RestStatus;
 import org.opensearch.index.IndexSettings;
 import org.opensearch.indices.replication.common.ReplicationType;
 import org.opensearch.test.OpenSearchIntegTestCase;
@@ -27,7 +25,6 @@
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.STRICT;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.Direction.NONE;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.Direction.REMOTE_STORE;
-import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.TEST_INDEX;
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.assertNodeInCluster;
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.prepareIndexWithoutReplica;
 import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.setClusterMode;
@@ -98,21 +95,6 @@ private void assertRemoteStoreBackedIndex(String indexName) {
         );
     }
 
-    // restore indices from a snapshot
-    private void restoreSnapshot(String snapshotRepoName, String snapshotName, String restoredIndexName) {
-        RestoreSnapshotResponse restoreSnapshotResponse = client.admin()
-            .cluster()
-            .prepareRestoreSnapshot(snapshotRepoName, snapshotName)
-            .setWaitForCompletion(false)
-            .setIndices(TEST_INDEX)
-            .setRenamePattern(TEST_INDEX)
-            .setRenameReplacement(restoredIndexName)
-            .get();
-
-        assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED);
-        ensureGreen(restoredIndexName);
-    }
-
     private void initializeCluster(boolean remoteClusterManager) {
         addRemote = remoteClusterManager;
         internalCluster().setBootstrapClusterManagerNodeIndex(0);

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

@@ -319,8 +319,6 @@ 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";

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

@@ -101,7 +101,6 @@
 import org.opensearch.indices.ShardLimitValidator;
 import org.opensearch.indices.SystemIndices;
 import org.opensearch.indices.replication.common.ReplicationType;
-import org.opensearch.node.Node;
 import org.opensearch.node.remotestore.RemoteStoreNodeAttribute;
 import org.opensearch.threadpool.ThreadPool;
 
@@ -947,8 +946,7 @@ static Settings aggregateIndexSettings(
         indexSettingsBuilder.put(SETTING_INDEX_UUID, UUIDs.randomBase64UUID());
 
         updateReplicationStrategy(indexSettingsBuilder, request.settings(), settings, combinedTemplateSettings);
-        updateRemoteStoreSettings(indexSettingsBuilder, settings);
-        updateRemoteStoreSettingsForMigration(indexSettingsBuilder, currentState, clusterSettings, request.index());
+        updateRemoteStoreSettings(indexSettingsBuilder, currentState, clusterSettings, settings, request.index());
 
         if (sourceMetadata != null) {
             assert request.resizeType() != null;
@@ -1026,51 +1024,28 @@ private static void updateReplicationStrategy(
     /**
      * Updates index settings to enable remote store by default based on node attributes
      * @param settingsBuilder index settings builder to be updated with relevant settings
-     * @param clusterSettings cluster level settings
-     */
-    public static void updateRemoteStoreSettings(Settings.Builder settingsBuilder, Settings clusterSettings) {
-        if (isRemoteDataAttributePresent(clusterSettings)) {
-            settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true)
-                .put(
-                    SETTING_REMOTE_SEGMENT_STORE_REPOSITORY,
-                    clusterSettings.get(
-                        Node.NODE_ATTRIBUTES.getKey() + RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY
-                    )
-                )
-                .put(
-                    SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY,
-                    clusterSettings.get(
-                        Node.NODE_ATTRIBUTES.getKey() + RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY
-                    )
-                );
-        }
-    }
-
-    /**
-     * Updates index settings to enable remote store by default for migration towards remote store backed clusters
-     * @param settingsBuilder index settings builder to be updated with relevant settings
      * @param clusterState state of cluster
      * @param clusterSettings cluster level settings
+     * @param nodeSettings node level settings
+     * @param indexName name of index
      */
-    public static void updateRemoteStoreSettingsForMigration(
+    public static void updateRemoteStoreSettings(
         Settings.Builder settingsBuilder,
         ClusterState clusterState,
         ClusterSettings clusterSettings,
+        Settings nodeSettings,
         String indexName
     ) {
-        String value = settingsBuilder.get(SETTING_REMOTE_STORE_ENABLED);
-        if (value != null && value.toLowerCase(Locale.ROOT).equals("true")) {
-            return;
-        }
-
-        if (isMigratingToRemoteStore(clusterSettings)) {
+        if (isRemoteDataAttributePresent(nodeSettings) || isMigratingToRemoteStore(clusterSettings)) {
             String segmentRepo, translogRepo;
+
             Optional<DiscoveryNode> remoteNode = clusterState.nodes()
                 .getNodes()
                 .values()
                 .stream()
                 .filter(DiscoveryNode::isRemoteStoreNode)
                 .findFirst();
+
             if (remoteNode.isPresent()) {
                 translogRepo = remoteNode.get()
                     .getAttributes()

server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java

@@ -236,6 +236,6 @@ public static boolean isMigratingToRemoteStore(ClusterSettings clusterSettings)
         boolean isMixedMode = clusterSettings.get(REMOTE_STORE_COMPATIBILITY_MODE_SETTING).equals(CompatibilityMode.MIXED);
         boolean isRemoteStoreMigrationDirection = clusterSettings.get(MIGRATION_DIRECTION_SETTING).equals(Direction.REMOTE_STORE);
 
-        return (isMixedMode == true && isRemoteStoreMigrationDirection == true);
+        return (isMixedMode && isRemoteStoreMigrationDirection);
     }
 }

server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java

@@ -922,16 +922,16 @@ private DiscoveryNode newDiscoveryNode(Map<String, String> attributes) {
         );
     }
 
-    private static final String SEGMENT_REPO = "segment-repo";
-    private static final String TRANSLOG_REPO = "translog-repo";
+    public static final String SEGMENT_REPO = "segment-repo";
+    public static final String TRANSLOG_REPO = "translog-repo";
     private static final String CLUSTER_STATE_REPO = "cluster-state-repo";
     private static final String COMMON_REPO = "remote-repo";
 
-    private Map<String, String> remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName) {
+    public static Map<String, String> remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName) {
         return remoteStoreNodeAttributes(segmentRepoName, translogRepoName, CLUSTER_STATE_REPO);
     }
 
-    private Map<String, String> remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName, String clusterStateRepo) {
+    private static Map<String, String> remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName, String clusterStateRepo) {
         String segmentRepositoryTypeAttributeKey = String.format(
             Locale.getDefault(),
             REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT,
@@ -968,7 +968,7 @@ private Map<String, String> remoteStoreNodeAttributes(String segmentRepoName, St
         };
     }
 
-    private Map<String, String> remoteStateNodeAttributes(String clusterStateRepo) {
+    private static Map<String, String> remoteStateNodeAttributes(String clusterStateRepo) {
         String clusterStateRepositoryTypeAttributeKey = String.format(
             Locale.getDefault(),
             REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT,

server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java

@@ -121,6 +121,8 @@
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singleton;
 import static java.util.Collections.singletonList;
+import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.SEGMENT_REPO;
+import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.TRANSLOG_REPO;
 import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING;
 import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING;
 import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_READ_ONLY_BLOCK;
@@ -139,6 +141,8 @@
 import static org.opensearch.cluster.metadata.MetadataCreateIndexService.getIndexNumberOfRoutingShards;
 import static org.opensearch.cluster.metadata.MetadataCreateIndexService.parseV1Mappings;
 import static org.opensearch.cluster.metadata.MetadataCreateIndexService.resolveAndValidateAliases;
+import static org.opensearch.cluster.routing.allocation.RemoteStoreMigrationAllocationDeciderTests.getNonRemoteNode;
+import static org.opensearch.cluster.routing.allocation.RemoteStoreMigrationAllocationDeciderTests.getRemoteNode;
 import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE_MIGRATION_EXPERIMENTAL;
 import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING;
 import static org.opensearch.index.IndexSettings.INDEX_REFRESH_INTERVAL_SETTING;
@@ -1355,18 +1359,20 @@ public void testClusterForceReplicationTypeInValidateIndexSettings() {
     }
 
     public void testRemoteStoreNoUserOverrideExceptReplicationTypeSegmentIndexSettings() {
+        ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT)
+            .nodes(DiscoveryNodes.builder().add(getRemoteNode()).build())
+            .build();
         Settings settings = Settings.builder()
             .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT)
-            .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1")
-            .put(translogRepositoryNameAttributeKey, "my-translog-repo-1")
+            .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO)
+            .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO)
             .build();
-
         request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");
         final Settings.Builder requestSettings = Settings.builder();
         requestSettings.put(SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT);
         request.settings(requestSettings.build());
         Settings indexSettings = aggregateIndexSettings(
-            ClusterState.EMPTY_STATE,
+            clusterState,
             request,
             Settings.EMPTY,
             null,
@@ -1379,24 +1385,27 @@ public void testRemoteStoreNoUserOverrideExceptReplicationTypeSegmentIndexSettin
         verifyRemoteStoreIndexSettings(
             indexSettings,
             "true",
-            "my-segment-repo-1",
-            "my-translog-repo-1",
+            SEGMENT_REPO,
+            TRANSLOG_REPO,
             ReplicationType.SEGMENT.toString(),
             IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
         );
     }
 
     public void testRemoteStoreImplicitOverrideReplicationTypeToSegmentForRemoteStore() {
+        ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT)
+            .nodes(DiscoveryNodes.builder().add(getRemoteNode()).build())
+            .build();
         Settings settings = Settings.builder()
-            .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1")
-            .put(translogRepositoryNameAttributeKey, "my-translog-repo-1")
+            .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO)
+            .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO)
             .build();
 
         request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");
         final Settings.Builder requestSettings = Settings.builder();
         request.settings(requestSettings.build());
         Settings indexSettings = aggregateIndexSettings(
-            ClusterState.EMPTY_STATE,
+            clusterState,
             request,
             Settings.EMPTY,
             null,
@@ -1409,23 +1418,26 @@ public void testRemoteStoreImplicitOverrideReplicationTypeToSegmentForRemoteStor
         verifyRemoteStoreIndexSettings(
             indexSettings,
             "true",
-            "my-segment-repo-1",
-            "my-translog-repo-1",
+            SEGMENT_REPO,
+            TRANSLOG_REPO,
             ReplicationType.SEGMENT.toString(),
             IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
         );
     }
 
     public void testRemoteStoreNoUserOverrideIndexSettings() {
+        ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT)
+            .nodes(DiscoveryNodes.builder().add(getRemoteNode()).build())
+            .build();
         Settings settings = Settings.builder()
             .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
-            .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1")
-            .put(translogRepositoryNameAttributeKey, "my-translog-repo-1")
+            .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO)
+            .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO)
             .build();
 
         request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");
         Settings indexSettings = aggregateIndexSettings(
-            ClusterState.EMPTY_STATE,
+            clusterState,
             request,
             Settings.EMPTY,
             null,
@@ -1438,8 +1450,8 @@ public void testRemoteStoreNoUserOverrideIndexSettings() {
         verifyRemoteStoreIndexSettings(
             indexSettings,
             "true",
-            "my-segment-repo-1",
-            "my-translog-repo-1",
+            SEGMENT_REPO,
+            TRANSLOG_REPO,
             ReplicationType.SEGMENT.toString(),
             IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
         );
@@ -1561,13 +1573,7 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
         FeatureFlags.initializeFeatureFlags(Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build());
 
         // non-remote cluster manager node
-        DiscoveryNode nonRemoteClusterManagerNode = new DiscoveryNode(
-            UUIDs.base64UUID(),
-            buildNewFakeTransportAddress(),
-            emptyMap(),
-            singleton(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE),
-            Version.CURRENT
-        );
+        DiscoveryNode nonRemoteClusterManagerNode = getNonRemoteNode();
 
         DiscoveryNodes discoveryNodes = DiscoveryNodes.builder()
             .add(nonRemoteClusterManagerNode)
@@ -1602,16 +1608,7 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
         );
 
         // remote data node
-        Map<String, String> attributes = new HashMap<>();
-        attributes.put(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, "my-segment-repo-1");
-        attributes.put(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY, "my-translog-repo-1");
-        DiscoveryNode remoteDataNode = new DiscoveryNode(
-            UUIDs.base64UUID(),
-            buildNewFakeTransportAddress(),
-            attributes,
-            Set.of(DiscoveryNodeRole.INGEST_ROLE, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE, DiscoveryNodeRole.DATA_ROLE),
-            Version.CURRENT
-        );
+        DiscoveryNode remoteDataNode = getRemoteNode();
 
         discoveryNodes = DiscoveryNodes.builder(discoveryNodes).add(remoteDataNode).localNodeId(remoteDataNode.getId()).build();
 
@@ -1639,8 +1636,8 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
         verifyRemoteStoreIndexSettings(
             indexSettings,
             "true",
-            "my-segment-repo-1",
-            "my-translog-repo-1",
+            SEGMENT_REPO,
+            TRANSLOG_REPO,
             ReplicationType.SEGMENT.toString(),
             IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
         );

server/src/test/java/org/opensearch/cluster/routing/allocation/RemoteStoreMigrationAllocationDeciderTests.java

@@ -60,16 +60,16 @@
 import org.opensearch.node.remotestore.RemoteStoreNodeService;
 
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Locale;
-import java.util.Map;
 
+import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.SEGMENT_REPO;
+import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.TRANSLOG_REPO;
+import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.remoteStoreNodeAttributes;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE;
 import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE_MIGRATION_EXPERIMENTAL;
-import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_KEY;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.MIGRATION_DIRECTION_SETTING;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING;
 import static org.hamcrest.core.Is.is;
@@ -659,21 +659,16 @@ private ClusterState getInitialClusterState(
     }
 
     // get a dummy non-remote node
-    private DiscoveryNode getNonRemoteNode() {
+    public static DiscoveryNode getNonRemoteNode() {
         return new DiscoveryNode(UUIDs.base64UUID(), buildNewFakeTransportAddress(), Version.CURRENT);
     }
 
     // get a dummy remote node
-    public DiscoveryNode getRemoteNode() {
-        Map<String, String> attributes = new HashMap<>();
-        attributes.put(
-            REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_KEY,
-            "REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_VALUE"
-        );
+    public static DiscoveryNode getRemoteNode() {
         return new DiscoveryNode(
             UUIDs.base64UUID(),
             buildNewFakeTransportAddress(),
-            attributes,
+            remoteStoreNodeAttributes(SEGMENT_REPO, TRANSLOG_REPO),
             DiscoveryNodeRole.BUILT_IN_ROLES,
             Version.CURRENT
         );