Increase minimum threshold in shard balancer (#115831)

Support for thresholds between 0.0 and 1.0 was deprecated in #92100.
This commit removes this support in 9.0.

Author: DaveCTurner

docs/changelog/115831.yaml

@@ -0,0 +1,13 @@
+pr: 115831
+summary: Increase minimum threshold in shard balancer
+area: Allocation
+type: breaking
+issues: []
+breaking:
+  title: Minimum shard balancer threshold is now 1.0
+  area: Cluster and node setting
+  details: >-
+    Earlier versions of {es} accepted any non-negative value for `cluster.routing.allocation.balance.threshold`, but values smaller than
+    `1.0` do not make sense and have been ignored since version 8.6.1. From 9.0.0 these nonsensical values are now forbidden.
+  impact: Do not set `cluster.routing.allocation.balance.threshold` to a value less than `1.0`.
+  notable: false

qa/full-cluster-restart/src/javaRestTest/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

@@ -16,9 +16,11 @@
 import org.apache.http.util.EntityUtils;
 import org.elasticsearch.Build;
 import org.elasticsearch.client.Request;
+import org.elasticsearch.client.RequestOptions;
 import org.elasticsearch.client.Response;
 import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.client.RestClient;
+import org.elasticsearch.client.WarningsHandler;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.MetadataIndexStateService;
 import org.elasticsearch.common.Strings;
@@ -27,6 +29,7 @@
 import org.elasticsearch.common.xcontent.support.XContentMapValues;
 import org.elasticsearch.core.Booleans;
 import org.elasticsearch.core.CheckedFunction;
+import org.elasticsearch.core.UpdateForV10;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.IndexVersion;
 import org.elasticsearch.index.IndexVersions;
@@ -72,6 +75,7 @@
 import static java.util.stream.Collectors.toList;
 import static org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.SYSTEM_INDEX_ENFORCEMENT_INDEX_VERSION;
 import static org.elasticsearch.cluster.routing.UnassignedInfo.INDEX_DELAYED_NODE_LEFT_TIMEOUT_SETTING;
+import static org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.THRESHOLD_SETTING;
 import static org.elasticsearch.cluster.routing.allocation.decider.MaxRetryAllocationDecider.SETTING_ALLOCATION_MAX_RETRY;
 import static org.elasticsearch.test.MapMatcher.assertMap;
 import static org.elasticsearch.test.MapMatcher.matchesMap;
@@ -1949,4 +1953,35 @@ public static void assertNumHits(String index, int numHits, int totalShards) thr
         assertThat(XContentMapValues.extractValue("_shards.successful", resp), equalTo(totalShards));
         assertThat(extractTotalHits(resp), equalTo(numHits));
     }
+
+    @UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // this test is just about v8->v9 upgrades, remove it in v10
+    public void testBalancedShardsAllocatorThreshold() throws Exception {
+        assumeTrue("test only applies for v8->v9 upgrades", getOldClusterTestVersion().getMajor() == 8);
+
+        final var chosenValue = randomFrom("0", "0.1", "0.5", "0.999");
+
+        if (isRunningAgainstOldCluster()) {
+            final var request = newXContentRequest(
+                HttpMethod.PUT,
+                "/_cluster/settings",
+                (builder, params) -> builder.startObject("persistent").field(THRESHOLD_SETTING.getKey(), chosenValue).endObject()
+            );
+            request.setOptions(RequestOptions.DEFAULT.toBuilder().setWarningsHandler(WarningsHandler.PERMISSIVE));
+            assertOK(client().performRequest(request));
+        }
+
+        final var clusterSettingsResponse = ObjectPath.createFromResponse(
+            client().performRequest(new Request("GET", "/_cluster/settings"))
+        );
+
+        final var settingsPath = "persistent." + THRESHOLD_SETTING.getKey();
+        final var settingValue = clusterSettingsResponse.evaluate(settingsPath);
+
+        if (isRunningAgainstOldCluster()) {
+            assertEquals(chosenValue, settingValue);
+        } else {
+            assertNull(settingValue);
+            assertNotNull(clusterSettingsResponse.<String>evaluate("persistent.archived." + THRESHOLD_SETTING.getKey()));
+        }
+    }
 }

server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

@@ -32,16 +32,13 @@
 import org.elasticsearch.cluster.routing.allocation.decider.AllocationDeciders;
 import org.elasticsearch.cluster.routing.allocation.decider.Decision;
 import org.elasticsearch.cluster.routing.allocation.decider.Decision.Type;
