Address comments

Signed-off-by: Lakshya Taragi <[email protected]>
opensearch-project · Apr 24, 2024 · aa7df1a · aa7df1a
1 parent 1af4108
commit aa7df1a
Show file tree

Hide file tree

Showing 4 changed files with 153 additions and 129 deletions.
diff --git a/...ClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java b/...ClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java
@@ -8,28 +8,15 @@
 
 package org.opensearch.remotemigration;
 
-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.repositories.blobstore.BlobStoreRepository;
-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;
-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.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.MIXED;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.STRICT;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.Direction.REMOTE_STORE;
@@ -110,11 +97,12 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix
 
         logger.info("Create snapshot of non remote stored backed index");
 
-        createSnapshot(snapshotRepoName, snapshotName);
+        createSnapshot(snapshotRepoName, snapshotName, TEST_INDEX);
 
         logger.info("Restore index from snapshot under NONE direction");
         String restoredIndexName1 = TEST_INDEX + "-restored1";
         restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName1);
+        ensureGreen(restoredIndexName1);
 
         logger.info("Verify that restored index is non remote-backed");
         assertNonRemoteStoreBackedIndex(restoredIndexName1);
@@ -123,6 +111,7 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix
         setDirection(REMOTE_STORE.direction);
         String restoredIndexName2 = TEST_INDEX + "-restored2";
         restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName2);
+        ensureGreen(restoredIndexName2);
 
         logger.info("Verify that restored index is non remote-backed");
         assertRemoteStoreBackedIndex(restoredIndexName2);
@@ -159,116 +148,6 @@ public void testSwitchToStrictMode() throws Exception {
         setClusterMode(STRICT.mode);
     }
 
