feat(operator): swap package/interrupt execution to Jobs (#303 part 1) - #350
Merged
Conversation
github-actions
Bot
added
component/operator
Contributor
|
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 operator now uses namespace-scoped batch Jobs for package stages and interrupts, with dedicated Job and Pod reconcilers. Job lifecycle validation, optimistic node-state updates, executor creation and cleanup, reboot handling, RBAC, cache scope, package metadata, image selection, and pod failure classification were updated. Tests and design documentation cover Job ownership, reruns, cleanup, and migration behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Jul 21, 2026
github-actions
Bot
added
the
doc
ayuskauskas
force-pushed
the
jobs-migration/303a-swap
branch
from
July 22, 2026 00:22
d776950 to
c6b9dee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@chart/templates/manager-rbac.yaml`:
- Around line 88-103: Move the pods/log and batch/jobs permissions from the
cluster-wide RBAC definition into a namespace-scoped Role and matching
RoleBinding in chart/templates/manager-rbac.yaml, preserving the required verbs
while limiting access to the operator namespace. Update
docs/designs/2026-07-10-package-execution-as-jobs.md to document the
namespace-scoped, least-privilege behavior.
🪄 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: 91699b68-2a4e-4fd6-a7ae-801a562f07c3
📒 Files selected for processing (2)
chart/templates/manager-rbac.yamldocs/designs/2026-07-10-package-execution-as-jobs.md
This was referenced Jul 27, 2026
Base automatically changed from
jobs-migration/302c-job-reconcile
to
feature/package-as-jobs
July 28, 2026 16:24
Wire the reconcile path to create batch/v1 Jobs instead of raw pods for package and interrupt stage execution. All Job machinery landed in #300-#302 (builders, JobReconcile, dal accessors, log tail) but was unwired; this is the cutover. - ApplyPackage/Interrupt build Jobs (createJobFromPackage / createInterruptJobFromPackage), stamp the package annotation on both the Job and its pod template (setJobPackage), gate creation on JobExists, and resolve AlreadyExists against the deterministic name via handleExistingJob. - JobExists is the migration-window union: an unfinished Job OR a pre-upgrade raw pod (legacyPodExists), so a single call gates both worlds. The !jobFinished filter is load-bearing: retained (TTL) finished Jobs must not read as running, unlike pods which were reaped on completion. - HasRunningPackages spans Jobs + legacy pods. ValidateRunningPackages becomes an orphan sweep: shouldDeleteFinishedJob rerun predicate, jobIsStale -> InvalidPackage, validateLegacyPods. TrackReboots sweeps node Jobs after Reset. - pod_controller dual-path: Job-owned pods route to jobPodReconcile (surfaces in-flight erroring only; JobReconcile owns completion), legacy pods keep the delete-on-complete path. podFailureIsGenuine skips disruption/unknown noise. - main.go registers a namespace-scoped Job informer. RBAC (batch/jobs, pods/log) mirrored into config/rbac and chart/templates. The user-facing contract is unchanged: same labels, nodeState annotations, metrics, CRD, and CLI. Behavioral mechanics are documented in the design doc. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
The swap's `done` container (which replaces the forever-running pause container so the pod can reach Succeeded) ran `/bin/sh -c "exit 0"` on the agent image. A minimal agent image (e.g. the agentless test image on arm64) has no /bin/sh, so the container StartErrors (exit 128), the pod never succeeds, the Job never completes, and the skyhook hangs in_progress forever. Use the package image instead: the init-copy container already invokes /bin/sh from it, so the package image is guaranteed to have a shell, and it is already pulled (no new image). Verified end-to-end on a local kind cluster (arm64, agentless): the package Jobs now complete and the skyhook reaches complete. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Jobs were wired as a prefixed "job---<name>" request on the Skyhook queue, mirroring the pod path. That shape exists to serialize writers, not because it fits: JobReconcile is already per-object and returns a per-object Result, so routing it through the whole-world pass means folding a per-Job requeue and a per-Job backoff into a queue that has neither. JobReconciler now owns Jobs on its own watch, with the name-label gate as a predicate so foreign Jobs (a CronJob's, say) never enter the workqueue rather than being filtered inside Reconcile. That removes the serialization the unlocked node patches were relying on. Two controllers now write nodewright.nvidia.com/nodeState_<name>, one annotation key holding every package, so an unconditional patch would silently drop whichever write landed second and the state-recorded marker would stop anything retrying it. Both Job-path writes go through patchNodeState: read, mutate, patch under an optimistic-lock precondition, retry on conflict. The guards moved inside the retry closure, since a retry starts from state another writer just changed and a decision made against the previous read is not reusable. The heavy pass gets the precondition on its node patches too, where a conflict escapes to the existing error aggregate: it cannot retry in place because the snapshot it computed from is stale, so the pass requeues and re-derives. TrackReboots is deliberately left unlocked, with a comment saying why. Its spec requires the reset to land on a node whose resourceVersion moved under other controllers, because losing it strands a stale "complete" and the package is never reapplied. Drops the Job case from globalDelayHandler.relevant: a Job event no longer needs to wake the heavy pass, since the node write JobReconcile makes is itself a Node event the existing watch already picks up. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
force-pushed
the
jobs-migration/303a-swap
branch
from
July 28, 2026 17:19
c6b9dee to
1f4f59b
Compare
Contributor
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)
operator/internal/controller/skyhook_controller.go (1)
656-669: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRebuild the optimistic-lock patch before the second write. Both
HandleMigrationsandSaveNodesAndSkyhookreuse the pre-patchclusterState.tracker.GetOriginal(...)snapshot, so once the first patch lands the second call can carry a stale resourceVersion and hit a 409 Conflict.🤖 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 656 - 669, In the changed-node handling shared by HandleMigrations and SaveNodesAndSkyhook, rebuild the optimistic-lock patch after the status patch completes instead of reusing the original pre-patch snapshot. Refresh the node’s current state/resourceVersion, then create the MergeFromWithOptimisticLock patch from that refreshed baseline before calling r.Patch, while preserving the existing conflict handling.
🤖 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/job_controller.go`:
- Around line 94-99: Update JobReconciler.SetupWithManager to configure
controller options with an explicitly raised MaxConcurrentReconciles value when
building the controller. Preserve the existing manager, job type, ownedJob
predicate, controller name, and Complete(r) wiring while enabling concurrent
reconciliation.
---
Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 656-669: In the changed-node handling shared by HandleMigrations
and SaveNodesAndSkyhook, rebuild the optimistic-lock patch after the status
patch completes instead of reusing the original pre-patch snapshot. Refresh the
node’s current state/resourceVersion, then create the
MergeFromWithOptimisticLock patch from that refreshed baseline before calling
r.Patch, while preserving the existing conflict handling.
🪄 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: a87732c5-6335-4a18-b4f0-30ae9e68ee7c
📒 Files selected for processing (14)
chart/templates/manager-rbac.yamldocs/designs/2026-07-10-package-execution-as-jobs.mdoperator/cmd/manager/main.gooperator/config/rbac/role.yamloperator/internal/controller/event_handler.gooperator/internal/controller/event_handler_test.gooperator/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.go
Comment on lines
+94
to
+99
| func (r *JobReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| Named("job"). | ||
| For(&batchv1.Job{}, builder.WithPredicates(ownedJob())). | ||
| Complete(r) | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Consider raising JobReconciler's reconcile concurrency.
SetupWithManager has no WithOptions(controller.Options{MaxConcurrentReconciles: ...}), so it defaults to 1 — every stage/interrupt Job across the whole operator is reconciled serially. Each Job's node-state write already goes through patchNodeState's optimistic-lock+retry, so concurrent reconciles across different Jobs (the common case, since most touch different nodes) look safe. At the design doc's stated scale (nodes × packages × 3 stages), single-threaded Job reconciliation could become a throughput bottleneck.
🤖 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 94 - 99, Update
JobReconciler.SetupWithManager to configure controller options with an
explicitly raised MaxConcurrentReconciles value when building the controller.
Preserve the existing manager, job type, ownedJob predicate, controller name,
and Complete(r) wiring while enabling concurrent reconciliation.
The dual path guarded an upgrade landing while raw package pods were still mid-flight. That state is unreachable: the rename has never shipped (no released operator carries the nodewright API group at all), the swap ships with it, and NodeWright reconcile is gated behind legacyMigrationHold, which holds on any legacy Skyhook whose status is not empty/complete/paused/disabled and tells the operator to finish, roll back, or delete it on the pre-rename operator first. Nothing raw can be running by the time the Jobs path takes over. So this removes validateLegacyPods, legacyPodExists, the legacy branch of the interrupt gate, and the legacy pod deletion in the config-update path. PodReconcile collapses to what was the Job-owned branch: the watch now only surfaces in-flight erroring, and never deletes a pod or records completion, both of which would race JobReconcile for the same node-state key. isJobOwnedPod and HandleInvalidPackage lose their last callers with it; the Job path covers invalidation through IsInvalidPackage plus a foreground delete. Known gap, deliberately not re-covered: the hold skips paused and disabled legacy Skyhooks, and pre-rename pause did not suspend an in-flight pod, so a Skyhook paused mid-stage could still have one running at takeover. Narrow enough not to justify keeping two execution paths alive for a release. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Package pods are owned by their Job now, not by the CR directly, so every assert naming NodeWright as the pod's owner fails against this branch. Those seven files were staged in the e2e PR two steps down the chain, which left #350 and #351 red in between and made a real regression indistinguishable from the expected breakage. They are a consequence of this change, so they belong with it. Also drops a restarts > 0 check that only held when a failed step restarted in place; package Jobs run restartPolicy Never, so a retry is a fresh pod. Adds assert_jobs.yaml for the half nothing covered: assert_pods.yaml proves pod -> Job, this proves Job -> NodeWright as a controller reference, which is what makes the whole tree GC with the CR. It also pins the run-to-completion defaults the pod assert cannot see (unlimited backoff, podReplacementPolicy Failed, restartPolicy Never, and the 1h JOB_STAGE_TIMEOUT default). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Cluster-wide grants the operator never exercises. Every Job it touches lives in its own namespace: the informer is scoped there in main.go, and all six GetJobs calls pass client.InNamespace. Pod logs are only ever read off those Jobs' child pods for the deadline snapshot. The namespace= field on the kubebuilder rbac markers moves both onto a Role. kustomize's namespace transformer rewrites the literal, and the chart templates .Release.Namespace, so nothing is pinned to the default install namespace. The RoleBinding is hand-written because controller-gen generates roles but never bindings. pods/status stays cluster-wide despite sharing a generated rule with pods/log: drain reads workload pods on any node. Raised by review on the chart RBAC; the design doc said the role stays cluster-scoped, which is no longer true. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
github-actions
Bot
added
the
component/tests
Contributor
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 (4)
operator/internal/controller/swap_test.go (2)
69-85: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse envtest for reconciliation tests.
newReconcilerconstructs a fake client while these tests exercise Job/Pod creation, ownership, and status behavior. Convert this helper to envtest, or keep fake-client coverage limited to pure predicates.As per coding guidelines, use envtest for unit tests and Ginkgo/Gomega for operator tests.
🤖 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/swap_test.go` around lines 69 - 85, Update the newReconciler helper in swap_test.go to initialize and use an envtest Kubernetes control plane instead of fake.NewClientBuilder. Preserve the registered schemes, pod node-name index, clientset, recorder, and validOpts setup while ensuring reconciliation tests exercise real Job/Pod creation, ownership, and status behavior; reserve fake-client usage for pure predicate tests.Source: Coding guidelines
103-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual Job identity, not only the owner type.
The unit fixture is not actually owner-linked, and the Chainsaw assertions only require
apiVersion: batch/v1andkind: Job. A legacy/unowned Pod or a Pod owned by the wrong Job could therefore satisfy these checks.
operator/internal/controller/swap_test.go#L103-L119: add a matching JobownerReference, not just thebatch.kubernetes.io/job-namelabel.k8s-tests/chainsaw/nodewright/cleanup-pods/assert-config-complete.yaml#L35-L36: assert the deterministic Job name and controller flag.k8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yaml#L90-L91: assert the expected apply Job identity.k8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yaml#L126-L127: assert the expected config Job identity.k8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yaml#L162-L163: assert the expected package Job identity.k8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yaml#L86-L87: assert the expected apply Job identity.k8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yaml#L120-L121: assert the expected config Job identity.k8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yaml#L157-L158: assert the expected interrupt Job identity.k8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yaml#L191-L192: assert the expected post-interrupt Job identity.k8s-tests/chainsaw/nodewright/interrupt/chainsaw-test.yaml#L90-L91: assert the expected apply Job identity.k8s-tests/chainsaw/nodewright/interrupt/chainsaw-test.yaml#L216-L217: assert the expected interrupt Job identity.k8s-tests/chainsaw/nodewright/package-upgrade/chainsaw-test.yaml#L116-L117: assert the expected upgrade Job identity.k8s-tests/chainsaw/nodewright/simple-nodewright/assert_pods.yaml#L35-L36: assert the expected spencer Job identity.k8s-tests/chainsaw/nodewright/simple-nodewright/assert_pods.yaml#L89-L90: assert the expected foobar Job identity.🤖 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/swap_test.go` around lines 103 - 119, Strengthen Job ownership assertions across all listed sites. In jobOwnedPod, add an ownerReference matching the expected Job name, batch/v1 Job kind, and controller ownership; in the listed Chainsaw YAML assertions, require each deterministic expected Job name and controller flag in addition to the existing Job identity checks. Apply the corresponding expected identities for cleanup-pods (apply/config/package), interrupt-grouping (apply/config/interrupt/post-interrupt), interrupt (apply/interrupt), package-upgrade (upgrade), and simple-nodewright (spencer/foobar) at their specified file ranges.operator/internal/controller/job_controller.go (1)
166-181: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRedundant node fetch before
recordJobCompletion/recordJobErroring.
handleCompleteJobandhandleFailedJobeach callr.dal.GetNodeand short-circuit onnode == nil, but the functions they call afterward (recordJobCompletion,recordJobErroring) go throughpatchNodeState, which re-fetches the node itself and silently no-ops (returnsnil) when the node is gone — after which both paths callmarkJobProcessedregardless. Removing the outer check preserves identical behavior while cutting a duplicate apiserverGetNodecall per terminal Job event.♻️ Proposed simplification for handleCompleteJob (same pattern applies to handleFailedJob)
node, err := r.dal.GetNode(ctx, jobNodeName(job)) if err != nil { return ctrl.Result{}, fmt.Errorf("getting node for job %s: %w", job.Name, err) } - // Node gone → there is no state to record; mark processed rather than error-loop on - // a node that will never return. - if node == nil { - return ctrl.Result{}, r.markJobProcessed(ctx, job, r.opts.JobTTLSucceeded) - } - if err := r.recordJobCompletion(ctx, job, pkg); err != nil { return ctrl.Result{}, err }Also applies to: 301-314
🤖 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 166 - 181, Remove the redundant GetNode lookup and node == nil short-circuit from handleCompleteJob and handleFailedJob. Let recordJobCompletion and recordJobErroring delegate node existence handling to patchNodeState, then preserve the existing markJobProcessed call for both terminal paths.operator/internal/controller/pod_controller.go (1)
69-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winErroring-state update failures are swallowed, so they're never retried.
When
UpdateNodeStatefails (Line 86-88), the error is only logged;PodReconcilestill returnsctrl.Result{}, nil. Controller-runtime will not requeue on anilerror, so a transient failure — e.g. a resourceVersion conflict, which is now more likely givenJobReconciler'spatchNodeStateand the main reconciler also write node state concurrently — permanently drops this in-flight erroring signal until another pod event happens to fire (which may never occur for an already-terminal pod).UpdateNodeState's ownPatchcall has no optimistic-lock option or retry-on-conflict, unlike the newerpatchNodeStatehelper injob_controller.go.Propagate the error (or apply the same
retry.RetryOnConflict+MergeFromWithOptimisticLockpattern used injob_controller.go'spatchNodeState) so a failed update is actually retried.🔧 Proposed fix
containerName, state, restarts := containerExitedSuccessfully(pod) if state == containerStateFailed && podFailureIsGenuine(pod) { if _, err := r.UpdateNodeState(ctx, pod, v1alpha1.StateErroring, containerName, restarts); err != nil { - logger.Error(err, "error updating node state for job pod failure", "pod", pod.Name) + return ctrl.Result{}, fmt.Errorf("error updating node state for job pod failure %s: %w", pod.Name, err) } } return ctrl.Result{}, nil🤖 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/pod_controller.go` around lines 69 - 91, Update PodReconcile’s UpdateNodeState failure path so the reconciliation error is propagated instead of only logged and returning success. Preserve the existing error log, but return ctrl.Result{} with the update error from the containerStateFailed branch, ensuring controller-runtime retries transient conflicts and other update failures.
🤖 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 166-181: Remove the redundant GetNode lookup and node == nil
short-circuit from handleCompleteJob and handleFailedJob. Let
recordJobCompletion and recordJobErroring delegate node existence handling to
patchNodeState, then preserve the existing markJobProcessed call for both
terminal paths.
In `@operator/internal/controller/pod_controller.go`:
- Around line 69-91: Update PodReconcile’s UpdateNodeState failure path so the
reconciliation error is propagated instead of only logged and returning success.
Preserve the existing error log, but return ctrl.Result{} with the update error
from the containerStateFailed branch, ensuring controller-runtime retries
transient conflicts and other update failures.
In `@operator/internal/controller/swap_test.go`:
- Around line 69-85: Update the newReconciler helper in swap_test.go to
initialize and use an envtest Kubernetes control plane instead of
fake.NewClientBuilder. Preserve the registered schemes, pod node-name index,
clientset, recorder, and validOpts setup while ensuring reconciliation tests
exercise real Job/Pod creation, ownership, and status behavior; reserve
fake-client usage for pure predicate tests.
- Around line 103-119: Strengthen Job ownership assertions across all listed
sites. In jobOwnedPod, add an ownerReference matching the expected Job name,
batch/v1 Job kind, and controller ownership; in the listed Chainsaw YAML
assertions, require each deterministic expected Job name and controller flag in
addition to the existing Job identity checks. Apply the corresponding expected
identities for cleanup-pods (apply/config/package), interrupt-grouping
(apply/config/interrupt/post-interrupt), interrupt (apply/interrupt),
package-upgrade (upgrade), and simple-nodewright (spencer/foobar) at their
specified file ranges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1dd2f862-9d43-46d7-8f1b-f3829f724a04
📒 Files selected for processing (18)
chart/templates/manager-rbac.yamldocs/designs/2026-07-10-package-execution-as-jobs.mdk8s-tests/chainsaw/nodewright/cleanup-pods/assert-config-complete.yamlk8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/failure-nodewright/node-assert.yamlk8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/interrupt/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/package-upgrade/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/assert_pods.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/chainsaw-test.yamloperator/config/rbac/kustomization.yamloperator/config/rbac/namespaced_role_binding.yamloperator/config/rbac/role.yamloperator/internal/controller/job_controller.gooperator/internal/controller/pod_controller.gooperator/internal/controller/skyhook_controller.gooperator/internal/controller/swap_test.go
The precondition added with JobReconciler turned every concurrent write into a conflict the pass could not recover from. It patches every node from one whole-world snapshot, so a conflict cannot be retried in place: the state the patch was computed from is already stale, and the pass just re-runs and re-conflicts. e2e went from 0 conflicts to 156 on a single lifecycle run, and three suites (config-skyhook, cleanup-pods, interrupt) stalled with node state pinned at in_progress and the log full of "error processing skyhook". JobReconciler keeps its own lock and retry, which does converge because a single object can be re-read cheaply. That leaves the write race one-sided: the pass can still overwrite a completion that landed between its read and its patch. Closing it properly means narrowing what the pass patches, not gating a snapshot it cannot recompute, and that is its own change. Also drops the pod-finalization suite. It hand-created a raw pod and asserted the Pod watch drove node state to complete, which was the legacy path; under Jobs the operator owns node placement, so a hand-built executor cannot be attributed to a node without pinning one, and a nodeSelector leaves the Job template's nodeName empty. Nineteen other suites assert a package reaching complete through real operator-driven rollouts, and the JobReconcile unit specs cover the recording path directly. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Coverage Report for CI Build 30480160225Warning 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: 78.983%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Collaborator
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@k8s-tests/chainsaw/nodewright/cleanup-pods/assert-config-complete.yaml`:
- Around line 35-36: Strengthen the Job ownership assertions by matching each
deterministic Job name, requiring ownerReferences.controller to be true, and
verifying the expected NodeWright owner. Update
cleanup-pods/assert-config-complete.yaml lines 35-36 for cleanup-pods;
cleanup-pods/chainsaw-test.yaml lines 108-109, 144-145, and 180-181 for aa
apply/config and bb apply; interrupt-grouping/chainsaw-test.yaml lines 86-87,
120-121, 157-158, and 191-192 for dax apply/config/interrupt/post-interrupt;
interrupt/chainsaw-test.yaml lines 90-91 and 216-217 for jason apply and dexter
interrupt; and package-upgrade/chainsaw-test.yaml lines 116-117 for nullptr
upgrade.
In `@k8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yaml`:
- Around line 69-86: In the cleanup-pods test flow, first assert or wait for the
unfinished bb-1.2 config Job on kind-worker using the existing identifying
labels before removing the node annotation. Keep the subsequent reset/reaping
flow and negative Job assertion unchanged so the test proves an existing Job was
deleted.
In `@operator/internal/controller/job_controller.go`:
- Around line 241-258: Update patchNodeState to read the node through an
uncached API reader inside the RetryOnConflict callback instead of d.GetNode’s
cached client. Use the controller manager’s GetAPIReader() dependency, while
preserving the existing mutate, optimistic-lock patch, nil handling, and retry
behavior.
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 2362-2386: Align the documentation with the implemented behavior
by removing the claims about legacy raw-pod detection and sweeping from
JobExists, deleteConfigUpdateExecutors, and ValidateRunningPackages, unless an
existing legacy-pod helper is confirmed and intentionally added to those flows.
Keep the comments accurate to the current batchv1.Job-only logic and avoid
introducing a new pod-existence implementation without an established symbol to
reuse.
🪄 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: 1e20be03-ce3c-4cef-bfca-420ed1410cac
📒 Files selected for processing (31)
chart/templates/manager-rbac.yamldocs/designs/2026-07-10-package-execution-as-jobs.mdk8s-tests/chainsaw/nodewright/cleanup-pods/README.mdk8s-tests/chainsaw/nodewright/cleanup-pods/assert-config-complete.yamlk8s-tests/chainsaw/nodewright/cleanup-pods/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/failure-nodewright/node-assert.yamlk8s-tests/chainsaw/nodewright/interrupt-grouping/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/interrupt/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/interrupt/pod.yamlk8s-tests/chainsaw/nodewright/package-upgrade/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/pod-finalizer/README.mdk8s-tests/chainsaw/nodewright/pod-finalizer/chainsaw-test.yamlk8s-tests/chainsaw/nodewright/pod-finalizer/pod.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/assert_jobs.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/assert_pods.yamlk8s-tests/chainsaw/nodewright/simple-nodewright/chainsaw-test.yamloperator/cmd/manager/main.gooperator/config/rbac/kustomization.yamloperator/config/rbac/namespaced_role_binding.yamloperator/config/rbac/role.yamloperator/internal/controller/event_handler.gooperator/internal/controller/event_handler_test.gooperator/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
💤 Files with no reviewable changes (3)
- k8s-tests/chainsaw/nodewright/pod-finalizer/pod.yaml
- k8s-tests/chainsaw/nodewright/pod-finalizer/chainsaw-test.yaml
- k8s-tests/chainsaw/nodewright/pod-finalizer/README.md
The pod watch recorded in-flight erroring unconditionally, so it could create a node-state entry the operator never wrote. Under restartPolicy Never a failing Job mints a fresh pod per attempt indefinitely, which turns that into a repeating write with two failure modes: - After a node-state reset, the next attempt re-pins the package to the stage the reset just cleared. jobIsStale then reads the Job as matching node state, so the sweep never invalidates it and JobExists gates the package forever: the reset can never take effect. This is the cleanup-pods e2e failure. - A pod for a package absent from the spec writes an entry for a package that was never applied, polluting node state and flipping the Skyhook to erroring. This is the interrupt e2e failure, which HandleInvalidPackage used to mask by deleting the pod. Guard the write the way JobReconcile's shouldRecordCompletion and recordJobErroring already are: record only when the entry is present, still at this pod's stage, and not already complete. The pod watch is evidence, not authority. Finish the controller split started for Jobs: - The pod watch becomes a real controller (PodReconciler) instead of a pod---<name> request routed through the heavy pass, so it gets a real requeue and its own backoff. - Both per-object controllers hold their own dependencies rather than embedding SkyhookReconciler. Embedding inherited a Reconcile each had to shadow, so deleting the shadow would still compile and silently run the whole-world pass on every pod or Job event. - patchNodeState and deleteJobForeground become free functions, the only two helpers with callers on both sides. - The Job env knobs group into JobOperatorOptions, embedded in SkyhookOperatorOptions so field promotion leaves the builder chain and Validate untouched, and the env names stay flat. Leaving the heavy pass's single-threaded queue means the pod watch no longer serializes against it, so its node-state write now goes through the optimistic-locked patchNodeState like the Job path. That retry re-reads the Node uncached from attempt 1 onward: dal reads through the cached client, and the informer has usually not seen the write that just beat us, so a cached re-read rebuilds the same doomed precondition and burns every attempt. Attempt 0 stays cached, so the hot path costs nothing extra. Narrow the cache and RBAC to the operator namespace for the kinds that never leave it. ConfigMaps and Secrets join Jobs: all ConfigMap access already passes InNamespace and a package's spec.configMap is mounted by the kubelet rather than read, and the only Secret is the operator's own webhook serving cert. Cache scope and RBAC scope have to move together, since a cluster-wide informer under a namespaced Role is rejected at LIST/WATCH — which is why webhookBootstrapMgr, a second manager that owns the only Secret watch, gets its own scoped cache here. Pods stay cluster-wide: drain must see workload pods on any node, and scoping them would report a node drained while workloads still run. The reasoning for every kind, scoped or not, is recorded at the cache options. Removes the unreachable StateComplete half of UpdateNodeState, and corrects three comments that described a legacy raw-pod path deleted with the migration window. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
force-pushed
the
jobs-migration/303a-swap
branch
from
July 29, 2026 16:53
7a7ec49 to
9963c6d
Compare
The precondition assert added for the reset check also required bb's config
Job to be unfinished:
status:
(conditions[?type == 'Complete' && status == 'True']): []
An active Job has no status.conditions field at all, so that filter
evaluates to null rather than an empty list and the assert can never pass
while the Job is running — it failed with "Invalid value: null: value is
null" against a Job that was present and correct.
Existence is the whole precondition the assert needs: the step above
already waits for the Skyhook to report erroring, so this Job cannot have
completed. Drop the status clause and note why a conditions filter does not
belong here.
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Scoping the ConfigMap informer to the operator namespace correlated with intermittent apply-to-config stalls in e2e/core: simple-update-skyhook and simple-skyhook timed out with a package parked at apply/in_progress, on a different k8s version each run. No mechanism has been identified — every ConfigMap access site passes client.InNamespace and a package's spec.configMap is a kubelet-resolved mount rather than an operator read — so this backs the change out as a single-variable bisect rather than as a diagnosis. Cache scope and RBAC scope move together: a cluster-wide informer under a namespaced Role is rejected at LIST/WATCH, so the kubebuilder marker and the chart mirror revert with it. Jobs and Secrets stay scoped. Secrets are not exercised by e2e at all (make run sets ENABLE_WEBHOOKS=false, so the manager owning the only Secret watch never starts), and Jobs have been scoped since c274004, well before these failures appeared. The reasoning for re-scoping ConfigMaps later is kept at the cache options so the memory win is not lost, with a note not to re-apply it without a reproduction first. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
This was referenced Jul 29, 2026
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
#350) * feat(operator): swap package/interrupt execution to Jobs (#303 part 1) Wire the reconcile path to create batch/v1 Jobs instead of raw pods for package and interrupt stage execution. All Job machinery landed in #300-#302 (builders, JobReconcile, dal accessors, log tail) but was unwired; this is the cutover. - ApplyPackage/Interrupt build Jobs (createJobFromPackage / createInterruptJobFromPackage), stamp the package annotation on both the Job and its pod template (setJobPackage), gate creation on JobExists, and resolve AlreadyExists against the deterministic name via handleExistingJob. - JobExists is the migration-window union: an unfinished Job OR a pre-upgrade raw pod (legacyPodExists), so a single call gates both worlds. The !jobFinished filter is load-bearing: retained (TTL) finished Jobs must not read as running, unlike pods which were reaped on completion. - HasRunningPackages spans Jobs + legacy pods. ValidateRunningPackages becomes an orphan sweep: shouldDeleteFinishedJob rerun predicate, jobIsStale -> InvalidPackage, validateLegacyPods. TrackReboots sweeps node Jobs after Reset. - pod_controller dual-path: Job-owned pods route to jobPodReconcile (surfaces in-flight erroring only; JobReconcile owns completion), legacy pods keep the delete-on-complete path. podFailureIsGenuine skips disruption/unknown noise. - main.go registers a namespace-scoped Job informer. RBAC (batch/jobs, pods/log) mirrored into config/rbac and chart/templates. The user-facing contract is unchanged: same labels, nodeState annotations, metrics, CRD, and CLI. Behavioral mechanics are documented in the design doc. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> * fix(operator): run the Job exit-0 container on the package image (#303) The swap's `done` container (which replaces the forever-running pause container so the pod can reach Succeeded) ran `/bin/sh -c "exit 0"` on the agent image. A minimal agent image (e.g. the agentless test image on arm64) has no /bin/sh, so the container StartErrors (exit 128), the pod never succeeds, the Job never completes, and the skyhook hangs in_progress forever. Use the package image instead: the init-copy container already invokes /bin/sh from it, so the package image is guaranteed to have a shell, and it is already pulled (no new image). Verified end-to-end on a local kind cluster (arm64, agentless): the package Jobs now complete and the skyhook reaches complete. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> * feat(operator): give package-stage Jobs their own controller Jobs were wired as a prefixed "job---<name>" request on the Skyhook queue, mirroring the pod path. That shape exists to serialize writers, not because it fits: JobReconcile is already per-object and returns a per-object Result, so routing it through the whole-world pass means folding a per-Job requeue and a per-Job backoff into a queue that has neither. JobReconciler now owns Jobs on its own watch, with the name-label gate as a predicate so foreign Jobs (a CronJob's, say) never enter the workqueue rather than being filtered inside Reconcile. That removes the serialization the unlocked node patches were relying on. Two controllers now write nodewright.nvidia.com/nodeState_<name>, one annotation key holding every package, so an unconditional patch would silently drop whichever write landed second and the state-recorded marker would stop anything retrying it. Both Job-path writes go through patchNodeState: read, mutate, patch under an optimistic-lock precondition, retry on conflict. The guards moved inside the retry closure, since a retry starts from state another writer just changed and a decision made against the previous read is not reusable. The heavy pass gets the precondition on its node patches too, where a conflict escapes to the existing error aggregate: it cannot retry in place because the snapshot it computed from is stale, so the pass requeues and re-derives. TrackReboots is deliberately left unlocked, with a comment saying why. Its spec requires the reset to land on a node whose resourceVersion moved under other controllers, because losing it strands a stale "complete" and the package is never reapplied. Drops the Job case from globalDelayHandler.relevant: a Job event no longer needs to wake the heavy pass, since the node write JobReconcile makes is itself a Node event the existing watch already picks up. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * refactor(operator): drop the pod-to-Job migration window The dual path guarded an upgrade landing while raw package pods were still mid-flight. That state is unreachable: the rename has never shipped (no released operator carries the nodewright API group at all), the swap ships with it, and NodeWright reconcile is gated behind legacyMigrationHold, which holds on any legacy Skyhook whose status is not empty/complete/paused/disabled and tells the operator to finish, roll back, or delete it on the pre-rename operator first. Nothing raw can be running by the time the Jobs path takes over. So this removes validateLegacyPods, legacyPodExists, the legacy branch of the interrupt gate, and the legacy pod deletion in the config-update path. PodReconcile collapses to what was the Job-owned branch: the watch now only surfaces in-flight erroring, and never deletes a pod or records completion, both of which would race JobReconcile for the same node-state key. isJobOwnedPod and HandleInvalidPackage lose their last callers with it; the Job path covers invalidation through IsInvalidPackage plus a foreground delete. Known gap, deliberately not re-covered: the hold skips paused and disabled legacy Skyhooks, and pre-rename pause did not suspend an in-flight pod, so a Skyhook paused mid-stage could still have one running at takeover. Narrow enough not to justify keeping two execution paths alive for a release. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * test(e2e): move chainsaw assertions onto the Jobs execution model Package pods are owned by their Job now, not by the CR directly, so every assert naming NodeWright as the pod's owner fails against this branch. Those seven files were staged in the e2e PR two steps down the chain, which left #350 and #351 red in between and made a real regression indistinguishable from the expected breakage. They are a consequence of this change, so they belong with it. Also drops a restarts > 0 check that only held when a failed step restarted in place; package Jobs run restartPolicy Never, so a retry is a fresh pod. Adds assert_jobs.yaml for the half nothing covered: assert_pods.yaml proves pod -> Job, this proves Job -> NodeWright as a controller reference, which is what makes the whole tree GC with the CR. It also pins the run-to-completion defaults the pod assert cannot see (unlimited backoff, podReplacementPolicy Failed, restartPolicy Never, and the 1h JOB_STAGE_TIMEOUT default). Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * fix(operator): scope Jobs and pod-log RBAC to the operator namespace Cluster-wide grants the operator never exercises. Every Job it touches lives in its own namespace: the informer is scoped there in main.go, and all six GetJobs calls pass client.InNamespace. Pod logs are only ever read off those Jobs' child pods for the deadline snapshot. The namespace= field on the kubebuilder rbac markers moves both onto a Role. kustomize's namespace transformer rewrites the literal, and the chart templates .Release.Namespace, so nothing is pinned to the default install namespace. The RoleBinding is hand-written because controller-gen generates roles but never bindings. pods/status stays cluster-wide despite sharing a generated rule with pods/log: drain reads workload pods on any node. Raised by review on the chart RBAC; the design doc said the role stays cluster-scoped, which is no longer true. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * fix(operator): stop optimistic-locking the heavy pass's node writes The precondition added with JobReconciler turned every concurrent write into a conflict the pass could not recover from. It patches every node from one whole-world snapshot, so a conflict cannot be retried in place: the state the patch was computed from is already stale, and the pass just re-runs and re-conflicts. e2e went from 0 conflicts to 156 on a single lifecycle run, and three suites (config-skyhook, cleanup-pods, interrupt) stalled with node state pinned at in_progress and the log full of "error processing skyhook". JobReconciler keeps its own lock and retry, which does converge because a single object can be re-read cheaply. That leaves the write race one-sided: the pass can still overwrite a completion that landed between its read and its patch. Closing it properly means narrowing what the pass patches, not gating a snapshot it cannot recompute, and that is its own change. Also drops the pod-finalization suite. It hand-created a raw pod and asserted the Pod watch drove node state to complete, which was the legacy path; under Jobs the operator owns node placement, so a hand-built executor cannot be attributed to a node without pinning one, and a nodeSelector leaves the Job template's nodeName empty. Nineteen other suites assert a package reaching complete through real operator-driven rollouts, and the JobReconcile unit specs cover the recording path directly. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * fix(operator): stop the pod watch from resurrecting node state The pod watch recorded in-flight erroring unconditionally, so it could create a node-state entry the operator never wrote. Under restartPolicy Never a failing Job mints a fresh pod per attempt indefinitely, which turns that into a repeating write with two failure modes: - After a node-state reset, the next attempt re-pins the package to the stage the reset just cleared. jobIsStale then reads the Job as matching node state, so the sweep never invalidates it and JobExists gates the package forever: the reset can never take effect. This is the cleanup-pods e2e failure. - A pod for a package absent from the spec writes an entry for a package that was never applied, polluting node state and flipping the Skyhook to erroring. This is the interrupt e2e failure, which HandleInvalidPackage used to mask by deleting the pod. Guard the write the way JobReconcile's shouldRecordCompletion and recordJobErroring already are: record only when the entry is present, still at this pod's stage, and not already complete. The pod watch is evidence, not authority. Finish the controller split started for Jobs: - The pod watch becomes a real controller (PodReconciler) instead of a pod---<name> request routed through the heavy pass, so it gets a real requeue and its own backoff. - Both per-object controllers hold their own dependencies rather than embedding SkyhookReconciler. Embedding inherited a Reconcile each had to shadow, so deleting the shadow would still compile and silently run the whole-world pass on every pod or Job event. - patchNodeState and deleteJobForeground become free functions, the only two helpers with callers on both sides. - The Job env knobs group into JobOperatorOptions, embedded in SkyhookOperatorOptions so field promotion leaves the builder chain and Validate untouched, and the env names stay flat. Leaving the heavy pass's single-threaded queue means the pod watch no longer serializes against it, so its node-state write now goes through the optimistic-locked patchNodeState like the Job path. That retry re-reads the Node uncached from attempt 1 onward: dal reads through the cached client, and the informer has usually not seen the write that just beat us, so a cached re-read rebuilds the same doomed precondition and burns every attempt. Attempt 0 stays cached, so the hot path costs nothing extra. Narrow the cache and RBAC to the operator namespace for the kinds that never leave it. ConfigMaps and Secrets join Jobs: all ConfigMap access already passes InNamespace and a package's spec.configMap is mounted by the kubelet rather than read, and the only Secret is the operator's own webhook serving cert. Cache scope and RBAC scope have to move together, since a cluster-wide informer under a namespaced Role is rejected at LIST/WATCH — which is why webhookBootstrapMgr, a second manager that owns the only Secret watch, gets its own scoped cache here. Pods stay cluster-wide: drain must see workload pods on any node, and scoping them would report a node drained while workloads still run. The reasoning for every kind, scoped or not, is recorded at the cache options. Removes the unreachable StateComplete half of UpdateNodeState, and corrects three comments that described a legacy raw-pod path deleted with the migration window. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * fix(e2e): drop the unsatisfiable conditions filter from cleanup-pods The precondition assert added for the reset check also required bb's config Job to be unfinished: status: (conditions[?type == 'Complete' && status == 'True']): [] An active Job has no status.conditions field at all, so that filter evaluates to null rather than an empty list and the assert can never pass while the Job is running — it failed with "Invalid value: null: value is null" against a Job that was present and correct. Existence is the whole precondition the assert needs: the step above already waits for the Skyhook to report erroring, so this Job cannot have completed. Drop the status clause and note why a conditions filter does not belong here. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * revert(operator): leave the ConfigMap cache and RBAC cluster-wide Scoping the ConfigMap informer to the operator namespace correlated with intermittent apply-to-config stalls in e2e/core: simple-update-skyhook and simple-skyhook timed out with a package parked at apply/in_progress, on a different k8s version each run. No mechanism has been identified — every ConfigMap access site passes client.InNamespace and a package's spec.configMap is a kubelet-resolved mount rather than an operator read — so this backs the change out as a single-variable bisect rather than as a diagnosis. Cache scope and RBAC scope move together: a cluster-wide informer under a namespaced Role is rejected at LIST/WATCH, so the kubebuilder marker and the chart mirror revert with it. Jobs and Secrets stay scoped. Secrets are not exercised by e2e at all (make run sets ENABLE_WEBHOOKS=false, so the manager owning the only Secret watch never starts), and Jobs have been scoped since c274004, well before these failures appeared. The reasoning for re-scoping ConfigMaps later is kept at the cache options so the memory win is not lost, with a note not to re-apply it without a reproduction first. Signed-off-by: Brian Lockwood <lockwobr@gmail.com> --------- Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> Signed-off-by: Brian Lockwood <lockwobr@gmail.com> Co-authored-by: Brian Lockwood <lockwobr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First increment of #303 (part 1 — the core swap). Stacked on #348 (base
jobs-migration/302c-job-reconcile) so the diff stays focused; will re-point tofeature/package-as-jobsas the stack lands.Wires the reconcile path to create
batch/v1Jobs instead of raw pods for package and interrupt stage execution. All the Job machinery landed in #300–#302 (builders,JobReconcile, dal accessors, log tail) but was unwired; this is the cutover. The pause→suspendcascade follows in part 2.The swap
ApplyPackage/Interruptbuild Jobs (createJobFromPackage/createInterruptJobFromPackage), stamp the package annotation on both the Job and its pod template (setJobPackage), gate creation onJobExists, and resolveAlreadyExistsagainst the deterministic name viahandleExistingJob(matching-spec unfinished → leave; parkedDeadlineExceededat an erroring entry → leave to absorb the recreate; unprocessed completion → leave forJobReconcile; stale-spec or processed-finished → foreground-delete so the name frees).JobExistsis the migration-window union: an unfinished Job or a pre-upgrade raw pod (legacyPodExists), so one call gates both worlds for the one-minor window. The!jobFinishedfilter is load-bearing — retained (TTL) finished Jobs must not read as "still running", unlike pods which were reaped on completion;legacyPodExistskeeps the old no-filter semantics because legacy pods still go through delete-on-complete.HasRunningPackagesspans Jobs + legacy pods.ValidateRunningPackagesbecomes an orphan sweep:shouldDeleteFinishedJobrerun predicate (processed-and-no-longer-recorded-done → delete; parked-at-erroring → keep),jobIsStale→InvalidPackage,validateLegacyPods.TrackRebootssweeps node Jobs afternode.Reset().pod_controllerdual-path: Job-owned pods (batch.kubernetes.io/job-namelabel) route tojobPodReconcile— surfaces in-flight erroring only, no delete-on-success (JobReconcileowns completion); legacy pods keep the original delete-on-complete path.podFailureIsGenuineskips disruption /ContainerStatusUnknownnoise and handles interrupt-JobCrashLoopBackOff.main.goregisters a namespace-scoped Job informer. RBAC (batch/jobs,pods/log) mirrored intoconfig/rbac/role.yamlandchart/templates/manager-rbac.yaml.Notes for review
dispatchExecutorRequestextracted fromReconcileto keep it under the gocyclo bar as thejob---routing lands; the pod/Job existence gate collapses two calls intoJobExistsrather than calling both explicitly. Called out per the repo's "justify new patterns" rule.legacyPodExists+ its call fromJobExists, and the dual-path branch inpod_controller, are deleted when the one-minor migration window closes (marked in-code at theJobExistsfall-through).nodeState_*annotations,skyhook_package_stage_countmetric, CRD, CLI). Mechanics live in the (current) design doc; no user doc describes the pod mechanism in a way the swap makes stale.Verification
swap_test.go): ApplyPackage-creates-Job,JobExists(unfinished / finished / legacy), pod dual-path (no-delete-on-success, disruption-silent), Job-owned-pod genuine failure records erroring (the case that catches the template-annotation regression), and 4 directshouldDeleteFinishedJobrerun-predicate specs.make lint0 issues;gofmtclean;go buildclean.setJobPackage), one major (the erroring test gap that would have caught it), one nit (DeletionTimestampguard onJobReconcile) — all folded in.Same inherited-base caveat as the rest of the stack: the full
make unit-testsaborts on a pre-existing, unrelated CLI test hang (Package Rerun … multiple --node flags) before the controller suite runs; the controller/dal suites were run directly and pass.e2e status (correction)
Correction: an earlier revision of this description claimed the failing
e2e/*andhelm-testschecks were inherited from thefix/api-renamebase. That was wrong — the base (#348) is fully green. The swap introduced them.e2e/*(chainsaw) failures are migrated to the Jobs model in test(e2e): migrate chainsaw assertions to the Jobs execution model (#304) #353 (sub-issue Jobs migration 6/7: migrate chainsaw e2e assertions to Jobs #304): package/interrupt pods are now owned by theirbatch/v1Job rather than the NodeWright CR, and completed-stage pods linger under the Job TTL. This PR'se2e/*stays red until test(e2e): migrate chainsaw assertions to the Jobs execution model (#304) #353 lands above it in the stack.helm-tests(helm-chart-webhook) failure is a separate functional regression (skyhook staysin_progressunder the helm-deployed operator) — not an assertion issue, tracked separately.unit-testsremains the gate for this PR and passes.