fix(nvca): fix the envtest suite at the test layer and re-enable matrix tests by balajinvda · Pull Request #239 · NVIDIA/nvcf

balajinvda · 2026-07-17T16:23:10Z

Why

The nvca bazel matrix row was build-only: enabling bazel test //... failed on
a set of envtest and environment issues, documented in #238. Leaving
the failures commented out is not a fix; this PR fixes every one of them at the
test layer (no reconciler or production changes) and re-enables the tests.

What changed

Test-layer fixes, in dependency order:

Version-adaptive Job completion (completeJob in pkg/storage):
k8s >= 1.34 requires SuccessCriteriaMet before Complete, while 1.30-1.33
reject it on this NonIndexed writer Job. Apply the modern shape and fall back
to the pre-1.34 shape only when the apiserver returns an Invalid validation
error (apierrors.IsInvalid). Also applied to the SharedFS/Samba envtests
added on main by feat(helm-model): select model cache backend by storage class #227, which inlined the modern shape and failed on older
apiservers.
Vendored dra-driver init panic: featuregates init() panics unless a real
version is stamped over internal/info's "unknown" default. The module's
own Makefile stamps it via -ldflags -X; mirrored in Bazel with x_defs on
the vendored info library. Unblocks internal/miniservice, pkg/dra,
pkg/nvca, pkg/webhook (test binaries died at init).
SPDX license-stamping fallout: the header block before the first YAML ---
broke decodeInferenceManifests (empty-GVK doc; contentless docs now
skipped), and headers appear once per stamped source file, so rendered
operator assets and testdata goldens disagree (stripSPDXHeaders normalizes
both sides; Kubernetes ignores YAML comments). Fixes
TestCreateMiniService_*, TestGetMiniServiceRBACCmData,
TestGetNetworkPoliciesData*.
Hermetic kubeconfig subtest: empty KUBECONFIG falls back to the machine's
real ~/.kube/config, so the expected error never happened on a dev machine;
it now points at a nonexistent path.
.github/workflows/bazel.yml: nvca matrix row flipped to run tests
(tests_skip false), comment updated to describe the fixes instead of the
failures.

Testing

Full nvca suite under go test: 66 packages, zero failures (envtest 1.34.1,
with a real kubeconfig present on the machine - the strong hermeticity check).
All three ModelCache envtests also pass against envtest 1.30.3, exercising
the completeJob fallback path (6/6 across both apiserver generations).
Previously init-panicking //pkg/dra and //pkg/webhook targets pass under
real bazel test with the x_defs stamp (2/2).
This PR's own bazel (nvca) matrix job now runs the full suite under the
bazel-ci envtest - the end-to-end proof.

References

Closes #238

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>

coderabbitai · 2026-07-17T16:23:36Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

The model cache reconciliation tests now use a shared helper to complete writer Jobs across Kubernetes apiserver versions. The Bazel workflow comment documents the adaptive condition handling and updated nvca test status.

Changes

Model cache test compatibility

Layer / File(s)	Summary
Adaptive writer Job completion `src/compute-plane-services/nvca/pkg/storage/modelcache_test.go`, `.github/workflows/bazel.yml`	The three model cache tests call `completeJob`, which retries with a legacy-compatible condition set when modern Job validation fails. The workflow comment documents this behavior and related test status.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

NVIDIA/nvcf#227: Related model-cache test changes for writer Job lifecycle validation.

Suggested reviewers: apartha-nv

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses `#238` by making the ModelCache test version-adaptive and updating the Bazel workflow to enable nvca test runs.
Out of Scope Changes check	✅ Passed	The changes stay focused on the ModelCache test fix and the nvca Bazel workflow update, with no obvious unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: adapting nvca envtest tests and re-enabling matrix test execution.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/nvca-modelcache-envtest-job-conditions

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-17T16:25:45Z

🛡️ CodeQL Analysis

🚨 Found 2 issue(s)

Severity Breakdown:

🔴 Errors: 0
🟡 Warnings: 0
🔵 Notes: 0

📋 Top Issues

🔗 View full details in Security tab

_{🕐 Last updated: 2026-07-17 16:25:44 UTC | Commit: cc616cf}

coderabbitai

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

📥 Commits

Reviewing files that changed from the base of the PR and between ed2f0bd and cc616cf.

📒 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>

balajinvda · 2026-07-17T17:21:12Z

Folded the #238 enablement into this PR so the Closes #238 is accurate (addresses CodeRabbit's Linked-Issues pre-merge warning):

