Writable warm replica replication/recovery #17390

skumawat2025 · 2025-02-19T03:33:14Z

Description

This PR aims to implement the Replica Replication and Recovery flow for indices with partial store locality (also known as writable warm indices), which were introduced with the composite directory in PR #12782. It builds upon and addresses comments received on the pending PR #14670.
For writable indices, where primary shards now consist of both localDirectory and remoteDirectory, we've implemented file uploads to remote storage and maintain an LRU fileCache on the node instead of storing all files locally.
We've made several changes to the replication and recovery processes:

During replication events on replicas, we now only update the NRTReplicationReaderManager with the latest CheckpointInfoResponse, avoiding unnecessary downloads of actual file diffs on replica.
For index close, we are skipping doing a flush on replica shards as during re-open we will anyway sync from remote store.
During remote store sync in recovery scenarios, we are skipping coping the actually segment files from remote storage to local and create a new commit with the latest commit info from remote segment metadata file.

Related Issues

Resolves #13647

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2025-02-19T05:12:00Z

❌ Gradle check result for 6fde641: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

gbbafna · 2025-02-19T09:05:11Z

server/src/main/java/org/opensearch/cluster/routing/allocation/IndexMetadataUpdater.java

@@ -238,7 +238,8 @@ private IndexMetadata.Builder updateInSyncAllocations(
                    allocationId = RecoverySource.ExistingStoreRecoverySource.FORCED_ALLOCATION_ID;
                } else {
                    assert (recoverySource instanceof RecoverySource.SnapshotRecoverySource
-                        || recoverySource instanceof RecoverySource.RemoteStoreRecoverySource) : recoverySource;
+                        || recoverySource instanceof RecoverySource.RemoteStoreRecoverySource
+                        || recoverySource instanceof RecoverySource.ExistingStoreRecoverySource) : recoverySource;


why is this required and what will be the implication ?

In search role nodes when Recovery is happening for existing remote indexes. It was assumed that recovery will happen from snapsho restore. See here
We've changed this flow for warm indices where recovery for indices with EXISTING_STORE source will go through through remote store. Without this change the recovery will be treated as EMPTY_STORE.

We are weakening the assertion here . For ExistingStoreRecoverySource , it can only be a tiered index . But here we are not/cannot check for same.

server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java

gbbafna · 2025-02-20T04:59:13Z

server/src/main/java/org/opensearch/index/store/remote/utils/cache/SegmentedCache.java

