Clamp auto-expand replicas to the closest value

docs/changelog/87505.yaml

@@ -0,0 +1,6 @@
+pr: 87505
+summary: Clamp auto-expand replicas to the closest value
+area: Allocation
+type: bug
+issues:
+ - 84788

server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.cluster.metadata;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.test.ESIntegTestCase;
+
+import static org.hamcrest.Matchers.equalTo;
+
+@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, minNumDataNodes = 2, numClientNodes = 0)
+public class AutoExpandReplicasIT extends ESIntegTestCase {
+
+    /**
+     * Verifies that when no data node is available to host a replica, number of replicas are contracted to 0.
+     */
+    public void testClampToMinMax() throws Exception {
+        final String indexName = "myindex";
+        final String autoExpandValue = randomFrom("0-1", "0-all");
+        createIndex(
+            indexName,
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, autoExpandValue)
+                .build()
+        );
+
+        final int initialReplicas = autoExpandValue.equals("0-1") ? 1 : internalCluster().numDataNodes() - 1;
+
+        assertBusy(() -> {
+            assertThat(
+                client().admin()
+                    .indices()
+                    .prepareGetSettings(indexName)
+                    .setNames("index.number_of_replicas")
+                    .get()
+                    .getSetting(indexName, "index.number_of_replicas"),
+                equalTo(String.valueOf(initialReplicas))
+            );
+        });
+
+        updateIndexSettings(indexName, Settings.builder().put("index.routing.allocation.require._id", "non-existing-node"));
+
+        assertBusy(() -> {
+            assertThat(
+                client().admin()
+                    .indices()
+                    .prepareGetSettings(indexName)
+                    .setNames("index.number_of_replicas")
+                    .get()
+                    .getSetting(indexName, "index.number_of_replicas"),
+                equalTo("0")
+            );
+        });
+
+        // Remove the setting
+        updateIndexSettings(indexName, Settings.builder().put("index.routing.allocation.require._id", ""));
+
+        assertBusy(() -> {
+            assertThat(
+                client().admin()
+                    .indices()
+                    .prepareGetSettings(indexName)
+                    .setNames("index.number_of_replicas")
+                    .get()
+                    .getSetting(indexName, "index.number_of_replicas"),
+                equalTo(String.valueOf(initialReplicas))
+            );
+        });
+    }
+}

server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java

@@ -18,7 +18,6 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.OptionalInt;
 import java.util.function.Supplier;
 
 import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.isIndexVerifiedBeforeClosed;
@@ -93,15 +92,11 @@ private static AutoExpandReplicas parse(String value) {
         }
     }
 
-    int getMaxReplicas(int numDataNodes) {
-        return Math.min(maxReplicas, numDataNodes - 1);
-    }
-
     public boolean expandToAllNodes() {
         return maxReplicas == Integer.MAX_VALUE;
     }
 
-    public OptionalInt getDesiredNumberOfReplicas(IndexMetadata indexMetadata, RoutingAllocation allocation) {
+    public int getDesiredNumberOfReplicas(IndexMetadata indexMetadata, RoutingAllocation allocation) {
         assert enabled : "should only be called when enabled";
         int numMatchingDataNodes = 0;
         for (DiscoveryNode discoveryNode : allocation.nodes().getDataNodes().values()) {
@@ -110,20 +105,19 @@ public OptionalInt getDesiredNumberOfReplicas(IndexMetadata indexMetadata, Routi
                 numMatchingDataNodes++;
             }
         }
+        return calculateDesiredNumberOfReplicas(numMatchingDataNodes);
+    }
 
-        final int min = minReplicas();
-        final int max = getMaxReplicas(numMatchingDataNodes);
+    // package private for testing
+    int calculateDesiredNumberOfReplicas(int numMatchingDataNodes) {
         int numberOfReplicas = numMatchingDataNodes - 1;
-        if (numberOfReplicas < min) {
-            numberOfReplicas = min;
-        } else if (numberOfReplicas > max) {
-            numberOfReplicas = max;
+        // Make sure number of replicas is always between min and max
+        if (numberOfReplicas < minReplicas) {
+            numberOfReplicas = minReplicas;
+        } else if (numberOfReplicas > maxReplicas) {
+            numberOfReplicas = maxReplicas;
         }
-
-        if (numberOfReplicas >= min && numberOfReplicas <= max) {
-            return OptionalInt.of(numberOfReplicas);
-        }
-        return OptionalInt.empty();
+        return numberOfReplicas;
     }
 
     @Override
@@ -153,11 +147,10 @@ public static Map<Integer, List<String>> getAutoExpandReplicaChanges(
                 if (allocation == null) {
                     allocation = allocationSupplier.get();
                 }
-                autoExpandReplicas.getDesiredNumberOfReplicas(indexMetadata, allocation).ifPresent(numberOfReplicas -> {
-                    if (numberOfReplicas != indexMetadata.getNumberOfReplicas()) {
-                        nrReplicasChanged.computeIfAbsent(numberOfReplicas, ArrayList::new).add(indexMetadata.getIndex().getName());
-                    }
-                });
+                int numberOfReplicas = autoExpandReplicas.getDesiredNumberOfReplicas(indexMetadata, allocation);
+                if (numberOfReplicas != indexMetadata.getNumberOfReplicas()) {
+                    nrReplicasChanged.computeIfAbsent(numberOfReplicas, ArrayList::new).add(indexMetadata.getIndex().getName());
+                }
             }
         }
         return nrReplicasChanged;

server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java

@@ -50,22 +50,18 @@ public void testParseSettings() {
             Settings.builder().put("index.auto_expand_replicas", "0-5").build()
         );
         assertEquals(0, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(8));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(5, autoExpandReplicas.maxReplicas());
         assertFalse(autoExpandReplicas.expandToAllNodes());
 
         autoExpandReplicas = AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "0-all").build());
         assertEquals(0, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(6));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(Integer.MAX_VALUE, autoExpandReplicas.maxReplicas());
         assertTrue(autoExpandReplicas.expandToAllNodes());
 
         autoExpandReplicas = AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "1-all").build());
         assertEquals(1, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(6));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(Integer.MAX_VALUE, autoExpandReplicas.maxReplicas());
         assertTrue(autoExpandReplicas.expandToAllNodes());
-
     }
 
     public void testInvalidValues() {
@@ -282,4 +278,17 @@ public void testOnlyAutoExpandAllocationFilteringAfterAllNodesUpgraded() {
             terminate(threadPool);
         }
     }
+
+    public void testCalculateDesiredNumberOfReplicas() {
+        int lowerBound = between(0, 9);
+        int upperBound = between(lowerBound + 1, 10);
+        String settingValue = lowerBound + "-" + randomFrom(upperBound, "all");
+        AutoExpandReplicas autoExpandReplicas = AutoExpandReplicas.SETTING.get(
+            Settings.builder().put(SETTING_AUTO_EXPAND_REPLICAS, settingValue).build()
+        );
+        int max = autoExpandReplicas.maxReplicas();
+        int matchingNodes = between(0, max);
+        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(matchingNodes), equalTo(Math.max(lowerBound, matchingNodes - 1)));
+        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(max + 1), equalTo(max));
+    }
 }