Skip to content

Commit 61a6b6d

Browse files
jasontedorjasontedor
committed
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 e0e60b1 commit 61a6b6d

File tree

5 files changed

+95
-20
lines changed

5 files changed

+95
-20
lines changed

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

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
package org.elasticsearch.cluster.metadata;
2121

2222
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
23-
import org.apache.logging.log4j.Logger;
2423
import org.apache.logging.log4j.LogManager;
24+
import org.apache.logging.log4j.Logger;
2525
import org.apache.logging.log4j.message.ParameterizedMessage;
2626
import org.elasticsearch.ElasticsearchException;
2727
import org.elasticsearch.ResourceAlreadyExistsException;
@@ -438,6 +438,13 @@ public ClusterState execute(ClusterState currentState) throws Exception {
438438
indexScopedSettings);
439439
}
440440
final Settings actualIndexSettings = indexSettingsBuilder.build();
441+
442+
/*
443+
* We can not check the shard limit until we have applied templates, otherwise we do not know the actual number of shards
444+
* that will be used to create this index.
445+
*/
446+
checkShardLimit(actualIndexSettings, currentState);
447+
441448
tmpImdBuilder.settings(actualIndexSettings);
442449

443450
if (recoverFromIndex != null) {
@@ -593,7 +600,7 @@ static int getNumberOfShards(final Settings.Builder indexSettingsBuilder) {
593600
assert Version.CURRENT.major == 7;
594601
final int numberOfShards;
595602
final Version indexVersionCreated =
596-
Version.fromId(Integer.parseInt(indexSettingsBuilder.get(IndexMetaData.SETTING_INDEX_VERSION_CREATED.getKey())));
603+
Version.fromId(Integer.parseInt(indexSettingsBuilder.get(IndexMetaData.SETTING_INDEX_VERSION_CREATED.getKey())));
597604
if (indexVersionCreated.before(Version.V_7_0_0)) {
598605
numberOfShards = 5;
599606
} else {
@@ -602,6 +609,10 @@ static int getNumberOfShards(final Settings.Builder indexSettingsBuilder) {
602609
return numberOfShards;
603610
}
604611

612+
protected void checkShardLimit(final Settings settings, final ClusterState clusterState) {
613+
MetaDataCreateIndexService.checkShardLimit(settings, clusterState);
614+
}
615+
605616
@Override
606617
public void onFailure(String source, Exception e) {
607618
if (e instanceof ResourceAlreadyExistsException) {
@@ -622,9 +633,6 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
622633
final boolean forbidPrivateIndexSettings) throws IndexCreationException {
623634
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings);
624635

625-
Optional<String> shardAllocation = checkShardLimit(settings, clusterState);
626-
shardAllocation.ifPresent(validationErrors::add);
627-
628636
if (validationErrors.isEmpty() == false) {
629637
ValidationException validationException = new ValidationException();
630638
validationException.addValidationErrors(validationErrors);
@@ -635,15 +643,21 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
635643
/**
636644
* Checks whether an index can be created without going over the cluster shard limit.
637645
*
638-
* @param settings The settings of the index to be created.
639-
* @param clusterState The current cluster state.
640-
* @return If present, an error message to be used to reject index creation. If empty, a signal that this operation may be carried out.
646+
* @param settings the settings of the index to be created
647+
* @param clusterState the current cluster state
648+
* @throws ValidationException if creating this index would put the cluster over the cluster shard limit
641649
*/
642-
static Optional<String> checkShardLimit(Settings settings, ClusterState clusterState) {
643-
int shardsToCreate = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings)
644-
* (1 + IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings));
650+
public static void checkShardLimit(final Settings settings, final ClusterState clusterState) {
651+
final int numberOfShards = IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings);
652+
final int numberOfReplicas = IndexMetaData.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings);
653+
final int shardsToCreate = numberOfShards * (1 + numberOfReplicas);
645654

646-
return IndicesService.checkShardLimit(shardsToCreate, clusterState);
655+
final Optional<String> shardLimit = IndicesService.checkShardLimit(shardsToCreate, clusterState);
656+
if (shardLimit.isPresent()) {
657+
final ValidationException e = new ValidationException();
658+
e.addValidationError(shardLimit.get());
659+
throw e;
660+
}
647661
}
648662

649663
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
@@ -288,6 +288,7 @@ public ClusterState execute(ClusterState currentState) {
288288
indexMdBuilder.settings(Settings.builder()
289289
.put(snapshotIndexMetaData.getSettings())
290290
.put(IndexMetaData.SETTING_INDEX_UUID, UUIDs.randomBase64UUID()));
291+
MetaDataCreateIndexService.checkShardLimit(snapshotIndexMetaData.getSettings(), currentState);
291292
if (!request.includeAliases() && !snapshotIndexMetaData.getAliases().isEmpty()) {
292293
// Remove all aliases - they shouldn't be restored
293294
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;
@@ -53,7 +53,7 @@
5353
import java.util.Comparator;
5454
import java.util.HashSet;
5555
import java.util.List;
56-
import java.util.Optional;
56+
import java.util.Locale;
5757
import java.util.Set;
5858
import java.util.function.Consumer;
5959
import java.util.stream.Collectors;
@@ -65,8 +65,10 @@
6565
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_VERSION_CREATED;
6666
import static org.elasticsearch.cluster.shards.ClusterShardLimitIT.ShardCounts.forDataNodeCount;
6767
import static org.elasticsearch.indices.IndicesServiceTests.createClusterForShardLimitTest;
68+
import static org.hamcrest.Matchers.containsString;
6869
import static org.hamcrest.Matchers.endsWith;
6970
import static org.hamcrest.Matchers.equalTo;
71+
import static org.hamcrest.Matchers.hasToString;
7072

7173
public class MetaDataCreateIndexServiceTests extends ESTestCase {
7274

@@ -489,14 +491,19 @@ public void testShardLimit() {
489491
.put(SETTING_NUMBER_OF_REPLICAS, counts.getFailingIndexReplicas())
490492
.build();
491493

492-
DeprecationLogger deprecationLogger = new DeprecationLogger(logger);
493-
Optional<String> errorMessage = MetaDataCreateIndexService.checkShardLimit(indexSettings, state);
494+
final ValidationException e = expectThrows(
495+
ValidationException.class,
496+
() -> MetaDataCreateIndexService.checkShardLimit(indexSettings, state));
494497
int totalShards = counts.getFailingIndexShards() * (1 + counts.getFailingIndexReplicas());
495498
int currentShards = counts.getFirstIndexShards() * (1 + counts.getFirstIndexReplicas());
496499
int maxShards = counts.getShardsPerNode() * nodesInCluster;
497-
assertTrue(errorMessage.isPresent());
498-
assertEquals("this action would add [" + totalShards + "] total shards, but this cluster currently has [" + currentShards
499-
+ "]/[" + maxShards + "] maximum shards open", errorMessage.get());
500+
final String expectedMessage = String.format(
501+
Locale.ROOT,
502+
"this action would add [%d] total shards, but this cluster currently has [%d]/[%d] maximum shards open",
503+
totalShards,
504+
currentShards,
505+
maxShards);
506+
assertThat(e, hasToString(containsString(expectedMessage)));
500507
}
501508

502509
}

‎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)