nf-k8s: Handle node disruption as an infrastructure failure in Pod and Job mode - #7464
Open
KasperWolsink99 wants to merge 4 commits into
Open
Conversation
Detect the `DisruptionTarget` pod condition (set by K8s on involuntary disruptions such as Spot/preemptible node preemption or graceful node shutdown) and surface it as a NodeTerminationException so the task is retried, unless the container already exited successfully. This complements the existing `Shutdown` and pod-not-found (404) detections, which do not fire reliably for graceful preemptions. Assisted-by: Claude Opus 4.8 (1M context) Signed-off-by: Kasper Wolsink <k.wolsink@hartwigmedicalfoundation.nl>
In Job mode, `K8sClient.jobState()` caught any `NodeTerminationException` thrown by `podState()` -- including the `DisruptionTarget`/`Shutdown` detections -- and fell back to the Job status. With `backoffLimit: 0` the Job reports a generic "backoff limit" failure, so the node-termination signal was lost and the task retried via the normal errorStrategy path, counting against `maxRetries`. Pass the caught exception into `jobStateFallback0()` and re-throw it when the Job has failed, so a genuine node disruption is surfaced as an infrastructure failure and retried without counting against `maxRetries` -- matching Pod mode. The succeeded-but-reaped fallback is unchanged and still runs first, so a completed task is never re-run. Also drop the misleading "Pod not found" warning, which was inaccurate for a present pod carrying a disruption condition. Assisted-by: Claude Opus 4.8 (1M context) Signed-off-by: Kasper Wolsink <k.wolsink@hartwigmedicalfoundation.nl> (cherry picked from commit 9040a5de0d17cd0ab383ad37cbad22b31421cf35)
Cover the three previously-untested branches of K8sClient.jobStateFallback0(): - a failed Job with a node-termination exception -- re-thrown so the task is retried as an infrastructure failure - a failed Job without one -- ProcessFailedException carrying the message from the Job's Failed condition - a Job with no pods scheduled yet -- empty-map fall-through Assisted-by: Claude Opus 4.8 (1M context) Signed-off-by: Kasper Wolsink <k.wolsink@hartwigmedicalfoundation.nl>
Collapse the duplicated re-surfacing rationale that appeared at both the catch site and the throw site into a single concise explanation. Assisted-by: Claude Opus 4.8 (1M context) Signed-off-by: Kasper Wolsink <k.wolsink@hartwigmedicalfoundation.nl>
✅ Deploy Preview for nextflow-docs canceled.
|
KasperWolsink99
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detect involuntary K8s node disruptions (e.g spot-vm pre-emption) and treat them as infrastructure failures so the affected task is resubmitted without consuming the retry budget, rather than surfacing them through the user-facing
errorStrategyretry path.Changes
DisruptionTargetpod condition inK8sClient.podState()and surface it as aNodeTerminationException. Complements the existingShutdownand pod-not-found (404) detections, which don't fire reliably for graceful preemptions.jobStateFallback0()now re-throws the originalNodeTerminationExceptioninstead of throwing aProcessFailedExceptionif the issue was caused by a node termination/disruption. This ensures that these disruptions are resubmitted without consuming retry limit, which now functions the same as how it behaves in pod mode.The second bullet point is interesting since it also affects already existing
NodeTerminationException. Inmasterthese surface as aProcessFailedExceptionin job mode, which differs from how it behaves in pod mode. This PR also fixes this discrepancy.Closes: #7378