Address comments and fix multiple failing tests

Signed-off-by: Ashish Singh <[email protected]>

server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java

@@ -36,6 +36,8 @@
 import org.opensearch.gateway.remote.ClusterMetadataManifest;
 import org.opensearch.gateway.remote.ClusterMetadataManifest.UploadedIndexMetadata;
 import org.opensearch.gateway.remote.RemoteClusterStateService;
+import org.opensearch.index.remote.RemoteStoreEnums.PathType;
+import org.opensearch.repositories.blobstore.BlobStoreRepository;
 import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.OpenSearchIntegTestCase;
 import org.junit.Before;
@@ -486,9 +488,15 @@ private void verifyRestoredRepositories(Path repoPath) {
         assertTrue(SYSTEM_REPOSITORY_SETTING.get(repositoriesMetadata.repository(REPOSITORY_NAME).settings()));
         assertTrue(SYSTEM_REPOSITORY_SETTING.get(repositoriesMetadata.repository(REPOSITORY_2_NAME).settings()));
         assertEquals("fs", repositoriesMetadata.repository("custom-repo").type());
+        Settings settings = repositoriesMetadata.repository("custom-repo").settings();
+        PathType pathType = BlobStoreRepository.SHARD_PATH_TYPE.get(settings);
         assertEquals(
-            Settings.builder().put("location", repoPath).put("compress", false).build(),
-            repositoriesMetadata.repository("custom-repo").settings()
+            Settings.builder()
+                .put("location", repoPath)
+                .put("compress", false)
+                .put(BlobStoreRepository.SHARD_PATH_TYPE.getKey(), pathType)
+                .build(),
+            settings
         );
 
         // repo cleanup post verification

server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java

@@ -33,6 +33,7 @@
 package org.opensearch.snapshots;
 
 import org.opensearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
+import org.opensearch.action.admin.cluster.repositories.put.PutRepositoryRequestBuilder;
 import org.opensearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse;
 import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
 import org.opensearch.action.bulk.BulkRequest;
@@ -232,7 +233,16 @@ public void testRepositoryAckTimeout() throws Exception {
             .put("location", randomRepoPath())
             .put("compress", randomBoolean())
             .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES);
-        createRepository("test-repo-1", "fs", settings, "0s");
+        PutRepositoryRequestBuilder requestBuilder = OpenSearchIntegTestCase.putRepositoryRequestBuilder(
+            client().admin().cluster(),
+            "test-repo-1",
+            "fs",
+            true,
+            settings,
+            "0s",
+            false
+        );
+        assertFalse(requestBuilder.get().isAcknowledged());
 
         logger.info("-->  creating repository test-repo-2 with standard timeout - should ack");
         settings = Settings.builder()

server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java

@@ -394,7 +394,7 @@ public void testSnapshotFileFailureDuringSnapshot() throws InterruptedException
             .put("location", randomRepoPath())
             .put("random", randomAlphaOfLength(10))
             .put("random_control_io_exception_rate", 0.2);
-        OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", settings);
+        OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", false, settings);
 
         createIndexWithRandomDocs("test-idx", 100);
 

server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java

@@ -223,6 +223,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
                 IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID,
                 IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME,
                 IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID,
+                IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE,
 
                 // Settings for remote translog
                 IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING,

server/src/main/java/org/opensearch/index/IndexSettings.java

@@ -48,6 +48,7 @@
 import org.opensearch.core.common.unit.ByteSizeUnit;
 import org.opensearch.core.common.unit.ByteSizeValue;
 import org.opensearch.core.index.Index;
+import org.opensearch.index.remote.RemoteStoreEnums.PathType;
 import org.opensearch.index.remote.RemoteStorePathStrategy;
 import org.opensearch.index.remote.RemoteStoreUtils;
 import org.opensearch.index.translog.Translog;
@@ -677,6 +678,14 @@ public static IndexMergePolicy fromString(String text) {
         Property.InternalIndex
     );
 
+    public static final Setting<PathType> SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE = new Setting<>(
+        "index.searchable_snapshot.shard_path_type",
+        PathType.FIXED.toString(),
+        PathType::parseString,
+        Property.IndexScope,
+        Property.InternalIndex
+    );
+
     public static final Setting<String> DEFAULT_SEARCH_PIPELINE = new Setting<>(
         "index.search.default_pipeline",
         SearchPipelineService.NOOP_PIPELINE_ID,

server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java

@@ -11,14 +11,15 @@
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.FSDirectory;
 import org.opensearch.common.blobstore.BlobContainer;
-import org.opensearch.common.blobstore.BlobPath;
 import org.opensearch.index.IndexSettings;
+import org.opensearch.index.remote.RemoteStoreEnums.PathType;
 import org.opensearch.index.shard.ShardPath;
 import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot;
 import org.opensearch.index.snapshots.blobstore.IndexShardSnapshot;
 import org.opensearch.index.store.remote.filecache.FileCache;
 import org.opensearch.index.store.remote.utils.TransferManager;
 import org.opensearch.plugins.IndexStorePlugin;
+import org.opensearch.repositories.IndexId;
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.repositories.Repository;
 import org.opensearch.repositories.blobstore.BlobStoreRepository;
@@ -74,10 +75,11 @@ private Future<RemoteSnapshotDirectory> createRemoteSnapshotDirectoryFromSnapsho
         ShardPath localShardPath,
         BlobStoreRepository blobStoreRepository
     ) throws IOException {
-        final BlobPath blobPath = blobStoreRepository.basePath()
-            .add("indices")
-            .add(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.get(indexSettings.getSettings()))
-            .add(Integer.toString(localShardPath.getShardId().getId()));
+        // The below information like the snapshot generated indexId, shard_path_type and shardId are used for
+        // creating the shard BlobContainer. This information has been updated as per the hashed_prefix snapshots.
+        String indexId = IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.get(indexSettings.getSettings());
+        PathType pathType = IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE.get(indexSettings.getSettings());
+        int shardId = localShardPath.getShardId().getId();
         final SnapshotId snapshotId = new SnapshotId(
             IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.get(indexSettings.getSettings()),
             IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.get(indexSettings.getSettings())
@@ -89,7 +91,12 @@ private Future<RemoteSnapshotDirectory> createRemoteSnapshotDirectoryFromSnapsho
         // this trick is needed to bypass assertions in BlobStoreRepository::assertAllowableThreadPools in case of node restart and a remote
         // index restore is invoked
         return threadPool.executor(ThreadPool.Names.SNAPSHOT).submit(() -> {
-            final BlobContainer blobContainer = blobStoreRepository.blobStore().blobContainer(blobPath);
+            // shardContainer(IndexId, shardId) method uses the id and pathType information to generate the blobPath and
+            // hence the blobContainer. We have used a dummy name as it plays no relevance in the blobPath generation.
+            final BlobContainer blobContainer = blobStoreRepository.shardContainer(
+                new IndexId("DUMMY", indexId, pathType.getCode()),
+                shardId
+            );
             final IndexShardSnapshot indexShardSnapshot = blobStoreRepository.loadShardSnapshot(blobContainer, snapshotId);
             assert indexShardSnapshot instanceof BlobStoreIndexShardSnapshot
                 : "indexShardSnapshot should be an instance of BlobStoreIndexShardSnapshot";

server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java

@@ -1833,14 +1833,17 @@ private void executeOneStaleIndexDelete(
                 logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId);
                 List<String> matchingShardPaths = findMatchingShardPaths(indexSnId, snapshotShardPaths);
                 Optional<String> highestGenShardPaths = findHighestGenerationShardPaths(matchingShardPaths);
+
+                // The shardInfo can be null for 1) snapshots that pre-dates the hashed prefix snapshots.
+                // 2) Snapshot shard paths file upload failed
+                // In such cases, we fallback to fixed_path for cleanup of the data.
                 ShardInfo shardInfo = getShardInfo(highestGenShardPaths, idToShardInfoMap, indexSnId);
 
                 if (remoteStoreLockManagerFactory != null) {
                     cleanupRemoteStoreLocks(indexEntry, shardInfo, remoteStoreLockManagerFactory);
                 }
 
-                // The below deleteShardData would be no-op for FIXED path type snapshots. Otherwise, this deletes the
-                // shard level data for the underlying index based on the pathType present in IndexId
+                // Deletes the shard level data for the underlying index based on the shardInfo that was obtained above.
                 DeleteResult deleteResult = deleteShardData(shardInfo);
 
                 // If there are matchingShardPaths, then we delete them after we have deleted the shard data.

server/src/main/java/org/opensearch/snapshots/RestoreService.java

@@ -86,6 +86,7 @@
 import org.opensearch.core.index.shard.ShardId;
 import org.opensearch.index.IndexModule;
 import org.opensearch.index.IndexSettings;
+import org.opensearch.index.remote.RemoteStoreEnums.PathType;
 import org.opensearch.index.shard.IndexShard;
 import org.opensearch.index.snapshots.IndexShardSnapshotStatus;
 import org.opensearch.index.store.remote.filecache.FileCacheStats;
@@ -1328,6 +1329,7 @@ private static IndexMetadata addSnapshotToIndexSettings(IndexMetadata metadata,
             .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID())
             .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName())
             .put(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.getKey(), indexId.getId())
+            .put(IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE.getKey(), PathType.fromCode(indexId.getShardPathType()))
             .build();
         return IndexMetadata.builder(metadata).settings(newSettings).build();
     }