Skip to content

Always prefer YES over NOT_PREFERRED when allocating unassigned shards #138464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nicktindall merged 2 commits into from
Nov 24, 2025
Merged

Always prefer YES over NOT_PREFERRED when allocating unassigned shards #138464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9624cf9
Always prefer YES over NOT_PREFERRED when allocating unassigned shards
nicktindall Nov 24, 2025
ab1d4d6
Update docs/changelog/138464.yaml
nicktindall Nov 24, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/138464.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 138464
summary: Always prefer YES over NOT_PREFERRED when allocating unassigned shards
area: Allocation
type: bug
issues: []
4 changes: 0 additions & 4 deletions .../java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
85 changes: 61 additions & 24 deletions .../org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"));
Copy link
Contributor Author

@nicktindall nicktindall Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new case triggers the bug.
// 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));
Expand All @@ -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
) {
Copy link
Contributor Author

@nicktindall nicktindall Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to switch over to this approach (removing AllocationService from the test) so that we could randomise the debugMode
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.
Expand Down