fix(executor): enforce TaskMetadata.timeout as a per-attempt max runtime - #7910
fix(executor): enforce TaskMetadata.timeout as a per-attempt max runtime#7910shaon-chowdhury-euc wants to merge 3 commits into
Conversation
The SDK already serializes max_runtime onto the TaskAction, but the executor never read it, so hung tasks ran to success. Abort and finalize the plugin at the deadline, emit TIMED_OUT, and retry only within the existing attempt budget. Closes flyteorg#7901 Signed-off-by: shaon-chowdhury-euc <shaon.chowdhury@eucalyptus.vc>
Anchor the attempt clock on the plugin's reported start rather than inferring it from PhaseHistory, ignoring reports that predate the attempt. Collapse three deadline predicates into one, and persist the resume marker before cleanup. Fix DeepEqual on *metav1.Time, and resolve error info and fail rate for TIMED_OUT actions. Signed-off-by: shaon-chowdhury-euc <shaon.chowdhury@eucalyptus.vc>
a13073e to
43a77c7
Compare
| // service dedupes on insert (see recordEvent), so this stays at-least-once | ||
| // end to end rather than pushing the problem onto consumers. | ||
| timeoutEvent := r.buildActionEvent(ctx, taskAction, timeoutInfo) | ||
| if err := r.recordEvent(ctx, timeoutEvent); err != nil { |
There was a problem hiding this comment.
When we record timeout event here, the run service will treat the action as terminal and close UI watch stream here. I think we should set a gate in run service to terminate the stream only if all retries are completed.
There was a problem hiding this comment.
agreed, this closes the stream mid-retry. i'd keep the per-attempt TIMED_OUT event though (silent overruns are the bug this fixes) and gate the close in run service instead: only close when the terminal attempt is also the action's current attempt. status.Attempts moves to N+1 in the same reconcile that emits TIMED_OUT(N) so a retrying attempt never satisfies that. will add the gate plus a timeout-then-retry watch test. also want to check the actions table doesn't treat the intermediate TIMED_OUT as sticky-terminal and reject the Queued that follows.
There was a problem hiding this comment.
pushed the gate in b2f437c ie. close only when the action phase is terminal and the last event attempt is terminal and equals status.Attempts (same equality buildActionDetails already uses). added a unit table for both write orderings plus an api test that replays the executor's event-then-row sequence; it fails on the old predicate. also checked the actions-table side: TIMED_OUT is already in retryablePhases so the Queued that follows is accepted.
| if deadline, hasDeadline := taskAttemptDeadline(taskAction, maxRuntime); hasDeadline { | ||
| var overran bool | ||
| if handleErr != nil { | ||
| overran = !r.now().Before(deadline) |
There was a problem hiding this comment.
Why do we need this if branch? I think attemptOverran is enough to tell if we are overran when we have handleErr?
There was a problem hiding this comment.
they compute the same thing today (with handleErr set the transition is the zero value, so attemptOverran degenerates to !now.Before(deadline)). the branch exists because transition isn't meaningful when Handle returns an error, and attemptOverran's OccurredAt escape (a terminal result proving it finished before the deadline) only makes sense for a successful Handle. didn't want timeout enforcement depending on what a zero-value Transition happens to return. can inline it behind a comment if you'd rather.
…is done A timed-out attempt emits a terminal TIMED_OUT event for attempt N before the action restarts as attempt N+1, and closing on attempt terminality alone ended the WatchActionDetails stream mid-retry. Close only when the action's phase is terminal and action_events has caught up with it: the highest-numbered attempt is terminal and is the action's current attempt. Checked the other half of the concern too: the actions-table updater already lists TIMED_OUT as a retryable phase, so the Queued row that follows a mid-retry timeout is accepted — no stickiness to fix there. Signed-off-by: shaon-chowdhury-euc <shaon.chowdhury@eucalyptus.vc>
c376411 to
b2f437c
Compare
2 participants
Summary
TaskMetadata.timeout(max_runtime) in the TaskAction executor: persistAttemptStartedAt/TimeoutAt, abort and finalize the plugin at the deadline, emitTIMED_OUT, and retry only within the existing attempt budget.v2.0.42ignored it, so a 30s task could run for minutes and finishSUCCEEDED.activeDeadlineSecondsas a substitute — on current builds the kubelet kill leaves the Pod object behind and the executor recreates it without consuming an attempt.The bound is enforced against the controller clock. A terminal transition observed before the deadline wins whatever timestamp it carries; from the deadline onward it wins only if
TaskInfo.OccurredAtproves it finished in time. A terminal report with no usable timestamp is indistinguishable from an overrun, so the bound takes precedence.TIMED_OUTwas already terminal for condition actions but rare in practice. Two places that special-casedACTION_PHASE_FAILEDare updated to keep behaviour consistent now that tasks reach it: action-detail error resolution and task-group fail rate.Test plan
Verified:
go test -raceacross./executor/...,./actions/...,./runs/...InvalidSpec; ordinary failures not misclassified as timeouts; system retries not consuming a user attempt; serialized cache reservations held until terminal.make manifests generateproduces no drift; all four CRD copies carry the new status fields.Not yet verified on a live cluster:
flyte.Timeout(max_runtime=30s)task sleeping 300s, end to end through the SDK.