Always prefer YES over NOT_PREFERRED when allocating unassigned shards (elastic#138464)

Author: nicktindall

docs/changelog/138464.yaml

@@ -0,0 +1,5 @@
+pr: 138464
+summary: Always prefer YES over NOT_PREFERRED when allocating unassigned shards
+area: Allocation
+type: bug
+issues: []

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

@@ -1416,10 +1416,6 @@ private AllocateUnassignedDecision decideAllocateUnassigned(final ProjectIndex i
 
                 // weight of this index currently on the node
                 float currentWeight = weightFunction.calculateNodeWeightWithIndex(this, node, index);
-                // moving the shard would not improve the balance, and we are not in explain mode, so short circuit
-                if (currentWeight > minWeight && explain == false) {
-                    continue;
-                }
 
                 Decision currentDecision = allocation.deciders().canAllocate(shard, node.getRoutingNode(), allocation);
                 if (explain) {

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

@@ -36,7 +36,6 @@
 import org.elasticsearch.cluster.routing.ShardRouting;
 import org.elasticsearch.cluster.routing.ShardRoutingState;
 import org.elasticsearch.cluster.routing.allocation.AllocateUnassignedDecision;
-import org.elasticsearch.cluster.routing.allocation.AllocationService;
 import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
 import org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.Balancer.PrioritiseByShardWriteLoadComparator;
 import org.elasticsearch.cluster.routing.allocation.decider.AllocationDecider;
@@ -1165,44 +1164,54 @@ public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, Routing
             }
         };
 
-        final var allocationService = new MockAllocationService(
-            new AllocationDeciders(List.of(notPreferredDecider)),
-            new TestGatewayAllocator(),
-            new BalancedShardsAllocator(BalancerSettings.DEFAULT, TEST_WRITE_LOAD_FORECASTER, new NodeNameDrivenBalancingWeightsFactory()),
-            () -> ClusterInfo.EMPTY,
-            SNAPSHOT_INFO_SERVICE_WITH_NO_SHARD_SIZES
-        );
+        final var allocationDeciders = new AllocationDeciders(List.of(notPreferredDecider));
+        final var balancingWeightsFactory = new NodeNameDrivenBalancingWeightsFactory();
 
         // No allocation when NO
-        assertUnassigned(allocationService, shuffledList("no"));
+        assertUnassigned(allocationDeciders, balancingWeightsFactory, shuffledList("no"));
         // No allocation when THROTTLE
-        assertUnassigned(allocationService, shuffledList("throttle"));
+        assertUnassigned(allocationDeciders, balancingWeightsFactory, shuffledList("throttle"));
         // NOT_PREFERRED when no other choice
-        assertAssignedTo(allocationService, "not-preferred", shuffledList("not-preferred"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "not-preferred", shuffledList("not-preferred"));
         // NOT_PREFERRED over NO
-        assertAssignedTo(allocationService, "not-preferred", shuffledList("not-preferred", "no"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "not-preferred", shuffledList("not-preferred", "no"));
         // THROTTLE (No allocation) over NOT_PREFERRED/NO
-        assertUnassigned(allocationService, shuffledList("throttle", "not-preferred", "no"));
+        assertUnassigned(allocationDeciders, balancingWeightsFactory, shuffledList("throttle", "not-preferred", "no"));
         // THROTTLE (No allocation) over NOT_PREFERRED
-        assertUnassigned(allocationService, shuffledList("throttle", "not-preferred"));
+        assertUnassigned(allocationDeciders, balancingWeightsFactory, shuffledList("throttle", "not-preferred"));
         // YES over THROTTLE/NO/NOT_PREFERRED
-        assertAssignedTo(allocationService, "yes", shuffledList("not-preferred", "yes", "throttle", "no"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "yes", shuffledList("not-preferred", "yes", "throttle", "no"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "yes-high", shuffledList("not-preferred-low", "yes-high"));
         // prioritize YES/THROTTLE by weight
-        assertUnassigned(allocationService, shuffledList("throttle-low", "yes-high", "yes"));
-        assertAssignedTo(allocationService, "yes-low", shuffledList("yes-low", "throttle", "throttle-high"));
+        assertUnassigned(allocationDeciders, balancingWeightsFactory, shuffledList("throttle-low", "yes-high", "yes"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "yes-low", shuffledList("yes-low", "throttle", "throttle-high"));
         // prioritize YES over THROTTLE when weights equal
-        assertAssignedTo(allocationService, "yes-low", shuffledList("yes-low", "throttle-low"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "yes-low", shuffledList("yes-low", "throttle-low"));
         // prioritize YES by weight
-        assertAssignedTo(allocationService, "yes-low", shuffledList("yes-low", "yes", "yes-high"));
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, "yes-low", shuffledList("yes-low", "yes", "yes-high"));
         // prioritize NOT_PREFERRED by weight
-        assertAssignedTo(allocationService, "not-preferred-low", shuffledList("not-preferred-low", "not-preferred", "not-preferred-high"));
+        assertAssignedTo(
+            allocationDeciders,
+            balancingWeightsFactory,
+            "not-preferred-low",
+            shuffledList("not-preferred-low", "not-preferred", "not-preferred-high")
+        );
     }
 
-    private void assertUnassigned(AllocationService allocationService, List<String> allNodeIds) {
-        assertAssignedTo(allocationService, null, allNodeIds);
+    private void assertUnassigned(
+        AllocationDeciders allocationDeciders,
+        BalancingWeightsFactory balancingWeightsFactory,
+        List<String> allNodeIds
+    ) {
+        assertAssignedTo(allocationDeciders, balancingWeightsFactory, null, allNodeIds);
     }
 
-    private void assertAssignedTo(AllocationService allocationService, @Nullable String expectedNodeId, List<String> allNodeIds) {
+    private void assertAssignedTo(
+        AllocationDeciders allocationDeciders,
+        BalancingWeightsFactory balancingWeightsFactory,
+        @Nullable String expectedNodeId,
+        List<String> allNodeIds
+    ) {
         final var discoveryNodesBuilder = DiscoveryNodes.builder();
         for (String nodeName : allNodeIds) {
             discoveryNodesBuilder.add(newNode(nodeName));
@@ -1220,13 +1229,41 @@ private void assertAssignedTo(AllocationService allocationService, @Nullable Str
             .putRoutingTable(ProjectId.DEFAULT, routingTableBuilder.build())
             .build();
 
-        clusterState = startInitializingShardsAndReroute(allocationService, clusterState);
+        clusterState = runAssignment(allocationDeciders, balancingWeightsFactory, clusterState);
 
         final RoutingTable routingTable = clusterState.routingTable(ProjectId.DEFAULT);
         final ShardRouting primaryShard = routingTable.shardRoutingTable(indexMetadata.getIndex().getName(), 0).primaryShard();
         assertThat(primaryShard.currentNodeId(), equalTo(expectedNodeId));
     }
 
+    private ClusterState runAssignment(
+        AllocationDeciders allocationDeciders,
+        BalancingWeightsFactory balancingWeightsFactory,
+        ClusterState clusterState
+    ) {
+        final var routingAllocation = new RoutingAllocation(
+            allocationDeciders,
+            clusterState.getRoutingNodes().mutableCopy(),
+            clusterState,
+            ClusterInfo.EMPTY,
+            SnapshotShardSizeInfo.EMPTY,
+            System.nanoTime()
+        );
+
+        // Debug mode should not change the outcome
+        routingAllocation.setDebugMode(randomFrom(RoutingAllocation.DebugMode.values()));
+
+        final var balancedShardsAllocator = new BalancedShardsAllocator(
+            BalancerSettings.DEFAULT,
+            TEST_WRITE_LOAD_FORECASTER,
+            balancingWeightsFactory
+        );
+        balancedShardsAllocator.allocate(routingAllocation);
+        return ClusterState.builder(clusterState)
+            .routingTable(clusterState.globalRoutingTable().rebuild(routingAllocation.routingNodes(), clusterState.metadata()))
+            .build();
+    }
+
     /**
      * Returns specific values for {@link WeightFunction#calculateNodeWeightWithIndex} depending on the
      * suffix of the node name.