feat(operator): treat a stage timeout as a retryable failure (#373) - #402
Conversation
github-actions
Bot
added
doc
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/internal/controller/skyhook_controller.go (1)
2502-2508: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve every unprocessed finished Job.
Line 2505 protects an unprocessed
JobComplete, but an unprocessedJobFailedreaches Line 2508 and is foreground-deleted. This can delete a terminal failure beforeJobReconciler.handleFailedJobclassifies it, writeserroring, and setsstate-recorded. The next pass can then recreate the stage instead of parking a genuine failure.Return early for all
!jobProcessed(existing)outcomes. Add a regression test for an unprocessed failed Job whose node entry is not yeterroring.Proposed fix
+ if !jobProcessed(existing) { + return nil // JobReconcile owns unrecorded terminal outcomes + } + if isParkedJob(existing) && r.entryErroringAtStage(skyhookNode, _package, stage) { return nil // parked: the finished Job is doing its job, absorb this recreate attempt } - if hasJobCondition(existing, batchv1.JobComplete) && !jobProcessed(existing) { - return nil // unrecorded completion; JobReconcile owns it, do not discard it - } return deleteJobForeground(ctx, r.Client, existing)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/skyhook_controller.go` around lines 2502 - 2508, Update the existing Job preservation logic around isParkedJob and hasJobCondition so every unprocessed terminal Job, including JobFailed, returns early instead of reaching deleteJobForeground; retain deletion for processed Jobs. Add a regression test covering an unprocessed failed Job whose node entry is not yet erroring, verifying it is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 173-177: Update the “Log visibility” section to state that
deadline expiry terminates the Pod’s containers and marks it Failed with reason
DeadlineExceeded, without deleting the Pod object; distinguish separate Job/Pod
garbage collection from log retention while preserving the rationale and
existing best-effort log-tail snapshot behavior.
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 150-152: Update the backoffLimit descriptions in
operator/internal/controller/skyhook_controller.go lines 150-152,
operator/config/manager/manager.yaml lines 112-114, and
docs/designs/2026-07-10-package-execution-as-jobs.md line 79 to describe the
value as retries after the initial attempt, or explicitly state that total
attempts equal backoffLimit plus one; preserve the existing zero-value
semantics.
---
Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 2502-2508: Update the existing Job preservation logic around
isParkedJob and hasJobCondition so every unprocessed terminal Job, including
JobFailed, returns early instead of reaching deleteJobForeground; retain
deletion for processed Jobs. Add a regression test covering an unprocessed
failed Job whose node entry is not yet erroring, verifying it is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e380a9a2-af95-4b06-a7d5-d705a0533d71
📒 Files selected for processing (20)
chart/templates/deployment.yamlchart/templates/nodewright-crd.yamlchart/values.yamldocs/designs/2026-07-10-package-execution-as-jobs.mddocs/operator-status-definitions.mdk8s-tests/chainsaw/nodewright/failure-nodewright/assert_parked_job.yamlk8s-tests/chainsaw/nodewright/failure-nodewright/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yamloperator/api/nodewright/v1alpha1/nodewright_types.gooperator/config/crd/bases/nodewright.nvidia.com_nodewrights.yamloperator/config/manager/manager.yamloperator/internal/controller/job_builder.gooperator/internal/controller/job_builder_test.gooperator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.gooperator/internal/controller/pod_controller.gooperator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.gooperator/internal/controller/swap_test.gooperator/internal/controller/workload_migration_test.go
Coverage Report for CI Build 31534479143Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 79.018%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…-expiry text CodeRabbit review on #402, both findings valid: - backoffLimit: 3 permits one initial attempt plus three retries. Four comments described it as the attempt count; say retries and spell out backoffLimit+1 total attempts. - The Log visibility section still described deadline expiry as deleting the running pod. That is now only true of the Job-level ceiling; a per-attempt expiry terminates the containers and marks the pod Failed with reason DeadlineExceeded in place, so the attempt survives as an ordinary archive with its logs. Distinguish the two and keep the snapshot rationale, which the ceiling case still needs. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Addressed both CodeRabbit findings in a31ccbe — both were valid.
On CI: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/designs/2026-07-10-package-execution-as-jobs.md (1)
40-40: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the Job-level ceiling as a second terminal path.
The summary says that only exhausted retry budgets produce parked
erroringstages. Per-attemptDeadlineExceededfailures are retried, but the derived Job-levelDeadlineExceededalso parks the stage when pods never receiveStartTime. Rewrite this sentence to distinguish per-attempt deadline retries from Job-level ceiling parking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/designs/2026-07-10-package-execution-as-jobs.md` at line 40, Update the lifecycle summary to distinguish per-attempt DeadlineExceeded failures, which are killed and retried, from the derived Job-level DeadlineExceeded ceiling, which parks the stage when pods never receive StartTime; identify both retry-budget exhaustion and the Job-level ceiling as terminal paths producing an erroring parked stage.operator/internal/controller/skyhook_controller.go (1)
2483-2483: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve unprocessed terminal
FailedJobs in theAlreadyExistspath.When
existingis finished, aFailedJob withoutstate-recordedreaches the deletion branch unless its entry is alreadyerroring. Return without deletion for every unprocessed finished Job. Add a regression test for thisAlreadyExistscase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/skyhook_controller.go` at line 2483, Update the AlreadyExists handling around the finished-job logic in operator/internal/controller/skyhook_controller.go:2483 so every finished Job lacking state-recorded, including Failed Jobs, is returned without deletion regardless of whether its entry is erroring; add a regression test covering this AlreadyExists case. The related documentation at docs/designs/2026-07-10-package-execution-as-jobs.md:213-223 requires no direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 151-153: Update the documentation at
operator/internal/controller/skyhook_controller.go:151-153,
chart/values.yaml:92-96, operator/config/manager/manager.yaml:114-115, and
docs/designs/2026-07-10-package-execution-as-jobs.md:113-115 to separate
retry-budget counting from parking classification. Preserve JobBackoffLimit+1
and zero-value semantics, clarify that ignored disruptions do not count while
admission-rejected attempts may consume the budget, and state that parking
requires genuine failure evidence rather than occurring unconditionally after
exhaustion.
---
Outside diff comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Line 40: Update the lifecycle summary to distinguish per-attempt
DeadlineExceeded failures, which are killed and retried, from the derived
Job-level DeadlineExceeded ceiling, which parks the stage when pods never
receive StartTime; identify both retry-budget exhaustion and the Job-level
ceiling as terminal paths producing an erroring parked stage.
In `@operator/internal/controller/skyhook_controller.go`:
- Line 2483: Update the AlreadyExists handling around the finished-job logic in
operator/internal/controller/skyhook_controller.go:2483 so every finished Job
lacking state-recorded, including Failed Jobs, is returned without deletion
regardless of whether its entry is erroring; add a regression test covering this
AlreadyExists case. The related documentation at
docs/designs/2026-07-10-package-execution-as-jobs.md:213-223 requires no direct
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 802e440d-f389-4350-aafd-c2d3e557b172
📒 Files selected for processing (5)
chart/values.yamldocs/designs/2026-07-10-package-execution-as-jobs.mdk8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yamloperator/config/manager/manager.yamloperator/internal/controller/skyhook_controller.go
|
Addressed in 83741d3 — valid, and it's about the distinction this PR introduces. The comments conflated two decisions the code deliberately keeps apart: A Failed pod now falls into one of three classes, stated explicitly in the design doc's
That paragraph also still carried the pre-PR claim that the |
CodeRabbit review on #402. The comments conflated two decisions the code deliberately keeps apart: backoffLimit counts Failed pods, but exhausting it only takes the Job terminal — the stage parks as erroring solely when a retained attempt genuinely failed. A Failed pod falls into one of three classes: an ignored disruption spends nothing; an attempt the kubelet refused to admit spends an attempt without being the package's failure; a genuine step failure or per-attempt timeout spends an attempt and is. Only the third parks. Say so in the options docstring, manager.yaml, values.yaml, and the design doc's backoffLimit paragraph, which still claimed only genuine failures could count at all. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/designs/2026-07-10-package-execution-as-jobs.md (1)
161-163: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign
stageTimeoutdocumentation with the API validation comment.
stageTimeoutcontrols package Pod attempt deadlines, not the Job-level deadline. Update the webhook comment to describe the derived Job ceiling and the separate interrupt Job deadline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/designs/2026-07-10-package-execution-as-jobs.md` around lines 161 - 163, Update the stageTimeout API validation comment to clarify that it derives the package Pod attempt deadline and the corresponding Job ceiling, while the interrupt Job retains a separate deadline. Keep the documented defaults, zero-value behavior, and retry-budget semantics aligned with the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 113-115: Update the sentence in the retry and failed-attempt
explanation so the genuine step failure or per-attempt timeout clause ends with
“and is genuine failure evidence.” Preserve the surrounding distinctions between
ignored disruptions, admission failures, and genuine failures.
---
Outside diff comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 161-163: Update the stageTimeout API validation comment to clarify
that it derives the package Pod attempt deadline and the corresponding Job
ceiling, while the interrupt Job retains a separate deadline. Keep the
documented defaults, zero-value behavior, and retry-budget semantics aligned
with the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9a8c451a-376e-4ee5-8ce5-1b9ceca0e344
📒 Files selected for processing (4)
chart/values.yamldocs/designs/2026-07-10-package-execution-as-jobs.mdoperator/config/manager/manager.yamloperator/internal/controller/skyhook_controller.go
…it paragraph CodeRabbit review on #402: the elliptical "and is" left the third failure class without a predicate. Spell it out, keeping the parallelism with the preceding "is not the package's failure" rather than switching terms. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Fixed in e399d4b. Took the point but not the exact wording — "and is the package's failure" keeps the parallelism with the preceding "is not the package's failure", where "is genuine failure evidence" switches terms mid-sentence. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/designs/2026-07-10-package-execution-as-jobs.md (1)
124-125: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist genuine-failure evidence before child Pods can disappear.
jobFailureIsGenuinereturnsfalsewhen no retained failed child Pod remains. The Job-tracking finalizer protects Kubernetes Job accounting, but not the later operator classification. Pod GC orpruneFailedAttemptscan remove the only genuine-failure archive, causing a realBackoffLimitExceededJob to be marked and swept instead of parked. Persist the verdict on the Job before terminal processing, or add a durable fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/designs/2026-07-10-package-execution-as-jobs.md` around lines 124 - 125, Update the terminal Job-processing flow around jobFailureIsGenuine so the genuine-failure verdict is persisted on the Job before Pod GC or pruneFailedAttempts can remove the last failed archive. Ensure BackoffLimitExceeded with genuine evidence remains classified as erroring and parked, while kubelet-admission cases retain the existing marker, TTL, and sweep behavior by using the persisted verdict or another durable fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 124-125: Update the terminal Job-processing flow around
jobFailureIsGenuine so the genuine-failure verdict is persisted on the Job
before Pod GC or pruneFailedAttempts can remove the last failed archive. Ensure
BackoffLimitExceeded with genuine evidence remains classified as erroring and
parked, while kubelet-admission cases retain the existing marker, TTL, and sweep
behavior by using the persisted verdict or another durable fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e7f1fa21-7a84-4abe-b31c-71f06b0fa699
📒 Files selected for processing (1)
docs/designs/2026-07-10-package-execution-as-jobs.md
CodeRabbit review on #402 read jobFailureIsGenuine as the sole gate on parking and concluded that losing the archives to pod GC loses the park. It is not the sole gate, but nothing said so, so write it down. The park predicate is (terminal Failed, entry at (stage, erroring)) in both places that evaluate it. jobFailureIsGenuine only decides whether the terminal path is the one to write that entry; the Pod watch writes it live, while the archive still exists, using the same classification. Each covers the other's blind spot — the Pod watch survives archives being GC'd, the terminal path survives the operator being down through the retries. Both must miss to lose a park, and the stage then re-runs and parks on the next cycle rather than churning. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Pushed c85ee6e — but as documentation, not the suggested fix. The mechanism is real; the conclusion isn't, and I'd rather not add a persisted verdict for a gap that mostly doesn't exist.
Neither reads a verdict.
So a genuine failure that loses its archives to pod GC still parks — the Pod watch already wrote the entry. Both writers must miss, which needs the operator down through the entire retry window and the archives gone. And that case is bounded, not churn: the Job is swept, the stage re-runs, and the next cycle's Pod-watch evidence (operator now up) parks it. One extra retry cycle. Two smaller corrections: What was genuinely missing is that none of this was written down, which is why it read as a single-gate design. Now in the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
operator/internal/controller/job_controller.go (2)
786-796: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRetain unprocessed Failed Jobs until
state-recordedis written.
isParkedJobnow recognizes every terminal Failed Job, but the existing-Job path only retains it when the stage entry is alreadyerroring. If that path runs before terminal Job reconciliation, it can delete the Job and its retained failure evidence.
operator/internal/controller/job_controller.go#L786-L796: deferJobFailed && !jobProcessedJobs until terminal reconciliation writesstate-recorded.docs/designs/2026-07-10-package-execution-as-jobs.md#L216-L217: keep the lifecycle rule aligned with the controller and add a regression test for concurrent reconcile ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/job_controller.go` around lines 786 - 796, The existing-Job retention logic must defer terminal Failed Jobs until terminal reconciliation records state-recorded, rather than relying only on the stage being erroring. Update isParkedJob and its callers in operator/internal/controller/job_controller.go (lines 786-796) to preserve JobFailed && !jobProcessed Jobs until that marker exists; update docs/designs/2026-07-10-package-execution-as-jobs.md (lines 216-217) to document the aligned lifecycle rule and add a regression test covering concurrent reconcile ordering.
439-442: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap propagated errors with operation context.
The changed failure path returns bare
errat Lines [442], [465], and [500]. These functions perform multiple operations. The errors do not identify whether classification, state recording, or child-pod inspection failed.Wrap each error with
fmt.Errorf("...: %w", err).Suggested wrapping
- return ctrl.Result{}, err + return ctrl.Result{}, fmt.Errorf("classifying failed Job %s: %w", job.Name, err) - return ctrl.Result{}, err + return ctrl.Result{}, fmt.Errorf("recording failure for Job %s: %w", job.Name, err) - return false, err + return false, fmt.Errorf("checking failure evidence for Job %s: %w", job.Name, err)As per coding guidelines, wrap propagated errors with
fmt.Errorf("...: %w", err)and do not return a bareerrfrom functions that perform more than one operation.Also applies to: 464-466, 498-500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/job_controller.go` around lines 439 - 442, Update handleFailedJob and the corresponding error paths around jobFailureIsGenuine, state recording, and child-pod inspection to wrap each propagated error with fmt.Errorf using descriptive operation context and %w. Replace the bare err returns at all three referenced paths while preserving the existing control flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@operator/internal/controller/job_controller.go`:
- Around line 786-796: The existing-Job retention logic must defer terminal
Failed Jobs until terminal reconciliation records state-recorded, rather than
relying only on the stage being erroring. Update isParkedJob and its callers in
operator/internal/controller/job_controller.go (lines 786-796) to preserve
JobFailed && !jobProcessed Jobs until that marker exists; update
docs/designs/2026-07-10-package-execution-as-jobs.md (lines 216-217) to document
the aligned lifecycle rule and add a regression test covering concurrent
reconcile ordering.
- Around line 439-442: Update handleFailedJob and the corresponding error paths
around jobFailureIsGenuine, state recording, and child-pod inspection to wrap
each propagated error with fmt.Errorf using descriptive operation context and
%w. Replace the bare err returns at all three referenced paths while preserving
the existing control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b9ad03b8-2e9b-43f4-aa7c-ccf82cdce67d
📒 Files selected for processing (2)
docs/designs/2026-07-10-package-execution-as-jobs.mdoperator/internal/controller/job_controller.go
…th too CodeRabbit review on #402. shouldDeleteFinishedJob was taught to wait for the state-recorded marker on both outcomes, but handleExistingJob — its mirror, and the path that reaches the window first — still carved out only unprocessed Complete Jobs. A finished Job does not satisfy JobExists, so the next pass creates over its deterministic name; landing there before JobReconcile processed a terminal Failed Job deleted it, taking the retained attempts with it and restarting the stage on a fresh budget. Generalize the carve-out to any unprocessed finished Job so both paths reach the same verdict, and cover the ordering with specs on handleExistingJob, which had none. Also wrap the two propagated errors in handleFailedJob so a failure names whether classification or the state write broke. Left jobFailureIsGenuine's childPods error bare: childPods already contextualizes it, and its two siblings in this file (snapshotFailureLogs, pruneFailedAttempts) pass it through the same way. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Both addressed in 3e74f0d. The first was a real bug — thanks. Retain unprocessed Failed Jobs (Major). Correct, and it's an asymmetry I introduced: I taught Generalized the carve-out to any unprocessed finished Job, so both paths now reach the same verdict for the same Job. Added three specs on
Design doc rule 3 updated to match. Error wrapping (Minor). Wrapped the two in Left
|
Review feedback on #402. The API group is nodewright.nvidia.com on this branch, so user-visible event text should not say skyhook. Only the event this PR added is changed. Six pre-existing [skyhook:%s] event strings remain in pod_controller.go and skyhook_controller.go; those are untouched by this PR and sweeping them here would collide with other in-flight branches in the epic. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
7d065f7 to
a3dd68c
Compare
#316 set the stage deadline on JobSpec.ActiveDeadlineSeconds, which is terminal: exceeding it fails the Job permanently with no replacement pod, so the first expiry parked. That made a timeout the one failure class with no retry. Move the bound to the pod template so an expired attempt is Failed and replaced like any other failure, and give up on a finite backoffLimit instead of math.MaxInt32. No operator-side retry counter to persist. Package Jobs now carry three bounds: spec.template.spec.activeDeadlineSeconds = stageTimeout (per attempt) spec.backoffLimit = JOB_BACKOFF_LIMIT (default 3) spec.activeDeadlineSeconds = derived ceiling The ceiling is (backoffLimit+1) * (stageTimeout + gracefulShutdown + 10m), clamped to MaxInt32. It exists only for the case a per-attempt clock cannot bound: that clock runs from pod.Status.StartTime, which a pod the kubelet never acknowledges never gets. gracefulShutdown is in the formula because podReplacementPolicy Failed waits out every shutdown; without it a slow shutdown could truncate the retry budget into a DeadlineExceeded that reads as a hang. Deriving it rather than adding a fourth knob makes a ceiling below the retry budget unrepresentable. Interrupt Jobs are deliberately unchanged. Under OnFailure backoffLimit counts container restarts, so a finite budget would be spent by the in-place restart that is the reboot recovery, and the bound must span the reboot, which a per-attempt clock cannot. The park signal inverts, as #373 anticipated: BackoffLimitExceeded becomes the park and DeadlineExceeded becomes a routine retry. Two consequences that were not in the issue and are worth review attention: - BackoffLimitExceeded alone is not sufficient to park. These pods carry spec.nodeName rather than going through the scheduler, so kubelet admission is the only gate they face; a node at capacity or returning from a reboot can reject several node-pinned replacements in a row, each Failed with no container statuses and no DisruptionTarget for the Ignore rule to match. At MaxInt32 that cost an archive slot; at 3 it exhausts the budget in ~70s and would park a package that never ran a line of script. Terminal failure is now believed only when a retained archive really failed. The Job-level ceiling needs no such evidence. - pod_controller.go could not stay untouched. A pod killed by its own deadline whose container never started (unpullable image, missing configmap - the hang the deadline exists for) has no exit code, and podFailureIsGenuine rejects both shapes it can take. The Pod watch now also keys on the pod-level DeadlineExceeded reason, which nothing else sets; without it a hang would read in_progress for the whole retry budget, worse than the single deadline this replaces. Also: shouldDeleteFinishedJob now requires the state-recorded marker for Failed Jobs, not just Complete. A finite backoffLimit takes a Job from first failure to terminal in about a minute, so the sweep would otherwise race the erroring write into a fresh, equally doomed attempt. Behavior note for the changelog: backoffLimit bounds every failure class, not just timeouts. A crash-looping package parks after 4 attempts (~70s) where it previously retried for the whole stageTimeout (~1h, ~10 attempts). Packages that ride out transient environment flakiness lose that hour of self-healing, which is why the limit is an operator knob rather than a constant. Default stays 3 per the issue. Docs: stageTimeout docstring and both CRD copies, the design doc's Job shape / retry / stage-deadline / pause / finished-Job-rules / rejected- alternatives sections and its stale skyhook_types.go path, and the erroring row in operator-status-definitions.md (it now also means "gave up"). Chart and kustomize both carry JOB_BACKOFF_LIMIT. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
| // isParkedJob reports whether a Job is a parked failure — a terminal Failed Job that the | ||
| // finished-Job rules deliberately leave in place while its stage sits erroring, so nothing | ||
| // recreates the stage until a rerun/reset/config-change/TTL clears it. | ||
| // | ||
| // Every terminal failure qualifies, deliberately: with a finite backoffLimit the reason no longer | ||
| // separates a real failure from a backstop. jobFailureIsGenuine draws that line instead, and only | ||
| // a genuine failure leaves the entry at (stage, erroring) — the other half of every park | ||
| // predicate, without which a non-genuine failure is swept and the stage re-runs. |
There was a problem hiding this comment.
Can we find a new name? Parked seems like a human controlled name, like paused. I feel like timedout is more clear.
| spec.BackoffLimit = ptr(opts.JobBackoffLimit) | ||
| if timeout > 0 { | ||
| spec.Template.Spec.ActiveDeadlineSeconds = ptr(deadlineSeconds(timeout)) | ||
| spec.ActiveDeadlineSeconds = ptr(jobCeilingSeconds(opts, _package, timeout)) |
There was a problem hiding this comment.
Sort of wonder if we need this set?
If have n retry, and a try has a timeout, do we need a global retry?
There was a problem hiding this comment.
I guess would cover some edge cases, like were the pod can schedule or run maybe
| // Without this the first timeout would write nothing and a hang would read in_progress until the | ||
| // entire retry budget burned down. | ||
| func podDeadlineExceeded(pod *corev1.Pod) bool { | ||
| return pod.Status.Phase == corev1.PodFailed && pod.Status.Reason == podReasonDeadlineExceeded |
There was a problem hiding this comment.
does the sdk have this const? podReasonDeadlineExceeded would stay in sync better if so
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
Cross-review summary
Automated multi-reviewer analysis of b27c75f6, cross-reviewed to consensus with adversarial verification of every confirmed finding. Nothing was run against a cluster and no tests were executed; every claim comes from reading the pinned commit. CI is green on this commit (all 34 checks, including all four e2e pools across k8s 1.33-1.36).
Findings fall into two groups. Confirmed ones reached agreement across independent reviewers and then survived a fresh reviewer whose only job was to refute them. Unadjudicated ones were raised late in the process and never cross-evaluated, so each carries a single reporter's position: read them, don't treat them as agreed. Two further findings were dropped after verification showed the arithmetic behind them was wrong, and are not reproduced here.
Findings that anchor to changed lines are inline. The rest follow.
Confirmed
operator/internal/controller/job_controller.go:667 - the pruner and the classifier disagree on "genuine"
pruneFailedAttempts builds its archive set filtering only on Status.Phase == PodFailed && !hasDisruptionTarget(...), then keeps first+last and deletes the middle. jobFailureIsGenuine applies a stricter predicate to whatever survived: podDeadlineExceeded(...) || podFailureIsGenuine(...). A kubelet-admission rejection satisfies the pruner's filter but not the classifier's, so a real step failure sandwiched between two rejections is prunable while both rejections survive.
When the terminal classifier is the only writer that ran (an operator restart or leadership change spanning the Pod-watch window), a genuinely failing stage is then classified non-genuine: handleFailedJob marks the Job processed without recording erroring, and the sweep clears it so the stage re-runs. Repeated host-side work, and the real failure stays invisible until a later cycle parks it.
The dual-writer design bounds this, and the docstring at 482-484 acknowledges the gap as erring toward re-running rather than parking. But the pruner could select its keep-pair with the same podFailureIsGenuine/podDeadlineExceeded predicate and close it outright, which its own comment ("the first genuine failure") suggests was the intent.
operator/RELEASE_NOTES.md - the declared behavior change has no release note
The PR description has a section titled "Behavior change worth a changelog line", but no hand-authored note landed. operator/CHANGELOG.md:3 is marked generated, with hand-authored behavior/upgrade notes directed to RELEASE_NOTES.md; that file's ## Unreleased / ### Changed section is untouched by this diff, as is chart/RELEASE_NOTES.md despite chart/values.yaml gaining a new user-settable jobBackoffLimit.
An operator upgrading gets a self-heal window shortened from roughly an hour to roughly 70 seconds for packages that ride out transient environment flakiness, with nothing telling them the mitigation is to raise JOB_BACKOFF_LIMIT.
Worth noting this is an epic-wide pattern rather than a one-off: #348 / #350 / #351 / #390 also added jobTtlSucceeded / jobStageTimeout / stageTimeout with no RELEASE_NOTES entry. If the intent is to write these once at the epic's landing PR, that is reasonable, but it is worth being deliberate about.
Unadjudicated (single reporter, never cross-evaluated)
operator/internal/controller/job_controller.go:614 - a prior admission rejection suppresses the last-logs snapshot
snapshotFailureLogs returns as soon as it sees any PodFailed non-disruption child pod, on the reasoning that "a genuine failed archive already holds full logs". That is the same loose predicate as the pruner finding above. A pod with no init-container statuses (the kubelet-admission rejection this PR adds handling for) is not genuine and carries no logs at all.
So on a Job that reaches FailureTarget via the Job-level ceiling after an earlier admission rejection, the snapshot is skipped: the surviving archive has no logs and no container statuses, and nodewright.nvidia.com/last-logs is never written. The parked tombstone names nothing, in exactly the never-started-container case the waiting-reason fallback exists for. Best-effort evidence only, no state-machine effect.
Open questions
- Interrupt gating loosens as a side effect.
HasRunningPackagescounts unfinished Jobs only, and a crash-looping package Job now goes terminal in ~70s instead of holding Active for the wholestageTimeout. Another package's, or another NodeWright's, interrupt (cordon / drain / reboot) can now proceed on a node where a parked erroring package sits, where previously the Active Job held the gate. Intended? - DeploymentPolicy thresholds trip much sooner for a crash-looping package, for the same reason. Nothing asserts the old timing, but rollout shaping on large fleets will behave differently after upgrade.
- Should
pruneFailedAttemptsuse the classifier's predicate? That closes the first confirmed finding entirely. - Is the unbounded hang on
stageTimeout: 0an accepted trade-off? If so, the CRD field doc andchart/values.yamlshould say so explicitly rather than implying the retry budget bounds it. See the inline comment on the field. spec.suspendand the failure count. The design doc's "suspend-deletions are excluded from the failure count upstream (verified empirically)" is load-bearing now that the budget is finite: were it false, four pause/resume cycles would spend the whole budget and park every in-flight stage. Could not be verified from this repo (k8s.io/kubernetesis not vendored) and nothing was run.- Pod-template
activeDeadlineSecondsbounds.jobCeilingSecondsclamps toMaxInt32, butdeadlineSecondsdoes not, so astageTimeoutabove roughly 68 years emits an unclamped pod-template value the apiserver may reject, turning every Create into a non-AlreadyExistserror loop. The webhook only rejects negatives. Unrealistic input, noted for completeness. chart/README.md's values table omitsjobBackoffLimit- though it already omitsjobTtlSucceeded/jobTtlFailed/jobStageTimeoutfrom the prior chart PR. Is that table meant to be exhaustive, or a curated subset?docs/designs/2026-07-10-package-execution-as-jobs.md:130(an unchanged context line) still says the operator "prunes failed attempts to one archive", while the same file's archive bullet andpruneFailedAttemptskeep two. Pre-existing, but this PR revises that file heavily.
| // killed and retried like any other failed attempt, and the package surfaces as erroring | ||
| // once the operator's retry budget (JOB_BACKOFF_LIMIT) is spent. Interrupt stages are the | ||
| // exception: their attempt must span a reboot, so it bounds the whole stage instead. | ||
| // Unset uses the operator default (JOB_STAGE_TIMEOUT); "0" removes the time bound for this |
There was a problem hiding this comment.
Confirmed finding. This says 0 leaves "the retry budget as its only limit", but with 0 there is no limit at all for one failure class.
job_builder.go:177-179 gates both spec.template.spec.activeDeadlineSeconds and the Job-level spec.activeDeadlineSeconds behind if timeout > 0, so zero omits both. The design doc in this same commit explains that the Job-level ceiling exists precisely because the per-attempt clock runs from pod.Status.StartTime, which a pod the kubelet never acknowledges never gets, so "a Job whose pods never start would sit Active forever". With stageTimeout: 0 that ceiling is gone too, and such a pod stays Pending rather than becoming Failed, so it never consumes a retry either.
A user who sets stageTimeout: 0 (or the operator-wide jobStageTimeout: "0") on the strength of this wording gets no bound whatsoever on an unacknowledged or unpullable attempt: the package sits in_progress indefinitely, with no erroring signal and no DeploymentPolicy failure accounting.
The same wording appears in chart/values.yaml:88-90 for the operator-wide knob. Either the zero case should keep the ceiling, or both texts should name the exception.
| // only for the case a per-attempt clock cannot see. | ||
| spec.BackoffLimit = ptr(opts.JobBackoffLimit) | ||
| if timeout > 0 { | ||
| spec.Template.Spec.ActiveDeadlineSeconds = ptr(deadlineSeconds(timeout)) |
There was a problem hiding this comment.
Confirmed finding (about a file not in this diff). This line moves the package-Job bound to the pod template, and the PR correspondingly rewrote the stageTimeout description in five places: nodewright_types.go, config/crd/bases/nodewright.nvidia.com_nodewrights.yaml, chart/templates/nodewright-crd.yaml, chart/values.yaml, and config/manager/manager.yaml.
operator/api/nodewright/v1alpha1/nodewright_webhook.go:339 was missed and still reads:
// stageTimeout maps to a Job activeDeadlineSeconds; 0 disables it, negatives are meaningless.Both halves are now false for package Jobs: it maps to the pod template's activeDeadlineSeconds, and 0 no longer disables the bound outright since the retry budget remains. No runtime impact, the >= 0 validation is unchanged and still correct. But leaving one copy behind reintroduces the two-sources-of-truth problem the rest of the sweep was closing.
| // shouldDeleteFinishedJob applies: a Complete one holds an unrecorded completion, and a | ||
| // Failed one has not yet had its chance to write erroring. Deleting the Failed case here | ||
| // would take its retained attempts with it and restart the stage on a fresh budget, and | ||
| // this path is reachable in exactly that window — a finished Job does not satisfy |
There was a problem hiding this comment.
Unadjudicated, raised late, single reporter - but the highest-severity thing surfaced, so worth a careful look.
Claim: a parked stage is never unparked by a package spec change, and the finite backoffLimit makes the park reachable in ~70s instead of after the 1h stage deadline, so "edit the NodeWright to fix a broken package" now usually does nothing until the 24h failure TTL.
The chain as reported:
- Spec-drift invalidation is scoped to unfinished Jobs.
ValidateRunningPackagesreturns early for any finished Job (2710-2719), sojobIsStale/jobMatchesPackage- the only code comparing a Job to the current package spec (env, image, resources) - never sees a terminal Job. - The two paths that do see it both keep it:
shouldDeleteFinishedJobreturns false when the entry sits at (stage,erroring), andhandleExistingJobabsorbs the recreate for the same pair just above this line. - Job names are deterministic with no config or spec hash, so the fixed spec collides with the parked name.
- The remaining unpark paths are the orphaned-node sweep, the reboot-reset sweep, a node-state change from rerun/reset,
deleteConfigUpdateExecutors- whichHandleConfigUpdatescalls only for Config/Interrupt/PostInterrupt, not Apply/Upgrade/Uninstall - and TTL expiry.
If that holds, docs/designs/2026-07-10-package-execution-as-jobs.md:165 promises something the code does not do: "The park clears on any explicit signal - package rerun/reset, a config update, a spec change, or failure-TTL expiry."
The carve-out predates this PR, but previously it needed a 1h JobSpec.activeDeadlineSeconds to fire, so in practice a spec fix always landed while the Job was still Active. At ~70s that is no longer true.
The reporter also claims k8s-tests/chainsaw/nodewright/uninstall-fix-config exercises exactly this shape (fixing EXIT_CODE 1 -> 0 at the same package version and asserting the uninstall completes) and becomes a race against the ~70s park. That test is currently passing in CI on this commit, which is evidence against the strong form of the claim - but a race that passes is still a race, so it is worth confirming which of the two readings is right.
| failed, reason := jobFailure(job) | ||
| return failed && reason == batchv1.JobReasonDeadlineExceeded | ||
| failed, _ := jobFailure(job) | ||
| return failed |
There was a problem hiding this comment.
Unadjudicated, single reporter. Naming nit with a trap in it: isParkedJob no longer checks anything about parking. It is now exactly jobFailure(job)'s first return value.
Two callers pair it with the entry check that actually makes a Job parked (handleExistingJob with entryErroringAtStage, shouldDeleteFinishedJob with the inline stage/state comparison). deleteConfigUpdateExecutors calls !jobFinished(job) || isParkedJob(job) with no entry check, where it can only mean "terminally failed" - including the all-kubelet-rejections case that jobFailureIsGenuine deliberately classifies as not parked.
Current behavior is correct and matches the design doc's "unfinished or terminally failed". The risk is a future caller trusting the name. Renaming to something like jobTerminallyFailed, and letting the two real park sites compose it with the entry check they already perform, removes the trap.
| // handleExistingJob resolves an AlreadyExists on create against the deterministic Job name. It | ||
| // never records in_progress (the create did not happen this pass): an unfinished Job that matches | ||
| // is a benign race won by another pass; a parked deadline failure whose entry sits erroring is | ||
| // is a benign race won by another pass; a parked failure whose entry sits erroring is |
There was a problem hiding this comment.
Unadjudicated, single reporter. This comment block was partially updated: line 2486 changed "a parked deadline failure" to "a parked failure", but the next clause still says "an unprocessed completion is left for JobReconcile; and a stale-spec unfinished Job or a processed finished Job is foreground-deleted".
The body now returns early for any unprocessed finished Job (if !jobProcessed(existing) { return nil }), Complete or Failed. Since the adjacent line in the same block was updated by this diff, this reads as a partial sweep rather than a deliberate carve-out - and the case it omits (Failed-but-unprocessed) is the one the new inline comment just below calls out as the risky one. No runtime effect.
… running Item 4 of #411, resolved as documentation. Editing stageTimeout, JOB_STAGE_TIMEOUT or JOB_BACKOFF_LIMIT changes what the next Job is built with and does not reach a Job already running. jobMatchesPackage compares only the pod-template subset podMatchesPackage looks at — labels and per-init-container name/image/env/resources — so the changed value is not staleness and nothing is replaced on the strength of it. Making it reach in-flight work is not a small fix, and Kubernetes does not offer a clean one. The per-attempt bound lives on the Job's pod template, which is immutable, so it cannot be patched — and a template edit would only reach pods created after it anyway. The running pod's own activeDeadlineSeconds is mutable but may only be DECREASED, which is backwards from the edit that motivates the change: people raise a timeout because a stage needs longer. Applying an increase means replacing the Job, which kills the in-flight attempt in order to give it more time. So the contract is stated rather than engineered around: the new value applies at the package's next stage, and to apply it to work already under way the user clears the Job — `kubectl nodewright package rerun`, or deleting it — and the stage restarts under the new value. Covers the CRD docstring (regenerated into both CRD copies), the chart's jobStageTimeout comment, and the design doc's stage-deadline section, which also records why the Job-level ceiling is left unpatched even though it alone is mutable: patching it would leave the ceiling and the per-attempt bound disagreeing. Refs #411 (item 4). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
The previous commit picked these up by accident. make manifests and make unit-tests both run addlicense, which strips the Apache header from the generated deepcopy files and drops one blank comment line from the header of every touched file under operator/config/. I reverted that churn before committing, then ran the test gate, which reintroduced it — and git add -A caught it the second time. No content change: only the headers, back to what is committed on the branch. The stageTimeout description added to nodewright.nvidia.com_nodewrights.yaml in the previous commit is untouched. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
…two unfounded claims CodeRabbit review on #421; all three findings were right. FailureTarget is a later threshold than podFailurePolicy, not the same one. podFailurePolicy and DisruptionTarget are beta-on from 1.26, but 1.29-1.30 report a deadline expiry directly as JobFailed — the delayed-terminal behaviour that raises FailureTarget first arrives in 1.31. Everything the operator hangs off that condition therefore breaks a full five minors earlier than the table claimed, and it is more than the log snapshot: failureTargetStale, the path that surfaces erroring for a Job wedged on an unreachable node, keys on it too. Split into its own row, removed from the 1.26 row, and the intro band split accordingly. The "race, not corruption" line was a guarantee I had no business making. Flag files make re-execution idempotent; they are not a lock and do not order two concurrent cp -r runs into the same copyDir, so a step script can read a file another attempt is mid-way through overwriting. Now described as a possible-corruption configuration. The 1.26 row cited a finite JOB_BACKOFF_LIMIT defaulting to 3. That knob does not exist on this branch — it arrives with #402, and here BackoffLimit is MaxInt32. Rephrased to hold in both regimes rather than forward-referencing an unmerged PR. Also de-duplicated the snapshot loss out of the 1.27 row: the table is cumulative, and by 1.27 it has already been lost at 1.31. Refs #411 (item 8). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Cross-link from #434 (#411 item 9): that PR adds Whichever of the two merges second should add the missing row — one line in the same table. Both PRs also touch |
pruneFailedAttempts selected archives on Failed-and-not-disrupted, while jobFailureIsGenuine applied the stricter podDeadlineExceeded || podFailureIsGenuine to whatever survived. A kubelet admission rejection satisfies the first and not the second, so a real step failure sandwiched between two rejections was prunable while both rejections survived. When the terminal classifier is the only writer that ran — an operator restart or leadership change spanning the Pod-watch window — it then read only rejections, called a genuinely failing stage non-genuine, and let the sweep clear it to run the whole thing again. snapshotFailureLogs had the same looseness from the other side: it returned early on any Failed non-disrupted pod, on the reasoning that a genuine archive already holds the logs. A rejected attempt has no container statuses and no logs, so an earlier rejection suppressed the snapshot and left the timed-out stage with no evidence at all. Both now call podFailedGenuinely, alongside the classifier. Non-genuine Failed pods are consequently never pruned either; they are bounded by backoffLimit+1 and go with the Job at its TTL. Also records why podReasonDeadlineExceeded is declared locally: the SDK has no constant for the pod-level reason, and batchv1's same-valued JobReasonDeadlineExceeded is the Job controller's condition reason on a different object. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Spec-drift invalidation was scoped to unfinished Jobs, so a stage that had spent its retry budget sat behind its terminal Job until the failure TTL — up to 24h in which editing the package to fix exactly what broke it did nothing. With a finite backoffLimit that state is now reachable in about 70 seconds, so the window is no longer theoretical. The carve-out that keeps a timed-out Job in place now also requires the Job to still match the package it was built from, in both places that apply it: the sweep's rerun predicate and the AlreadyExists-on-create path. An edited package therefore deletes the Job and the stage re-runs from the entry that is already sitting at (stage, erroring) — the same position any other erroring package is in. The spec comparison used by the unfinished-Job staleness check is extracted so both arms ask the identical question. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Two clocks over the same work can only disagree. Derived from the retry budget the ceiling said nothing the budget did not; set independently it could sit below the budget and truncate it into a DeadlineExceeded that reads as a hang. Package Jobs now carry exactly two bounds: the per-attempt deadline on the pod template, and backoffLimit. The one case the ceiling covered goes with it: the per-attempt clock runs from pod.Status.StartTime, so a pod the kubelet never acknowledges never starts it, never fails, and never spends an attempt. That is now unbounded and documented as node health rather than stage health — no field the operator can set bounds a pod the kubelet has not accepted, and activeDeadlineSeconds on the pod is exactly the clock that never starts. Interrupt Jobs are unchanged: their whole-stage deadline has to span a reboot, which a per-attempt clock cannot. deadlineSeconds now clamps to int32. The apiserver rejects anything larger, so a stageTimeout past ~68 years turned every Create into an error loop instead of a value the user could see was wrong. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
The field said "0 removes the time bound, leaving the retry budget as its only limit", which reads as though something still bounds the stage. A retry budget is only spent by attempts that FAIL, so with 0 an attempt that hangs hangs forever, and a pod the kubelet never acknowledges is unbounded at any value. Also fixes the webhook comment missed when the bound moved to the pod template: it still claimed stageTimeout maps to a Job activeDeadlineSeconds and that 0 disables it, both now false for package Jobs. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
"Parked" reads as something a human did, like paused, when it is the operator giving up on a stage that spent its deadline or its retry budget. Renamed through the design doc, the status definitions, the chainsaw fixtures and the code comments. isParkedJob becomes jobFailedTerminally rather than isTimedOutJob: it checks a terminal Failed condition and nothing else, so naming it for the conclusion its callers draw was the trap the review flagged. A timed-out stage is that plus the entry sitting at (stage, erroring), which is what the two callers that mean it actually test; deleteConfigUpdateExecutors means only "terminally failed" and now reads correctly. The design doc also drops the claim that the pruner can lose a real failure to the classifier, which is no longer true. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
The epic has been landing user-visible changes for several PRs — Jobs as the execution unit, retained failure logs, stageTimeout, the pause cascade, four new env knobs — with nothing in either RELEASE_NOTES. The behavior change that most needs saying: a crash-looping package now gives up after roughly 70 seconds where it used to retry for up to an hour, so anything that self-healed through a registry blip or a slow mount needs JOB_BACKOFF_LIMIT raised. The TTL floor of 1m is called out too, since "0" reads as "no retention" and is actually a startup crash. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
All eleven items from the cross-review are addressed in Confirmed1 — pruner vs classifier ( 2 — no release note. Written in Unadjudicated3 — snapshot suppressed by a prior rejection. Confirmed and fixed in the same commit: same predicate, so an admission rejection no longer counts as "the archive already holds the logs". The old spec for this asserted nothing (no stuck pod was present, so nothing would have been snapshotted either way); it now has one, plus a new spec for the rejection case. 4 — a timed-out stage is never unparked by a spec change. Fixed in 5 — 6 — partially-updated comment block ( Inline7 — "parked" → timed out. Done in 8 — remove the Job-level ceiling. Done in 9 — SDK constant for 10 — 11 — stale webhook comment. Fixed in the same commit. Also from the open questions
Left as they stand: interrupt gating and DeploymentPolicy thresholds tripping sooner are the intended consequence of a bounded budget, and the upstream suspend-deletion claim is still unverified from this tree. Verification: controller suite 302/302, webhook 43/43 and 130/130, wrappers 84/84, |
Removing the ceiling left this fixture asserting activeDeadlineSeconds: 16800 on both Jobs, so e2e/core failed on every k8s version. The per-attempt assertion on the pod template stays — that is the bound that still exists. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Correction to the verification note above: e2e/core failed on all four k8s versions after that push, and it was my miss, not a flake. Fixed in Worth naming the gap: the unit suites and lint were green on the previous push precisely because no unit test can see a chainsaw fixture. For a change that removes a field from a generated object, the e2e fixtures are part of the blast radius and I should have grepped them before claiming verification. |
3 participants
Closes #373. Part of #223, based on
feature/package-as-jobs(notmain).#316 set the stage deadline on
JobSpec.ActiveDeadlineSeconds, which is terminal: exceeding it fails the Job permanently with no replacement pod, so the first expiry parked. That made a timeout the one failure class with no retry. This moves the bound to the pod template so an expired attempt isFailedand replaced like any other failure, and gives up on a finitebackoffLimitinstead ofmath.MaxInt32. No operator-side retry counter to persist.Three bounds instead of one
Package Jobs:
spec.template.spec.activeDeadlineSecondsstageTimeout— per attemptspec.backoffLimitJOB_BACKOFF_LIMIT(new, default 3)spec.activeDeadlineSecondsThe ceiling is
(backoffLimit+1) × (stageTimeout + gracefulShutdown + 10m), clamped toMaxInt32. It exists only for the case a per-attempt clock cannot bound: that clock runs frompod.Status.StartTime, which a pod the kubelet never acknowledges never gets.gracefulShutdownis in the formula becausepodReplacementPolicy: Failedwaits out every shutdown — without it a slow shutdown could truncate the retry budget into aDeadlineExceededthat reads as a hang. Deriving it rather than adding a fourth knob makes a ceiling below the retry budget unrepresentable.stageTimeout: 0now removes the time bound but keeps the retry budget. That semantic shift is called out in the docstring.Interrupt Jobs are deliberately unchanged (constraint 2). Under
OnFailurebackoffLimitcounts container restarts, so a finite budget would be spent by the in-place restart that is the reboot recovery, and the bound must span the reboot, which a per-attempt clock cannot.Two things not in the issue — please look here first
The park signal inverts as #373 anticipated, but a bare
BackoffLimitExceededturned out not to be safe to park on, andpod_controller.gocould not stay untouched.1.
BackoffLimitExceededalone does not park. These pods carryspec.nodeNamerather than going through the scheduler, so kubelet admission is the only gate they face; a node at capacity or returning from a reboot can reject several node-pinned replacements in a row, eachFailedwith no container statuses and noDisruptionTargetfor theIgnorerule to match. NopodFailurePolicyrule can absorb them —onExitCodesneeds container statuses there are none of, the same argument that keeps ImagePullBackOff out of the policy. The design doc already documents this at line 269; atMaxInt32it cost an archive slot, at 3 it exhausts the budget in ~70s and would park a package that never ran a line of script. Terminal failure is now believed only when a retained archive really failed (nonzero exit, or killed by its own deadline). The Job-level ceiling needs no such evidence — nothing finished inside the entire budget.Accepted gap: the pruner keeps only first+last, so a genuine failure sandwiched between rejections can be pruned before the check runs. That errs toward re-running rather than parking, which is the safe direction.
2.
pod_controller.gocould not stay untouched. Constraint 4 assumed "a deadline-killed container exits nonzero," which only holds if a container was running. For the headline case — unpullable image, dead registry — the stuck init container isWaiting{ImagePullBackOff}when the pod deadline fires, or gets rewritten toTerminated{ContainerStatusUnknown}by the kubelet on termination.podFailureIsGenuinerejects both. Left alone, a hang would have readin_progressfor4 × stageTimeout(4h at defaults) — worse than the 1h #316 gives today, and a regression against the exact case #316 targeted. The Pod watch now also keys on the pod-levelDeadlineExceededreason, which nothing else sets.So on constraint 4 the answer is "accept it": first genuine failure writes
erroring, a successful retry recordscompleteover it. Nothing writesin_progressback —ApplyPackagebails on the unfinished Job before the upsert — so the sequence isin_progress → erroring → complete, not a flap.Also included
shouldDeleteFinishedJobnow requires thestate-recordedmarker forFailedJobs, not justComplete. A finitebackoffLimittakes a Job from first failure to terminal in about a minute, so the heavy pass would otherwise race the erroring write into a fresh, equally doomed attempt. The window this closes existed before, but at ~1h wide it was theoretical.Behavior change worth a changelog line
backoffLimitbounds every failure class, not just timeouts. A crash-looping package parks after 4 attempts (~70s, paced by the Job controller's 10s/20s/40s backoff) where it previously retried for the wholestageTimeout(~1h, ~10 attempts). Packages that ride out transient environment flakiness — a registry blip, a mount not up yet — lose that hour of self-healing. That is why the limit is an operator knob rather than a constant; the default stays 3 per the issue.Verify
backoffLimit" question from the issue was confirmed empirically by @lockwobr before this work; nothing here depends on re-checking it, butpause-suspends-jobsremains the e2e that would catch a regression.spec.suspendresetsstatus.startTimebut notstatus.failed, so pausing to investigate a half-failed stage and resuming leaves the remaining budget unchanged. Noted in the design doc's pause section.Testing
make unit-tests— 14 suites pass; 21 new/changed specs, including the ceiling arithmetic and its overflow clamp, the interrupt asymmetry,BackoffLimitExceededparking only on genuine evidence (4-entry table), and pod-deadline erroring for a container that never started.make lint(0 issues),make license-check,helm lint/helm templateall clean.simple-nodewright/assert_jobs.yamlpins the new three-bound shape;failure-nodewrightgainsassert_parked_job.yamlpinning the park itself — terminalBackoffLimitExceeded,failed: 4,state-recorded, and the 24h failure TTL.Docs
stageTimeoutdocstring + regenerated CRD + chart mirror; the design doc's Job shape, retry, stage-deadline, pause, finished-Job-rules and rejected-alternatives sections, plus the staleoperator/api/v1alpha1/skyhook_types.gopath in its file list (the field lives underapi/nodewright/); and theerroringrow inoperator-status-definitions.md, which now also means "gave up and will not retry without intervention".JOB_BACKOFF_LIMITis wired throughoperator/config/manager/manager.yaml,chart/templates/deployment.yamlandchart/values.yaml, per the scope note carried over from #372.Also closes #424 (#411 item 3 —
handleExistingJob/shouldDeleteFinishedJobnow exempt any unprocessed finished Job,7f5ef8aa) and #425 (#411 item 4 — thestageTimeoutcontract is documented rather than made live,4c11b177).