Snapshot _status API to return correct status for partial snapshots (#…

…12812) * Snapshot _status API to return correct status for partial snapshots Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Updated CHANGELOG.md Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Updated test case Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Setting snapshot status to SUCCESS for older versions for bwc Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Setting snapshot status to SUCCESS for older versions for bwc Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Moved BWC change to SnapshotsInProgress.java for partial snapshots Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Fix for flaky test testSnapshotStatusOnPartialSnapshot Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Updated the testcases to reuse existing getSnapshotStatus() method Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Fixed formatting issues detected in spotlessJavaCheck Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> * Moved the entry to CHANGELOG.md Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> --------- Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com> Signed-off-by: aggarwalShivani <39588384+aggarwalShivani@users.noreply.github.com>
opensearch-project · Apr 17, 2024 · b899a27 · b899a27
1 parent 1fcb79d
commit b899a27
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048))
 - Client with Java 8 runtime and Apache HttpClient 5 Transport fails with java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer ([#13100](https://github.com/opensearch-project/opensearch-java/pull/13100))
 - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098))
+- Fix snapshot _status API to return correct status for partial snapshots ([#12812](https://github.com/opensearch-project/OpenSearch/pull/12812))
 
 ### Security
 

diff --git a/.../internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/.../internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java
@@ -572,7 +572,7 @@ public void testRestoreIndexWithMissingShards() throws Exception {
                 List<SnapshotStatus> snapshotStatuses = snapshotsStatusResponse.getSnapshots();
                 assertEquals(snapshotStatuses.size(), 1);
                 logger.trace("current snapshot status [{}]", snapshotStatuses.get(0));
-                assertTrue(snapshotStatuses.get(0).getState().completed());
+                assertThat(getSnapshot("test-repo", "test-snap-2").state(), equalTo(SnapshotState.PARTIAL));
             }, 1, TimeUnit.MINUTES);
             SnapshotsStatusResponse snapshotsStatusResponse = clusterAdmin().prepareSnapshotStatus("test-repo")
                 .setSnapshots("test-snap-2")
@@ -589,7 +589,6 @@ public void testRestoreIndexWithMissingShards() throws Exception {
             // After it was marked as completed in the cluster state - we need to check if it's completed on the file system as well
             assertBusy(() -> {
                 SnapshotInfo snapshotInfo = getSnapshot("test-repo", "test-snap-2");
-                assertTrue(snapshotInfo.state().completed());
                 assertEquals(SnapshotState.PARTIAL, snapshotInfo.state());
             }, 1, TimeUnit.MINUTES);
         } else {

diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SnapshotStatusApisIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SnapshotStatusApisIT.java
@@ -40,7 +40,6 @@
 import org.opensearch.action.admin.cluster.snapshots.status.SnapshotIndexShardStatus;
 import org.opensearch.action.admin.cluster.snapshots.status.SnapshotStats;
 import org.opensearch.action.admin.cluster.snapshots.status.SnapshotStatus;
-import org.opensearch.action.admin.cluster.snapshots.status.SnapshotsStatusRequest;
 import org.opensearch.action.admin.cluster.snapshots.status.SnapshotsStatusResponse;
 import org.opensearch.client.Client;
 import org.opensearch.cluster.SnapshotsInProgress;
@@ -101,13 +100,9 @@ public void testStatusApiConsistency() {
         assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS));
         assertThat(snapshotInfo.version(), equalTo(Version.CURRENT));
 
-        final List<SnapshotStatus> snapshotStatus = clusterAdmin().snapshotsStatus(
-            new SnapshotsStatusRequest("test-repo", new String[] { "test-snap" })
-        ).actionGet().getSnapshots();
-        assertThat(snapshotStatus.size(), equalTo(1));
-        final SnapshotStatus snStatus = snapshotStatus.get(0);
-        assertEquals(snStatus.getStats().getStartTime(), snapshotInfo.startTime());
-        assertEquals(snStatus.getStats().getTime(), snapshotInfo.endTime() - snapshotInfo.startTime());
+        final SnapshotStatus snapshotStatus = getSnapshotStatus("test-repo", "test-snap");
+        assertEquals(snapshotStatus.getStats().getStartTime(), snapshotInfo.startTime());
+        assertEquals(snapshotStatus.getStats().getTime(), snapshotInfo.endTime() - snapshotInfo.startTime());
     }
 
     public void testStatusAPICallForShallowCopySnapshot() {
@@ -357,6 +352,22 @@ public void testSnapshotStatusOnFailedSnapshot() throws Exception {
         assertEquals(SnapshotsInProgress.State.FAILED, snapshotsStatusResponse.getSnapshots().get(0).getState());
     }
 
+    public void testSnapshotStatusOnPartialSnapshot() throws Exception {
+        final String dataNode = internalCluster().startDataOnlyNode();
+        final String repoName = "test-repo";
+        final String snapshotName = "test-snap";
+        final String indexName = "test-idx";
+        createRepository(repoName, "fs");
+        // create an index with a single shard on the data node, that will be stopped
+        createIndex(indexName, singleShardOneNode(dataNode));
+        index(indexName, "_doc", "some_doc_id", "foo", "bar");
+        logger.info("--> stopping data node before creating snapshot");
+        stopNode(dataNode);
+        startFullSnapshot(repoName, snapshotName, true).get();
+        final SnapshotStatus snapshotStatus = getSnapshotStatus(repoName, snapshotName);
+        assertEquals(SnapshotsInProgress.State.PARTIAL, snapshotStatus.getState());
+    }
+
     public void testStatusAPICallInProgressShallowSnapshot() throws Exception {
         internalCluster().startClusterManagerOnlyNode();
         internalCluster().startDataOnlyNode();

diff --git a/.../org/opensearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java b/.../org/opensearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java
@@ -356,11 +356,11 @@ private void loadRepositoryData(
                             state = SnapshotsInProgress.State.FAILED;
                             break;
                         case SUCCESS:
-                        case PARTIAL:
-                            // Translating both PARTIAL and SUCCESS to SUCCESS for now
-                            // TODO: add the differentiation on the metadata level in the next major release
                             state = SnapshotsInProgress.State.SUCCESS;
                             break;
+                        case PARTIAL:
+                            state = SnapshotsInProgress.State.PARTIAL;
+                            break;
                         default:
                             throw new IllegalArgumentException("Unknown snapshot state " + snapshotInfo.state());
                     }

diff --git a/server/src/main/java/org/opensearch/cluster/SnapshotsInProgress.java b/server/src/main/java/org/opensearch/cluster/SnapshotsInProgress.java
@@ -747,7 +747,12 @@ public void writeTo(StreamOutput out) throws IOException {
             snapshot.writeTo(out);
             out.writeBoolean(includeGlobalState);
             out.writeBoolean(partial);
-            out.writeByte(state.value());
+            if ((out.getVersion().before(Version.V_3_0_0)) && state == State.PARTIAL) {
+                // Setting to SUCCESS for partial snapshots in older versions to maintain backward compatibility
+                out.writeByte(State.SUCCESS.value());
+            } else {
+                out.writeByte(state.value());
+            }
             out.writeList(indices);
             out.writeLong(startTime);
             out.writeMap(shards, (o, v) -> v.writeTo(o), (o, v) -> v.writeTo(o));
@@ -937,7 +942,8 @@ public enum State {
         STARTED((byte) 1, false),
         SUCCESS((byte) 2, true),
         FAILED((byte) 3, true),
-        ABORTED((byte) 4, false);
+        ABORTED((byte) 4, false),
+        PARTIAL((byte) 5, false);
 
         private final byte value;
 
@@ -968,6 +974,8 @@ public static State fromValue(byte value) {
                     return FAILED;
                 case 4:
                     return ABORTED;
+                case 5:
+                    return PARTIAL;
                 default:
                     throw new IllegalArgumentException("No snapshot state for value [" + value + "]");
             }