You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The Jobs migration (#223) merged to main in #459 on 2026-08-15. The feature/package-as-jobs branch has been deleted — base this work on main,
which now executes packages as Jobs.
A multi-reviewer cross-review of feature/package-as-jobs at a3dd68c8 (against main at a437d9bd, 59 files, +6106/-1800) produced 14 findings that reached reviewer consensus and survived adversarial verification, plus 6 that stayed contested. This issue tracks the ones worth acting on before the feature branch merges to main.
Nothing was built, tested, or executed during the review, so every item below is from static reading of the pinned diff plus the surrounding tree. Items are grouped by the decision they need, not by severity, because several collapse into one call.
Some of this overlaps existing sub-issues and is called out inline: #382 (JobReconciler workqueue), #373 (stage timeout as retryable failure), #306 (ImagePullBackOff as erroring), #305 (release notes and upgrade verification).
1. Node-state lost update between the heavy pass and JobReconcile
SaveNodesAndSkyhook (skyhook_controller.go:1263) patches the Node with client.StrategicMergeFrom and no precondition, while patchNodeState (job_controller.go:266) uses MergeFromWithOptimisticLock plus RetryOnConflict.
Before this change, Pod events rode the heavy reconciler's own queue via the pod--- prefix, and that controller runs MaxConcurrentReconciles: 1, so pod-driven node-state writes were serialized against the whole-world pass. NewJobReconciler and NewPodReconciler now register independent controllers with their own workqueues (cmd/manager/main.go:215,219), so the two writers are genuinely concurrent.
nodewright.nvidia.com/nodeState_<name> is a single JSON string holding every package on the node. A heavy-pass write for package B rewrites the whole value from a snapshot taken before a concurrent JobReconcile recorded package A complete, reverting A to in_progress. The Job is already annotated state-recorded, so JobReconcile will not re-record it; recovery only comes from the rerun predicate deleting the retained Job and re-executing the whole stage on that node.
Optimistic locking on one side of a two-writer race does not close it.
Relationship to #382:#382 proposes keying the JobReconciler workqueue by node, which gives per-node serialization within JobReconciler. It does not address this, because the heavy pass is a separate controller with its own queue keyed by the NodeWright CR. Both writers still land on the same Node annotation.
The heavy pass's unlocked patches are deliberate and documented (skyhook_controller.go:655-661,1104-1108) as a tradeoff against a conflict storm, and #382 records the measured cost of the earlier attempt (0 to 156 conflicts, since reverted). Those comments predate the Job and Pod controllers becoming concurrent, so the tradeoff is worth re-examining in that light rather than simply re-applying the lock.
2. Legacy raw-pod upgrade path: implement it or correct the docs
This is one decision, not four bugs. The design doc added in this same change (docs/designs/2026-07-10-package-execution-as-jobs.md:224-232) specifies four accommodations for packages still running as raw pods across an upgrade. None are implemented.
ValidateRunningPackages (skyhook_controller.go:2639) has a doc comment claiming "A legacy raw-pod sweep runs alongside during the one-minor migration window". The body walks Jobs only.
PodReconcile (pod_controller.go:104-123) never records completion and never deletes a pod, so an in-flight pre-upgrade package pod is never finished or reaped.
JobExists (skyhook_controller.go:2450) counts Jobs only, so it does not OR-in a legacy raw pod. The first post-upgrade reconcile can start a second executor for a stage already running as a pre-upgrade pod. Both share the same hostPath copy dir, since copyDir is keyed identically for the pod and the Job.
HasRunningPackages (skyhook_controller.go:2118) counts unfinished Jobs only, so interrupt gating no longer waits on legacy raw pods. The controller can cordon, drain, and reboot a node underneath active legacy package work.
Scope, verified during review.reconcileLegacyLabeledWorkloads (skyhook_controller.go:783) does sweep legacy package pods, but selects only on skyhook.nvidia.com/name. At main, skyhook_controller.go imports api/nodewright/v1alpha1, where METADATA_PREFIX is nodewright.nvidia.com, so pods created by the current main tip are not covered by that sweep. The latest release (operator/v0.17.0) still uses skyhook.nvidia.com, so pods from a released operator are covered.
So the gap becomes a live upgrade path only if the rename ships a release before this branch merges. That is the question to settle first.
The rollout does not wedge.RunNext gates on complete, so a package left at in_progress is re-driven every pass and ApplyPackage falls through to createJobFromPackage. The hazard is the concurrent double-execution above, not a permanent stall. An earlier framing of this as a stall was checked and refuted during the review.
If the shipped design is the migration hold (migration_hold.go) plus the label sweep, rather than the design doc's four in-place accommodations, then the defect is the documentation: the design doc's Upgrade section and the ValidateRunningPackages comment both need correcting, and #305 needs an explicit "drain all package pods before upgrading" instruction in the release notes.
3. Unprocessed failed Jobs are deleted before JobReconciler records them
handleExistingJob (skyhook_controller.go:2498) and shouldDeleteFinishedJob (skyhook_controller.go:2723) both guard with hasJobCondition(job, batchv1.JobComplete) && !jobProcessed(job), so they exempt only unprocessed Complete Jobs.
jobProcessed is the state-recorded annotation, and isParkedJob covers only JobReasonDeadlineExceeded. A Job that failed via BackoffLimitExceeded, before JobReconciler has annotated it, therefore matches neither carve-out and falls through to deleteJobForeground. The heavy pass erases it before JobReconcile snapshots the log tail and flips node state to erroring, so the failure handoff is lost and the stage can recreate as if nothing happened.
This defeats the retained-failure-logs behavior that is one of the headline reasons for the Jobs migration.
shouldDeleteFinishedJob's own doc comment states the intended contract as "a processed finished Job (Failed, or Complete and state-recorded)", so the guard appears simply to have been written narrower than intended. Extending both conditions from hasJobCondition(JobComplete) && !jobProcessed to !jobProcessed looks like the intended shape, but the parked-deadline carve-out below it needs to stay reachable.
4. stageTimeout changes do not reach in-flight Jobs
jobMatchesPackage (job_builder.go:163-169) compares only the pod-template subset via podMatchesPackage. ActiveDeadlineSeconds is set separately at job_builder.go:167-168 from effectiveStageTimeout(opts, _package) and is never compared.
Editing package.stageTimeout, or the operator-wide JOB_STAGE_TIMEOUT default, therefore leaves an existing Job looking current, so validation will not replace a Job that now carries the wrong deadline. The new CRD knob has no effect on in-flight work until the Job finishes or is deleted by hand.
Related to #373. This was the one item the reviewers genuinely split on, and it was independently reported by a third reviewer on a re-run, so it is recorded here as confirmed rather than contested.
5. Needs triage: reviewer-flagged, not cross-confirmed
These were raised late in the review and were never cross-evaluated, so each carries one reviewer's position only. They read as plausible but should be confirmed against the code before any change.
Pause-then-disable un-suspends Jobs (skyhook_controller.go:463). resumeSuspendedJobs appears to run for a disabled NodeWright, so removing the pause annotation while adding disable would un-suspend previously stopped stage Jobs on the next reconcile. That would make disable strictly weaker than pause for in-flight work, which is the opposite of how docs/cli.md:513-519 presents it.
Interrupt completion can resurrect a removed node-state entry (job_controller.go:223). shouldRecordCompletion's interrupt branch returns true before the presence check, so a completed-but-unprocessed interrupt Job could re-pin a package that package rerun or a finalizer-driven uninstall had removed. Because the entry then records the stage as done, shouldDeleteFinishedJob keeps the Job and the stage never re-runs.
Interrupt Jobs carry the package image on the exit-0 "done" container (job_builder.go:107), where nothing else pulls that image. Init containers run first, so the interrupt has already executed; if kubelet image GC evicted the image or the registry is unreachable post-reboot, the pod parks in ImagePullBackOff and the package sits at (interrupt, in_progress) until activeDeadlineSeconds flips it to erroring, with the host already interrupted. Related to Surface ImagePullBackOff/ErrImagePull on package pods as state: erroring #306.
6. Restart accounting goes silent
Package stage Jobs use restartPolicy: Never (job_builder.go:151), so child-pod RestartCount is always 0. PodReconciler is the only writer during the retry window and passes that value through (pod_controller.go:117 to :150), and cluster_state_v2.go:1502 sums it into skyhook_package_restarts_count.
docs/metrics/README.md:31 still documents that metric as "Number of restarts for this package on this node", and docs/metrics/dashboards/skyhook-dashboard.json:2898 graphs it. For a genuinely crash-looping package the counter now stays 0 for up to JOB_STAGE_TIMEOUT (1h default), so any dashboard or alert on it goes quiet exactly when a package is failing. The restarts-greater-than-zero assertion was deleted from k8s-tests/chainsaw/nodewright/failure-nodewright/node-assert.yaml in this change.
Either re-derive the field from job.Status.Failed during the retry window, or retire the metric and update the docs and dashboard.
7. tailAndSanitize can return more than maxBytes
tailAndSanitize (dal/dal.go:241-265) enforces the byte cap inside the read loop, then applies strings.ToValidUTF8(string(tail), "�") afterwards.
U+FFFD is three bytes and replaces each run of invalid bytes, so input that alternates valid and invalid single bytes inflates the result well past the cap. The doc comment promises the result "holds at most maxBytes plus one chunk in memory", and callers rely on the returned string fitting the annotation budget, which is the constraint that actually matters here since the log tail is written into node state.
Re-apply the cap after the repair, trimming back to a rune boundary so the truncation cannot itself reintroduce a partial rune.
8. Kubernetes version support
docs/kubernetes-support.md:7 states the operator "relies only on core, long-stable Kubernetes APIs" and "gates on no version-specific features", plausibly back to around 1.23, and the table at line 13 marks roughly 1.23 to 1.32 accordingly.
The Jobs path depends on podFailurePolicy, podReplacementPolicy, and .status.childPods. On an older apiserver these fields are dropped silently rather than rejected, so the disruption-ignore rule stops applying (evictions count toward backoff and mark packages erroring), the no-overlap guarantee on the shared hostPath is lost, and an empty childPods list makes pruning, the deadline log snapshot, and the succeeded-container-name lookup no-op.
The exact per-minor availability was not verified against upstream docs during the review; what is verified is that the code uses these fields at all. Please confirm the floor before editing the table.
9. User-facing surfaces not documented
The new Package.stageTimeout CRD field and the jobTtlSucceeded / jobTtlFailed / jobStageTimeout env knobs are documented nowhere outside the design doc.
jobTtlSucceeded and jobTtlFailed have an undocumented hard minimum: SkyhookOperatorOptions.Validate (skyhook_controller.go:203-211) rejects anything under 1 minute, so setting "0" to disable retention crashes the operator at startup with no hint in chart/values.yaml.
The three new values are missing from the values table in chart/README.md. (legacyCleanupDelay is also absent, so there is precedent, but these three are permanent knobs rather than a migration shim.)
docs/cli.md: pause now has operator-version-dependent stop semantics, but the compatibility matrix at line 46 was not updated. Only free prose at line 522 mentions it. Repo convention makes that matrix authoritative for version-gated behavior.
docs/operator_resources_at_scale.md: the design doc names this page as authoritative for sizing the new retained-Job population and the added namespace-scoped Jobs informer, but it was not updated and does not mention pods, Jobs, or retention.
10. Stale references
docs/uninstall.md:357 attributes a failure mode to SkyhookReconciler.UpdateNodeState, a method removed by this change.
operator/api/v1alpha1/legacy_readonly_webhook.go:37 documents a guard that depends on scripts/gen_nodewright.sh, deleted here. No build or CI break: nothing referenced the script or a verify-nodewright-gen target at the parent commit either.
The design doc's reference list (docs/designs/2026-07-10-package-execution-as-jobs.md:283) points at the legacy skyhook_types.go for Package.stageTimeout and its webhook validation. Both live in the nodewright group.
The design doc's Job label set and its three copy-pasteable kubectl get jobs -l ... examples (around line 215) use the skyhook.nvidia.com/* prefix; the implementation stamps nodewright.nvidia.com/*, so every example command selects nothing.
cmd/manager/main.go:131: the Cache-options comment states that nothing on the main manager reads a Secret. SecretCertWatcher is registered on that manager and does. A future reader trusting the comment could delete the scoped entry and break webhook cert sync with a runtime 403 rather than a compile error.
Open questions for the epic
Was the legacy raw-pod migration window deliberately dropped before the squash, or is section 2 an oversight? This determines whether section 2 is a code change or a docs change.
pruneFailedAttempts is invoked only from handleActiveJob, never from the terminal handlers, so archives beyond two can survive on a Job that goes terminal before the pruner sees the last attempt. Intentional?
Job names use generateSafeName(63, ...) with the same inputs as the old pod names, so a raw pod and its Job can share a name across an upgrade. Child-pod names run to roughly 69 characters. Was that checked against any consumer assuming 63?
Removing the // Code generated by gen_nodewright; DO NOT EDIT. markers means golangci-lint now applies dupl, gocyclo, goconst, unparam, and unused to api/nodewright/v1alpha1/*.go for the first time. Unverified, since the review did not run lint.
k8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yaml pins activeDeadlineSeconds: 3600, assuming the CI operator uses the default JOB_STAGE_TIMEOUT=1h. Both install paths default to 1h, but which path the e2e pools use was not confirmed.
Not covered by the review
No build, test, generator, or make target was run. Compile-level drift in the regenerated mocks, the reworked fixtures against the new NewSkyhookReconciler and dal.New signatures, and whether the new chainsaw suites pass on a real cluster are all unverified. Separately, the Job and Pod reconcilers are registered only in cmd/manager/main.go and not in suite_test.go, so no envtest spec exercises the JobReconciler and SkyhookReconciler handoff end to end; the new coverage is fake-client unit tests plus chainsaw.
Note
The Jobs migration (#223) merged to
mainin #459 on 2026-08-15. Thefeature/package-as-jobsbranch has been deleted — base this work onmain,which now executes packages as Jobs.
Part of #223.
Summary
A multi-reviewer cross-review of
feature/package-as-jobsata3dd68c8(againstmainata437d9bd, 59 files, +6106/-1800) produced 14 findings that reached reviewer consensus and survived adversarial verification, plus 6 that stayed contested. This issue tracks the ones worth acting on before the feature branch merges tomain.Nothing was built, tested, or executed during the review, so every item below is from static reading of the pinned diff plus the surrounding tree. Items are grouped by the decision they need, not by severity, because several collapse into one call.
Some of this overlaps existing sub-issues and is called out inline: #382 (JobReconciler workqueue), #373 (stage timeout as retryable failure), #306 (ImagePullBackOff as erroring), #305 (release notes and upgrade verification).
1. Node-state lost update between the heavy pass and JobReconcile
SaveNodesAndSkyhook(skyhook_controller.go:1263) patches the Node withclient.StrategicMergeFromand no precondition, whilepatchNodeState(job_controller.go:266) usesMergeFromWithOptimisticLockplusRetryOnConflict.Before this change, Pod events rode the heavy reconciler's own queue via the
pod---prefix, and that controller runsMaxConcurrentReconciles: 1, so pod-driven node-state writes were serialized against the whole-world pass.NewJobReconcilerandNewPodReconcilernow register independent controllers with their own workqueues (cmd/manager/main.go:215,219), so the two writers are genuinely concurrent.nodewright.nvidia.com/nodeState_<name>is a single JSON string holding every package on the node. A heavy-pass write for package B rewrites the whole value from a snapshot taken before a concurrent JobReconcile recorded package A complete, reverting A toin_progress. The Job is already annotated state-recorded, so JobReconcile will not re-record it; recovery only comes from the rerun predicate deleting the retained Job and re-executing the whole stage on that node.Optimistic locking on one side of a two-writer race does not close it.
Relationship to #382: #382 proposes keying the JobReconciler workqueue by node, which gives per-node serialization within JobReconciler. It does not address this, because the heavy pass is a separate controller with its own queue keyed by the NodeWright CR. Both writers still land on the same Node annotation.
The heavy pass's unlocked patches are deliberate and documented (
skyhook_controller.go:655-661,1104-1108) as a tradeoff against a conflict storm, and #382 records the measured cost of the earlier attempt (0 to 156 conflicts, since reverted). Those comments predate the Job and Pod controllers becoming concurrent, so the tradeoff is worth re-examining in that light rather than simply re-applying the lock.2. Legacy raw-pod upgrade path: implement it or correct the docs
This is one decision, not four bugs. The design doc added in this same change (
docs/designs/2026-07-10-package-execution-as-jobs.md:224-232) specifies four accommodations for packages still running as raw pods across an upgrade. None are implemented.ValidateRunningPackages(skyhook_controller.go:2639) has a doc comment claiming "A legacy raw-pod sweep runs alongside during the one-minor migration window". The body walks Jobs only.PodReconcile(pod_controller.go:104-123) never records completion and never deletes a pod, so an in-flight pre-upgrade package pod is never finished or reaped.JobExists(skyhook_controller.go:2450) counts Jobs only, so it does not OR-in a legacy raw pod. The first post-upgrade reconcile can start a second executor for a stage already running as a pre-upgrade pod. Both share the same hostPath copy dir, sincecopyDiris keyed identically for the pod and the Job.HasRunningPackages(skyhook_controller.go:2118) counts unfinished Jobs only, so interrupt gating no longer waits on legacy raw pods. The controller can cordon, drain, and reboot a node underneath active legacy package work.Scope, verified during review.
reconcileLegacyLabeledWorkloads(skyhook_controller.go:783) does sweep legacy package pods, but selects only onskyhook.nvidia.com/name. Atmain,skyhook_controller.goimportsapi/nodewright/v1alpha1, whereMETADATA_PREFIXisnodewright.nvidia.com, so pods created by the currentmaintip are not covered by that sweep. The latest release (operator/v0.17.0) still usesskyhook.nvidia.com, so pods from a released operator are covered.So the gap becomes a live upgrade path only if the rename ships a release before this branch merges. That is the question to settle first.
The rollout does not wedge.
RunNextgates oncomplete, so a package left atin_progressis re-driven every pass andApplyPackagefalls through tocreateJobFromPackage. The hazard is the concurrent double-execution above, not a permanent stall. An earlier framing of this as a stall was checked and refuted during the review.If the shipped design is the migration hold (
migration_hold.go) plus the label sweep, rather than the design doc's four in-place accommodations, then the defect is the documentation: the design doc's Upgrade section and theValidateRunningPackagescomment both need correcting, and #305 needs an explicit "drain all package pods before upgrading" instruction in the release notes.3. Unprocessed failed Jobs are deleted before JobReconciler records them
handleExistingJob(skyhook_controller.go:2498) andshouldDeleteFinishedJob(skyhook_controller.go:2723) both guard withhasJobCondition(job, batchv1.JobComplete) && !jobProcessed(job), so they exempt only unprocessed Complete Jobs.jobProcessedis thestate-recordedannotation, andisParkedJobcovers onlyJobReasonDeadlineExceeded. A Job that failed viaBackoffLimitExceeded, before JobReconciler has annotated it, therefore matches neither carve-out and falls through todeleteJobForeground. The heavy pass erases it before JobReconcile snapshots the log tail and flips node state to erroring, so the failure handoff is lost and the stage can recreate as if nothing happened.This defeats the retained-failure-logs behavior that is one of the headline reasons for the Jobs migration.
shouldDeleteFinishedJob's own doc comment states the intended contract as "a processed finished Job (Failed, or Complete and state-recorded)", so the guard appears simply to have been written narrower than intended. Extending both conditions fromhasJobCondition(JobComplete) && !jobProcessedto!jobProcessedlooks like the intended shape, but the parked-deadline carve-out below it needs to stay reachable.4.
stageTimeoutchanges do not reach in-flight JobsjobMatchesPackage(job_builder.go:163-169) compares only the pod-template subset viapodMatchesPackage.ActiveDeadlineSecondsis set separately atjob_builder.go:167-168fromeffectiveStageTimeout(opts, _package)and is never compared.Editing
package.stageTimeout, or the operator-wideJOB_STAGE_TIMEOUTdefault, therefore leaves an existing Job looking current, so validation will not replace a Job that now carries the wrong deadline. The new CRD knob has no effect on in-flight work until the Job finishes or is deleted by hand.Related to #373. This was the one item the reviewers genuinely split on, and it was independently reported by a third reviewer on a re-run, so it is recorded here as confirmed rather than contested.
5. Needs triage: reviewer-flagged, not cross-confirmed
These were raised late in the review and were never cross-evaluated, so each carries one reviewer's position only. They read as plausible but should be confirmed against the code before any change.
skyhook_controller.go:463).resumeSuspendedJobsappears to run for a disabled NodeWright, so removing the pause annotation while adding disable would un-suspend previously stopped stage Jobs on the next reconcile. That would make disable strictly weaker than pause for in-flight work, which is the opposite of howdocs/cli.md:513-519presents it.job_controller.go:223).shouldRecordCompletion's interrupt branch returns true before the presence check, so a completed-but-unprocessed interrupt Job could re-pin a package thatpackage rerunor a finalizer-driven uninstall had removed. Because the entry then records the stage as done,shouldDeleteFinishedJobkeeps the Job and the stage never re-runs.job_builder.go:107), where nothing else pulls that image. Init containers run first, so the interrupt has already executed; if kubelet image GC evicted the image or the registry is unreachable post-reboot, the pod parks in ImagePullBackOff and the package sits at(interrupt, in_progress)untilactiveDeadlineSecondsflips it to erroring, with the host already interrupted. Related to Surface ImagePullBackOff/ErrImagePull on package pods as state: erroring #306.6. Restart accounting goes silent
restartPolicy: Never(job_builder.go:151), so child-podRestartCountis always 0.PodReconcileris the only writer during the retry window and passes that value through (pod_controller.go:117to:150), andcluster_state_v2.go:1502sums it intoskyhook_package_restarts_count.docs/metrics/README.md:31still documents that metric as "Number of restarts for this package on this node", anddocs/metrics/dashboards/skyhook-dashboard.json:2898graphs it. For a genuinely crash-looping package the counter now stays 0 for up toJOB_STAGE_TIMEOUT(1h default), so any dashboard or alert on it goes quiet exactly when a package is failing. The restarts-greater-than-zero assertion was deleted fromk8s-tests/chainsaw/nodewright/failure-nodewright/node-assert.yamlin this change.Either re-derive the field from
job.Status.Failedduring the retry window, or retire the metric and update the docs and dashboard.7.
tailAndSanitizecan return more thanmaxBytestailAndSanitize(dal/dal.go:241-265) enforces the byte cap inside the read loop, then appliesstrings.ToValidUTF8(string(tail), "�")afterwards.U+FFFD is three bytes and replaces each run of invalid bytes, so input that alternates valid and invalid single bytes inflates the result well past the cap. The doc comment promises the result "holds at most maxBytes plus one chunk in memory", and callers rely on the returned string fitting the annotation budget, which is the constraint that actually matters here since the log tail is written into node state.
Re-apply the cap after the repair, trimming back to a rune boundary so the truncation cannot itself reintroduce a partial rune.
8. Kubernetes version support
docs/kubernetes-support.md:7states the operator "relies only on core, long-stable Kubernetes APIs" and "gates on no version-specific features", plausibly back to around 1.23, and the table at line 13 marks roughly 1.23 to 1.32 accordingly.The Jobs path depends on
podFailurePolicy,podReplacementPolicy, and.status.childPods. On an older apiserver these fields are dropped silently rather than rejected, so the disruption-ignore rule stops applying (evictions count toward backoff and mark packages erroring), the no-overlap guarantee on the shared hostPath is lost, and an emptychildPodslist makes pruning, the deadline log snapshot, and the succeeded-container-name lookup no-op.The exact per-minor availability was not verified against upstream docs during the review; what is verified is that the code uses these fields at all. Please confirm the floor before editing the table.
9. User-facing surfaces not documented
Package.stageTimeoutCRD field and thejobTtlSucceeded/jobTtlFailed/jobStageTimeoutenv knobs are documented nowhere outside the design doc.jobTtlSucceededandjobTtlFailedhave an undocumented hard minimum:SkyhookOperatorOptions.Validate(skyhook_controller.go:203-211) rejects anything under 1 minute, so setting"0"to disable retention crashes the operator at startup with no hint inchart/values.yaml.chart/README.md. (legacyCleanupDelayis also absent, so there is precedent, but these three are permanent knobs rather than a migration shim.)docs/cli.md: pause now has operator-version-dependent stop semantics, but the compatibility matrix at line 46 was not updated. Only free prose at line 522 mentions it. Repo convention makes that matrix authoritative for version-gated behavior.docs/operator_resources_at_scale.md: the design doc names this page as authoritative for sizing the new retained-Job population and the added namespace-scoped Jobs informer, but it was not updated and does not mention pods, Jobs, or retention.10. Stale references
docs/uninstall.md:357attributes a failure mode toSkyhookReconciler.UpdateNodeState, a method removed by this change.operator/api/v1alpha1/legacy_readonly_webhook.go:37documents a guard that depends onscripts/gen_nodewright.sh, deleted here. No build or CI break: nothing referenced the script or averify-nodewright-gentarget at the parent commit either.docs/designs/2026-07-10-package-execution-as-jobs.md:283) points at the legacyskyhook_types.goforPackage.stageTimeoutand its webhook validation. Both live in the nodewright group.kubectl get jobs -l ...examples (around line 215) use theskyhook.nvidia.com/*prefix; the implementation stampsnodewright.nvidia.com/*, so every example command selects nothing.cmd/manager/main.go:131: the Cache-options comment states that nothing on the main manager reads a Secret.SecretCertWatcheris registered on that manager and does. A future reader trusting the comment could delete the scoped entry and break webhook cert sync with a runtime 403 rather than a compile error.Open questions for the epic
pruneFailedAttemptsis invoked only fromhandleActiveJob, never from the terminal handlers, so archives beyond two can survive on a Job that goes terminal before the pruner sees the last attempt. Intentional?generateSafeName(63, ...)with the same inputs as the old pod names, so a raw pod and its Job can share a name across an upgrade. Child-pod names run to roughly 69 characters. Was that checked against any consumer assuming 63?// Code generated by gen_nodewright; DO NOT EDIT.markers means golangci-lint now appliesdupl,gocyclo,goconst,unparam, andunusedtoapi/nodewright/v1alpha1/*.gofor the first time. Unverified, since the review did not run lint.k8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yamlpinsactiveDeadlineSeconds: 3600, assuming the CI operator uses the defaultJOB_STAGE_TIMEOUT=1h. Both install paths default to 1h, but which path the e2e pools use was not confirmed.Not covered by the review
No build, test, generator, or
maketarget was run. Compile-level drift in the regenerated mocks, the reworked fixtures against the newNewSkyhookReconcileranddal.Newsignatures, and whether the new chainsaw suites pass on a real cluster are all unverified. Separately, the Job and Pod reconcilers are registered only incmd/manager/main.goand not insuite_test.go, so no envtest spec exercises the JobReconciler and SkyhookReconciler handoff end to end; the new coverage is fake-client unit tests plus chainsaw.