Assert aborted snapshots have no pending shard work (elastic#102621)

When a snapshot is aborted, all shard snapshots must either be complete (`SUCCESS`, `FAILED` or `MISSING`) or else they must be in state `ABORTED` while the data node finishes its work. It seems we do not check this invariant anywhere today, so this commit adds an assertion of this invariant.
timgrein · Nov 30, 2023 · 4bc4911 · 4bc4911
1 parent b493409
commit 4bc4911
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 4 deletions.
diff --git a/server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java b/server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java
@@ -343,14 +343,20 @@ private static boolean assertConsistentEntries(Map<String, ByRepo> entries) {
                 assert entry.repository().equals(repository) : "mismatched repository " + entry + " tracked under " + repository;
                 for (Map.Entry<RepositoryShardId, ShardSnapshotStatus> shard : entry.shardsByRepoShardId().entrySet()) {
                     final RepositoryShardId sid = shard.getKey();
+                    final ShardSnapshotStatus shardSnapshotStatus = shard.getValue();
                     assert assertShardStateConsistent(
                         entriesForRepository,
                         assignedShards,
                         queuedShards,
                         sid.indexName(),
                         sid.shardId(),
-                        shard.getValue()
+                        shardSnapshotStatus
                     );
+
+                    assert entry.state() != State.ABORTED
+                        || shardSnapshotStatus.state == ShardState.ABORTED
+                        || shardSnapshotStatus.state().completed()
+                        : sid + " is in state " + shardSnapshotStatus.state() + " in aborted snapshot " + entry.snapshot;
                 }
             }
             // make sure in-flight-shard-states can be built cleanly for the entries without tripping assertions

diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotsInProgressSerializationTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotsInProgressSerializationTests.java
@@ -523,8 +523,15 @@ public void testXContent() throws IOException {
     }
 
     public static State randomState(Map<ShardId, SnapshotsInProgress.ShardSnapshotStatus> shards) {
-        return SnapshotsInProgress.completed(shards.values())
-            ? randomFrom(State.SUCCESS, State.FAILED)
-            : randomFrom(State.STARTED, State.INIT, State.ABORTED);
+        if (SnapshotsInProgress.completed(shards.values())) {
+            return randomFrom(State.SUCCESS, State.FAILED);
+        }
+        if (shards.values()
+            .stream()
+            .map(SnapshotsInProgress.ShardSnapshotStatus::state)
+            .allMatch(st -> st.completed() || st == ShardState.ABORTED)) {
+            return State.ABORTED;
+        }
+        return randomFrom(State.STARTED, State.INIT);
     }
 }