Skip to content

Commit 5cdbefd

Browse files
jasontedorjasontedor
authored
Check shard limit after applying index templates (#44619)
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.
1 parent c5051dc commit 5cdbefd

File tree

5 files changed

+94
-19
lines changed

5 files changed

+94
-19
lines changed

‎server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,13 @@ public ClusterState execute(ClusterState currentState) throws Exception {
437437
indexScopedSettings);
438438
}
439439
final Settings actualIndexSettings = indexSettingsBuilder.build();
440+
441+
/*
442+
* We can not check the shard limit until we have applied templates, otherwise we do not know the actual number of shards
443+
* that will be used to create this index.
444+
*/
445+
checkShardLimit(actualIndexSettings, currentState);
446+
440447
tmpImdBuilder.settings(actualIndexSettings);
441448

442449
if (recoverFromIndex != null) {
@@ -587,6 +594,10 @@ public ClusterState execute(ClusterState currentState) throws Exception {
587594
}
588595
}
589596

597+
protected void checkShardLimit(final Settings settings, final ClusterState clusterState) {
598+
MetaDataCreateIndexService.checkShardLimit(settings, clusterState);
599+
}
600+
590601
@Override
591602
public void onFailure(String source, Exception e) {
592603
if (e instanceof ResourceAlreadyExistsException) {
@@ -607,9 +618,6 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
607618
final boolean forbidPrivateIndexSettings) throws IndexCreationException {
608619
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings);
609620

610-
Optional<String> shardAllocation = checkShardLimit(settings, clusterState);
611-
shardAllocation.ifPresent(validationErrors::add);
612-
613621
if (validationErrors.isEmpty() == false) {
614622
ValidationException validationException = new ValidationException();
615623
validationException.addValidationErrors(validationErrors);
@@ -620,15 +628,21 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
620628
/**
621629
* Checks whether an index can be created without going over the cluster shard limit.
622630
*
623-
* @param settings The settings of the index to be created.
624-
* @param clusterState The current cluster state.
625-
* @return If present, an error message to be used to reject index creation. If empty, a signal that this operation may be carried out.
631+
* @param settings the settings of the index to be created
632+
* @param clusterState the current cluster state
633+
* @throws ValidationException if creating this index would put the cluster over the cluster shard limit
626634
*/
627-
static Optional<String> checkShardLimit(Settings settings, ClusterState clusterState) {
628-
int shardsToCreate = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings)
629-
* (1 + IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings));
630-
631-
return IndicesService.checkShardLimit(shardsToCreate, clusterState);
635+
public static void checkShardLimit(final Settings settings, final ClusterState clusterState) {
636+
final int numberOfShards = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings);
637+
final int numberOfReplicas = IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings);
638+
final int shardsToCreate = numberOfShards * (1 + numberOfReplicas);
639+
640+
final Optional<String> shardLimit = IndicesService.checkShardLimit(shardsToCreate, clusterState);
641+
if (shardLimit.isPresent()) {
642+
final ValidationException e = new ValidationException();
643+
e.addValidationError(shardLimit.get());
644+
throw e;
645+
}
632646
}
633647

