From 78a5287d51d7213983293378c798ef3f6d437a05 Mon Sep 17 00:00:00 2001 From: Abhishek Dhanwani Date: Tue, 30 Jun 2026 22:55:33 +0530 Subject: [PATCH] Add cross-setting validator for balance factor sum constraint Setting both cluster.routing.allocation.balance.shard and cluster.routing.allocation.balance.index to 0.0 causes an unrecoverable cluster-manager crash loop. Each setting independently allows 0.0 as minimum, but the WeightFunction requires their sum to be > 0. This adds a Setting.Validator for both balance factor settings that validates the sum > 0 constraint before the settings enter cluster state, preventing the poison pill from being published. Fixes https://github.com/opensearch-project/OpenSearch/issues/22305 Signed-off-by: Abhishek Dhanwani --- .../allocator/BalancedShardsAllocator.java | 67 ++++++++++ .../BalancedShardsAllocatorSettingsTests.java | 123 ++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 server/src/test/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorSettingsTests.java diff --git a/server/src/main/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java b/server/src/main/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java index abddf1b17fce0..bdbbe726c5c6d 100644 --- a/server/src/main/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java +++ b/server/src/main/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java @@ -59,9 +59,11 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.core.action.ActionListener; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; @@ -99,6 +101,8 @@ public class BalancedShardsAllocator implements ShardsAllocator { "cluster.routing.allocation.balance.index", 0.55f, 0.0f, + Float.MAX_VALUE, + new IndexBalanceFactorValidator(), Property.Dynamic, Property.NodeScope ); @@ -106,6 +110,8 @@ public class BalancedShardsAllocator implements ShardsAllocator { "cluster.routing.allocation.balance.shard", 0.45f, 0.0f, + Float.MAX_VALUE, + new ShardBalanceFactorValidator(), Property.Dynamic, Property.NodeScope ); @@ -518,6 +524,67 @@ public boolean getPreferPrimaryBalance() { return preferPrimaryShardBalance; } + /** + * Validates that the index balance factor, combined with the shard balance factor, sums to a value greater than zero. + * + * @opensearch.internal + */ + static final class IndexBalanceFactorValidator implements Setting.Validator { + + @Override + public void validate(Float value) {} + + @Override + public void validate(final Float value, final Map, Object> settings) { + final float shardBalance = (Float) settings.get(SHARD_BALANCE_FACTOR_SETTING); + doValidateBalanceFactorSum(value, shardBalance); + } + + @Override + public Iterator> settings() { + final List> settings = Collections.singletonList(SHARD_BALANCE_FACTOR_SETTING); + return settings.iterator(); + } + } + + /** + * Validates that the shard balance factor, combined with the index balance factor, sums to a value greater than zero. + * + * @opensearch.internal + */ + static final class ShardBalanceFactorValidator implements Setting.Validator { + + @Override + public void validate(Float value) {} + + @Override + public void validate(final Float value, final Map, Object> settings) { + final float indexBalance = (Float) settings.get(INDEX_BALANCE_FACTOR_SETTING); + doValidateBalanceFactorSum(indexBalance, value); + } + + @Override + public Iterator> settings() { + final List> settings = Collections.singletonList(INDEX_BALANCE_FACTOR_SETTING); + return settings.iterator(); + } + } + + static void doValidateBalanceFactorSum(float indexBalance, float shardBalance) { + float sum = indexBalance + shardBalance; + if (sum <= 0.0f) { + throw new IllegalArgumentException( + "Balance factors [" + + INDEX_BALANCE_FACTOR_SETTING.getKey() + + "] and [" + + SHARD_BALANCE_FACTOR_SETTING.getKey() + + "] must sum to a value greater than zero but was [" + + sum + + "]" + ); + } + } + /** * This class is the primary weight function used to create balanced over nodes and shards in the cluster. * Currently this function has 3 properties: diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorSettingsTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorSettingsTests.java new file mode 100644 index 0000000000000..9b5c63d15ab2b --- /dev/null +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorSettingsTests.java @@ -0,0 +1,123 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.cluster.routing.allocation.allocator; + +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; +import org.opensearch.test.OpenSearchTestCase; + +import static org.opensearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.INDEX_BALANCE_FACTOR_SETTING; +import static org.opensearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.SHARD_BALANCE_FACTOR_SETTING; + +public class BalancedShardsAllocatorSettingsTests extends OpenSearchTestCase { + + public void testBothBalanceFactorsZeroIsRejectedOnConstruction() { + final Settings settings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .build(); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> new BalancedShardsAllocator(settings, new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + assertThat(e.getMessage(), org.hamcrest.Matchers.containsString("must sum to a value greater than zero")); + } + + public void testBothBalanceFactorsZeroIsRejectedOnDynamicUpdate() { + final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + new BalancedShardsAllocator(Settings.EMPTY, clusterSettings); + + final Settings newSettings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .build(); + + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> clusterSettings.applySettings(newSettings) + ); + assertThat(e.getMessage(), org.hamcrest.Matchers.containsString("illegal value can't update")); + assertNotNull(e.getCause()); + assertThat(e.getCause().getMessage(), org.hamcrest.Matchers.containsString("must sum to a value greater than zero")); + } + + public void testIndexBalanceZeroWithNonZeroShardBalanceIsAccepted() { + final Settings settings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "1.0") + .build(); + final ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + BalancedShardsAllocator allocator = new BalancedShardsAllocator(settings, clusterSettings); + assertEquals(0.0f, allocator.getIndexBalance(), 0.0f); + assertEquals(1.0f, allocator.getShardBalance(), 0.0f); + } + + public void testShardBalanceZeroWithNonZeroIndexBalanceIsAccepted() { + final Settings settings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "1.0") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .build(); + final ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + BalancedShardsAllocator allocator = new BalancedShardsAllocator(settings, clusterSettings); + assertEquals(1.0f, allocator.getIndexBalance(), 0.0f); + assertEquals(0.0f, allocator.getShardBalance(), 0.0f); + } + + public void testDynamicUpdateIndexBalanceToZeroWhileShardBalanceAlreadyZeroIsRejected() { + final Settings initialSettings = Settings.builder() + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "1.0") + .build(); + final ClusterSettings clusterSettings = new ClusterSettings(initialSettings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + new BalancedShardsAllocator(initialSettings, clusterSettings); + + final Settings newSettings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .build(); + + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> clusterSettings.applySettings(newSettings) + ); + assertThat(e.getCause().getMessage(), org.hamcrest.Matchers.containsString("must sum to a value greater than zero")); + } + + public void testDynamicUpdateShardBalanceToZeroWhileIndexBalanceAlreadyZeroIsRejected() { + final Settings initialSettings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "1.0") + .build(); + final ClusterSettings clusterSettings = new ClusterSettings(initialSettings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + new BalancedShardsAllocator(initialSettings, clusterSettings); + + final Settings newSettings = Settings.builder() + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.0") + .build(); + + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> clusterSettings.applySettings(newSettings) + ); + assertThat(e.getCause().getMessage(), org.hamcrest.Matchers.containsString("must sum to a value greater than zero")); + } + + public void testValidDynamicUpdateIsAccepted() { + final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + BalancedShardsAllocator allocator = new BalancedShardsAllocator(Settings.EMPTY, clusterSettings); + + final Settings newSettings = Settings.builder() + .put(INDEX_BALANCE_FACTOR_SETTING.getKey(), "0.3") + .put(SHARD_BALANCE_FACTOR_SETTING.getKey(), "0.7") + .build(); + + clusterSettings.applySettings(newSettings); + assertEquals(0.3f, allocator.getIndexBalance(), 0.001f); + assertEquals(0.7f, allocator.getShardBalance(), 0.001f); + } +}