feat(operator): jobMatchesPackage staleness check (#302 part 2a)#319
Open
ayuskauskas wants to merge 2 commits into
Open
feat(operator): jobMatchesPackage staleness check (#302 part 2a)#319ayuskauskas wants to merge 2 commits into
ayuskauskas wants to merge 2 commits into
Conversation
github-actions
Bot
added
component/operator
Contributor
📝 WalkthroughWalkthroughThe controller adds Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![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 `@operator/internal/controller/job_controller.go`:
- Around line 62-67: Update podMatchesPackage in
operator/internal/controller/job_controller.go to compare expected and actual
init-container counts before entering the comparison loop, returning false on
any mismatch to prevent incorrect matches and panics. Add missing- and
extra-init-container test cases in
operator/internal/controller/job_controller_test.go covering both mismatches and
asserting false without panicking.
🪄 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: 86c0ee72-3fe4-444e-b695-bdb94ffded74
📒 Files selected for processing (2)
operator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.go
ayuskauskas
added a commit
that referenced
this pull request
Jul 17, 2026
…n jobMatchesPackage tests Address CodeRabbit + adversarial review on #319: - podMatchesPackage indexed expectedPod.Spec.InitContainers[i] by the actual chain's length, so an extra actual init container panicked and a missing one silently matched. Add a length check before the compare — this fixes both the pod and the Job path (jobMatchesPackage delegates here). - jobMatchesPackage specs: add a negative interrupt case (drifted version), an image-change case (package label matches but the init-copy image differs), and an init-container count-mismatch case (extra + missing, must return false without panicking). Vary the arbitrary argEncode/stage the still-unwired interrupt builder receives, and note that init-container Args are intentionally not part of the match. - Tighten the jobMatchesPackage doc comment: podMatchesPackage also branches on the interrupt label and compares init-container resources, not just the label. Part of #302. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
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 `@operator/internal/controller/job_controller.go`:
- Around line 53-63: Update jobMatchesPackage and the underlying
podMatchesPackage comparison so it excludes only the per-generation resource ID
environment variable, not every variable returned by getAgentConfigEnvVars.
Ensure static operator-configured environment changes cause stale Jobs to be
detected and replaced. Add a regression test covering a non-ID operator
environment variable 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: 9682163a-27e5-4a1d-8d24-185c753c80f8
📒 Files selected for processing (3)
operator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.gooperator/internal/controller/skyhook_controller.go
ayuskauskas
force-pushed
the
jobs-migration/302-job-controller
branch
from
July 17, 2026 20:41
6344cdb to
5ce2fa8
Compare
Adds jobMatchesPackage, the Job analogue of podMatchesPackage: does an existing stage Job still match what the operator would build for this package+stage now? Later increments use it to decide, on an AlreadyExists race or a validation sweep, whether a Job is stale and must be replaced. Still unwired. The Job builder wraps the raw pod without changing its initContainers or the package label — the only things podMatchesPackage compares — so this reuses podMatchesPackage on the Job's pod template rather than duplicating (and risking drift in) the env-filtering / resource comparison. Next increment: JobReconcile (completion recording + state-recorded marker + outcome TTL + DeadlineExceeded/park + failed-attempt pruner + FailureTarget log-tail snapshot) and dal.GetPodLogTail. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
…n jobMatchesPackage tests Address CodeRabbit + adversarial review on #319: - podMatchesPackage indexed expectedPod.Spec.InitContainers[i] by the actual chain's length, so an extra actual init container panicked and a missing one silently matched. Add a length check before the compare — this fixes both the pod and the Job path (jobMatchesPackage delegates here). - jobMatchesPackage specs: add a negative interrupt case (drifted version), an image-change case (package label matches but the init-copy image differs), and an init-container count-mismatch case (extra + missing, must return false without panicking). Vary the arbitrary argEncode/stage the still-unwired interrupt builder receives, and note that init-container Args are intentionally not part of the match. - Tighten the jobMatchesPackage doc comment: podMatchesPackage also branches on the interrupt label and compares init-container resources, not just the label. Part of #302. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
ayuskauskas
force-pushed
the
jobs-migration/302b-job-reconcile
branch
from
July 17, 2026 20:43
0bf386e to
6d3d744
Compare
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 (1)
operator/internal/controller/skyhook_controller.go (1)
131-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new job timing env vars
JOB_TTL_SUCCEEDED,JOB_TTL_FAILED, andJOB_STAGE_TIMEOUTare only called out in CRD comments and the design doc; add them to the chart/operator config docs so users can discover the runtime settings.🤖 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 131 - 137, Update the chart/operator configuration documentation to list JOB_TTL_SUCCEEDED, JOB_TTL_FAILED, and JOB_STAGE_TIMEOUT, including their purpose and default values. Keep the descriptions consistent with the comments for JobTTLSucceeded, JobTTLFailed, and JobStageTimeout so users can discover and configure these runtime settings.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/skyhook_controller.go`:
- Around line 131-137: Update the chart/operator configuration documentation to
list JOB_TTL_SUCCEEDED, JOB_TTL_FAILED, and JOB_STAGE_TIMEOUT, including their
purpose and default values. Keep the descriptions consistent with the comments
for JobTTLSucceeded, JobTTLFailed, and JobStageTimeout so users can discover and
configure these runtime settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 884be993-2f81-4c26-be9a-4fac3dfa66fd
📒 Files selected for processing (3)
operator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.gooperator/internal/controller/skyhook_controller.go
Coverage Report for CI Build 29612235543Warning No base build found for commit Coverage: 78.054%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
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
2 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.
Continues #302, stacked on #318 (base
jobs-migration/302-job-controller) so the diff stays focused; re-points as the stack lands. Still unwired.What
jobMatchesPackage— the Job analogue ofpodMatchesPackage: does an existing stage Job still match what the operator would build for this package+stage now (same package/version, same init-container chain and env, ignoring the per-generation resource-id)? Later increments use it on anAlreadyExistsrace or a validation sweep to decide whether a Job is stale and must be replaced.Implementation note (reuse, not a new pattern): the Job builder (#301) wraps the raw pod without touching its
initContainersor the package label — the only thingspodMatchesPackagecompares — sojobMatchesPackageevaluatespodMatchesPackageon the Job's pod template rather than duplicating the env-filtering/resource comparison and letting the two drift.Increment boundary
JobReconcile(the completion recording +state-recordedmarker + outcome TTL,DeadlineExceeded/park, backstop, failed-attempt pruner,FailureTargetlog-tail snapshot) anddal.GetPodLogTailare the next increment. That completion flow needs careful sharing of the existingUpdateNodeState/HandleCompletePodpath (which the Job path can reuse by passing the terminal child pod) plus the crash-window postcondition guards — the subtle heart of the migration — so it gets its own focused PR + adversarial review rather than being rushed in here.Verification
make lint0 issues.jobMatchesPackagespecs (match, version-changed → false, interrupt Job) +jobHandlerFunc; controller suite 216/216.Same inherited-base caveat as the rest of the stack (
e2e/*/cli-e2e/helm-testsfail from the rename base, not this change;unit-testsis the gate).