diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java b/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java index 619959923e9..a81bfdd1732 100644 --- a/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java +++ b/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 { } 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 { } 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 shards) { + Set 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); diff --git a/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java b/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java index 81bc7db0b6b..9d44dbbca38 100644 --- a/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java +++ b/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 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 : original.shards().values()) { + Set 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); } } diff --git a/core/src/test/java/org/elasticsearch/cluster/health/ClusterStateHealthTests.java b/core/src/test/java/org/elasticsearch/cluster/health/ClusterStateHealthTests.java index eb5c88d7e83..feaeee703b6 100644 --- a/core/src/test/java/org/elasticsearch/cluster/health/ClusterStateHealthTests.java +++ b/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 nodeIds = new ArrayList<>(); + final Set 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 shardEntry : indexRoutingTable.getShards().values()) { final IndexShardRoutingTable shardRoutingTable = shardEntry.value; + final String primaryNodeId = shardRoutingTable.primaryShard().currentNodeId(); + Set 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; - do { - replicaNodeId = nodeIds.get(randomIntBetween(0, numNodes - 1)); - } while (primaryNodeId.equals(replicaNodeId)); - newIndexRoutingTable.addShard( - shardRouting.initialize(replicaNodeId, null, shardRouting.getExpectedShardSize()) - ); + String replicaNodeId = randomFrom(Sets.difference(nodeIds, allocatedNodes)); + newIndexRoutingTable.addShard(shardRouting.initialize(replicaNodeId, null, shardRouting.getExpectedShardSize())); + allocatedNodes.add(replicaNodeId); } else { newIndexRoutingTable.addShard(shardRouting); } diff --git a/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java b/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java index c3064c7fa9d..69773e99921 100644 --- a/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java +++ b/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 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; diff --git a/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java b/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java index 6ed42ee45aa..e26fece7c6d 100644 --- a/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java +++ b/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); diff --git a/core/src/test/java/org/elasticsearch/indices/store/IndicesStoreTests.java b/core/src/test/java/org/elasticsearch/indices/store/IndicesStoreTests.java index 1b2083fabec..7f0e9350488 100644 --- a/core/src/test/java/org/elasticsearch/indices/store/IndicesStoreTests.java +++ b/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"; - String relocationNodeId = randomBoolean() ? null : randomBoolean() ? localNode.getId() : "xyz"; - routingTable.addShard(TestShardRouting.newShardRouting("test", i, nodeId, relocationNodeId, true, ShardRoutingState.STARTED)); + int localNodeIndex = randomInt(numReplicas); + boolean primaryOnLocalNode = i == localShardId && localNodeIndex == numReplicas; + 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())); - } }