Tighten on when THROTTLE decision can be returned

[ywangd]

For allocation simulation, allocation decisions should not be THROTTLE for moveShards and balance. Allocate unassigned shards should not see THROTTLE unless there is prior shard assignment. This PR makes this statement explicit by fixing test allocation decider and HasFrozenAllocationDecider as well as adding relevant assertions.

Relates: ES-12955

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[ywangd]

Initially, I wanted to ensure that no THROTTLE can be returned at all unless there is prior shard movement. But HasFrozenCacheAllocationDecider is not compatible with the idea. We also have existing code explicitly checking for it. So it seems that we have to live with this exception. As a result, I added assertions and fixed RandomAllocationDecider to make this exception more explicit. I am open to suggestions.

[ywangd]

It would be great if we could check that the THROTTLE decision is actually from the HasFrozenCacheAllocationDecider. But it's not really feasible. Instead, we can check it's a frozen index. Not really ideal, but seems to be the best we can do.

[nicktindall]

I'm not a big fan of putting knowledge of specific deciders in here, as discussed, I wonder if HasFrozenAllocationDecider is abusing THROTTLE anyway. My reasoning was:

In the canAllocateUnassigned logic we'll wait for the outcome of a lower-weight THROTTLE when we have higher-weight YES so that would mean this decider can defer the allocation of a partial searchable snapshot while a non-frozen node was initialising doesn't it?
I would think we'd return NO until the setting was loaded?

It seems like returning THROTTLED while we wait to find out if the node can host frozen shards could mean we defer allocating the shard to another node that we KNOW can host frozen shards, while waiting to find out if an initialising node is a frozen node.

[ywangd]

we defer allocating the shard to another node that we KNOW can host frozen shards, while waiting to find out if an initialising node is a frozen node.

That is true. I also wonder whether it is intentional since otherwise the shards will be assigned to an existing node only to relocate later momentarily. That said, after reading through code around HasFrozenAllocationDecider, I believe it is OK to return NO since it should not change the beahviour. That is because we always call allocateExistingUnassignedShards before calling into the allocator. This method uses SearchableSnapshotAllocator to allocate unassigne shards first. It also checks cache status and reject allocation when cache state is still being fetched. It in turns places the shard in the ignored list and BalancedShardAllocator ignores them as well. In short, the allocation decision for paritially mounted shards are made earlier in a different place so that it should not matter for the decider to return NO.

[nicktindall]

All this change LGTM pending the question of whether we can make HasFrozenCacheAllocationDecider return NO while we're initialising the node so that we can remove the special knowledge of it from here?

[DiannaHohensee]

The purpose of this task is to gain confidence that the THROTTLE decision is not encountered, but right now this patch doesn't provide that confidence. This isn't in the hot-path, so I don't think we need to rush the change. I'd rather sort out the Decider before proceeding.

[ywangd]

@nicktindall @DiannaHohensee The PR is ready for another look. In summary, I have taken Nick's suggestion for returning NO from HasFrozenAllocationDecider.

I spent quite some time reading through relevant code for searchable snapshot indices allocation. I believe it is safe to do so.

1. Unassigned searchable snapshot shards are allocated elsewhere, see also this comment.
2. For relocations, throttle is effectively a NO and unlike unassigned shards, there is no urgency for them.

[ywangd]

HasFrozenAllocationDecider does not differentiate between fetching node state (FETCHING) and no-such-node (UNKNOWN). The later can only happen when a node leaves the cluster which seems more justified to say NO. In any case, they do not really impact SearchableSnapshotAllocator which is what matters for allocating unassigned searchable snapshot shards.
Btw, I can also undo the split if it is considered unrelated.

[ywangd]

The comment about HasFrozenCacheAllocationDecider had me concerned for some time. Adding this assertion to make it clear that throttled status can only come from DesiredBalanceInput#ignoredShards, i.e. they are not produced by BalancedShardAllocator.

[nicktindall]

LGTM, only comment is I wonder if we should do the change to NO for "still fetching" and the additional assertions as separate PRs, but happy with whatever you decide there.

[ywangd]