634648
List<String> getIndexSettingsValidationErrors(final Settings settings, final boolean forbidPrivateIndexSettings) {

‎server/src/main/java/org/elasticsearch/snapshots/RestoreService.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@ public ClusterState execute(ClusterState currentState) {
276276
indexMdBuilder.settings(Settings.builder()
277277
.put(snapshotIndexMetaData.getSettings())
278278
.put(IndexMetaData.SETTING_INDEX_UUID, UUIDs.randomBase64UUID()));
279+
MetaDataCreateIndexService.checkShardLimit(snapshotIndexMetaData.getSettings(), currentState);
279280
if (!request.includeAliases() && !snapshotIndexMetaData.getAliases().isEmpty()) {
280281
// Remove all aliases - they shouldn't be restored
281282
indexMdBuilder.removeAllAliases();

‎server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,14 @@ private ClusterState executeTask() throws Exception {
429429
setupRequest();
430430
final MetaDataCreateIndexService.IndexCreationTask task = new MetaDataCreateIndexService.IndexCreationTask(
431431
logger, allocationService, request, listener, indicesService, aliasValidator, xContentRegistry, clusterStateSettings.build(),
432-
validator, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS);
432+
validator, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS) {
433+
434+
@Override
435+
protected void checkShardLimit(final Settings settings, final ClusterState clusterState) {
436+
// we have to make this a no-op since we are not mocking enough for this method to be able to execute
437+
}
438+
439+
};
433440
return task.execute(state);
434441
}
435442

‎server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
import org.elasticsearch.cluster.routing.allocation.decider.MaxRetryAllocationDecider;
3838
import org.elasticsearch.cluster.shards.ClusterShardLimitIT;
3939
import org.elasticsearch.common.Strings;
40-
import org.elasticsearch.common.logging.DeprecationLogger;
40+
import org.elasticsearch.common.ValidationException;
4141
import org.elasticsearch.common.settings.IndexScopedSettings;
4242
import org.elasticsearch.common.settings.Setting;
4343
import org.elasticsearch.common.settings.Settings;
@@ -52,7 +52,7 @@
5252
import java.util.Collections;
5353
import java.util.Comparator;
5454
import java.util.List;
55-
import java.util.Optional;
55+
import java.util.Locale;
5656
import java.util.Set;
5757
import java.util.function.Consumer;
5858
import java.util.stream.Collectors;
@@ -64,8 +64,10 @@
6464
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_VERSION_CREATED;
6565
import static org.elasticsearch.cluster.shards.ClusterShardLimitIT.ShardCounts.forDataNodeCount;
6666
import static org.elasticsearch.indices.IndicesServiceTests.createClusterForShardLimitTest;
67+
import static org.hamcrest.Matchers.containsString;
6768
import static org.hamcrest.Matchers.endsWith;
6869
import static org.hamcrest.Matchers.equalTo;
70+
import static org.hamcrest.Matchers.hasToString;
6971

7072
public class MetaDataCreateIndexServiceTests extends ESTestCase {
7173

@@ -466,14 +468,19 @@ public void testShardLimit() {
466468
.put(SETTING_NUMBER_OF_REPLICAS, counts.getFailingIndexReplicas())
467469
.build();
468470

469-
DeprecationLogger deprecationLogger = new DeprecationLogger(logger);
470-
Optional<String> errorMessage = MetaDataCreateIndexService.checkShardLimit(indexSettings, state);
471+
final ValidationException e = expectThrows(
472+
ValidationException.class,
473+
() -> MetaDataCreateIndexService.checkShardLimit(indexSettings, state));
471474
int totalShards = counts.getFailingIndexShards() * (1 + counts.getFailingIndexReplicas());
472475
int currentShards = counts.getFirstIndexShards() * (1 + counts.getFirstIndexReplicas());
473476
int maxShards = counts.getShardsPerNode() * nodesInCluster;
474-
assertTrue(errorMessage.isPresent());
475-
assertEquals("this action would add [" + totalShards + "] total shards, but this cluster currently has [" + currentShards
476-
+ "]/[" + maxShards + "] maximum shards open", errorMessage.get());
477+
final String expectedMessage = String.format(
478+
Locale.ROOT,
479+
"this action would add [%d] total shards, but this cluster currently has [%d]/[%d] maximum shards open",
480+
totalShards,
481+
currentShards,
482+
maxShards);
483+
assertThat(e, hasToString(containsString(expectedMessage)));
477484
}
478485

479486
}

‎server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.elasticsearch.snapshots.SnapshotState;
3838
import org.elasticsearch.test.ESIntegTestCase;
3939

40+
import java.util.Collections;
4041
import java.util.List;
4142

4243
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
@@ -101,6 +102,51 @@ public void testIndexCreationOverLimit() {
101102
assertFalse(clusterState.getMetaData().hasIndex("should-fail"));
102103
}
103104

105+
public void testIndexCreationOverLimitFromTemplate() {
106+
int dataNodes = client().admin().cluster().prepareState().get().getState().getNodes().getDataNodes().size();
107+
108+
final ShardCounts counts;
109+
{
110+
final ShardCounts temporaryCounts = ShardCounts.forDataNodeCount(dataNodes);
111+
/*
112+
* 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
113+
* template is used instead of one shard.
114+
*/
115+
counts = new ShardCounts(
116+
temporaryCounts.shardsPerNode,
117+
temporaryCounts.firstIndexShards - 1,
118+
temporaryCounts.firstIndexReplicas,
119+
temporaryCounts.failingIndexShards + 1,
120+
temporaryCounts.failingIndexReplicas);
121+
}
122+
setShardsPerNode(counts.getShardsPerNode());
123+
124+
if (counts.firstIndexShards > 0) {
125+
createIndex(
126+
"test",
127+
Settings.builder()
128+
.put(indexSettings())
129+
.put(SETTING_NUMBER_OF_SHARDS, counts.getFirstIndexShards())
130+
.put(SETTING_NUMBER_OF_REPLICAS, counts.getFirstIndexReplicas()).build());
131+
}
132+
133+
assertAcked(client().admin()
134+
.indices()
135+
.preparePutTemplate("should-fail*")
136+
.setPatterns(Collections.singletonList("should-fail"))
137+
.setOrder(1)
138+
.setSettings(Settings.builder()
139+
.put(SETTING_NUMBER_OF_SHARDS, counts.getFailingIndexShards())
140+
.put(SETTING_NUMBER_OF_REPLICAS, counts.getFailingIndexReplicas()))
141+
.get());
142+
143+
final IllegalArgumentException e =
144+
expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("should-fail").get());
145+
verifyException(dataNodes, counts, e);
146+
ClusterState clusterState = client().admin().cluster().prepareState().get().getState();
147+
assertFalse(clusterState.getMetaData().hasIndex("should-fail"));
148+
}
149+
104150
public void testIncreaseReplicasOverLimit() {
105151
int dataNodes = client().admin().cluster().prepareState().get().getState().getNodes().getDataNodes().size();
106152

0 commit comments

Comments
 (0)