@@ -190,8 +190,10 @@ public CacheStats stats() {
    public void logCurrentState() {
        int i = 0;
        for (RefCountedCache<K, V> cache : table) {
-            logger.trace("SegmentedCache " + i);
-            ((LRUCache<K, V>) cache).logCurrentState();
+            if (cache.size() > 0) {


can we handle size() == 0 case inside logCurrentState ?

As this is for logging purpose. Skipping logging the empty cache details.

gbbafna · 2025-02-20T05:15:38Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

        logger.trace("Composite Directory[{}]: Local Directory files - {}", this::toString, () -> Arrays.toString(localFiles));
-        logger.trace("Composite Directory[{}]: Remote Directory files - {}", this::toString, () -> Arrays.toString(remoteFiles));
+        // logger.trace("Composite Directory[{}]: Remote Directory files - {}", this::toString, () -> Arrays.toString(remoteFiles));
        Set<String> nonBlockLuceneFiles = allFiles.stream()


why are we returning just the nonBlockLuceneFiles here ? won't the list be incomplete here as some files can get evicted ?

Remote will have files that are marked as block. Meanwhile we will evaluate the listAll() implementation for CompositeDirectory(). As some tests are flaky with this Implementation that i've muted with this PR.

Signed-off-by: Sandeep Kumawat <[email protected]>

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

github-actions · 2025-02-24T04:28:41Z

❌ Gradle check result for 1963b58: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2025-02-25T07:23:10Z

❌ Gradle check result for 3ce8970: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

skumawat2025 · 2025-02-25T07:25:56Z

❌ Gradle check result for 1963b58: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Failing tests are flaky: #17364, #15806, #16145

github-actions · 2025-02-25T09:52:18Z

❌ Gradle check result for 505a59c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

skumawat2025 · 2025-02-25T09:59:07Z

❌ Gradle check result for 505a59c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Flaky: #15806, #17364

gbbafna · 2025-02-20T06:41:40Z

...erTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java

+                if (action.equals(SegmentReplicationTargetService.Actions.FILE_CHUNK)) {
+                    FileChunkRequest req = (FileChunkRequest) request;
+                    logger.debug("file chunk [{}] lastChunk: {}", req, req.lastChunk());
+                    if (req.name().endsWith("cfs") && req.lastChunk()) {


where are we doing file copy ?

gbbafna · 2025-02-20T06:42:51Z

...erTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java

@@ -0,0 +1,2087 @@
+/*


With Warm only RemoteStoreSegmentReplication is supported . So we can rename the file as WarmIndexReplication/Recovery etc and segregate functions as well .

.../src/main/java/org/opensearch/cluster/routing/allocation/allocator/RemoteShardsBalancer.java

gbbafna · 2025-02-26T04:25:24Z

server/src/main/java/org/opensearch/cluster/routing/allocation/IndexMetadataUpdater.java

@@ -238,7 +238,8 @@ private IndexMetadata.Builder updateInSyncAllocations(
                    allocationId = RecoverySource.ExistingStoreRecoverySource.FORCED_ALLOCATION_ID;
                } else {
                    assert (recoverySource instanceof RecoverySource.SnapshotRecoverySource
-                        || recoverySource instanceof RecoverySource.RemoteStoreRecoverySource) : recoverySource;
+                        || recoverySource instanceof RecoverySource.RemoteStoreRecoverySource
+                        || recoverySource instanceof RecoverySource.ExistingStoreRecoverySource) : recoverySource;


We are weakening the assertion here . For ExistingStoreRecoverySource , it can only be a tiered index . But here we are not/cannot check for same.

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

gbbafna · 2025-02-26T05:44:21Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

+        // For warm indices, it doesn't make sense to check for local existence of files.
+        overrideLocal = overrideLocal && indexSettings.isStoreLocalityPartial() == false;


for remote restore, we always do this . I don't think we should change that behavior as we want to go back in time.

This make sense. Even for partial locality index it is safe to override the files. Thanks for this comment

gbbafna · 2025-02-26T11:13:12Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

+                if (indexSettings.isStoreLocalityPartial() == false) {
+                    assert Arrays.stream(store.directory().listAll()).filter(f -> f.startsWith(IndexFileNames.SEGMENTS)).findAny().isEmpty()
+                        : "There should not be any segments file in the dir";
+                }


a better way to do it

assert ... || indexSettings.isStoreLocalityPartial()

gbbafna · 2025-02-26T11:21:42Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

+                logger.info("The file [{}] exist in local but not part of FileCache, deleting it from local", name);
+                localDirectory.deleteFile(name);
+            } else {
+                fileCache.remove(getFilePath(name));


where are the block files removed ?

We are using listLocalFiles() to check if localDirectory contains that file. listLocalFiles() will return blocked files as well. So when delete for block files is called. We will delete it in this method.

Added logic to delete block files as well.

server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java

server/src/main/java/org/opensearch/index/store/remote/utils/cache/SegmentedCache.java

Signed-off-by: Sandeep Kumawat <[email protected]>

github-actions · 2025-02-27T11:41:48Z

❌ Gradle check result for 8339c0f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

rayshrey · 2025-02-27T20:44:43Z

server/src/main/java/org/opensearch/index/store/remote/utils/FileTypeUtils.java

@@ -18,12 +18,14 @@
 @ExperimentalApi
 public class FileTypeUtils {

+    public static String BLOCK_FILE_SUFFIX = "_block_";


nit: BLOCK_FILE_IDENTIFIER (since it is not really a suffix)

rayshrey · 2025-02-27T20:46:33Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

+    // TODO: Revisit listAll() implementation, Check if we should include the remote files as well.
    @Override
    public String[] listAll() throws IOException {
        ensureOpen();
        logger.trace("Composite Directory[{}]: listAll() called", this::toString);
        String[] localFiles = localDirectory.listAll();
-        Set<String> allFiles = new HashSet<>(Arrays.asList(localFiles));
        String[] remoteFiles = getRemoteFiles();
+        Set<String> allFiles = new HashSet<>(Arrays.asList(localFiles));


Changes not required, please remove them

rayshrey · 2025-02-27T20:49:36Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

+     * Returns names of all files stored in local directory
+     * @throws IOException in case of I/O error
+     */
+    protected String[] listLocalFiles() throws IOException {


Should be private
We do not want to expose local or remote related distinctions from this class.

rayshrey · 2025-02-27T20:52:09Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

+     * Returns names of all block files stored in local directory for a given file
+     * @throws IOException in case of I/O error
+     */
+    protected List<String> listBlockFiles(String fileName) throws IOException {


Should be private as well.
Also since this will include the full file as well if present locally, should we consider renaming this as listLocalFiles as well (without any parameter, it will return all local files, with filename as parameter will return all local files related to that file) ?

I think this will become confusing. I believe we should call it listBlockFiles() only and explain in the docs that it will return all the block files as well as that file itself.

rayshrey · 2025-02-27T20:54:19Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

+        String[] allFiles = listLocalFiles();
+        List<String> blockFiles = new ArrayList<>();
+
+        // Add the original file if it exists
+        if (Arrays.asList(allFiles).contains(fileName)) {
+            blockFiles.add(fileName);
+        }
+
+        // Find and add all block files
+        String prefix = fileName + FileTypeUtils.BLOCK_FILE_SUFFIX;
+        for (String file : allFiles) {
+            if (file.startsWith(prefix)) {
+                blockFiles.add(file);
+            }
+        }
+        return blockFiles;
+    }


Nit:

protected List<String> listBlockFiles(String fileName) throws IOException { return Stream.of(listLocalFiles()) .filter(file -> file.equals(fileName) || file.startsWith(fileName + FileTypeUtils.BLOCK_FILE_SUFFIX)) .collect(Collectors.toList()); }

rayshrey · 2025-02-27T20:56:38Z

server/src/main/java/org/opensearch/index/store/CompositeDirectory.java

-        } else if (Arrays.asList(listAll()).contains(name) == false) {
-            throw new NoSuchFileException("File " + name + " not found in directory");


Why are we removing this ?
If we get delete call for a non-existent file we should return a NoSuchFileException by contract.

I am understanding your concern. Let me explain why we are doing this change:
In CompositeDirectory, we are having both local and remote files.
Lucene is using listAll() to get files and then calling deleteFile(). But in our case, a file may be in remote even if not in local. So if we throw NoSuchFileException just because file is not in local, it is not correct.
I am thinking we should do like this:
We will throw NoSuchFileException only when file is not in both local and remote.
If file is in any place (local or remote), we will do deletion process.
This way, we are not saying file is missing when it is actually in remote. But we are still following Lucene's rule of throwing exception for truly missing files.
What you are thinking about this?

rayshrey · 2025-02-27T21:00:28Z

server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java

+    public void testlistBlockFiles() throws IOException {
+        List<String> actualFileNames = compositeDirectory.listBlockFiles(FILE_PRESENT_LOCALLY);
+        String[] expectedFileNames = new String[] { "_1.cfe", "_1.cfe_block_0", "_1.cfe_block_1", };
+        assertArrayEquals(expectedFileNames, actualFileNames.toArray());
+    }
+
+    public void testLocalFiles() throws IOException {
+        String[] actualFileNames = compositeDirectory.listLocalFiles();
+        String[] expectedFileNames = new String[] {
+            "_0.cfe_block_7",
+            "_0.cfs_block_7",
+            "_1.cfe",
+            "_1.cfe_block_0",
+            "_1.cfe_block_1",
+            "_2.cfe",
+            "temp_file.tmp" };
+        assertArrayEquals(expectedFileNames, actualFileNames);
+    }
+


Separate tests not needed for these functions, adding more cases in testDeleteFile should suffice as these will be private functions and will be used in the delete call only.

gbbafna · 2025-02-28T07:47:23Z

server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java

+
+        // Return an empty list for warm indices, In this case, replica shards don't require downloading files from remote storage
+        // as they can will sync all files from remote in case failure.
+        if (indexShard.indexSettings().isStoreLocalityPartial() == true) {


nit : remove == true.

github-actions bot added enhancement Enhancement or improvement to existing feature or request Roadmap:Search Project-wide roadmap label Storage:Remote v2.16.0 Issues and PRs related to version 2.16.0 labels Feb 19, 2025

skumawat2025 added skip-changelog and removed skip-changelog labels Feb 19, 2025

skumawat2025 changed the title ~~Writable warm replica relocation/recovery~~ Writable warm replica replication/recovery Feb 19, 2025

opensearch-ci-bot mentioned this pull request Feb 14, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteIndexShardTests #16116

Open

gbbafna reviewed Feb 19, 2025

View reviewed changes

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java Outdated Show resolved Hide resolved

gbbafna reviewed Feb 20, 2025

View reviewed changes

Writable warm replica relocation/recovery

37f1490

Signed-off-by: Sandeep Kumawat <[email protected]>

skumawat2025 force-pushed the warm-replica-backup branch 2 times, most recently from 719a67d to 1963b58 Compare February 24, 2025 04:14

skumawat2025 commented Feb 24, 2025

View reviewed changes

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java Outdated Show resolved Hide resolved

skumawat2025 force-pushed the warm-replica-backup branch from 1963b58 to 3ce8970 Compare February 25, 2025 06:29

skumawat2025 force-pushed the warm-replica-backup branch from 3ce8970 to 505a59c Compare February 25, 2025 08:58

skumawat2025 marked this pull request as ready for review February 25, 2025 09:02

skumawat2025 requested review from anasalkouz, andrross, ashking94, Bukhtawar, CEHENKLE, cwperks, dblock, dbwiddis and jed326 as code owners February 25, 2025 09:02

skumawat2025 requested review from mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, VachaShah, jainankitk and linuxpi as code owners February 25, 2025 09:02

This was referenced Feb 25, 2025

[AUTOCUT] Gradle Check Flaky Test Report for DedicatedClusterSnapshotRestoreIT #15806

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemotePrimaryRelocationIT #17364

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreIT #16145

Open

gbbafna reviewed Feb 26, 2025

View reviewed changes

Bukhtawar reviewed Feb 26, 2025

View reviewed changes

server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java Outdated Show resolved Hide resolved

Bukhtawar reviewed Feb 26, 2025

View reviewed changes

server/src/main/java/org/opensearch/index/store/remote/utils/cache/SegmentedCache.java Outdated Show resolved Hide resolved

Fix for failure IT's

8339c0f

Signed-off-by: Sandeep Kumawat <[email protected]>

skumawat2025 force-pushed the warm-replica-backup branch from 505a59c to 8339c0f Compare February 27, 2025 10:48

This was referenced Feb 27, 2025

[AUTOCUT] Gradle Check Flaky Test Report for MinimumClusterManagerNodesIT #14289

Open

[AUTOCUT] Gradle Check Flaky Test Report for WorkloadManagementIT #17154

Open

rayshrey reviewed Feb 27, 2025

View reviewed changes

gbbafna reviewed Feb 28, 2025

View reviewed changes

skumawat2025 mentioned this pull request Feb 28, 2025

Writable warm relocation recovery #14670

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Writable warm replica replication/recovery #17390

Writable warm replica replication/recovery #17390

skumawat2025 commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025

gbbafna Feb 19, 2025

skumawat2025 Feb 25, 2025

gbbafna Feb 26, 2025

gbbafna Feb 20, 2025

skumawat2025 Feb 25, 2025

skumawat2025 Feb 27, 2025

gbbafna Feb 20, 2025

skumawat2025 Feb 25, 2025

github-actions bot commented Feb 24, 2025

github-actions bot commented Feb 25, 2025

skumawat2025 commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

skumawat2025 commented Feb 25, 2025

gbbafna Feb 20, 2025

gbbafna Feb 20, 2025

gbbafna Feb 26, 2025

gbbafna Feb 26, 2025

skumawat2025 Feb 27, 2025

gbbafna Feb 26, 2025

gbbafna Feb 26, 2025

skumawat2025 Feb 27, 2025

skumawat2025 Feb 27, 2025

github-actions bot commented Feb 27, 2025

rayshrey Feb 27, 2025

rayshrey Feb 27, 2025

rayshrey Feb 27, 2025

rayshrey Feb 27, 2025

skumawat2025 Feb 28, 2025

rayshrey Feb 27, 2025

rayshrey Feb 27, 2025

skumawat2025 Feb 28, 2025

rayshrey Feb 27, 2025

gbbafna Feb 28, 2025

		// For warm indices, it doesn't make sense to check for local existence of files.
		overrideLocal = overrideLocal && indexSettings.isStoreLocalityPartial() == false;

		} else if (Arrays.asList(listAll()).contains(name) == false) {
		throw new NoSuchFileException("File " + name + " not found in directory");

Writable warm replica replication/recovery #17390

Are you sure you want to change the base?

Writable warm replica replication/recovery #17390

Conversation

skumawat2025 commented Feb 19, 2025 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2025

github-actions bot commented Feb 25, 2025

skumawat2025 commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

skumawat2025 commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skumawat2025 commented Feb 19, 2025 •

edited

Loading