Fix testRelocationFailureNotRetriedForever

[idegtiarenko]

The test failure happens in the following scenario:

1. the exception is thrown, org.elasticsearch.cluster.routing.allocation.AllocationService#applyFailedShards is executed
2. applyFailedShards updates the shard failed_attempts=1, moves it back to the STARTED state and submits a new desired balance computation
3. the new computation is delayed, we observe the current state with no shard movements and ensureGreen is exiting
4. desired balance computation is completed, reconciliation starts and completes
5. test observes RELOCATING shard

We can also exit before all retry attempts are exhausted without even noticing that as error count is not asserted:

1. the exception is thrown, org.elasticsearch.cluster.routing.allocation.AllocationService#applyFailedShards is executed
2. applyFailedShards updates the shard failed_attempts=1, moves it back to the STARTED state and submits a new desired balance computation
3. the new computation is delayed, we observe the current state with no shard movements and ensureGreen is exiting
4. test observes STARTED shard (wile relocating failure counter is not exhausted yet)
5. desired balance computation is completed, reconciliation starts and completes starting another relocation attempt

Until we have a functionality to await no computation, the best is to assert busy the computation is complete.

Closes: #108951

[elasticsearchmachine]

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

[idegtiarenko]

Added this assertion to ensure we exhausted all attempts before exiting

[mhl-b]

why hardcoded and not actual max retry value?

[mhl-b]

I looked at this test before and still trying to understand how these assertions prove that we dont retry forever as test name indicates. Since ensureGreen not reliable as you mentioned in description, reaching maxRetries and still being STARTED on current node not necessarily means we will not retry in the future. I guess this assumption holds until we dont reset retries and MaxRetryAllocationDecider still there, but it's implicit in this test.

[DaveCTurner]

++ let's say SETTING_ALLOCATION_MAX_RETRY.get(Settings.EMPTY) rather than the literal 5.

Also can we assertBusy() that the relocation counter reaches this value (or, even better use org.elasticsearch.test.ClusterServiceUtils#addTemporaryStateListener to await that cluster state) and then assert that the shard remains STARTED?

[idegtiarenko]

Will update to rely on default rather then a constant.

that the relocation counter reaches this value

Do you mean the relocation failure counter (one already asserted on L152) or something else?

and then assert that the shard remains STARTED?

I guess this part is tricky. I can add another assert in the end but theoretically there can always be a scheduled task that issue another cluster state change right after we perform the last assertion. This can not even be excluded by waiting for languid tasks as long as we are not using deterministic queue here.

[DaveCTurner]

Do you mean the relocation failure counter (one already asserted on L152)

Yeah, but wait only for that to reach 5, not for the rest of the conditions in the assertBusy().

I can add another assert in the end but theoretically there can always be a scheduled task that issue another cluster state change right after we perform the last assertion.

Sure but it shouldn't be relocating that shard once it's reached 5 failures should it?

[idegtiarenko]

Sure but it shouldn't be relocating that shard once it's reached 5 failures should it?

That is correct assuming we know implementation details.
Assuming a blackbox approach (or inventive ways to break the code without breaking the test) this is not completely reliable.

[DaveCTurner]

Maybe I'm missing something, but I think the whole point of SETTING_ALLOCATION_MAX_RETRY is to limit this value to 5, blocking further recovery attempts once it reaches the limit.

[idegtiarenko]

Correct, as long as we can rely on it, this test is valid.

[mhl-b]

LGTM