-import org.elasticsearch.common.logging.DeprecationCategory;
-import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.core.Tuple;
-import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.gateway.PriorityComparator;
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.injection.guice.Inject;
@@ -109,7 +106,7 @@ public class BalancedShardsAllocator implements ShardsAllocator {
     public static final Setting<Float> THRESHOLD_SETTING = Setting.floatSetting(
         "cluster.routing.allocation.balance.threshold",
         1.0f,
-        0.0f,
+        1.0f,
         Property.Dynamic,
         Property.NodeScope
     );
@@ -140,34 +137,10 @@ public BalancedShardsAllocator(ClusterSettings clusterSettings, WriteLoadForecas
         clusterSettings.initializeAndWatch(INDEX_BALANCE_FACTOR_SETTING, value -> this.indexBalanceFactor = value);
         clusterSettings.initializeAndWatch(WRITE_LOAD_BALANCE_FACTOR_SETTING, value -> this.writeLoadBalanceFactor = value);
         clusterSettings.initializeAndWatch(DISK_USAGE_BALANCE_FACTOR_SETTING, value -> this.diskUsageBalanceFactor = value);
-        clusterSettings.initializeAndWatch(THRESHOLD_SETTING, value -> this.threshold = ensureValidThreshold(value));
+        clusterSettings.initializeAndWatch(THRESHOLD_SETTING, value -> this.threshold = value);
         this.writeLoadForecaster = writeLoadForecaster;
     }
 
-    /**
-     * Clamp threshold to be at least 1, and log a critical deprecation warning if smaller values are given.
-     *
-     * Once {@link org.elasticsearch.Version#V_7_17_0} goes out of scope, start to properly reject such bad values.
-     */
-    @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION)
-    private static float ensureValidThreshold(float threshold) {
-        if (1.0f <= threshold) {
-            return threshold;
-        } else {
-            DeprecationLogger.getLogger(BalancedShardsAllocator.class)
-                .critical(
-                    DeprecationCategory.SETTINGS,
-                    "balance_threshold_too_small",
-                    "ignoring value [{}] for [{}] since it is smaller than 1.0; "
-                        + "setting [{}] to a value smaller than 1.0 will be forbidden in a future release",
-                    threshold,
-                    THRESHOLD_SETTING.getKey(),
-                    THRESHOLD_SETTING.getKey()
-                );
-            return 1.0f;
-        }
-    }
-
     @Override
     public void allocate(RoutingAllocation allocation) {
         assert allocation.ignoreDisable() == false;

server/src/test/java/org/elasticsearch/cluster/routing/allocation/BalancedSingleShardTests.java

@@ -246,7 +246,7 @@ public void testNodeDecisionsRanking() {
         // return the same ranking as the current node
         ClusterState clusterState = ClusterStateCreationUtils.state(randomIntBetween(1, 10), new String[] { "idx" }, 1);
         ShardRouting shardToRebalance = clusterState.routingTable().index("idx").shardsWithState(ShardRoutingState.STARTED).get(0);
-        MoveDecision decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet(), -1);
+        MoveDecision decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet());
         int currentRanking = decision.getCurrentNodeRanking();
         assertEquals(1, currentRanking);
         for (NodeAllocationResult result : decision.getNodeDecisions()) {
@@ -258,7 +258,7 @@ public void testNodeDecisionsRanking() {
         clusterState = ClusterStateCreationUtils.state(1, new String[] { "idx" }, randomIntBetween(2, 10));
         shardToRebalance = clusterState.routingTable().index("idx").shardsWithState(ShardRoutingState.STARTED).get(0);
         clusterState = addNodesToClusterState(clusterState, randomIntBetween(1, 10));
-        decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet(), 0.01f);
+        decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet());
         for (NodeAllocationResult result : decision.getNodeDecisions()) {
             assertThat(result.getWeightRanking(), lessThan(decision.getCurrentNodeRanking()));
         }
@@ -285,7 +285,7 @@ public void testNodeDecisionsRanking() {
             }
         }
         clusterState = addNodesToClusterState(clusterState, 1);
-        decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet(), 0.01f);
+        decision = executeRebalanceFor(shardToRebalance, clusterState, emptySet());
         for (NodeAllocationResult result : decision.getNodeDecisions()) {
             if (result.getWeightRanking() < decision.getCurrentNodeRanking()) {
                 // highest ranked node should not be any of the initial nodes
@@ -298,22 +298,13 @@ public void testNodeDecisionsRanking() {
                 assertTrue(nodesWithTwoShards.contains(result.getNode().getId()));
             }
         }
-
-        assertCriticalWarnings("""
-            ignoring value [0.01] for [cluster.routing.allocation.balance.threshold] since it is smaller than 1.0; setting \
-            [cluster.routing.allocation.balance.threshold] to a value smaller than 1.0 will be forbidden in a future release""");
     }
 
     private MoveDecision executeRebalanceFor(
         final ShardRouting shardRouting,
         final ClusterState clusterState,
-        final Set<String> noDecisionNodes,
-        final float threshold
+        final Set<String> noDecisionNodes
     ) {
-        Settings settings = Settings.EMPTY;
-        if (Float.compare(-1.0f, threshold) != 0) {
-            settings = Settings.builder().put(BalancedShardsAllocator.THRESHOLD_SETTING.getKey(), threshold).build();
-        }
         AllocationDecider allocationDecider = new AllocationDecider() {
             @Override
             public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation) {
@@ -329,7 +320,7 @@ public Decision canRebalance(ShardRouting shardRouting, RoutingAllocation alloca
                 return Decision.YES;
             }
         };
-        BalancedShardsAllocator allocator = new BalancedShardsAllocator(settings);
+        BalancedShardsAllocator allocator = new BalancedShardsAllocator(Settings.EMPTY);
         RoutingAllocation routingAllocation = newRoutingAllocation(
             new AllocationDeciders(Arrays.asList(allocationDecider, rebalanceDecider)),
             clusterState

server/src/test/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorTests.java

@@ -441,15 +441,10 @@ public void testGetIndexDiskUsageInBytes() {
 
     public void testThresholdLimit() {
         final var badValue = (float) randomDoubleBetween(0.0, Math.nextDown(1.0f), true);
-        assertEquals(
-            1.0f,
-            new BalancedShardsAllocator(Settings.builder().put(BalancedShardsAllocator.THRESHOLD_SETTING.getKey(), badValue).build())
-                .getThreshold(),
-            0.0f
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> new BalancedShardsAllocator(Settings.builder().put(BalancedShardsAllocator.THRESHOLD_SETTING.getKey(), badValue).build())
         );
-        assertCriticalWarnings("ignoring value [" + badValue + """
-            ] for [cluster.routing.allocation.balance.threshold] since it is smaller than 1.0; setting \
-            [cluster.routing.allocation.balance.threshold] to a value smaller than 1.0 will be forbidden in a future release""");
 
         final var goodValue = (float) randomDoubleBetween(1.0, 10.0, true);
         assertEquals(