fix(nvca): make ModelCache envtest job completion version-adaptive#239
Open
balajinvda wants to merge 4 commits into
Open
fix(nvca): make ModelCache envtest job completion version-adaptive#239balajinvda wants to merge 4 commits into
balajinvda wants to merge 4 commits into
Conversation
TestReconcile_ModelCache drives the writer Job to completion by faking its status against the real envtest apiserver. The apiserver's Job-status validation differs by version: k8s 1.30-1.33 reject a SuccessCriteriaMet condition on this NonIndexed Job (it has no SuccessPolicy), while k8s 1.34+ require SuccessCriteriaMet before Complete and reject CompletionTime without Complete. No single static condition shape satisfies both, and the Job spec is reconciler-owned and immutable on update. The reconciler detects completion via CompletionTime + Succeeded, not the conditions. Extract a completeJob helper that applies the modern (>= 1.34) condition shape and falls back to the pre-1.34 shape only if the apiserver rejects it, so the test passes on both. Verified against envtest 1.30.3 and 1.34.1 (nvca pins 1.34). Closes #238 Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe model cache reconciliation test now uses a Kubernetes-version-compatible helper to complete the writer Job. The Bazel workflow documents the version-adaptive behavior and updated nvca test status. ChangesModel cache test compatibility
Workflow documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-17 16:25:44 UTC | Commit: cc616cf |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/compute-plane-services/nvca/pkg/storage/modelcache_test.go`:
- Around line 752-765: Update the initial c.Status().Update error handling
around the job status update to retry the legacy shape only when
apierrors.IsInvalid(err) is true. For all other errors, fail immediately instead
of proceeding to the re-fetch and second Status().Update; preserve the existing
fallback mutation for validation errors.
🪄 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: CHILL
Plan: Enterprise
Run ID: 182f5d5c-d46c-4b62-939d-d229bc18a192
📒 Files selected for processing (1)
src/compute-plane-services/nvca/pkg/storage/modelcache_test.go
…rrors Flip the nvca Bazel matrix row from tests_skip to tests-run now that TestReconcile_ModelCache is version-adaptive. The row's comment already said "tests run" but the field still skipped them; this makes the two agree. Also address review feedback on completeJob: only fall back to the pre-1.34 Job condition shape when the apiserver returns an Invalid (validation) error. Any other error (conflict, network, RBAC) now surfaces immediately instead of being masked by the second Status().Update. Verified pkg/storage against envtest 1.30.3 and 1.34.1. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Folded the #238 enablement into this PR so the
Verified |
The bazel matrix comments referenced internal issue IDs, which should not appear in the public mirror. Describe the test-skip reasons without them. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 @.github/workflows/bazel.yml:
- Around line 86-90: Update the comment describing TestReconcile_ModelCache in
the bazel workflow to state that the fallback occurs only when the apiserver
returns an Invalid validation error, rather than broadly saying it applies
whenever the apiserver rejects the condition shape. Preserve the surrounding
version-adaptive behavior and test-environment details.
🪄 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: CHILL
Plan: Enterprise
Run ID: c58c2a19-0d3f-421f-a14c-c00e2ecc78b7
📒 Files selected for processing (2)
.github/workflows/bazel.ymlsrc/compute-plane-services/nvca/pkg/storage/modelcache_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/compute-plane-services/nvca/pkg/storage/modelcache_test.go
Comment on lines
+86
to
+90
| # nvca: tests run. TestReconcile_ModelCache (NVCF-10347) drives the | ||
| # writer Job to completion and is version-adaptive: it applies the | ||
| # k8s >= 1.34 Job condition shape and falls back to the pre-1.34 | ||
| # shape if the apiserver rejects it, so it passes on the bazel-ci | ||
| # envtest (verified against 1.30.3 and 1.34.1). The only flaky test, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the fallback’s actual error boundary.
The helper falls back only for an apiserver Invalid validation error; “if the apiserver rejects it” is broader and could imply swallowing conflicts, network failures, or RBAC errors.
Proposed wording
- # shape if the apiserver rejects it, so it passes on the bazel-ci
+ # shape only when the apiserver returns an Invalid validation
+ # error, so it passes on the bazel-ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # nvca: tests run. TestReconcile_ModelCache (NVCF-10347) drives the | |
| # writer Job to completion and is version-adaptive: it applies the | |
| # k8s >= 1.34 Job condition shape and falls back to the pre-1.34 | |
| # shape if the apiserver rejects it, so it passes on the bazel-ci | |
| # envtest (verified against 1.30.3 and 1.34.1). The only flaky test, | |
| # nvca: tests run. TestReconcile_ModelCache (NVCF-10347) drives the | |
| # writer Job to completion and is version-adaptive: it applies the | |
| # k8s >= 1.34 Job condition shape and falls back to the pre-1.34 | |
| # shape only when the apiserver returns an Invalid validation | |
| # error, so it passes on the bazel-ci envtest (verified against 1.30.3 and 1.34.1). The only flaky test, |
🤖 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 @.github/workflows/bazel.yml around lines 86 - 90, Update the comment
describing TestReconcile_ModelCache in the bazel workflow to state that the
fallback occurs only when the apiserver returns an Invalid validation error,
rather than broadly saying it applies whenever the apiserver rejects the
condition shape. Preserve the surrounding version-adaptive behavior and
test-environment details.
3 tasks
Enabling `bazel test //...` for nvca surfaced failures beyond the ModelCache test this PR fixes: TestGetMiniServiceRBACCmData and TestGetNetworkPoliciesData* in //pkg/operator/reconcile, and the TestCreateMiniService_* (GVK/412) and Test_applyContainerTaskCreationMessage tests in //pkg/nvca. Those are separate test-layer issues for the nvca team. Revert the row to build-only so this PR stays a clean ModelCache fix. The full enablement backlog is tracked in #238. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
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.
Why
TestReconcile_ModelCache(pkg/storage) drives the writer Job to completion by faking its status against the real envtest apiserver. The apiserver's Job-status validation changed across k8s versions in incompatible directions:SuccessCriteriaMetcondition on this NonIndexed Job (it has noSuccessPolicy).SuccessCriteriaMetbeforeComplete, and rejectCompletionTimewithoutComplete.No single static condition shape satisfies both, and the Job spec is reconciler-owned and immutable on update, so the test cannot reshape it. This is why the
nvcarow in.github/workflows/bazel.ymlis currently build-only.What changed
The reconciler detects completion via
CompletionTime+Succeeded, not the Job conditions. Extract acompleteJobhelper that applies the modern (>= 1.34) condition shape and falls back to the pre-1.34 shape only if the apiserver rejects it. The test now passes regardless of the envtest version in play.Customer Release Notes
Not customer visible (test-only change).
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
Ran the full
pkg/storagepackage against both envtest versions:KUBEBUILDER_ASSETS=.../1.30.3->ok(previously failed:cannot set SuccessCriteriaMet to NonIndexed Job)KUBEBUILDER_ASSETS=.../1.34.1->ok(the version nvca pins viascripts/setup_envtest,K8S_MAJ_MIN_VERSION=1.34)Notes
Follow-up to actually enable the
nvcatest row on GHA is tracked in #238 (provision envtest /KUBEBUILDER_ASSETSin the matrix job, then droptests_skip: true). This PR only removes the underlying test fragility; it does not flip the matrix row.References
Closes #238
Related Merge Requests/Pull Requests
None
Dependencies
None
Github commit:
fix(nvca): make ModelCache envtest job completion version-adaptive
The writer-Job completion faked in TestReconcile_ModelCache tripped
apiserver Job-status validation that differs by k8s version. Apply the
modern condition shape and fall back to the pre-1.34 shape when the
apiserver rejects it, so the test passes on both. The reconciler keys off
CompletionTime + Succeeded, not the conditions.
Closes #238
Co-authored-by: Balaji Ganesan bganesan@nvidia.com
Co-authored-by: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
nvcadetect job to better describe the current test behavior and remaining envtest issues.