From b1b2d90dfd4da5ff4982d06c61a7db338cf941c3 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 19 Jul 2019 16:55:55 +0900 Subject: [PATCH 1/5] Check shard limit after applying index templates Today when creating an index and checking cluster shard limits, we check the number of shards before applying index templates. At this point, we do not know the actual number of shards that will be used to create the index. In a case when the defaults are used and a template would override, we could be grossly underestimating the number of shards that would be created, and thus incorrectly applying the limits. This commit addresses this by checking the shard limits after applying index templates. --- .../metadata/MetaDataCreateIndexService.java | 32 ++++++++----- .../snapshots/RestoreService.java | 1 + .../MetaDataCreateIndexServiceTests.java | 20 ++++++-- .../cluster/shards/ClusterShardLimitIT.java | 46 +++++++++++++++++++ 4 files changed, 83 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 56d8213c6ce87..be754666158bc 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -437,6 +437,13 @@ public ClusterState execute(ClusterState currentState) throws Exception { indexScopedSettings); } final Settings actualIndexSettings = indexSettingsBuilder.build(); + + /* + * We can not check the shard limit until we have applied templates, otherwise we do not know the actual + * number of shards that will be used to create this index. + */ + checkShardLimit(actualIndexSettings, currentState); + tmpImdBuilder.settings(actualIndexSettings); if (recoverFromIndex != null) { @@ -607,9 +614,6 @@ public void validateIndexSettings(String indexName, final Settings settings, fin final boolean forbidPrivateIndexSettings) throws IndexCreationException { List validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings); - Optional shardAllocation = checkShardLimit(settings, clusterState); - shardAllocation.ifPresent(validationErrors::add); - if (validationErrors.isEmpty() == false) { ValidationException validationException = new ValidationException(); validationException.addValidationErrors(validationErrors); @@ -620,15 +624,21 @@ public void validateIndexSettings(String indexName, final Settings settings, fin /** * Checks whether an index can be created without going over the cluster shard limit. * - * @param settings The settings of the index to be created. - * @param clusterState The current cluster state. - * @return If present, an error message to be used to reject index creation. If empty, a signal that this operation may be carried out. + * @param settings the settings of the index to be created + * @param clusterState the current cluster state + * @throws ValidationException if creating this index would put the cluster over the cluster shard limit */ - static Optional checkShardLimit(Settings settings, ClusterState clusterState) { - int shardsToCreate = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings) - * (1 + IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings)); - - return IndicesService.checkShardLimit(shardsToCreate, clusterState); + public static void checkShardLimit(final Settings settings, final ClusterState clusterState) { + final int numberOfShards = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings); + final int numberOfReplicas = IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings); + final int shardsToCreate = numberOfShards * (1 + numberOfReplicas); + + final Optional shardLimit = IndicesService.checkShardLimit(shardsToCreate, clusterState); + if (shardLimit.isPresent()) { + final ValidationException e = new ValidationException(); + e.addValidationError(shardLimit.get()); + throw e; + } } List getIndexSettingsValidationErrors(final Settings settings, final boolean forbidPrivateIndexSettings) { diff --git a/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java b/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java index 4d7ecf5b962a2..ad346426333c5 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java @@ -276,6 +276,7 @@ public ClusterState execute(ClusterState currentState) { indexMdBuilder.settings(Settings.builder() .put(snapshotIndexMetaData.getSettings()) .put(IndexMetaData.SETTING_INDEX_UUID, UUIDs.randomBase64UUID())); + MetaDataCreateIndexService.checkShardLimit(snapshotIndexMetaData.getSettings(), currentState); if (!request.includeAliases() && !snapshotIndexMetaData.getAliases().isEmpty()) { // Remove all aliases - they shouldn't be restored indexMdBuilder.removeAllAliases(); diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java index 667909d644beb..8eb3830781add 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java @@ -37,6 +37,7 @@ import org.elasticsearch.cluster.routing.allocation.decider.MaxRetryAllocationDecider; import org.elasticsearch.cluster.shards.ClusterShardLimitIT; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; @@ -52,6 +53,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Locale; import java.util.Optional; import java.util.Set; import java.util.function.Consumer; @@ -59,13 +61,16 @@ import java.util.stream.Stream; import static java.util.Collections.emptyMap; +import static java.util.Collections.max; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_VERSION_CREATED; import static org.elasticsearch.cluster.shards.ClusterShardLimitIT.ShardCounts.forDataNodeCount; import static org.elasticsearch.indices.IndicesServiceTests.createClusterForShardLimitTest; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasToString; public class MetaDataCreateIndexServiceTests extends ESTestCase { @@ -466,14 +471,19 @@ public void testShardLimit() { .put(SETTING_NUMBER_OF_REPLICAS, counts.getFailingIndexReplicas()) .build(); - DeprecationLogger deprecationLogger = new DeprecationLogger(logger); - Optional errorMessage = MetaDataCreateIndexService.checkShardLimit(indexSettings, state); + final ValidationException e = expectThrows( + ValidationException.class, + () -> MetaDataCreateIndexService.checkShardLimit(indexSettings, state)); int totalShards = counts.getFailingIndexShards() * (1 + counts.getFailingIndexReplicas()); int currentShards = counts.getFirstIndexShards() * (1 + counts.getFirstIndexReplicas()); int maxShards = counts.getShardsPerNode() * nodesInCluster; - assertTrue(errorMessage.isPresent()); - assertEquals("this action would add [" + totalShards + "] total shards, but this cluster currently has [" + currentShards - + "]/[" + maxShards + "] maximum shards open", errorMessage.get()); + final String expectedMessage = String.format( + Locale.ROOT, + "this action would add [%d] total shards, but this cluster currently has [%d]/[%d] maximum shards open", + totalShards, + currentShards, + maxShards); + assertThat(e, hasToString(containsString(expectedMessage))); } } diff --git a/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java b/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java index e79bf35dda335..849cc1ad4efb8 100644 --- a/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java @@ -37,6 +37,7 @@ import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.test.ESIntegTestCase; +import java.util.Collections; import java.util.List; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS; @@ -101,6 +102,51 @@ public void testIndexCreationOverLimit() { assertFalse(clusterState.getMetaData().hasIndex("should-fail")); } + public void testIndexCreationOverLimitFromTemplate() { + int dataNodes = client().admin().cluster().prepareState().get().getState().getNodes().getDataNodes().size(); + + final ShardCounts counts; + { + final ShardCounts temporaryCounts = ShardCounts.forDataNodeCount(dataNodes); + /* + * We are going to create an index that will bring us up to one below the limit; we go one below the limit to ensure the + * template is used instead of one shard. + */ + counts = new ShardCounts( + temporaryCounts.shardsPerNode, + temporaryCounts.firstIndexShards - 1, + temporaryCounts.firstIndexReplicas, + temporaryCounts.failingIndexShards + 1, + temporaryCounts.failingIndexReplicas); + } + setShardsPerNode(counts.getShardsPerNode()); + + if (counts.firstIndexShards > 0) { + createIndex( + "test", + Settings.builder() + .put(indexSettings()) + .put(SETTING_NUMBER_OF_SHARDS, counts.getFirstIndexShards()) + .put(SETTING_NUMBER_OF_REPLICAS, counts.getFirstIndexReplicas()).build()); + } + + assertAcked(client().admin() + .indices() + .preparePutTemplate("should-fail*") + .setPatterns(Collections.singletonList("should-fail")) + .setOrder(1) + .setSettings(Settings.builder() + .put(SETTING_NUMBER_OF_SHARDS, counts.getFailingIndexShards()) + .put(SETTING_NUMBER_OF_REPLICAS, counts.getFailingIndexReplicas())) + .get()); + + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("should-fail").get()); + verifyException(dataNodes, counts, e); + ClusterState clusterState = client().admin().cluster().prepareState().get().getState(); + assertFalse(clusterState.getMetaData().hasIndex("should-fail")); + } + public void testIncreaseReplicasOverLimit() { int dataNodes = client().admin().cluster().prepareState().get().getState().getNodes().getDataNodes().size(); From a6b7bfe5e8a013414a09f56cf9d6ef9d28b6dc30 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 19 Jul 2019 20:03:03 +0900 Subject: [PATCH 2/5] Fix imports --- .../cluster/metadata/MetaDataCreateIndexServiceTests.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java index 8eb3830781add..cfbc7895a1cce 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java @@ -38,7 +38,6 @@ import org.elasticsearch.cluster.shards.ClusterShardLimitIT; import org.elasticsearch.common.Strings; import org.elasticsearch.common.ValidationException; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; @@ -54,14 +53,12 @@ import java.util.Comparator; import java.util.List; import java.util.Locale; -import java.util.Optional; import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; import static java.util.Collections.emptyMap; -import static java.util.Collections.max; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_VERSION_CREATED; From c07f6822f39eece330c4a9fb1c4885971ad7bf50 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 19 Jul 2019 20:40:43 +0900 Subject: [PATCH 3/5] Fix IndexCreationTaskTests --- .../cluster/metadata/MetaDataCreateIndexService.java | 4 ++++ .../cluster/metadata/IndexCreationTaskTests.java | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index be754666158bc..042ae6e0d061d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -594,6 +594,10 @@ public ClusterState execute(ClusterState currentState) throws Exception { } } + protected void checkShardLimit(final Settings settings, final ClusterState clusterState) { + MetaDataCreateIndexService.checkShardLimit(settings, clusterState); + } + @Override public void onFailure(String source, Exception e) { if (e instanceof ResourceAlreadyExistsException) { diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java index f2a87d09eb1f2..ed3d5096cf66d 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java @@ -429,7 +429,14 @@ private ClusterState executeTask() throws Exception { setupRequest(); final MetaDataCreateIndexService.IndexCreationTask task = new MetaDataCreateIndexService.IndexCreationTask( logger, allocationService, request, listener, indicesService, aliasValidator, xContentRegistry, clusterStateSettings.build(), - validator, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS); + validator, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS) { + + @Override + protected void checkShardLimit(final Settings settings, final ClusterState clusterState) { + + } + + }; return task.execute(state); } From f1ea945a372883f0e201746c0a5aa3312c01645c Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 23 Jul 2019 07:47:54 +0900 Subject: [PATCH 4/5] Add comment --- .../elasticsearch/cluster/metadata/IndexCreationTaskTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java index ed3d5096cf66d..691e23bb87ad1 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java @@ -433,7 +433,7 @@ private ClusterState executeTask() throws Exception { @Override protected void checkShardLimit(final Settings settings, final ClusterState clusterState) { - + // we have to make this a no-op since we are not mocking enough for this method to be able to execute } }; From 469f75f4bf2e447abf3b1a48be02938d842a800d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 23 Jul 2019 10:50:49 +0900 Subject: [PATCH 5/5] Fix comment formatting --- .../cluster/metadata/MetaDataCreateIndexService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 042ae6e0d061d..3b500056dfbd9 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -439,8 +439,8 @@ public ClusterState execute(ClusterState currentState) throws Exception { final Settings actualIndexSettings = indexSettingsBuilder.build(); /* - * We can not check the shard limit until we have applied templates, otherwise we do not know the actual - * number of shards that will be used to create this index. + * We can not check the shard limit until we have applied templates, otherwise we do not know the actual number of shards + * that will be used to create this index. */ checkShardLimit(actualIndexSettings, currentState);