-    public void testNoShallowSnapshotInMixedMode() throws Exception {
-        logger.info("Initialize remote cluster");
-        initializeCluster(true);
-
-        logger.info("Add remote node");
-        String nodeName = internalCluster().startNode();
-        internalCluster().validateClusterFormed();
-        assertNodeInCluster(nodeName);
-
-        logger.info("Create remote backed index");
-        createIndex(TEST_INDEX, 0);
-        assertRemoteStoreBackedIndex(TEST_INDEX);
-
-        logger.info("Create shallow snapshot setting enabled repo");
-        String shallowSnapshotRepoName = "shallow-snapshot-repo-name";
-        Path shallowSnapshotRepoPath = randomRepoPath();
-        assertAcked(
-            clusterAdmin().preparePutRepository(shallowSnapshotRepoName)
-                .setType("fs")
-                .setSettings(
-                    Settings.builder()
-                        .put("location", shallowSnapshotRepoPath)
-                        .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), Boolean.TRUE)
-                )
-        );
-
-        logger.info("Verify shallow snapshot creation");
-        final String snapshot1 = "snapshot1";
-        SnapshotInfo snapshotInfo1 = createSnapshot(shallowSnapshotRepoName, snapshot1);
-        assertEquals(snapshotInfo1.isRemoteStoreIndexShallowCopyEnabled(), true);
-
-        logger.info("Set MIXED compatibility mode");
-        setClusterMode(MIXED.mode);
-
-        logger.info("Verify that new snapshot is not shallow");
-        final String snapshot2 = "snapshot2";
-        SnapshotInfo snapshotInfo2 = createSnapshot(shallowSnapshotRepoName, snapshot2);
-        assertEquals(snapshotInfo2.isRemoteStoreIndexShallowCopyEnabled(), false);
-    }
-
-    // create a snapshot
-    private SnapshotInfo createSnapshot(String snapshotRepoName, String snapshotName) {
-        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());
-        return snapshotInfo;
-    }
-
-    // create new index
-    private void createIndex(String indexName, int replicaCount) {
-        assertAcked(
-            internalCluster().client()
-                .admin()
-                .indices()
-                .prepareCreate(indexName)
-                .setSettings(
-                    Settings.builder()
-                        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
-                        .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, replicaCount)
-                        .build()
-                )
-                .get()
-        );
-    }
-
-    // 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);
-    }
-
-    // verify that the created index is not remote store backed
-    private void assertNonRemoteStoreBackedIndex(String indexName) {
-        Settings indexSettings = client.admin().indices().prepareGetIndex().execute().actionGet().getSettings().get(indexName);
-        assertEquals(ReplicationType.DOCUMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
-        assertNull(indexSettings.get(SETTING_REMOTE_STORE_ENABLED));
-        assertNull(indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY));
-        assertNull(indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY));
-    }
-
-    // verify that the created index is remote store backed
-    private void assertRemoteStoreBackedIndex(String indexName) {
-        Settings indexSettings = client.admin().indices().prepareGetIndex().execute().actionGet().getSettings().get(indexName);
-        assertEquals(ReplicationType.SEGMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
-        assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED));
-        assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY));
-        assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY));
-        assertEquals(
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL,
-            INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING.get(indexSettings)
-        );
-    }
-
     // bootstrap a cluster
     private void initializeCluster(boolean remoteClusterManager) {
         addRemote = remoteClusterManager;

diff --git a/.../java/org/opensearch/remotemigration/RemoteStoreMigrationShardAllocationBaseTestCase.java b/.../java/org/opensearch/remotemigration/RemoteStoreMigrationShardAllocationBaseTestCase.java
@@ -9,15 +9,27 @@
 package org.opensearch.remotemigration;
 
 import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
+import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
+import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.cluster.routing.IndexShardRoutingTable;
 import org.opensearch.cluster.routing.ShardRouting;
 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.snapshots.SnapshotInfo;
+import org.opensearch.snapshots.SnapshotState;
 
 import java.util.Map;
 import java.util.Optional;
 
+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.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
 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.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
@@ -98,4 +110,88 @@ protected ShardRouting getShardRouting(boolean isPrimary) {
         return (isPrimary ? table.primaryShard() : table.replicaShards().get(0));
     }
 
+    // create a snapshot
+    public static SnapshotInfo createSnapshot(String snapshotRepoName, String snapshotName, String... indices) {
+        SnapshotInfo snapshotInfo = internalCluster().client()
+            .admin()
+            .cluster()
+            .prepareCreateSnapshot(snapshotRepoName, snapshotName)
+            .setIndices(indices)
+            .setWaitForCompletion(true)
+            .get()
+            .getSnapshotInfo();
+
+        assertEquals(SnapshotState.SUCCESS, snapshotInfo.state());
+        assertTrue(snapshotInfo.successfulShards() > 0);
+        assertEquals(0, snapshotInfo.failedShards());
+        return snapshotInfo;
+    }
+
+    // create new index
+    public static void createIndex(String indexName, int replicaCount) {
+        assertAcked(
+            internalCluster().client()
+                .admin()
+                .indices()
+                .prepareCreate(indexName)
+                .setSettings(
+                    Settings.builder()
+                        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                        .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, replicaCount)
+                        .build()
+                )
+                .get()
+        );
+    }
+
+    // restore indices from a snapshot
+    public static RestoreSnapshotResponse restoreSnapshot(String snapshotRepoName, String snapshotName, String restoredIndexName) {
+        RestoreSnapshotResponse restoreSnapshotResponse = internalCluster().client()
+            .admin()
+            .cluster()
+            .prepareRestoreSnapshot(snapshotRepoName, snapshotName)
+            .setWaitForCompletion(false)
+            .setIndices(TEST_INDEX)
+            .setRenamePattern(TEST_INDEX)
+            .setRenameReplacement(restoredIndexName)
+            .get();
+        assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED);
+        return restoreSnapshotResponse;
+    }
+
+    // verify that the created index is not remote store backed
+    public static void assertNonRemoteStoreBackedIndex(String indexName) {
+        Settings indexSettings = internalCluster().client()
+            .admin()
+            .indices()
+            .prepareGetIndex()
+            .execute()
+            .actionGet()
+            .getSettings()
+            .get(indexName);
+        assertEquals(ReplicationType.DOCUMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
+        assertNull(indexSettings.get(SETTING_REMOTE_STORE_ENABLED));
+        assertNull(indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY));
+        assertNull(indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY));
+    }
+
+    // verify that the created index is remote store backed
+    public static void assertRemoteStoreBackedIndex(String indexName) {
+        Settings indexSettings = internalCluster().client()
+            .admin()
+            .indices()
+            .prepareGetIndex()
+            .execute()
+            .actionGet()
+            .getSettings()
+            .get(indexName);
+        assertEquals(ReplicationType.SEGMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
+        assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED));
+        assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY));
+        assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY));
+        assertEquals(
+            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL,
+            INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING.get(indexSettings)
+        );
+    }
 }
