Skip to content

Commit

Permalink
Ignoring system index in replica count validation creation & updation…
Browse files Browse the repository at this point in the history 
… path

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
  • Loading branch information
@gbbafna
gbbafna committed Jul 19, 2022
1 parent dc4ef54 commit ac848dd
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 28 deletions.
18 changes: 16 additions & 2 deletions ...rc/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,29 +608,43 @@ public void testUpdateNumberOfReplicasAllowNoIndices() {

public void testAwarenessReplicaBalance() {
createIndex("aware-replica", Settings.builder().put("index.number_of_replicas", 0).build());
createIndex(".system-index", Settings.builder().put("index.number_of_replicas", 0).build());
manageReplicaBalanceSetting(true);
int updated = 0;

try {
// replica count of 1 is ideal
client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.execute()
.actionGet();
updated++;

// system index - should be able to update
client().admin()
.indices()
.prepareUpdateSettings(".system-index")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2))
.execute()
.actionGet();
fail("should have thrown an exception about the replica count");
updated++;

client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2))
.execute()
.actionGet();
fail("should have thrown an exception about the replica count");

} catch (IllegalArgumentException e) {
assertEquals(
"Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [2];",
e.getMessage()
);
assertEquals(2, updated);
} finally {
manageReplicaBalanceSetting(false);
}
Expand Down
7 changes: 3 additions & 4 deletions ...r/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1037,18 +1037,17 @@ public void testAwarenessReplicaBalance() throws IOException {
.indices()
.preparePutTemplate("template_1")
.setPatterns(Arrays.asList("a*", "b*"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.get();

fail("should have thrown an exception about the replica count");

client().admin()
.indices()
.preparePutTemplate("template_1")
.setPatterns(Arrays.asList("a*", "b*"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0))
.get();

fail("should have thrown an exception about the replica count");
} catch (InvalidIndexTemplateException e) {
assertEquals(
"index_template [template_1] invalid, cause [Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [2];]",
Expand Down
18 changes: 17 additions & 1 deletion server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -978,15 +978,20 @@ public void testRestoreBalancedReplica() {
try {
createRepository("test-repo", "fs");
createIndex("test-index", Settings.builder().put("index.number_of_replicas", 0).build());
createIndex(".system-index", Settings.builder().put("index.number_of_replicas", 0).build());
ensureGreen();
clusterAdmin().prepareCreateSnapshot("test-repo", "snapshot-0").setIndices("test-index").setWaitForCompletion(true).get();
clusterAdmin().prepareCreateSnapshot("test-repo", "snapshot-0")
.setIndices("test-index", ".system-index")
.setWaitForCompletion(true)
.get();
manageReplicaBalanceSetting(true);

final IllegalArgumentException restoreError = expectThrows(
IllegalArgumentException.class,
() -> clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0")
.setRenamePattern("test-index")
.setRenameReplacement("new-index")
.setIndices("test-index")
.get()
);
assertThat(
Expand All @@ -995,10 +1000,21 @@ public void testRestoreBalancedReplica() {
);

RestoreSnapshotResponse restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0")
.setRenamePattern(".system-index")
.setRenameReplacement(".system-index-restore-1")
.setWaitForCompletion(true)
.setIndices(".system-index")
.execute()
.actionGet();

assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));

restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0")
.setRenamePattern("test-index")
.setRenameReplacement("new-index")
.setIndexSettings(Settings.builder().put("index.number_of_replicas", 1).build())
.setWaitForCompletion(true)
.setIndices("test-index")
.execute()
.actionGet();

Expand Down
32 changes: 21 additions & 11 deletions server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,7 @@ private void validate(CreateIndexClusterStateUpdateRequest request, ClusterState

public void validateIndexSettings(String indexName, final Settings settings, final boolean forbidPrivateIndexSettings)
throws IndexCreationException {
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings);
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings, indexName);

if (validationErrors.isEmpty() == false) {
ValidationException validationException = new ValidationException();
Expand All @@ -1164,21 +1164,31 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
}
}

List<String> getIndexSettingsValidationErrors(final Settings settings, final boolean forbidPrivateIndexSettings) {
List<String> getIndexSettingsValidationErrors(final Settings settings, final boolean forbidPrivateIndexSettings, String indexName) {
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings, Optional.of(indexName));
return validationErrors;
}

List<String> getIndexSettingsValidationErrors(
final Settings settings,
final boolean forbidPrivateIndexSettings,
Optional<String> indexName
) {
List<String> validationErrors = validateIndexCustomPath(settings, env.sharedDataDir());
if (forbidPrivateIndexSettings) {
validationErrors.addAll(validatePrivateSettingsNotExplicitlySet(settings, indexScopedSettings));
}

int replicaCount = settings.getAsInt(
IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY)
);
Optional<String> error = awarenessReplicaBalance.validate(replicaCount);
if (error.isPresent()) {
validationErrors.add(error.get());
if (indexName.isEmpty() || indexName.get().charAt(0) != '.') {
// Apply aware replica balance only to non system indices
int replicaCount = settings.getAsInt(
IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY)
);
Optional<String> error = awarenessReplicaBalance.validate(replicaCount);
if (error.isPresent()) {
validationErrors.add(error.get());
}
}

return validationErrors;
}

Expand Down
6 changes: 5 additions & 1 deletion server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,11 @@ private void validate(String name, @Nullable Settings settings, List<String> ind
validationErrors.add(t.getMessage());
}
}
List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(settings, true);
List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(
settings,
true,
Optional.empty()
);
validationErrors.addAll(indexSettingsValidation);
}

Expand Down
17 changes: 11 additions & 6 deletions server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,18 +198,23 @@ public ClusterState execute(ClusterState currentState) {
if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(openSettings)) {
final int updatedNumberOfReplicas = IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(openSettings);
if (preserveExisting == false) {
Optional<String> error = awarenessReplicaBalance.validate(updatedNumberOfReplicas);
if (error.isPresent()) {
ValidationException ex = new ValidationException();
ex.addValidationError(error.get());
throw ex;
for (Index index : request.indices()) {
if (index.getName().charAt(0) != '.') {
// No replica count validation for system indices
Optional<String> error = awarenessReplicaBalance.validate(updatedNumberOfReplicas);
if (error.isPresent()) {
ValidationException ex = new ValidationException();
ex.addValidationError(error.get());
throw ex;
}
}
}

// Verify that this won't take us over the cluster shard limit.
int totalNewShards = Arrays.stream(request.indices())
.mapToInt(i -> getTotalNewShards(i, currentState, updatedNumberOfReplicas))
.sum();
error = shardLimitValidator.checkShardLimit(totalNewShards, currentState);
Optional<String> error = shardLimitValidator.checkShardLimit(totalNewShards, currentState);
if (error.isPresent()) {
ValidationException ex = new ValidationException();
ex.addValidationError(error.get());
Expand Down
7 changes: 4 additions & 3 deletions server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -1099,7 +1100,7 @@ public void testvalidateIndexSettings() {
new AwarenessReplicaBalance(settings, clusterService.getClusterSettings())
);

List<String> validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false);
List<String> validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false, Optional.empty());
assertThat(validationErrors.size(), is(1));
assertThat(validationErrors.get(0), is("expected total copies needs to be a multiple of total awareness attributes [3]"));

Expand All @@ -1111,7 +1112,7 @@ public void testvalidateIndexSettings() {
.put(SETTING_NUMBER_OF_REPLICAS, 2)
.build();

validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false);
validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false, Optional.empty());
assertThat(validationErrors.size(), is(0));

threadPool.shutdown();
Expand Down Expand Up @@ -1243,7 +1244,7 @@ public void testIndexLifecycleNameSetting() {
new AwarenessReplicaBalance(Settings.EMPTY, clusterService.getClusterSettings())
);

final List<String> validationErrors = checkerService.getIndexSettingsValidationErrors(ilnSetting, true);
final List<String> validationErrors = checkerService.getIndexSettingsValidationErrors(ilnSetting, true, Optional.empty());
assertThat(validationErrors.size(), is(1));
assertThat(validationErrors.get(0), is("expected [index.lifecycle.name] to be private but it was not"));
}));
Expand Down

0 comments on commit ac848dd

Please sign in to comment.