Add more invariant checks to ISRT (#92705)

Today there are two invariants of all realistic `IndexShardRoutingTable`
instances which are not enforced, nor are they satisfied by all
instances created by tests:

- every `IndexShardRoutingTable` must have exactly one primary shard
- if the primary shard is not active (`STARTED` or `RELOCATING`) then the
  replicas must all be `UNASSIGNED`

This commit adds assertions to enforce these invariants, and fixes up
the tests which create instances that do not satisfy them.

Author: DaveCTurner

server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

@@ -291,7 +291,7 @@ private IndexRoutingTable randomIndexRoutingTable(String index, String[] nodeIds
                         nodeId,
                         null,
                         j == 0,
-                        ShardRoutingState.fromValue((byte) randomIntBetween(2, 3)),
+                        j == 0 ? ShardRoutingState.STARTED : randomFrom(ShardRoutingState.INITIALIZING, ShardRoutingState.STARTED),
                         unassignedInfo
                     )
                 );

server/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java

@@ -559,6 +559,7 @@ public IndexShardRoutingTable build() {
             // don't allow more than one shard copy with same id to be allocated to same node
             assert distinctNodes(shards) : "more than one shard with same id assigned to same node (shards: " + shards + ")";
             assert noDuplicatePrimary(shards) : "expected but did not find unique primary in shard routing table: " + shards;
+            assert noAssignedReplicaWithoutActivePrimary(shards) : "unexpected assigned replica with no active primary: " + shards;
             return new IndexShardRoutingTable(shardId, shards);
         }
 
@@ -589,9 +590,24 @@ static boolean noDuplicatePrimary(List<ShardRouting> shards) {
                     seenPrimary = true;
                 }
             }