@nicktindall I refactor the change for UNKNOWN state to a separate PR #137232. Let me know if this matches what you had in mind.

[DiannaHohensee]

Left some comments about nested assertions. In general, can we move the assertions out of the conditional paths and assert that not encountering THROTTLE during simulation is generally true, as opposed to conditionally? Unless I've misunderstood the necessity?

[DiannaHohensee]

Seeing the assert in a conditional branch makes this seem like certain paths are okay for THROTTLE. Suppose we were in a simulation and somehow got more than 1 relocating shard at once: that would be unsafe, right? Is there any reason we can't safely always have the assert in place? Like,

assert allocation.isSimulating() == false || routingNodes.getRelocatingShardCount() <= 1 : "...."; if (routingNodes.getRelocatingShardCount() > 0) { shardBalanced = true; } if (completeEarlyOnShardAssignmentChange && shardBalanced) { .... 

[DiannaHohensee]

Rather than putting this in a conditional, can we assert that either isSimulating==false || decision is not throttle up on line 1272? Or better yet, right after the decideAllocateUnassigned call, outside another layer of if-else condition?

[DiannaHohensee]

Can this be done right after the decideMove call? There's an early return path above on which we'd miss this assert.

[ywangd]

@DiannaHohensee Sorry for the late reply here. Got distracted by other work and then PTO. I pushed c62be8e based on your feedback for asserting in a more general path. Please let me know if the latest change works better for you.

[ywangd]

As discussed over the other PR. I am keeping throttle for non-simulation case for the time being. I added a link to the JIRA ticket for considering its removal.

[DiannaHohensee]

lgtm. I left some additional recommendations. Thanks for moving up the asserts to apply more generally.

[DiannaHohensee]

Sorry, I should have asked this in an earlier review round, but shouldn't we expect routingNodes.getRelocatingShardCount() to be exactly 1 when isSimulating is true? IIUC, tryRelocateShard will increment routingNodes.getRelocatingShardCount() when there's a YES answer but leave it unchanged for THROTTLE, per this code

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

Lines 1499 to 1502 in e8a9eb1

	canAllocateOrRebalance == Type.YES
	/* only allocate on the cluster if we are not throttled */
	? routingNodes.relocateShard(shard, minNode.getNodeId(), shardSize, "rebalance", allocation.changes()).v1()
	: shard.relocate(minNode.getNodeId(), shardSize)

Combined with the one shard move and then exit logic added recently, the assert would then be

simulation is false
OR
relocatingShardCount == 1 (and implicitly simulation is true)

[ywangd]

Yeah we can tighten it further see 6cdfdc1

[DiannaHohensee]

opt nit: might flip isSimulating above != THROTTLED check, seems easier to read.

[ywangd]

Sure see 1726061

[DiannaHohensee]

Unassigned replica shards can returning THROTTLE because only the primary shard path is THROTTLE free during simulation. At least until I get ES-12942 complete. I take it that's the problem here?

Since we have more or less ignored the allocateUnassigned code path for not-preferred, feel free to skip this assert: I'm not sure what benefit it adds right now. Or you can add a TODO referencing ES-12942 and I expect I should be able to tighten this up. We'll revisit allocateUnassigned for not-preferred in ES-13279, too.

[ywangd]

Unassigned replica shards can returning THROTTLE because only the primary shard path is THROTTLE free during simulation. At least until I get ES-12942 complete. I take it that's the problem here?

Yes ThrottlingAllocationDecider can still return throttling decision for unassigne replica shards. But ES-12942 is a separate issue. It's about early return in DesiredBalanceComputer instead of BalancedShardAllocator. It currently short-circuits only for primary shard assignement. Hence, changing ThrottlingAllocationDecider returning YES for replica shards is not really relevant since replica shards can already be assigned today but we don't publish the balance quickly enough after they are assigned.

In summary, let me raise a follow-up to fix ThrottlingAllocationDecider so that it does not return throttling in simulation. It's all part of the current JIRA ES-12955

[DiannaHohensee]

This is a public repo: should we shorten to ES-13378 since the link won't work for external users?

[ywangd]

Good catch. Pushed 228c35d