From a9b8224c9b983881f43767d3b87c800e2f3b2048 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 21 Nov 2017 09:06:02 +0100 Subject: [PATCH 1/3] Protect shard splitting from illegal target shards While we have an assertion that checks if the number of routing shards is a multiple of the number of shards we need a real hard exception that checks this way earlier. This change adds a check and test that is executed before we create the index. Relates to #26931 --- .../cluster/metadata/IndexMetaData.java | 7 +++++ .../cluster/metadata/IndexMetaDataTests.java | 4 ++- .../MetaDataCreateIndexServiceTests.java | 18 +++++++------ .../test/indices.split/10_basic.yml | 27 +++++++++++++++++++ 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java index 3d14670e52771..be31b16663bd5 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -1343,6 +1343,13 @@ public static ShardId selectSplitShard(int shardId, IndexMetaData sourceIndexMet + "] must be less that the number of target shards [" + numTargetShards + "]"); } int routingFactor = getRoutingFactor(numSourceShards, numTargetShards); + // now we verify that the numRoutingShards is valid in the source index + int routingNumShards = sourceIndexMetadata.getRoutingNumShards(); + int factor = routingNumShards / numTargetShards; + if (factor * numTargetShards != routingNumShards) { + throw new IllegalStateException("the number of routingShards [" + + routingNumShards + "] must be a multiple of the target shards [" + numTargetShards + "]"); + } // this is just an additional assertion that ensures we are a factor of the routing num shards. assert getRoutingFactor(numTargetShards, sourceIndexMetadata.getRoutingNumShards()) >= 0; return new ShardId(sourceIndexMetadata.getIndex(), shardId/routingFactor); diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java index e83d1fa706cfd..13c6534cf07ea 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java @@ -118,6 +118,8 @@ public void testSelectShrinkShards() { } public void testSelectResizeShards() { + int numTargetShards = randomFrom(4, 6, 8, 12); + IndexMetaData split = IndexMetaData.builder("foo") .settings(Settings.builder() .put("index.version.created", 1) @@ -125,6 +127,7 @@ public void testSelectResizeShards() { .put("index.number_of_replicas", 0) .build()) .creationDate(randomLong()) + .setRoutingNumShards(numTargetShards * 2) .build(); IndexMetaData shrink = IndexMetaData.builder("foo") @@ -135,7 +138,6 @@ public void testSelectResizeShards() { .build()) .creationDate(randomLong()) .build(); - int numTargetShards = randomFrom(4, 6, 8, 12); int shard = randomIntBetween(0, numTargetShards-1); assertEquals(Collections.singleton(IndexMetaData.selectSplitShard(shard, split, numTargetShards)), IndexMetaData.selectRecoverFromShards(shard, split, numTargetShards)); diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java index 39e4a18440931..bb90ee9fee9cf 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java @@ -56,10 +56,12 @@ public class MetaDataCreateIndexServiceTests extends ESTestCase { private ClusterState createClusterState(String name, int numShards, int numReplicas, Settings settings) { + int numRoutingShards = settings.getAsInt(IndexMetaData.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.getKey(), numShards); MetaData.Builder metaBuilder = MetaData.builder(); IndexMetaData indexMetaData = IndexMetaData.builder(name).settings(settings(Version.CURRENT) .put(settings)) - .numberOfShards(numShards).numberOfReplicas(numReplicas).build(); + .numberOfShards(numShards).numberOfReplicas(numReplicas) + .setRoutingNumShards(numRoutingShards).build(); metaBuilder.put(indexMetaData, false); MetaData metaData = metaBuilder.build(); RoutingTable.Builder routingTableBuilder = RoutingTable.builder(); @@ -204,10 +206,13 @@ public void testValidateSplitIndex() { } ).getMessage()); - + int targetShards; + do { + targetShards = randomIntBetween(numShards+1, 100); + } while (isSplitable(numShards, targetShards) == false); ClusterState clusterState = ClusterState.builder(createClusterState("source", numShards, 0, - Settings.builder().put("index.blocks.write", true).build())).nodes(DiscoveryNodes.builder().add(newNode("node1"))) - .build(); + Settings.builder().put("index.blocks.write", true).put("index.number_of_routing_shards", targetShards).build())) + .nodes(DiscoveryNodes.builder().add(newNode("node1"))).build(); AllocationService service = new AllocationService(Settings.builder().build(), new AllocationDeciders(Settings.EMPTY, Collections.singleton(new MaxRetryAllocationDecider(Settings.EMPTY))), new TestGatewayAllocator(), new BalancedShardsAllocator(Settings.EMPTY), EmptyClusterInfoService.INSTANCE); @@ -218,10 +223,7 @@ public void testValidateSplitIndex() { routingTable = service.applyStartedShards(clusterState, routingTable.index("source").shardsWithState(ShardRoutingState.INITIALIZING)).routingTable(); clusterState = ClusterState.builder(clusterState).routingTable(routingTable).build(); - int targetShards; - do { - targetShards = randomIntBetween(numShards+1, 100); - } while (isSplitable(numShards, targetShards) == false); + MetaDataCreateIndexService.validateSplitIndex(clusterState, "source", Collections.emptySet(), "target", Settings.builder().put("index.number_of_shards", targetShards).build()); } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml index 82881564a2043..4eabdb0872a9f 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml @@ -97,5 +97,32 @@ - match: { _id: "3" } - match: { _source: { foo: "hello world 3" } } + # try to do an illegal split with number_of_routing_shards set + - do: + catch: /illegal_argument_exception/ + indices.split: + index: "source" + target: "target" + wait_for_active_shards: 1 + master_timeout: 10s + body: + settings: + index.number_of_replicas: 0 + index.number_of_shards: 2 + index.number_of_routing_shards: 4 + + # try to do an illegal split with illegal number_of_shards + - do: + catch: /illegal_state_exception/ + indices.split: + index: "source" + target: "target" + wait_for_active_shards: 1 + master_timeout: 10s + body: + settings: + index.number_of_replicas: 0 + index.number_of_shards: 3 + From 9940256f978b4b5301b042a448b1d24f06532475 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 21 Nov 2017 10:25:07 +0100 Subject: [PATCH 2/3] use modulo and add simple unittest --- .../org/elasticsearch/cluster/metadata/IndexMetaData.java | 5 ++--- .../elasticsearch/cluster/metadata/IndexMetaDataTests.java | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java index be31b16663bd5..fd089b7035581 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -1345,9 +1345,8 @@ public static ShardId selectSplitShard(int shardId, IndexMetaData sourceIndexMet int routingFactor = getRoutingFactor(numSourceShards, numTargetShards); // now we verify that the numRoutingShards is valid in the source index int routingNumShards = sourceIndexMetadata.getRoutingNumShards(); - int factor = routingNumShards / numTargetShards; - if (factor * numTargetShards != routingNumShards) { - throw new IllegalStateException("the number of routingShards [" + if (routingNumShards % numTargetShards != 0) { + throw new IllegalStateException("the number of routing shards [" + routingNumShards + "] must be a multiple of the target shards [" + numTargetShards + "]"); } // this is just an additional assertion that ensures we are a factor of the routing num shards. diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java index 13c6534cf07ea..3f21bd29ff3b8 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java @@ -175,6 +175,9 @@ public void testSelectSplitShard() { assertEquals("the number of source shards [2] must be a must be a factor of [3]", expectThrows(IllegalArgumentException.class, () -> IndexMetaData.selectSplitShard(0, metaData, 3)).getMessage()); + + assertEquals("the number of routing shards [4] must be a multiple of the target shards [8]", + expectThrows(IllegalStateException.class, () -> IndexMetaData.selectSplitShard(0, metaData, 8)).getMessage()); } public void testIndexFormat() { From 6ad0727c953ec07d7e2053e5196d9bf62a5b6f1f Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 21 Nov 2017 10:34:32 +0100 Subject: [PATCH 3/3] fix test to not run against BWC version before we backported --- .../test/indices.split/10_basic.yml | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml index 4eabdb0872a9f..35253f1dd813d 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml @@ -1,8 +1,5 @@ --- -"Split index via API": - - skip: - version: " - 6.0.99" - reason: Added in 6.1.0 +setup: - do: indices.create: index: source @@ -33,6 +30,12 @@ id: "3" body: { "foo": "hello world 3" } +--- +"Split index via API": + - skip: + version: " - 6.0.99" + reason: Added in 6.1.0 + # make it read-only - do: indices.put_settings: @@ -97,6 +100,12 @@ - match: { _id: "3" } - match: { _source: { foo: "hello world 3" } } +--- +"Create illegal split indices": + - skip: + version: " - 6.99.99" + reason: fixed in 7.0.0 + # try to do an illegal split with number_of_routing_shards set - do: catch: /illegal_argument_exception/