Add assertion that checks that the same shard with same id is not added to same node (#21498)

Adds an assertion that checks that the same shard with same id is not added to same node. Previously we would just silently ignore the second shard being added.

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

@@ -33,6 +33,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
@@ -572,14 +573,6 @@ public class IndexShardRoutingTable implements Iterable<ShardRouting> {
         }
 
         public Builder addShard(ShardRouting shardEntry) {
-            for (ShardRouting shard : shards) {
-                // don't add two that map to the same node id
-                // we rely on the fact that a node does not have primary and backup of the same shard
-                if (shard.assignedToNode() && shardEntry.assignedToNode()
-                        && shard.currentNodeId().equals(shardEntry.currentNodeId())) {
-                    return this;
-                }
-            }
             shards.add(shardEntry);
             return this;
         }
@@ -590,9 +583,28 @@ public class IndexShardRoutingTable implements Iterable<ShardRouting> {
         }
 
         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 + ")";
             return new IndexShardRoutingTable(shardId, Collections.unmodifiableList(new ArrayList<>(shards)));
         }
 
+        static boolean distinctNodes(List<ShardRouting> shards) {
+            Set<String> nodes = new HashSet<>();
+            for (ShardRouting shard : shards) {
+                if (shard.assignedToNode()) {
+                    if (nodes.add(shard.currentNodeId()) == false) {
+                        return false;
+                    }
+                    if (shard.relocating()) {
+                        if (nodes.add(shard.relocatingNodeId()) == false) {
+                            return false;
+                        }
+                    }
+                }
+            }
+            return true;
+        }
+
         public static IndexShardRoutingTable readFrom(StreamInput in) throws IOException {
             Index index = new Index(in);
             return readFromThin(in, index);

core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

@@ -31,6 +31,7 @@ import org.elasticsearch.cluster.metadata.IndexTemplateMetaData;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.cluster.metadata.RepositoriesMetaData;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.snapshots.SnapshotId;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.node.DiscoveryNodes;
@@ -56,6 +57,7 @@ import org.elasticsearch.test.ESIntegTestCase;
 
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
@@ -238,13 +240,19 @@ public class ClusterStateDiffIT extends ESIntegTestCase {
         for (int i = 0; i < shardCount; i++) {
             IndexShardRoutingTable.Builder indexShard = new IndexShardRoutingTable.Builder(new ShardId(index, "_na_", i));
             int replicaCount = randomIntBetween(1, 10);
+            Set<String> availableNodeIds = Sets.newHashSet(nodeIds);
             for (int j = 0; j < replicaCount; j++) {
                 UnassignedInfo unassignedInfo = null;
                 if (randomInt(5) == 1) {
                     unassignedInfo = new UnassignedInfo(randomReason(), randomAsciiOfLength(10));
                 }
+                if (availableNodeIds.isEmpty()) {
+                    break;
+                }
+                String nodeId = randomFrom(availableNodeIds);
+                availableNodeIds.remove(nodeId);
                 indexShard.addShard(
-                        TestShardRouting.newShardRouting(index, i, randomFrom(nodeIds), null, j == 0,
+                        TestShardRouting.newShardRouting(index, i, nodeId, null, j == 0,
                                 ShardRoutingState.fromValue((byte) randomIntBetween(2, 3)), unassignedInfo));
             }
             builder.addIndexShard(indexShard.build());
@@ -258,8 +266,20 @@ public class ClusterStateDiffIT extends ESIntegTestCase {
     private IndexRoutingTable randomChangeToIndexRoutingTable(IndexRoutingTable original, String[] nodes) {
         IndexRoutingTable.Builder builder = IndexRoutingTable.builder(original.getIndex());
         for (ObjectCursor<IndexShardRoutingTable> indexShardRoutingTable :  original.shards().values()) {
+            Set<String> availableNodes = Sets.newHashSet(nodes);
             for (ShardRouting shardRouting : indexShardRoutingTable.value.shards()) {
-                final ShardRouting updatedShardRouting = randomChange(shardRouting, nodes);
+                availableNodes.remove(shardRouting.currentNodeId());
+                if (shardRouting.relocating()) {
+                    availableNodes.remove(shardRouting.relocatingNodeId());
+                }
+            }
+
+            for (ShardRouting shardRouting : indexShardRoutingTable.value.shards()) {
+                final ShardRouting updatedShardRouting = randomChange(shardRouting, availableNodes);
+                availableNodes.remove(updatedShardRouting.currentNodeId());
+                if (shardRouting.relocating()) {
+                    availableNodes.remove(updatedShardRouting.relocatingNodeId());
+                }
                 builder.addShard(updatedShardRouting);
             }
         }

core/src/test/java/org/elasticsearch/cluster/health/ClusterStateHealthTests.java

@@ -353,7 +353,7 @@ public class ClusterStateHealthTests extends ESTestCase {
                                                      final int numberOfReplicas,
                                                      final boolean withPrimaryAllocationFailures) {
         // generate random node ids
-        final List<String> nodeIds = new ArrayList<>();
+        final Set<String> nodeIds = new HashSet<>();
         final int numNodes = randomIntBetween(numberOfReplicas + 1, 10);
         for (int i = 0; i < numNodes; i++) {
             nodeIds.add(randomAsciiOfLength(8));
@@ -372,7 +372,7 @@ public class ClusterStateHealthTests extends ESTestCase {
             for (final ShardRouting shardRouting : shardRoutingTable.getShards()) {
                 if (shardRouting.primary()) {
                     newIndexRoutingTable.addShard(
-                        shardRouting.initialize(nodeIds.get(randomIntBetween(0, numNodes - 1)), null, shardRouting.getExpectedShardSize())
+                        shardRouting.initialize(randomFrom(nodeIds), null, shardRouting.getExpectedShardSize())
                     );
                 } else {
                     newIndexRoutingTable.addShard(shardRouting);
@@ -460,17 +460,15 @@ public class ClusterStateHealthTests extends ESTestCase {
         newIndexRoutingTable = IndexRoutingTable.builder(indexRoutingTable.getIndex());
         for (final ObjectCursor<IndexShardRoutingTable> shardEntry : indexRoutingTable.getShards().values()) {
             final IndexShardRoutingTable shardRoutingTable = shardEntry.value;
+            final String primaryNodeId = shardRoutingTable.primaryShard().currentNodeId();
+            Set<String> allocatedNodes = new HashSet<>();
+            allocatedNodes.add(primaryNodeId);
             for (final ShardRouting shardRouting : shardRoutingTable.getShards()) {
                 if (shardRouting.primary() == false) {
                     // give the replica a different node id than the primary
-                    final String primaryNodeId = shardRoutingTable.primaryShard().currentNodeId();
+                    String replicaNodeId = randomFrom(Sets.difference(nodeIds, allocatedNodes));
-                    String replicaNodeId;
+                    newIndexRoutingTable.addShard(shardRouting.initialize(replicaNodeId, null, shardRouting.getExpectedShardSize()));
-                    do {
+                    allocatedNodes.add(replicaNodeId);
-                        replicaNodeId = nodeIds.get(randomIntBetween(0, numNodes - 1));
-                    } while (primaryNodeId.equals(replicaNodeId));
-                    newIndexRoutingTable.addShard(
-                        shardRouting.initialize(replicaNodeId, null, shardRouting.getExpectedShardSize())
-                    );
                 } else {
                     newIndexRoutingTable.addShard(shardRouting);
                 }

core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java

@@ -19,6 +19,8 @@
 
 package org.elasticsearch.cluster.routing;
 
+import java.util.Set;
+
 import static org.elasticsearch.test.ESTestCase.randomAsciiOfLength;
 import static org.elasticsearch.test.ESTestCase.randomFrom;
 import static org.elasticsearch.test.ESTestCase.randomInt;
@@ -31,7 +33,7 @@ public final class RandomShardRoutingMutator {
 
     }
 
-    public static ShardRouting randomChange(ShardRouting shardRouting, String[] nodes) {
+    public static ShardRouting randomChange(ShardRouting shardRouting, Set<String> nodes) {
         switch (randomInt(2)) {
             case 0:
                 if (shardRouting.unassigned() == false && shardRouting.primary() == false) {
@@ -42,7 +44,7 @@ public final class RandomShardRoutingMutator {
                 }
                 break;
             case 1:
-                if (shardRouting.unassigned()) {
+                if (shardRouting.unassigned() && nodes.isEmpty() == false) {
                     shardRouting = shardRouting.initialize(randomFrom(nodes), null, -1);
                 }
                 break;

core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java

@@ -28,9 +28,12 @@ import org.elasticsearch.cluster.node.DiscoveryNodes;
 import org.elasticsearch.cluster.node.DiscoveryNodes.Builder;
 import org.elasticsearch.cluster.routing.allocation.AllocationService;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexNotFoundException;
+import org.elasticsearch.index.shard.ShardId;
 import org.junit.Before;
 
+import java.util.Arrays;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -328,6 +331,19 @@ public class RoutingTableTests extends ESAllocationTestCase {
         expectThrows(IllegalStateException.class, () -> indexRoutingTable.validate(metaData4));
     }
 
+    public void testDistinctNodes() {
+        ShardId shardId = new ShardId(new Index("index", "uuid"), 0);
+        ShardRouting routing1 = TestShardRouting.newShardRouting(shardId, "node1", randomBoolean(), ShardRoutingState.STARTED);
+        ShardRouting routing2 = TestShardRouting.newShardRouting(shardId, "node2", randomBoolean(), ShardRoutingState.STARTED);
+        ShardRouting routing3 = TestShardRouting.newShardRouting(shardId, "node1", randomBoolean(), ShardRoutingState.STARTED);
+        ShardRouting routing4 = TestShardRouting.newShardRouting(shardId, "node3", "node2", randomBoolean(), ShardRoutingState.RELOCATING);
+        assertTrue(IndexShardRoutingTable.Builder.distinctNodes(Arrays.asList(routing1, routing2)));
+        assertFalse(IndexShardRoutingTable.Builder.distinctNodes(Arrays.asList(routing1, routing3)));
+        assertFalse(IndexShardRoutingTable.Builder.distinctNodes(Arrays.asList(routing1, routing2, routing3)));
+        assertTrue(IndexShardRoutingTable.Builder.distinctNodes(Arrays.asList(routing1, routing4)));
+        assertFalse(IndexShardRoutingTable.Builder.distinctNodes(Arrays.asList(routing2, routing4)));
+    }
+
     /** reverse engineer the in sync aid based on the given indexRoutingTable **/
     public static IndexMetaData updateActiveAllocations(IndexRoutingTable indexRoutingTable, IndexMetaData indexMetaData) {
         IndexMetaData.Builder imdBuilder = IndexMetaData.builder(indexMetaData);

core/src/test/java/org/elasticsearch/indices/store/IndicesStoreTests.java

@@ -71,13 +71,7 @@ public class IndicesStoreTests extends ESTestCase {
     }
 
     public void testShardCanBeDeletedNoShardRouting() throws Exception {
-        int numShards = randomIntBetween(1, 7);
-        int numReplicas = randomInt(2);
-
-        ClusterState.Builder clusterState = ClusterState.builder(new ClusterName("test"));
-        clusterState.metaData(MetaData.builder().put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(numShards).numberOfReplicas(numReplicas)));
         IndexShardRoutingTable.Builder routingTable = new IndexShardRoutingTable.Builder(new ShardId("test", "_na_", 1));
-
         assertFalse(IndicesStore.shardCanBeDeleted(localNode.getId(), routingTable.build()));
     }
 
@@ -85,8 +79,6 @@ public class IndicesStoreTests extends ESTestCase {
         int numShards = randomIntBetween(1, 7);
         int numReplicas = randomInt(2);
 
-        ClusterState.Builder clusterState = ClusterState.builder(new ClusterName("test"));
-        clusterState.metaData(MetaData.builder().put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(numShards).numberOfReplicas(numReplicas)));
         IndexShardRoutingTable.Builder routingTable = new IndexShardRoutingTable.Builder(new ShardId("test", "_na_", 1));
 
         for (int i = 0; i < numShards; i++) {
@@ -102,7 +94,8 @@ public class IndicesStoreTests extends ESTestCase {
                 if (state == ShardRoutingState.UNASSIGNED) {
                     unassignedInfo = new UnassignedInfo(UnassignedInfo.Reason.INDEX_CREATED, null);
                 }
-                routingTable.addShard(TestShardRouting.newShardRouting("test", i, randomBoolean() ? localNode.getId() : randomAsciiOfLength(10), null, j == 0, state, unassignedInfo));
+                String relocatingNodeId = state == ShardRoutingState.RELOCATING ? randomAsciiOfLength(10) : null;
+                routingTable.addShard(TestShardRouting.newShardRouting("test", i, randomAsciiOfLength(10), relocatingNodeId, j == 0, state, unassignedInfo));
             }
         }
 
@@ -113,69 +106,19 @@ public class IndicesStoreTests extends ESTestCase {
         int numShards = randomIntBetween(1, 7);
         int numReplicas = randomInt(2);
 
-        ClusterState.Builder clusterState = ClusterState.builder(new ClusterName("test"));
-        clusterState.metaData(MetaData.builder().put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(numShards).numberOfReplicas(numReplicas)));
-        clusterState.nodes(DiscoveryNodes.builder().localNodeId(localNode.getId()).add(localNode).add(new DiscoveryNode("xyz",
-                buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT)));
         IndexShardRoutingTable.Builder routingTable = new IndexShardRoutingTable.Builder(new ShardId("test", "_na_", 1));
         int localShardId = randomInt(numShards - 1);
         for (int i = 0; i < numShards; i++) {
-            String nodeId = i == localShardId ? localNode.getId() : randomBoolean() ? "abc" : "xyz";
+            int localNodeIndex = randomInt(numReplicas);
-            String relocationNodeId = randomBoolean() ? null : randomBoolean() ? localNode.getId() : "xyz";
+            boolean primaryOnLocalNode = i == localShardId && localNodeIndex == numReplicas;
-            routingTable.addShard(TestShardRouting.newShardRouting("test", i, nodeId, relocationNodeId, true, ShardRoutingState.STARTED));
+            routingTable.addShard(TestShardRouting.newShardRouting("test", i, primaryOnLocalNode ? localNode.getId() : randomAsciiOfLength(10), true, ShardRoutingState.STARTED));
             for (int j = 0; j < numReplicas; j++) {
-                routingTable.addShard(TestShardRouting.newShardRouting("test", i, nodeId, relocationNodeId, false, ShardRoutingState.STARTED));
+                boolean replicaOnLocalNode = i == localShardId && localNodeIndex == j;
+                routingTable.addShard(TestShardRouting.newShardRouting("test", i, replicaOnLocalNode ? localNode.getId() : randomAsciiOfLength(10), false, ShardRoutingState.STARTED));
             }
         }
 
         // Shard exists locally, can't delete shard
         assertFalse(IndicesStore.shardCanBeDeleted(localNode.getId(), routingTable.build()));
     }
-
-    public void testShardCanBeDeletedNodeVersion() throws Exception {
-        int numShards = randomIntBetween(1, 7);
-        int numReplicas = randomInt(2);
-
-        // Most of the times don't test bwc and use current version
-        final Version nodeVersion = randomBoolean() ? CURRENT : randomVersion(random());
-        ClusterState.Builder clusterState = ClusterState.builder(new ClusterName("test"));
-        clusterState.metaData(MetaData.builder().put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(numShards).numberOfReplicas(numReplicas)));
-        clusterState.nodes(DiscoveryNodes.builder().localNodeId(localNode.getId()).add(localNode).add(new DiscoveryNode("xyz",
-                buildNewFakeTransportAddress(), emptyMap(), emptySet(), nodeVersion)));
-        IndexShardRoutingTable.Builder routingTable = new IndexShardRoutingTable.Builder(new ShardId("test", "_na_", 1));
-        for (int i = 0; i < numShards; i++) {
-            routingTable.addShard(TestShardRouting.newShardRouting("test", i, "xyz", null, true, ShardRoutingState.STARTED));
-            for (int j = 0; j < numReplicas; j++) {
-                routingTable.addShard(TestShardRouting.newShardRouting("test", i, "xyz", null, false, ShardRoutingState.STARTED));
-            }
-        }
-
-        // shard exist on other node (abc)
-        assertTrue(IndicesStore.shardCanBeDeleted(localNode.getId(), routingTable.build()));
-    }
-
-    public void testShardCanBeDeletedRelocatingNode() throws Exception {
-        int numShards = randomIntBetween(1, 7);
-        int numReplicas = randomInt(2);
-
-        ClusterState.Builder clusterState = ClusterState.builder(new ClusterName("test"));
-        clusterState.metaData(MetaData.builder().put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(numShards).numberOfReplicas(numReplicas)));
-        final Version nodeVersion = randomBoolean() ? CURRENT : randomVersion(random());
-
-        clusterState.nodes(DiscoveryNodes.builder().localNodeId(localNode.getId())
-                .add(localNode)
-                .add(new DiscoveryNode("xyz", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT))
-                .add(new DiscoveryNode("def", buildNewFakeTransportAddress(), emptyMap(), emptySet(), nodeVersion) // <-- only set relocating, since we're testing that in this test
-                ));
-        IndexShardRoutingTable.Builder routingTable = new IndexShardRoutingTable.Builder(new ShardId("test", "_na_", 1));
-        for (int i = 0; i < numShards; i++) {
-            routingTable.addShard(TestShardRouting.newShardRouting("test", i, "xyz", "def", true, ShardRoutingState.STARTED));
-            for (int j = 0; j < numReplicas; j++) {
-                routingTable.addShard(TestShardRouting.newShardRouting("test", i, "xyz", "def", false, ShardRoutingState.STARTED));
-            }
-        }
-
-        // shard exist on other node (abc and def)
-        assertTrue(IndicesStore.shardCanBeDeleted(localNode.getId(), routingTable.build()));
-    }
 }