From 3dc4fef3523f6a81b73464fd0cd2310d19fe2c5b Mon Sep 17 00:00:00 2001 From: Arpit-Bandejiya <109738717+Arpit-Bandejiya@users.noreply.github.com> Date: Thu, 3 Nov 2022 22:48:14 +0530 Subject: [PATCH] [Backport 2.x] Add AutoExpandReplica in the validation of replica count enforcement (#5053) * Add AutoExpandReplica in the validation of replica count enforcement Signed-off-by: Arpit Bandejiya --- CHANGELOG.md | 1 + .../admin/indices/rollover/RolloverIT.java | 56 +++++++++++-- .../settings/UpdateNumberOfReplicasIT.java | 17 +++- .../template/SimpleIndexTemplateIT.java | 11 +++ .../snapshots/RestoreSnapshotIT.java | 27 ++++++ .../cluster/metadata/AutoExpandReplicas.java | 8 ++ .../metadata/MetadataCreateIndexService.java | 3 +- .../MetadataUpdateSettingsService.java | 6 +- .../allocation/AwarenessReplicaBalance.java | 23 +++-- .../AwarenessReplicaBalanceTests.java | 84 +++++++++++++++++-- 10 files changed, 213 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b37f76118a85..1929bfc5aa1c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## [Unreleased] ### Added +- Add fix for auto expand replica validation ([#4994](https://github.com/opensearch-project/OpenSearch/pull/4994)) - Add support for s390x architecture ([#4001](https://github.com/opensearch-project/OpenSearch/pull/4001)) - Github workflow for changelog verification ([#4085](https://github.com/opensearch-project/OpenSearch/pull/4085)) - Point in time rest layer changes for create and delete PIT API ([#4064](https://github.com/opensearch-project/OpenSearch/pull/4064)) diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java index d0ff3ef19a028..53870ffb12541 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java @@ -237,19 +237,65 @@ public void testRolloverWithIndexSettingsBalancedReplica() throws Exception { containsString("expected total copies needs to be a multiple of total awareness attributes [2]") ); - final Settings balancedReplicaSettings = Settings.builder() - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) - .build(); + client().admin() + .indices() + .prepareRolloverIndex("test_alias") + .settings( + Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1).build() + ) + .alias(new Alias("extra_alias")) + .waitForActiveShards(0) + .get(); client().admin() .indices() .prepareRolloverIndex("test_alias") - .settings(balancedReplicaSettings) + .settings( + Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2) + .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1") + .build() + ) .alias(new Alias("extra_alias")) .waitForActiveShards(0) .get(); + client().admin() + .indices() + .prepareRolloverIndex("test_alias") + .settings( + Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2) + .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-all") + .build() + ) + .alias(new Alias("extra_alias")) + .waitForActiveShards(0) + .get(); + + final IllegalArgumentException restoreError2 = expectThrows( + IllegalArgumentException.class, + () -> client().admin() + .indices() + .prepareRolloverIndex("test_alias") + .settings( + Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2) + .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-0") + .build() + ) + .alias(new Alias("extra_alias")) + .get() + ); + + assertThat( + restoreError2.getMessage(), + containsString("expected max cap on auto expand to be a multiple of total awareness attributes [2]") + ); + manageReplicaBalanceSetting(false); } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java index 98001b447e8b2..9eb86b7d8df96 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java @@ -622,6 +622,19 @@ public void testAwarenessReplicaBalance() { .actionGet(); updated++; + // Since auto expand replica setting take precedence, this should pass + client().admin() + .indices() + .prepareUpdateSettings("aware-replica") + .setSettings( + Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2) + .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1") + ) + .execute() + .actionGet(); + updated++; + // system index - should be able to update client().admin() .indices() @@ -637,14 +650,14 @@ public void testAwarenessReplicaBalance() { .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2)) .execute() .actionGet(); - fail("should have thrown an exception about the replica count"); + 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); + assertEquals(3, updated); } finally { manageReplicaBalanceSetting(false); } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java index 42c0145676f2d..c97b8e932effc 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java @@ -1032,6 +1032,7 @@ public void testPartitionedTemplate() throws Exception { public void testAwarenessReplicaBalance() throws IOException { manageReplicaBalanceSetting(true); + int updated = 0; try { client().admin() .indices() @@ -1039,6 +1040,15 @@ public void testAwarenessReplicaBalance() throws IOException { .setPatterns(Arrays.asList("a*", "b*")) .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)) .get(); + updated++; + + client().admin() + .indices() + .preparePutTemplate("template_1") + .setPatterns(Arrays.asList("a*", "b*")) + .setSettings(Settings.builder().put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")) + .get(); + updated++; client().admin() .indices() @@ -1053,6 +1063,7 @@ public void testAwarenessReplicaBalance() throws IOException { "index_template [template_1] invalid, cause [Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [2];]", e.getMessage() ); + assertEquals(2, updated); } finally { manageReplicaBalanceSetting(false); } diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java index a40378b9c2dfa..8e2a1003585b9 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java @@ -999,6 +999,22 @@ public void testRestoreBalancedReplica() { containsString("expected total copies needs to be a multiple of total awareness attributes [2]") ); + final IllegalArgumentException restoreError2 = expectThrows( + IllegalArgumentException.class, + () -> clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0") + .setRenamePattern("test-index") + .setRenameReplacement("new-index-2") + .setIndexSettings( + Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 1).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-2").build() + ) + .setIndices("test-index") + .get() + ); + assertThat( + restoreError2.getMessage(), + containsString("expected max cap on auto expand to be a multiple of total awareness attributes [2]") + ); + RestoreSnapshotResponse restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0") .setRenamePattern(".system-index") .setRenameReplacement(".system-index-restore-1") @@ -1018,6 +1034,17 @@ public void testRestoreBalancedReplica() { .execute() .actionGet(); + restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0") + .setRenamePattern("test-index") + .setRenameReplacement("new-index-3") + .setIndexSettings( + Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 0).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1").build() + ) + .setWaitForCompletion(true) + .setIndices("test-index") + .execute() + .actionGet(); + assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0)); } finally { manageReplicaBalanceSetting(false); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/AutoExpandReplicas.java b/server/src/main/java/org/opensearch/cluster/metadata/AutoExpandReplicas.java index 108c05eb78b79..7c62ed87adec8 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/AutoExpandReplicas.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/AutoExpandReplicas.java @@ -134,6 +134,14 @@ int getMaxReplicas(int numDataNodes) { return Math.min(maxReplicas, numDataNodes - 1); } + public int getMaxReplicas() { + return maxReplicas; + } + + public boolean isEnabled() { + return enabled; + } + private OptionalInt getDesiredNumberOfReplicas(IndexMetadata indexMetadata, RoutingAllocation allocation) { if (enabled) { int numMatchingDataNodes = 0; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 79419810bdebd..46be91fff398b 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -1205,7 +1205,8 @@ List getIndexSettingsValidationErrors( IndexMetadata.SETTING_NUMBER_OF_REPLICAS, INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY) ); - Optional error = awarenessReplicaBalance.validate(replicaCount); + AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); + Optional error = awarenessReplicaBalance.validate(replicaCount, autoExpandReplica); if (error.isPresent()) { validationErrors.add(error.get()); } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java index eb142be815d27..4756e625d21cc 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -201,7 +201,11 @@ public ClusterState execute(ClusterState currentState) { for (Index index : request.indices()) { if (index.getName().charAt(0) != '.') { // No replica count validation for system indices - Optional error = awarenessReplicaBalance.validate(updatedNumberOfReplicas); + Optional error = awarenessReplicaBalance.validate( + updatedNumberOfReplicas, + AutoExpandReplicas.SETTING.get(openSettings) + ); + if (error.isPresent()) { ValidationException ex = new ValidationException(); ex.addValidationError(error.get()); diff --git a/server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java b/server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java index accf0b69a4f0e..19601483d5607 100644 --- a/server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java +++ b/server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java @@ -8,6 +8,7 @@ package org.opensearch.cluster.routing.allocation; +import org.opensearch.cluster.metadata.AutoExpandReplicas; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; @@ -101,12 +102,22 @@ public int maxAwarenessAttributes() { return awarenessAttributes; } - public Optional validate(int replicaCount) { - if ((replicaCount + 1) % maxAwarenessAttributes() != 0) { - String errorMessage = "expected total copies needs to be a multiple of total awareness attributes [" - + maxAwarenessAttributes() - + "]"; - return Optional.of(errorMessage); + public Optional validate(int replicaCount, AutoExpandReplicas autoExpandReplica) { + if (autoExpandReplica.isEnabled()) { + if ((autoExpandReplica.getMaxReplicas() != Integer.MAX_VALUE) + && ((autoExpandReplica.getMaxReplicas() + 1) % maxAwarenessAttributes() != 0)) { + String errorMessage = "expected max cap on auto expand to be a multiple of total awareness attributes [" + + maxAwarenessAttributes() + + "]"; + return Optional.of(errorMessage); + } + } else { + if ((replicaCount + 1) % maxAwarenessAttributes() != 0) { + String errorMessage = "expected total copies needs to be a multiple of total awareness attributes [" + + maxAwarenessAttributes() + + "]"; + return Optional.of(errorMessage); + } } return Optional.empty(); } diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java index e2431765709e6..f7b1b8694f91a 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java @@ -9,6 +9,7 @@ package org.opensearch.cluster.routing.allocation; import org.opensearch.cluster.OpenSearchAllocationTestCase; +import org.opensearch.cluster.metadata.AutoExpandReplicas; import org.opensearch.cluster.routing.allocation.decider.AwarenessAllocationDecider; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; @@ -16,6 +17,7 @@ import java.util.Optional; import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS; public class AwarenessReplicaBalanceTests extends OpenSearchAllocationTestCase { @@ -25,42 +27,108 @@ public class AwarenessReplicaBalanceTests extends OpenSearchAllocationTestCase { ); public void testNoForcedAwarenessAttribute() { - Settings settings = Settings.builder().put("cluster.routing.allocation.awareness.attributes", "rack_id").build(); - + Settings settings = Settings.builder() + .put("cluster.routing.allocation.awareness.attributes", "rack_id") + .put(SETTING_AUTO_EXPAND_REPLICAS, "0-1") + .build(); + AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); AwarenessReplicaBalance awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); assertThat(awarenessReplicaBalance.maxAwarenessAttributes(), equalTo(1)); - assertEquals(awarenessReplicaBalance.validate(0), Optional.empty()); - assertEquals(awarenessReplicaBalance.validate(1), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(0, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); } public void testForcedAwarenessAttribute() { + // When auto expand replica settings is as per zone awareness Settings settings = Settings.builder() .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) + .put(SETTING_AUTO_EXPAND_REPLICAS, "0-2") .build(); AwarenessReplicaBalance awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); + AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); assertThat(awarenessReplicaBalance.maxAwarenessAttributes(), equalTo(3)); - assertEquals(awarenessReplicaBalance.validate(2), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(2, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(0, autoExpandReplica), Optional.empty()); + + // When auto expand replica settings is passed as max cap + settings = Settings.builder() + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") + .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) + .put(SETTING_AUTO_EXPAND_REPLICAS, "0-all") + .build(); + + awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); + autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); + + assertEquals(awarenessReplicaBalance.validate(2, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(0, autoExpandReplica), Optional.empty()); + + // when auto expand is not valid set as per zone awareness + settings = Settings.builder() + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") + .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) + .put(SETTING_AUTO_EXPAND_REPLICAS, "0-1") + .build(); + + awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); + autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); + + assertEquals( + awarenessReplicaBalance.validate(1, autoExpandReplica), + Optional.of("expected max cap on auto expand to be a multiple of total awareness attributes [3]") + ); + assertEquals( + awarenessReplicaBalance.validate(2, autoExpandReplica), + Optional.of("expected max cap on auto expand to be a multiple of total awareness attributes [3]") + ); + + // When auto expand replica is not present + settings = Settings.builder() + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") + .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") + .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) + .build(); + + awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); + autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); + + assertEquals(awarenessReplicaBalance.validate(2, autoExpandReplica), Optional.empty()); assertEquals( - awarenessReplicaBalance.validate(1), + awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.of("expected total copies needs to be a multiple of total awareness attributes [3]") ); + assertEquals( + awarenessReplicaBalance.validate(0, autoExpandReplica), + Optional.of("expected total copies needs to be a multiple of total awareness attributes [3]") + ); + } public void testForcedAwarenessAttributeDisabled() { Settings settings = Settings.builder() .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) + .put(SETTING_AUTO_EXPAND_REPLICAS, "0-1") .build(); AwarenessReplicaBalance awarenessReplicaBalance = new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS); + AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings); + assertThat(awarenessReplicaBalance.maxAwarenessAttributes(), equalTo(1)); - assertEquals(awarenessReplicaBalance.validate(0), Optional.empty()); - assertEquals(awarenessReplicaBalance.validate(1), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(0, autoExpandReplica), Optional.empty()); + assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); } }