[Backport 2.4] Add AutoExpandReplica in the validation of replica cou…

…nt enforcement (#5059) * Add AutoExpandReplica in the validation of replica count enforcement Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
opensearch-project · Nov 3, 2022 · 017f76b · 017f76b
1 parent a5cf2ed
commit 017f76b
Show file tree

Hide file tree

Showing 10 changed files with 213 additions and 23 deletions.
diff --git 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/...src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java b/...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/...rc/internalClusterTest/java/org/opensearch/indices/settings/UpdateNumberOfReplicasIT.java b/...rc/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/...r/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java b/...r/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java
@@ -1032,13 +1032,23 @@ public void testPartitionedTemplate() throws Exception {
 
     public void testAwarenessReplicaBalance() throws IOException {
         manageReplicaBalanceSetting(true);
+        int updated = 0;
         try {
             client().admin()
                 .indices()
                 .preparePutTemplate("template_1")
                 .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
@@ -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
@@ -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
@@ -1205,7 +1205,8 @@ List<String> getIndexSettingsValidationErrors(
                 IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
                 INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY)
             );
-            Optional<String> error = awarenessReplicaBalance.validate(replicaCount);
+            AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings);
+            Optional<String> 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
@@ -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<String> error = awarenessReplicaBalance.validate(updatedNumberOfReplicas);
+                                    Optional<String> 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
@@ -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<String> 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<String> 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/...src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java b/...src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java
@@ -9,13 +9,15 @@
 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;
 
 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());
     }
 
 }