fix(nvca): keep the model-cache init namespace out of namespace GC and the metadata webhook - #1992
balajinvda wants to merge 2 commits into
Conversation
…d the metadata webhook The agent creates nvca-modelcache-init at startup and labels it workload-instance-type=miniservice so the unbound-DNS policy matches its cache writer jobs (#1116, #1303). Two other components select on that label and neither expects a namespace without a function instance: - internal/gc/namespace treats every labelled namespace without an ICMSRequest of the same name as orphaned and deletes it. The GC runs at startup and hourly, so the init namespace disappeared right after creation; the agent then logged "namespaces nvca-modelcache-init not found" on every permissions pass and Helm functions fell back to per-worker model downloads. - The miniservice mutating webhook's namespaceSelector matches the same label, and its Fail policy denies any pod whose namespace lacks the nvcf-miniservice-metadata ConfigMap. Writer pods in the init namespace have no instance metadata, so cache initialization could not start even when the namespace existed. The GC now skips storage.ModelCacheInitNamespace, and the webhook admits pods in it unchanged. The GC also logged nothing about any of this: controller-runtime starts runnables with its own context, on which core.GetLogger returns a discard logger. The agent now hands the GC a logging context so orphan detection and deletions are visible. Tests: the namespace cleaner test includes a labelled init namespace with no ICMSRequest and asserts it is not collected; the webhook test admits a writer pod in the init namespace without a metadata ConfigMap. Closes #1991 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
added
the
deploy-to-stg
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/nvcf/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughChangesModel-cache namespace handling
Image target repository naming
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The model-cache namespace bypass retains existing request logging and metrics coverage. No actionable regression remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The workflow change in Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /src/compute-plane-services/nvca:\n\tgithub.com/NVIDIA/KAI-scheduler@v0.12.6: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/NVIDIA/k8s-dra-driver-gpu@v0.0.0-20251017125642-cfe35ffd3d2c: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/NVIDIA/nvcf/src/libraries/go/lib@v0.0.0-20260722095202-f5e2792f5630: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/aws/aws-sdk-go@v1.55.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/bombsimon/logrusr/v4@v4.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/evanphx/json-patch/v5@v5.9.11: is explicitly required in ... [truncated 21721 characters] ... i: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/apiextensions-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/apimachinery: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/client-go: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/component-base: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tsigs.k8s.io/controller-runtime: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgolang.org/x/crypto: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
…sitory image-push mapped every target named `image` to the service repository. NVCA has four such targets, cmd/nvca, cmd/nvca-operator, cmd/cluster-validator and cmd/tools, so a deploy-to-stg build pushed all four to <registry>/nvca:<tag> and the last one won. When a subtree has more than one plain `image` target, the package leaf now names the repository, with the leaf equal to the service keeping the service name. Subtrees with a single `image` target keep mapping to the service wherever that target lives. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com> (cherry picked from commit 94b4dd0)
1 participant
Why
On a cluster running NVCA 3.12.4 with
HelmModelCachingon, Helm functions deployed but every worker downloaded its own model, and the agent logged every 30 minutes:The agent creates
nvca-modelcache-initat startup and labels itnvca.nvcf.nvidia.io/workload-instance-type: miniserviceso the unbound-DNS Kyverno policy matches cache writer jobs (#1116, #1303). Two other components select on that label and neither expects a namespace with no function instance behind it:internal/gc/namespacedeletes every labelled namespace without an ICMSRequest of the same name. It runs at startup and hourly, so the init namespace was removed right after creation. The first "not found" error appeared exactly one permissions cycle after agent start.namespaceSelectormatches the same label, and itsFailpolicy denies any pod whose namespace lacksnvcf-miniservice-metadata. Recreating the namespace by hand moved the failure here:admission webhook "miniservice-mutate-create.nvca.nvcf.nvidia.io" denied the request: namespace "nvca-modelcache-init" missing required ConfigMap "nvcf-miniservice-metadata".Neither was visible in the GC's own logs: controller-runtime starts runnables with its own context, on which
core.GetLoggerreturns anio.Discardlogger, so the 20k-line agent log had zero GC lines.What changed
internal/gc/namespace/cleaner.go: skipstorage.ModelCacheInitNamespacewhen collecting orphaned namespaces.pkg/webhook/miniservice_mutating_webhook.go: admit pods in the init namespace unchanged; there is no per-instance metadata to inject.internal/gc/gc.go+pkg/nvca/agent_manager.go:Runnable.SetLogContextlets the agent pass a logging context, so GC runs, orphan detection and deletions appear in the agent log.pkg/storageimport in the GC package.Customer Release Notes
Helm functions with models now use the shared model cache on clusters with
HelmModelCachingenabled; previously the cache init namespace was garbage-collected and each worker downloaded its own copy.Plan Summary
Not applicable. No chart or resource changes; the init namespace is created by the agent as before and now survives.
Usage
Not applicable.
Testing
go test ./internal/gc/... ./pkg/webhook/ ./pkg/nvca/insrc/compute-plane-services/nvca(webhook and nvca packages need-ldflags '-X github.com/NVIDIA/k8s-dra-driver-gpu/internal/info.version=v25.8.0', as the Makefile does; without it the package panics at init on main too). New tests:TestCleaner_collectOrphanedNamespacesnow includes a labelled init namespace with no ICMSRequest and asserts it is not collected;TestMiniserviceOperatorWebhook_ModelCacheInitNamespace_Allowedadmits a writer pod without a metadata ConfigMap. Live validation on a cluster to follow via the dev image.Notes
The GC deletion happened silently because of the discard logger; the third change is what would have made this a one-line log read instead of a source trace.
Issues
Closes #1991
References
None
Related Pull Requests
#1116 and #1303 added the label. #1969 fixed the neighbouring storage catalog ConfigMap gap.
Dependencies
None
Summary by CodeRabbit
Bug Fixes
Improvements
imagetargets with clearer repository naming while preserving existing behavior for single targets.