Fix Execution API state update conflicts in HA #59650
Conversation
When a SchedulerJob is marked failed, orphaned task instances may be reset and re-scheduled, incrementing try_number without recording the abandoned attempt. This change records the current attempt into task_instance_history before resetting so users have a complete audit trail of the failure.
ephraimbuddy
requested review from
XD-DENG,
amoghrajesh,
ashb and
kaxil
as code owners
boring-cyborg
bot
added
area:API
When a task pod reports a final state to the Execution API, another scheduler may have already moved the task instance to a terminal state. The API now treats state updates for already-terminal task instances as an idempotent no-op instead of returning 409, preventing races from failing the task pod.
ephraimbuddy
force-pushed
the
ignore-terminal-state-update
branch
from
8b1b6b0 to
68914ee
Compare
| # In HA, it's possible to receive a "late" finish/state update after another | ||
| # component already moved the TI to a terminal state. Treat this as an idempotent no-op to avoid | ||
| # crashing the process. | ||
| if previous_state in set(TerminalTIState): |
| # In HA, it's possible to receive a "late" finish/state update after another | ||
| # component already moved the TI to a terminal state. Treat this as an idempotent no-op to avoid | ||
| # crashing the process. |
There was a problem hiding this comment.
Wait -- how can this happen? If another component already moved to terminal state -- it means task was running in that other component/worker -- not the one that triggered this.
There was a problem hiding this comment.
2 workers shouldn't be running the same task!
There was a problem hiding this comment.
I think this might happen in some edge cases of CeleryExecutor race conditions when celery thinks that task has not completed and cancels it (but it finishes between it gets cancelled) and sends the task and it gets picked by another worker.
Possibly there are similar edge cases in K8S executor. I think It would be great to describe the semantics of task execution for both - > at most once (which we don't have), at least once (which I think we have), exactly-once (which I think is not really achievable easily - without adding a lot of complexity).
There was a problem hiding this comment.
2 workers shouldn't be running the same task!
That's true, but it's happening in the K-Executor :/ #57618 (comment)
There was a problem hiding this comment.
I'm going to verify this more but here's what I thought was going on:
Scheduler A tried to start X but the scheduler was marked failed. Scheduler B picked up the task(couldn't adopt) after resetting it and start X in another pod. At this point we now have two pods running. So I think one of the pods received an update state before the other.
There was a problem hiding this comment.
I have experienced the same state update issues with the CeleryExecutor, as well. It doesn't appear to be isolated to the k8s executor.
There was a problem hiding this comment.
@pdellarciprete That's a bug somewhere that needs fixing but the current fix here -- which this comment thread is for -- is too late. What needs to be fixed is workers not able to even run duplicate task -- not when task has run on both workers and both are trying to update it state.
This endpoint in this file is for the same.
There was a problem hiding this comment.
So it looks like the earlier PR fixed this issue and I wasn't testing on the right branch when I raised this. @pdellarciprete , can you confirm you are no longer seeing this issue after applying the previous fix(#59639)
Oh, disregard this. I was testing with 1 scheduler
|
I have done extensive tests and this PR:#59639 fixed the state update issue as well. |
Thank you! |
7 participants
When a task pod reports a final state to the Execution API, another
scheduler may have already moved the task instance to a terminal state.
The API now treats state updates for already-terminal task instances as
an idempotent no-op instead of returning 409, preventing races
from failing the task pod.
Depends on: #59639
I wonder if we should also guard the supervisor side? cc @ashb