-            // We should be able to return seenPrimary here, but in tests there are many routing tables with no primary (e.g. empty) so for
-            // now we leniently allow there to be no primary as well. TODO fix those tests and stop being lenient here.
-            return true;
+            return seenPrimary;
+        }
+
+        static boolean noAssignedReplicaWithoutActivePrimary(List<ShardRouting> shards) {
+            boolean seenAssignedReplica = false;
+            for (final var shard : shards) {
+                if (shard.currentNodeId() != null) {
+                    if (shard.primary()) {
+                        if (shard.active()) {
+                            return true;
+                        }
+                    } else {
+                        seenAssignedReplica = true;
+                    }
+                }
+            }
+
+            return seenAssignedReplica == false;
         }
 
         public static IndexShardRoutingTable.Builder readFrom(StreamInput in) throws IOException {

server/src/main/java/org/elasticsearch/indices/store/IndicesStore.java

@@ -184,6 +184,8 @@ public void clusterChanged(ClusterChangedEvent event) {
     }
 
     static boolean shardCanBeDeleted(String localNodeId, IndexShardRoutingTable indexShardRoutingTable) {
+        assert indexShardRoutingTable.size() > 0;
+
         // a shard can be deleted if all its copies are active, and its not allocated on this node
         if (indexShardRoutingTable.size() == 0) {
             // should not really happen, there should always be at least 1 (primary) shard in a

server/src/test/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthActionTests.java

@@ -72,20 +72,21 @@ ClusterState randomClusterStateWithInitializingShards(String index, final int in
             .build();
 
         final List<ShardRoutingState> shardRoutingStates = new ArrayList<>();
-        IntStream.range(0, between(1, 30))
-            .forEach(
-                i -> shardRoutingStates.add(
-                    randomFrom(ShardRoutingState.STARTED, ShardRoutingState.UNASSIGNED, ShardRoutingState.RELOCATING)
-                )
-            );
-        IntStream.range(0, initializingShards).forEach(i -> shardRoutingStates.add(ShardRoutingState.INITIALIZING));
-        Randomness.shuffle(shardRoutingStates);
-
-        // primary can not be unassigned, otherwise replicas can't in initializing or relocating state.
-        // (assertion in RoutingNodes disallows this)
-        if (shardRoutingStates.get(0) == ShardRoutingState.UNASSIGNED) {
-            // Don't randomly pick ShardRoutingState.UNASSIGNED, since that already has randomly been inserted based on initializingShards
-            shardRoutingStates.set(0, randomFrom(ShardRoutingState.STARTED, ShardRoutingState.RELOCATING));
+        if (initializingShards == 1 && randomBoolean()) {
+            shardRoutingStates.add(ShardRoutingState.INITIALIZING);
+            IntStream.range(0, between(0, 30)).forEach(i -> shardRoutingStates.add(ShardRoutingState.UNASSIGNED));
+        } else {
+            IntStream.range(0, between(0, 30))
+                .forEach(
+                    i -> shardRoutingStates.add(
+                        randomFrom(ShardRoutingState.STARTED, ShardRoutingState.UNASSIGNED, ShardRoutingState.RELOCATING)
+                    )
+                );
+            IntStream.range(0, initializingShards).forEach(i -> shardRoutingStates.add(ShardRoutingState.INITIALIZING));
+            Randomness.shuffle(shardRoutingStates);
+
+            // primary must be active, otherwise replicas can't in initializing or relocating state.
+            shardRoutingStates.add(0, randomFrom(ShardRoutingState.STARTED, ShardRoutingState.RELOCATING));
         }
 
         final ShardId shardId = new ShardId(indexMetadata.getIndex(), 0);

server/src/test/java/org/elasticsearch/cluster/action/shard/ShardStartedClusterStateTaskExecutorTests.java

@@ -26,7 +26,6 @@
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.index.shard.ShardLongFieldRange;
 
-import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.IntStream;
 import java.util.stream.Stream;
@@ -142,45 +141,57 @@ public void testNonInitializingShardAreMarkedAsSuccessful() throws Exception {
         assertSame(clusterState, executeTasks(clusterState, tasks));
     }
 
-    public void testStartedShards() throws Exception {
+    public void testStartPrimary() throws Exception {
         final String indexName = "test";
-        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING, ShardRoutingState.INITIALIZING);
+        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING);
 
         final IndexMetadata indexMetadata = clusterState.metadata().index(indexName);
         final ShardId shardId = new ShardId(indexMetadata.getIndex(), 0);
         final long primaryTerm = indexMetadata.primaryTerm(shardId.id());
         final ShardRouting primaryShard = clusterState.routingTable().shardRoutingTable(shardId).primaryShard();
         final String primaryAllocationId = primaryShard.allocationId().getId();
 
-        final List<StartedShardUpdateTask> tasks = new ArrayList<>();
-        tasks.add(
-            new StartedShardUpdateTask(
-                new StartedShardEntry(shardId, primaryAllocationId, primaryTerm, "test", ShardLongFieldRange.UNKNOWN),
-                createTestListener()
-            )
+        final var task = new StartedShardUpdateTask(
+            new StartedShardEntry(shardId, primaryAllocationId, primaryTerm, "test", ShardLongFieldRange.UNKNOWN),
+            createTestListener()
         );
-        if (randomBoolean()) {
-            final ShardRouting replicaShard = clusterState.routingTable().shardRoutingTable(shardId).replicaShards().iterator().next();
-            final String replicaAllocationId = replicaShard.allocationId().getId();
-            tasks.add(
-                new StartedShardUpdateTask(
-                    new StartedShardEntry(shardId, replicaAllocationId, primaryTerm, "test", ShardLongFieldRange.UNKNOWN),
-                    createTestListener()
-                )
-            );
-        }
 
-        final var resultingState = executeTasks(clusterState, tasks);
+        final var resultingState = executeTasks(clusterState, List.of(task));
         assertNotSame(clusterState, resultingState);
-        for (final var task : tasks) {
-            assertThat(
-                resultingState.routingTable()
-                    .shardRoutingTable(task.getEntry().shardId)
-                    .getByAllocationId(task.getEntry().allocationId)
-                    .state(),
-                is(ShardRoutingState.STARTED)
-            );
-        }
+        assertThat(
+            resultingState.routingTable()
+                .shardRoutingTable(task.getEntry().shardId)
+                .getByAllocationId(task.getEntry().allocationId)
+                .state(),
+            is(ShardRoutingState.STARTED)
+        );
+    }
+
+    public void testStartReplica() throws Exception {
+        final String indexName = "test";
+        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.STARTED, ShardRoutingState.INITIALIZING);
+
+        final IndexMetadata indexMetadata = clusterState.metadata().index(indexName);
+        final ShardId shardId = new ShardId(indexMetadata.getIndex(), 0);
+        final long primaryTerm = indexMetadata.primaryTerm(shardId.id());
+        final ShardRouting primaryShard = clusterState.routingTable().shardRoutingTable(shardId).primaryShard();
+
+        final ShardRouting replicaShard = clusterState.routingTable().shardRoutingTable(shardId).replicaShards().iterator().next();
+        final String replicaAllocationId = replicaShard.allocationId().getId();
+        final var task = new StartedShardUpdateTask(
+            new StartedShardEntry(shardId, replicaAllocationId, primaryTerm, "test", ShardLongFieldRange.UNKNOWN),
+            createTestListener()
+        );
+
+        final var resultingState = executeTasks(clusterState, List.of(task));
+        assertNotSame(clusterState, resultingState);
+        assertThat(
+            resultingState.routingTable()
+                .shardRoutingTable(task.getEntry().shardId)
+                .getByAllocationId(task.getEntry().allocationId)
+                .state(),
+            is(ShardRoutingState.STARTED)
+        );
     }
 
     public void testDuplicateStartsAreOkay() throws Exception {
@@ -215,12 +226,12 @@ public void testDuplicateStartsAreOkay() throws Exception {
         }
     }
 
-    public void testPrimaryTermsMismatch() throws Exception {
+    public void testPrimaryTermsMismatchOnPrimary() throws Exception {
         final String indexName = "test";
         final int shard = 0;
         final int primaryTerm = 2 + randomInt(200);
 
-        ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING, ShardRoutingState.INITIALIZING);
+        ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING);
         clusterState = ClusterState.builder(clusterState)
             .metadata(
                 Metadata.builder(clusterState.metadata())
@@ -272,8 +283,23 @@ public void testPrimaryTermsMismatch() throws Exception {
                     .state(),
                 is(ShardRoutingState.STARTED)
             );
-            clusterState = resultingState;
         }
+    }
+
+    public void testPrimaryTermsMismatchOnReplica() throws Exception {
+        final String indexName = "test";
+        final int shard = 0;
+        final int primaryTerm = 2 + randomInt(200);
+
+        ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.STARTED, ShardRoutingState.INITIALIZING);
+        clusterState = ClusterState.builder(clusterState)
+            .metadata(
+                Metadata.builder(clusterState.metadata())
+                    .put(IndexMetadata.builder(clusterState.metadata().index(indexName)).primaryTerm(shard, primaryTerm).build(), true)
+                    .build()
+            )
+            .build();
+        final ShardId shardId = new ShardId(clusterState.metadata().index(indexName).getIndex(), shard);
         {
             final long replicaPrimaryTerm = randomBoolean() ? primaryTerm : primaryTerm - 1;
             final String replicaAllocationId = clusterState.routingTable()
@@ -301,9 +327,9 @@ public void testPrimaryTermsMismatch() throws Exception {
         }
     }
 
-    public void testExpandsTimestampRange() throws Exception {
+    public void testExpandsTimestampRangeForPrimary() throws Exception {
         final String indexName = "test";
-        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING, ShardRoutingState.INITIALIZING);
+        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.INITIALIZING);
 
         final IndexMetadata indexMetadata = clusterState.metadata().index(indexName);
         final ShardId shardId = new ShardId(indexMetadata.getIndex(), 0);
@@ -317,47 +343,66 @@ public void testExpandsTimestampRange() throws Exception {
             : randomBoolean() ? ShardLongFieldRange.EMPTY
             : ShardLongFieldRange.of(1606407943000L, 1606407944000L);
 
-        final List<StartedShardUpdateTask> tasks = new ArrayList<>();
-        tasks.add(
-            new StartedShardUpdateTask(
-                new StartedShardEntry(shardId, primaryAllocationId, primaryTerm, "test", shardTimestampRange),
-                createTestListener()
-            )
+        final var task = new StartedShardUpdateTask(
+            new StartedShardEntry(shardId, primaryAllocationId, primaryTerm, "test", shardTimestampRange),
+            createTestListener()
         );
-        if (randomBoolean()) {
-            final ShardRouting replicaShard = clusterState.routingTable().shardRoutingTable(shardId).replicaShards().iterator().next();
-            final String replicaAllocationId = replicaShard.allocationId().getId();
-            tasks.add(
-                new StartedShardUpdateTask(
-                    new StartedShardEntry(shardId, replicaAllocationId, primaryTerm, "test", shardTimestampRange),
-                    createTestListener()
-                )
-            );
-        }
-        final var resultingState = executeTasks(clusterState, tasks);
+
+        final var resultingState = executeTasks(clusterState, List.of(task));
         assertNotSame(clusterState, resultingState);
-        for (final var task : tasks) {
-            assertThat(
-                resultingState.routingTable()
-                    .shardRoutingTable(task.getEntry().shardId)
-                    .getByAllocationId(task.getEntry().allocationId)
-                    .state(),
-                is(ShardRoutingState.STARTED)
-            );
+        assertThat(
+            resultingState.routingTable()
+                .shardRoutingTable(task.getEntry().shardId)
+                .getByAllocationId(task.getEntry().allocationId)
+                .state(),
+            is(ShardRoutingState.STARTED)
+        );
 
-            final var timestampRange = resultingState.metadata().index(indexName).getTimestampRange();
-            if (shardTimestampRange == ShardLongFieldRange.UNKNOWN) {
-                assertThat(timestampRange, sameInstance(IndexLongFieldRange.UNKNOWN));
-            } else if (shardTimestampRange == ShardLongFieldRange.EMPTY) {
-                assertThat(timestampRange, sameInstance(IndexLongFieldRange.EMPTY));
-            } else {
-                assertTrue(timestampRange.isComplete());
-                assertThat(timestampRange.getMin(), equalTo(shardTimestampRange.getMin()));
-                assertThat(timestampRange.getMax(), equalTo(shardTimestampRange.getMax()));
-            }
+        final var timestampRange = resultingState.metadata().index(indexName).getTimestampRange();
+        if (shardTimestampRange == ShardLongFieldRange.UNKNOWN) {
+            assertThat(timestampRange, sameInstance(IndexLongFieldRange.UNKNOWN));
+        } else if (shardTimestampRange == ShardLongFieldRange.EMPTY) {
+            assertThat(timestampRange, sameInstance(IndexLongFieldRange.EMPTY));
+        } else {
+            assertTrue(timestampRange.isComplete());
+            assertThat(timestampRange.getMin(), equalTo(shardTimestampRange.getMin()));
+            assertThat(timestampRange.getMax(), equalTo(shardTimestampRange.getMax()));
         }
     }
 
+    public void testExpandsTimestampRangeForReplica() throws Exception {
+        final String indexName = "test";
+        final ClusterState clusterState = state(indexName, randomBoolean(), ShardRoutingState.STARTED, ShardRoutingState.INITIALIZING);
+
+        final IndexMetadata indexMetadata = clusterState.metadata().index(indexName);
+        final ShardId shardId = new ShardId(indexMetadata.getIndex(), 0);
+        final long primaryTerm = indexMetadata.primaryTerm(shardId.id());
+
+        assertThat(indexMetadata.getTimestampRange(), sameInstance(IndexLongFieldRange.UNKNOWN));
+
+        final ShardLongFieldRange shardTimestampRange = randomBoolean() ? ShardLongFieldRange.UNKNOWN
+            : randomBoolean() ? ShardLongFieldRange.EMPTY
+            : ShardLongFieldRange.of(1606407943000L, 1606407944000L);
+
+        final ShardRouting replicaShard = clusterState.routingTable().shardRoutingTable(shardId).replicaShards().iterator().next();
+        final String replicaAllocationId = replicaShard.allocationId().getId();
+        final var task = new StartedShardUpdateTask(
+            new StartedShardEntry(shardId, replicaAllocationId, primaryTerm, "test", shardTimestampRange),
+            createTestListener()
+        );
+        final var resultingState = executeTasks(clusterState, List.of(task));
+        assertNotSame(clusterState, resultingState);
+        assertThat(
+            resultingState.routingTable()
+                .shardRoutingTable(task.getEntry().shardId)
+                .getByAllocationId(task.getEntry().allocationId)
+                .state(),
+            is(ShardRoutingState.STARTED)
+        );
+
+        assertThat(resultingState.metadata().index(indexName).getTimestampRange(), sameInstance(IndexLongFieldRange.UNKNOWN));
+    }
+
     private ClusterState executeTasks(final ClusterState state, final List<StartedShardUpdateTask> tasks) throws Exception {
         return ClusterStateTaskExecutorUtils.executeAndAssertSuccessful(state, executor, tasks);
     }