diff --git a/...internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java b/...internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java
@@ -13,8 +13,11 @@
 import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
 import org.opensearch.client.Client;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.repositories.blobstore.BlobStoreRepository;
+import org.opensearch.snapshots.SnapshotInfo;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
+import java.nio.file.Path;
 import java.util.List;
 
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.MIGRATION_DIRECTION_SETTING;
@@ -70,4 +73,52 @@ public void testMigrationDirections() {
         updateSettingsRequest.persistentSettings(Settings.builder().put(MIGRATION_DIRECTION_SETTING.getKey(), "random"));
         assertThrows(IllegalArgumentException.class, () -> client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
     }
+
+    public void testNoShallowSnapshotInMixedMode() throws Exception {
+        logger.info("Initialize remote cluster");
+        addRemote = true;
+        internalCluster().setBootstrapClusterManagerNodeIndex(0);
+        List<String> cmNodes = internalCluster().startNodes(1);
+        Client client = internalCluster().client(cmNodes.get(0));
+
+        logger.info("Add remote node");
+        internalCluster().startNode();
+        internalCluster().validateClusterFormed();
+
+        logger.info("Create remote backed index");
+        RemoteStoreMigrationShardAllocationBaseTestCase.createIndex("test", 0);
+        RemoteStoreMigrationShardAllocationBaseTestCase.assertRemoteStoreBackedIndex("test");
+
+        logger.info("Create shallow snapshot setting enabled repo");
+        String shallowSnapshotRepoName = "shallow-snapshot-repo-name";
+        Path shallowSnapshotRepoPath = randomRepoPath();
+        assertAcked(
+            clusterAdmin().preparePutRepository(shallowSnapshotRepoName)
+                .setType("fs")
+                .setSettings(
+                    Settings.builder()
+                        .put("location", shallowSnapshotRepoPath)
+                        .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), Boolean.TRUE)
+                )
+        );
+
+        logger.info("Verify shallow snapshot creation");
+        final String snapshot1 = "snapshot1";
+        SnapshotInfo snapshotInfo1 = RemoteStoreMigrationShardAllocationBaseTestCase.createSnapshot(
+            shallowSnapshotRepoName,
+            snapshot1,
+            "test"
+        );
+        assertEquals(snapshotInfo1.isRemoteStoreIndexShallowCopyEnabled(), true);
+
+        logger.info("Set MIXED compatibility mode");
+        ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest();
+        updateSettingsRequest.persistentSettings(Settings.builder().put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), "mixed"));
+        assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
+
+        logger.info("Verify that new snapshot is not shallow");
+        final String snapshot2 = "snapshot2";
+        SnapshotInfo snapshotInfo2 = RemoteStoreMigrationShardAllocationBaseTestCase.createSnapshot(shallowSnapshotRepoName, snapshot2);
+        assertEquals(snapshotInfo2.isRemoteStoreIndexShallowCopyEnabled(), false);
+    }
 }
diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java
@@ -346,12 +346,10 @@ public ClusterState execute(ClusterState currentState) {
 
                 boolean remoteStoreIndexShallowCopy = REMOTE_STORE_INDEX_SHALLOW_COPY.get(repository.getMetadata().settings());
                 logger.info("remote_store_index_shallow_copy setting is set as [{}]", remoteStoreIndexShallowCopy);
-                CompatibilityMode compatibilityMode = clusterService.getClusterSettings().get(REMOTE_STORE_COMPATIBILITY_MODE_SETTING);
-                if (remoteStoreIndexShallowCopy && compatibilityMode.equals(CompatibilityMode.STRICT) == false) {
+                if (remoteStoreIndexShallowCopy
+                    && clusterService.getClusterSettings().get(REMOTE_STORE_COMPATIBILITY_MODE_SETTING).equals(CompatibilityMode.MIXED)) {
                     // don't allow shallow snapshots if compatibility mode is not strict
-                    logger.warn(
-                        "Shallow snapshots are not allowed during migration, thus overriding remote_store_index_shallow_copy to false"
-                    );
+                    logger.debug("Shallow snapshots are not supported during migration. Falling back to full snapshot.");
                     remoteStoreIndexShallowCopy = false;
                 }
                 newEntry = SnapshotsInProgress.startedEntry(