.github/workflows/bazel.yml: flipped the nvca matrix row from tests_skip: true to run tests (nvca|src/compute-plane-services/nvca|false). The row's own comment already said "tests run"; the field now agrees. Updated the comment to describe the version-adaptive completion.
Addressed the inline review: gate the pre-1.34 fallback on apierrors.IsInvalid(err).

Verified pkg/storage green against envtest 1.30.3 and 1.34.1 locally. The GHA nvca job now runs bazel test //... against the baked bazel-ci envtest for real validation.

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>

coderabbitai

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

📥 Commits

Reviewing files that changed from the base of the PR and between cc616cf and 5722309.

📒 Files selected for processing (2)

.github/workflows/bazel.yml
src/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

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>

Main's model-cache-by-storage-class change (#227) added two envtests that inline the modern Job condition shape (SuccessCriteriaMet + Complete), which pre-1.34 apiservers reject; they were written against the CI envtest pin and could not use this branch's helper. Converting them to completeJob keeps every writer-Job completion version-adaptive. Also tightened the fallback comments to name the actual boundary (an Invalid validation error), per review. Verified locally: TestReconcile_ModelCache, ...SharedFS, and ...Samba all pass against envtest 1.34.1 (modern shape) and 1.30.3 (fallback path), 6/6. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

…ix tests Fix every failure that kept the nvca bazel matrix row build-only, all at the test layer (no reconciler or production changes), and flip the row to run tests: - Vendored dra-driver init panic: featuregates init() panics unless a real version is stamped over internal/info's "unknown" default. The module's own Makefile stamps it with go build -ldflags -X; mirror that in Bazel with x_defs on the vendored info go_library (relative key so rules_go qualifies it against importmap). Unblocks internal/miniservice, pkg/dra, pkg/nvca, and pkg/webhook, whose test binaries died before running a single test. - SPDX license stamping fallout: the header comment block before the first YAML doc separator made decodeInferenceManifests fail on an empty GVK (skip contentless docs in decodeYAMLFile), and headers stamped once per source file appear a different number of times in rendered operator assets vs testdata goldens (normalize both sides with stripSPDXHeaders; Kubernetes ignores YAML comments). Fixes TestCreateMiniService_*, TestGetMiniServiceRBACCmData, and TestGetNetworkPoliciesData*. - Hermetic kubeconfig test: the empty-KUBECONFIG subtest fell back to the machine's real ~/.kube/config and built a client where an error was expected; point it at a nonexistent path instead. Verified: the full nvca suite passes under go test (66 packages, envtest 1.34.1), the three ModelCache envtests also pass against envtest 1.30.3 (the completeJob fallback path), and the previously panicking pkg/dra and pkg/webhook targets pass under bazel test with the x_defs stamp. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

balaji-g

Reviewed: verification evidence in the PR description checks out (local + CI runs).

balajinvda requested a review from a team as a code owner July 17, 2026 16:23

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/compute-plane-services/nvca/pkg/storage/modelcache_test.go Outdated

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread .github/workflows/bazel.yml Outdated

balajinvda requested review from apartha-nv and kristinapathak July 17, 2026 17:28

coderabbitai Bot mentioned this pull request Jul 17, 2026

feat(helm-model): select model cache backend by storage class #227

Merged

3 tasks

balajinvda mentioned this pull request Jul 17, 2026

Enable nvca test execution in the GitHub Actions Bazel matrix #238

Closed

balajinvda and others added 2 commits July 19, 2026 10:04

balajinvda changed the title ~~fix(nvca): make ModelCache envtest job completion version-adaptive~~ fix(nvca): fix the envtest suite at the test layer and re-enable matrix tests Jul 19, 2026

balaji-g approved these changes Jul 20, 2026

View reviewed changes

apartha-nv approved these changes Jul 20, 2026

View reviewed changes

balajinvda added this pull request to the merge queue Jul 20, 2026

Merged via the queue into main with commit 69bbb84 Jul 20, 2026
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(nvca): fix the envtest suite at the test layer and re-enable matrix tests#239

fix(nvca): fix the envtest suite at the test layer and re-enable matrix tests#239
balajinvda merged 6 commits into
mainfrom
fix/nvca-modelcache-envtest-job-conditions

balajinvda commented Jul 17, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

balajinvda commented Jul 17, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

balaji-g left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

balajinvda commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Testing

References

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

github-actions Bot commented Jul 17, 2026

🛡️ CodeQL Analysis

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

balajinvda commented Jul 17, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

balaji-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

balajinvda commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading