feat(helm-model): select model cache backend by storage class#227
Merged
Conversation
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-17 05:00:17 UTC | Commit: e0ebd20 |
balajinvda
force-pushed
the
nvca-model-cache-backends-gh
branch
from
July 17, 2026 15:23
e0ebd20 to
b3717da
Compare
estroz
approved these changes
Jul 17, 2026
estroz
left a comment
estroz
left a comment
Contributor
There was a problem hiding this comment.
Discussed changes offline. LGTM
balajinvda
force-pushed
the
nvca-model-cache-backends-gh
branch
from
July 17, 2026 18:45
b3717da to
0ccbdec
Compare
📝 WalkthroughWalkthroughAdds backend-aware Helm model caching with NVMesh, shared filesystem, Samba, and ephemeral modes. The change introduces capability probing, durable cache lifecycle management, webhook injection, backend-labelled metrics, API conversion, and tests covering selection, provisioning, reuse, cleanup, and admission behavior. ChangesHelm model-cache backends
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Helm MiniService model caching was gated on the HelmCachingSupport feature flag, which assumes NVMesh 3.x is present in the cluster. That excludes clusters without NVMesh and clusters that bring their own shared storage. Remove the HelmCachingSupport flag; gate all caching on CachingSupport and choose the backend by storage class (SelectHelmCacheBackend): 1. nvcf-sc-30 present: NVMesh 3.x, existing cross-namespace NVMesh path. 2. else nvcf-miniservice-sc present: operator-provided shared storage, shared-FS populate path (single-writer populate on the shared class, per-namespace read-only reader PVC mounted by the existing webhook). Reader access mode (ROX preferred, RWX fallback) chosen by a CSI capability probe with a TTL-cached result. 3. else HelmSharedStorage enabled: NVCA deploys a Samba server on durable block storage (nvcf-sc) and publishes the nvcf-miniservice-sc SMB CSI StorageClass; the next reconcile resolves to shared-FS. 4. else: per-pod ephemeral emptyDir cache injected by the helm storage webhook, with the init env carried in the miniservice metadata ConfigMap. The shared-FS path tears down the init job and lease after population while retaining the writer PVC as the durable populated marker. The CSI probe deletes its PV explicitly so Retain-reclaim classes do not accumulate Released PVs. The HelmCacheBackend type lives in pkg/types so metrics use the same constants instead of duplicating the strings. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/compute-plane-services/nvca/internal/metrics/METRICS.md (1)
566-579: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove Markdown bold formatting from the changed documentation.
Headings such as
**Type:**,**Labels:**,**Usage:**, and**Tracing:**violate the repository text convention. Use plain text or regular headings.As per coding guidelines, "Committed documentation and text must use no Markdown bold."
Also applies to: 615-669
🤖 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 `@src/compute-plane-services/nvca/internal/metrics/METRICS.md` around lines 566 - 579, Remove Markdown bold markers from the changed metrics documentation, including labels such as Type, Labels, Usage, and Tracing in the model cache metric section and the additionally affected lines. Preserve the text and structure while rendering these labels as plain text or regular headings.Source: Coding guidelines
src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go (1)
170-178: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard malformed model-cache requests before cleanup.
Terminal validation of a nil
ModelCacheSpecis followed by cleanup, which dereferencesst.Spec.ModelCache.CacheHandleand panics.
src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go#L170-L178: validateModelCacheor pass an already validated cache handle into cleanup.src/compute-plane-services/nvca/pkg/storage/modelcache_test.go#L1215-L1237: exercisedoModelCache/doModelCacheRoutedand assert nil input returns a terminal error without panicking.🤖 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 `@src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go` around lines 170 - 178, Guard cleanupInitModelCache against a nil ModelCacheSpec by validating it before dereferencing CacheHandle, or pass an already validated handle into the cleanup flow. Update doModelCache and doModelCacheRouted tests in src/compute-plane-services/nvca/pkg/storage/modelcache_test.go:1215-1237 to verify nil input returns a terminal error without panicking; the cleanup change is in src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go:170-178.src/compute-plane-services/nvca/pkg/storage/controller_modelcache.go (1)
101-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not log and return the controller registration error.
Return the wrapped error to the caller and let it log once; optionally log only successful registration here.
Proposed fix
err := builder. ControllerManagedBy(mgr). Named(string(storageReqType)). For(&nvcav1new.StorageRequest{}, fanOutPredicateOption). Watches(&batchv1.Job{}, fanOutEventHandler). Watches(&corev1.Pod{}, fanOutEventHandler). Watches(&corev1.PersistentVolumeClaim{}, fanOutEventHandler). Watches(&coordv1.Lease{}, fanOutEventHandler). Watches(&corev1.PersistentVolume{}, fanOutEventHandler). Complete(r) - logf.Log.Info("buildControllerModelCache: controller registration complete", "type", string(storageReqType), "error", err) - return err + if err != nil { + return fmt.Errorf("register %s controller: %w", storageReqType, err) + } + logf.Log.Info("model cache controller registration complete", "type", string(storageReqType)) + return nilAs per coding guidelines, "When logging errors, preserve the originating error with
%wor equivalent wrapping and do not both log and return the same error."🤖 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 `@src/compute-plane-services/nvca/pkg/storage/controller_modelcache.go` around lines 101 - 112, The controller registration flow in the builder chain currently logs the same error that it returns. Update the code around Complete in buildControllerModelCache to return the error wrapped with its context, and remove the error-bearing log; optionally retain a success-only log after registration completes.Source: Coding guidelines
🟡 Minor comments (7)
src/compute-plane-services/nvca/internal/metrics/metrics_test.go-1567-1582 (1)
1567-1582: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover every newly initialized backend-labelled series.
This checks only the four counters. Add assertions for the
ModelCacheBackendsgauge and update the result initialization test to match each backend label; otherwise those initialization loops can regress unnoticed.As per coding guidelines, "When adding metric label values, update zero-value initialization in
internal/metrics/metrics.goand add corresponding assertions ininternal/metrics/metrics_test.go."🤖 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 `@src/compute-plane-services/nvca/internal/metrics/metrics_test.go` around lines 1567 - 1582, Extend the “ModelCache backend counters initialized to zero” test to include the ModelCacheBackends gauge, and update the result initialization test to assert zero values for every backend label in ModelCacheBackends. Ensure the assertions cover all newly initialized backend-labelled series and align with the zero-value initialization in the metrics implementation.Source: Coding guidelines
src/compute-plane-services/nvca/internal/metrics/metrics.go-74-79 (1)
74-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Godoc comments for the new exported constants.
The metric-name constants and
BackendLabelare exported but undocumented.As per coding guidelines, "Add Godoc comments to all exported Go symbols."
Also applies to: 156-156
🤖 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 `@src/compute-plane-services/nvca/internal/metrics/metrics.go` around lines 74 - 79, Add Godoc comments to each newly exported metric-name constant in the metrics constants block, including ModelCacheResultTotalMetricName, ModelCacheBackendsMetricName, ModelCacheBackendSelectedTotalMetricName, ModelCachePopulateTotalMetricName, ModelCacheReuseTotalMetricName, and ModelCacheReclaimedTotalMetricName, plus BackendLabel. Ensure every comment begins with the exact exported symbol name and briefly describes its purpose.Source: Coding guidelines
.github/workflows/codeql.yml-6-17 (1)
6-17: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winNarrow the CodeQL trigger paths.
src/**matches every file belowsrc, including non-Go/Rust content, so docs, charts, migrations, or other non-code files there still trigger the expensive scan. Remove the broad glob and list only compiled sources plus the build manifests that affect extraction, using the same list forpushandpull_request.Also applies to: 18-28
🤖 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/codeql.yml around lines 6 - 17, Update the CodeQL workflow path filters for both push and pull_request triggers, removing the broad src/** entry and retaining only compiled Go/Rust source patterns, their go.mod/go.sum and Cargo.toml/Cargo.lock manifests, and the workflow file itself.src/compute-plane-services/nvca/pkg/nvca/agent_manager.go-122-125 (1)
122-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude cluster context in the new structured logs.
The registration logs currently contain controller and caching fields, but no explicit cluster identifier. Add the established cluster context, such as
a.ClusterName, so these startup events remain attributable in aggregated logs.As per path instructions,
src/**/*.gochanges must check structured logging with required context fields (request/function/cluster/org id).Also applies to: 140-140
🤖 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 `@src/compute-plane-services/nvca/pkg/nvca/agent_manager.go` around lines 122 - 125, Update the structured registration logs in the controller-registration flow, including the occurrence around the `Info("Registering storage controllers")` call and the related log at the other referenced location, to include the established cluster context field using `a.ClusterName`. Preserve the existing controller and caching fields while ensuring each startup log has the required cluster attribution.Source: Path instructions
src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.go-73-78 (1)
73-78: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument
"samba"as a distinct backend.
src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.goLines 146-155 writesstring(HelmCacheBackendSamba)into this field, but the comment says the Samba-backed case is represented by"sharedfs". List"samba"explicitly and clarify the valid values for this field.Suggested documentation fix
- // cache: "nvmesh" (NVMesh 3.x, nvcf-sc-30) or "sharedfs" (a shared - // filesystem class, nvcf-miniservice-sc, including the Samba-backed case). Empty + // cache: "nvmesh" (NVMesh 3.x, nvcf-sc-30), "sharedfs" (a shared + // filesystem class), or "samba" (the Samba-backed shared filesystem). Empty🤖 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 `@src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.go` around lines 73 - 78, Update the Backend field comment to document "samba" as a distinct valid backend alongside "nvmesh" and "sharedfs", and clarify the meaning of each value without implying that Samba is represented by "sharedfs". Preserve the existing empty-value backward-compatibility behavior.src/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.go-73-77 (1)
73-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument every accepted backend value.
The reconciler persists
samba, but this public API comment only permitsnvmeshandsharedfs. Addsambaand clarify whetherephemeralis intentionally invalid for aStorageRequest.🤖 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 `@src/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.go` around lines 73 - 77, Update the Backend field comment to document every accepted value, adding "samba" alongside "nvmesh" and "sharedfs". Explicitly state that "ephemeral" is not valid for a StorageRequest, while preserving the existing empty-value backward-compatibility behavior.src/compute-plane-services/nvca/pkg/storage/modelcache_test.go-986-993 (1)
986-993: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the Samba lifecycle description.
The durable marker is the backing PVC label, and the writer PV is deleted after population. This comment currently states the opposite.
🤖 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 `@src/compute-plane-services/nvca/pkg/storage/modelcache_test.go` around lines 986 - 993, The comment above TestReconcile_ModelCacheSamba incorrectly describes the durable marker and writer PV lifecycle. Update it to state that the backing PVC label is the durable marker and that the writer PV is deleted after population, while preserving the remaining Samba lifecycle and cross-namespace behavior details.
🧹 Nitpick comments (3)
src/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.go (1)
30-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven tests for the new multi-scenario checks.
src/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.go#L30-L57: table-drive the four cache-launch inputs.src/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.go#L53-L63: combine missing image and missing handle into validation cases.src/compute-plane-services/nvca/pkg/nvca/agent_manager_test.go#L28-L45: table-drive caching disabled and enabled expectations.As per coding guidelines, "Use table-driven tests for multiple scenarios."
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.go` around lines 30 - 57, Convert the four scenarios in TestCacheLaunchRequested into a table-driven test, preserving the existing inputs and expected results. Also table-drive the validation cases in src/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.go lines 53-63 for missing image and missing handle, and the caching-disabled/enabled expectations in src/compute-plane-services/nvca/pkg/nvca/agent_manager_test.go lines 28-45; use each test’s existing functions and assertions.Source: Coding guidelines
.github/workflows/codeql.yml (1)
58-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
actions/checkoutto an immutable commit.The custom CodeQL action is SHA-pinned, but
actions/checkout@v4is mutable. Pin it to a verified full commit SHA to prevent unreviewed action changes from entering this security-sensitive workflow. GitHub recommends full-length SHAs for immutable action references. (docs.github.com)🤖 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/codeql.yml around lines 58 - 62, Update the Checkout repository step’s actions/checkout reference from the mutable v4 tag to a verified, full-length commit SHA, preserving the existing persist-credentials setting.src/compute-plane-services/nvca/pkg/storage/types.go (1)
59-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Godoc comments for each exported constant.
The descriptive block comments do not document the individual exported identifiers. Add comments beginning with
WebhookEphemeralModelCacheInitImageAnnotationKey,EphemeralModelCacheInitContainerName,EphemeralModelCacheConfigDataVolumeName, andEphemeralModelCacheConfigSharedMountPath.As per coding guidelines, "Add Godoc comments to all exported Go symbols."
Also applies to: 98-104
🤖 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 `@src/compute-plane-services/nvca/pkg/storage/types.go` around lines 59 - 67, Add Godoc comments immediately before each exported constant, beginning with its identifier: WebhookEphemeralModelCacheInitImageAnnotationKey, EphemeralModelCacheInitContainerName, EphemeralModelCacheConfigDataVolumeName, and EphemeralModelCacheConfigSharedMountPath. Preserve the existing descriptive details while making each comment explicitly document the corresponding constant.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.
Inline comments:
In `@docs/dev/sdd-central-model-cache-service.md`:
- Around line 67-69: Update the populate lifecycle description around
cleanupInitModelCache so sharedfs retains the writer/read-write PVC after
successful reconciliation, while only temporary initialization resources are
removed; keep deletion behavior for non-shared backends unchanged and align the
durable-marker section accordingly.
- Around line 11-15: Update the opening cache contract to promise
cross-namespace reuse only for the nvmesh, sharedfs, and samba backends.
Explicitly state that ephemeral caching is limited to the lifetime of an
individual pod and does not provide cross-namespace sharing, while preserving
the best-effort provisioning behavior.
- Around line 32-34: Revise the durable-marker definition and the related
sections around the sharedfs and Samba flows to define population by
backend-specific state rather than object existence. Document that Samba’s
backing PVC may exist before population and that the `modelcache-populated`
label represents safe cache reuse, while distinguishing reusable infrastructure
from a populated cache.
In `@migrations/cassandra/keyspaces/event_ledger/02_init_roles.up.sql`:
- Line 5: Update the event_ledger_app_v0 role migration to explicitly apply
SERVICE_ROLE_PASSWORD to an existing role, adding an ALTER ROLE ... WITH
PASSWORD migration alongside creation. Ensure both newly created and already
existing roles receive the current configured password during migration.
- Around line 7-8: Replace the direct system_auth INSERT and UPDATE statements
in the role initialization migration with a GRANT ROLE statement granting
event_ledger_app_access to event_ledger_app_v0. Remove both system_auth table
writes while preserving the same role membership.
In `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go`:
- Around line 676-679: Update the reconcile flow around SelectHelmCacheBackend
to call it only when the launch’s cache-request predicate indicates caching is
requested. When no cache specification is present, assign HelmCacheBackendNone
directly, preserving the existing error handling for requested-cache backend
selection.
- Around line 2085-2100: Replace the complete environment copy created in the
reconciliation flow around parseWorkloadEnvSet and maps.Clone with an allowlist
containing only variables required for model-cache initialization. Remove
sensitive launch variables such as API_KEY from miniservice metadata and webhook
cache, and resolve those values from the appropriate Kubernetes Secrets instead.
Preserve the existing InitImageEnv validation and INSTANCE_ID metadata behavior.
- Around line 2058-2073: Centralize the cache-request validity predicate used by
makeStorageRequests so durable and ephemeral backends make the same decision.
Update cacheLaunchRequested to accept the cache specification shape supported by
TestMakeStorageRequests_BackendHandling, including CacheArtifacts, a handle, and
the default zero CacheSize, and ensure makeStorageRequests reuses it without
backend-specific filtering.
In `@src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go`:
- Around line 198-208: Update sambaModelCacheServiceDNS to omit the hardcoded
cluster.local suffix and return the cluster-independent service name using the
service, namespace, and .svc components. Keep sambaModelCacheShareRoot unchanged
so it continues constructing the SMB source from sambaModelCacheServiceDNS.
- Around line 156-190: The Samba bootstrap flow should reconcile existing
resources rather than treating AlreadyExists as success. Replace the generic
ensureCreated usage in the resource setup with kind-specific updates for the
Deployment, Service, NetworkPolicy, and PVC, applying current image/resources
and expanding PVC capacity when requested; retain create-once behavior for the
rwSecret and roSecret credentials, using the existing resource-construction
helpers and ensureCreated where appropriate.
In `@src/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.go`:
- Around line 136-150: Separate Kubernetes control-plane errors from confirmed
unsupported CSI results: in
src/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.go lines 136-150,
propagate Create and wait errors instead of returning cached StateUnsupported;
in probe.go lines 263-267, propagate non-NotFound Get errors rather than polling
to timeout; in modelcache.go lines 345-352, requeue inconclusive probes and
reserve terminal failure for confirmed incompatibility; update
cacheprobe_test.go lines 191-211 to expect API timeouts to propagate and trigger
retry rather than cache unsupported.
- Around line 199-202: The cache probe can reuse completed resources from an
earlier run. In src/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.go
lines 199-202, update the resource-creation flow to validate ownership, spec,
and freshness or use a unique probe-run name instead of accepting any
AlreadyExists error; in lines 253-256, ensure only a succeeded pod belonging to
the current probe is accepted. In
src/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.go lines
111-112, add coverage for mismatched and stale pre-existing resources, replacing
coverage limited to identical recreation.
In `@src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go`:
- Around line 225-276: Update the cleanup flow around isVolumeDetached and
sambaWriterPV so a false detachment result retains both the writer PVC and
static PV, including skipping the PV deletion in that reconcile. Delete the
Samba writer PV only after detachment is confirmed, while preserving existing
error handling and requeue behavior.
In `@src/compute-plane-services/nvca/pkg/storage/modelcache.go`:
- Around line 177-198: Update the backend dispatch in the reconcile flow to
switch on selectedBackend, preserving the empty-value default to
HelmCacheBackendNVMesh. Keep explicit handlers for HelmCacheBackendSharedFS,
HelmCacheBackendSamba, and HelmCacheBackendNVMesh, and return a terminal
validation error for any unsupported value such as ephemeral instead of calling
doModelCacheNVMesh.
- Around line 473-484: The modelcache reconcile flow currently recreates the
Samba writer PV after initialization. In modelcache.go, guard the writer-PV
setup around ensureCreated and newSambaModelCachePV so it runs only while the
backing cache is unpopulated and initialization is required. In
modelcache_test.go, update the scenario covering reader creation, readiness, and
a subsequent reconcile to assert the writer PV remains absent.
- Around line 332-369: Update the reader-PVC creation flow around
resolveSharedFSStrategy to bind readers to the writer’s already-bound PV or
underlying shared-volume identity instead of dynamically provisioning a separate
claim; preserve the selected access-mode behavior where compatible. In
src/compute-plane-services/nvca/pkg/storage/modelcache.go lines 332-369, derive
the reader from the writer volume/share identity. In
src/compute-plane-services/nvca/pkg/storage/modelcache_test.go lines 929-944,
assert matching CSI volume identity or actual shared data visibility rather than
only access mode and PVC existence.
- Around line 304-321: The StorageCreating and StorageReady branches in the
shared-FS phase switch must require the durable populated marker before creating
or accepting the reader PVC. When the marker is absent, follow the existing
Samba/NVMesh behavior to fail or rebuild through the initialization path, and
prevent StorageReady from being reported for an unpopulated cache.
- Around line 1230-1238: The touchCacheReferenced method patches the PVC on
every reconcile, causing watch-triggered fan-out loops. Throttle updates to
primaryPVLastReferencedAnnotationKey using a coarse elapsed-time check against
the existing annotation, returning without patching when the timestamp is still
recent; preserve the current annotation initialization, timestamp format, and
Patch behavior when the interval has elapsed.
In `@src/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook.go`:
- Around line 278-295: Validate reserved ephemeral-cache resources in the
webhook before accepting the pod spec: update the volume handling around
hasVolumeNamed and the init-container handling around hasContainerNamed to
reject or replace same-name resources that do not match the expected emptyDir
volumes and initializer specification. Ensure the real model-cache-init
container is never silently skipped, and preserve mod tracking when reconciling
resources.
---
Outside diff comments:
In `@src/compute-plane-services/nvca/internal/metrics/METRICS.md`:
- Around line 566-579: Remove Markdown bold markers from the changed metrics
documentation, including labels such as Type, Labels, Usage, and Tracing in the
model cache metric section and the additionally affected lines. Preserve the
text and structure while rendering these labels as plain text or regular
headings.
In `@src/compute-plane-services/nvca/pkg/storage/controller_modelcache.go`:
- Around line 101-112: The controller registration flow in the builder chain
currently logs the same error that it returns. Update the code around Complete
in buildControllerModelCache to return the error wrapped with its context, and
remove the error-bearing log; optionally retain a success-only log after
registration completes.
In `@src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go`:
- Around line 170-178: Guard cleanupInitModelCache against a nil ModelCacheSpec
by validating it before dereferencing CacheHandle, or pass an already validated
handle into the cleanup flow. Update doModelCache and doModelCacheRouted tests
in src/compute-plane-services/nvca/pkg/storage/modelcache_test.go:1215-1237 to
verify nil input returns a terminal error without panicking; the cleanup change
is in src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go:170-178.
---
Minor comments:
In @.github/workflows/codeql.yml:
- Around line 6-17: Update the CodeQL workflow path filters for both push and
pull_request triggers, removing the broad src/** entry and retaining only
compiled Go/Rust source patterns, their go.mod/go.sum and Cargo.toml/Cargo.lock
manifests, and the workflow file itself.
In `@src/compute-plane-services/nvca/internal/metrics/metrics_test.go`:
- Around line 1567-1582: Extend the “ModelCache backend counters initialized to
zero” test to include the ModelCacheBackends gauge, and update the result
initialization test to assert zero values for every backend label in
ModelCacheBackends. Ensure the assertions cover all newly initialized
backend-labelled series and align with the zero-value initialization in the
metrics implementation.
In `@src/compute-plane-services/nvca/internal/metrics/metrics.go`:
- Around line 74-79: Add Godoc comments to each newly exported metric-name
constant in the metrics constants block, including
ModelCacheResultTotalMetricName, ModelCacheBackendsMetricName,
ModelCacheBackendSelectedTotalMetricName, ModelCachePopulateTotalMetricName,
ModelCacheReuseTotalMetricName, and ModelCacheReclaimedTotalMetricName, plus
BackendLabel. Ensure every comment begins with the exact exported symbol name
and briefly describes its purpose.
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.go`:
- Around line 73-77: Update the Backend field comment to document every accepted
value, adding "samba" alongside "nvmesh" and "sharedfs". Explicitly state that
"ephemeral" is not valid for a StorageRequest, while preserving the existing
empty-value backward-compatibility behavior.
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.go`:
- Around line 73-78: Update the Backend field comment to document "samba" as a
distinct valid backend alongside "nvmesh" and "sharedfs", and clarify the
meaning of each value without implying that Samba is represented by "sharedfs".
Preserve the existing empty-value backward-compatibility behavior.
In `@src/compute-plane-services/nvca/pkg/nvca/agent_manager.go`:
- Around line 122-125: Update the structured registration logs in the
controller-registration flow, including the occurrence around the
`Info("Registering storage controllers")` call and the related log at the other
referenced location, to include the established cluster context field using
`a.ClusterName`. Preserve the existing controller and caching fields while
ensuring each startup log has the required cluster attribution.
In `@src/compute-plane-services/nvca/pkg/storage/modelcache_test.go`:
- Around line 986-993: The comment above TestReconcile_ModelCacheSamba
incorrectly describes the durable marker and writer PV lifecycle. Update it to
state that the backing PVC label is the durable marker and that the writer PV is
deleted after population, while preserving the remaining Samba lifecycle and
cross-namespace behavior details.
---
Nitpick comments:
In @.github/workflows/codeql.yml:
- Around line 58-62: Update the Checkout repository step’s actions/checkout
reference from the mutable v4 tag to a verified, full-length commit SHA,
preserving the existing persist-credentials setting.
In
`@src/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.go`:
- Around line 30-57: Convert the four scenarios in TestCacheLaunchRequested into
a table-driven test, preserving the existing inputs and expected results. Also
table-drive the validation cases in
src/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.go lines
53-63 for missing image and missing handle, and the caching-disabled/enabled
expectations in src/compute-plane-services/nvca/pkg/nvca/agent_manager_test.go
lines 28-45; use each test’s existing functions and assertions.
In `@src/compute-plane-services/nvca/pkg/storage/types.go`:
- Around line 59-67: Add Godoc comments immediately before each exported
constant, beginning with its identifier:
WebhookEphemeralModelCacheInitImageAnnotationKey,
EphemeralModelCacheInitContainerName, EphemeralModelCacheConfigDataVolumeName,
and EphemeralModelCacheConfigSharedMountPath. Preserve the existing descriptive
details while making each comment explicitly document the corresponding
constant.
🪄 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: b336081d-55aa-46e3-becf-050b93e41e66
📒 Files selected for processing (61)
.coderabbit.yaml.github/workflows/codeql.ymldocs/dev/sdd-central-model-cache-service.mdmigrations/cassandra/keyspaces/event_ledger/01_init_keyspace.up.sqlmigrations/cassandra/keyspaces/event_ledger/02_init_roles.up.sqlmigrations/cassandra/keyspaces/event_ledger/03_init_tables.up.sqlsrc/compute-plane-services/byoo-otel-collector/CLAUDE.mdsrc/compute-plane-services/ess-agent/CLAUDE.mdsrc/compute-plane-services/image-credential-helper/CLAUDE.mdsrc/compute-plane-services/nvca/internal/metrics/METRICS.mdsrc/compute-plane-services/nvca/internal/metrics/metrics.gosrc/compute-plane-services/nvca/internal/metrics/metrics_test.gosrc/compute-plane-services/nvca/internal/metrics/modelcachetypes/types.gosrc/compute-plane-services/nvca/internal/miniservice/BUILD.bazelsrc/compute-plane-services/nvca/internal/miniservice/metadata_configmap.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_update_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/conversion.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/conversion_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag.gosrc/compute-plane-services/nvca/pkg/nvca/BUILD.bazelsrc/compute-plane-services/nvca/pkg/nvca/agent_manager.gosrc/compute-plane-services/nvca/pkg/nvca/agent_manager_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_modelcache.gosrc/compute-plane-services/nvca/pkg/storage/BUILD.bazelsrc/compute-plane-services/nvca/pkg/storage/cachebackend.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/BUILD.bazelsrc/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/configmap.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.gosrc/compute-plane-services/nvca/pkg/storage/controller_modelcache.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.gosrc/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.gosrc/compute-plane-services/nvca/pkg/storage/modelcache_test.gosrc/compute-plane-services/nvca/pkg/storage/reconcile.gosrc/compute-plane-services/nvca/pkg/storage/storagerequest.gosrc/compute-plane-services/nvca/pkg/storage/storagerequest_test.gosrc/compute-plane-services/nvca/pkg/storage/types.gosrc/compute-plane-services/nvca/pkg/types/BUILD.bazelsrc/compute-plane-services/nvca/pkg/types/miniservice_types.gosrc/compute-plane-services/nvca/pkg/types/modelcache_types.gosrc/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook.gosrc/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook_test.gosrc/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.gosrc/control-plane-services/function-autoscaler/CLAUDE.mdsrc/control-plane-services/function-autoscaler/local_env/CLAUDE.mdsrc/control-plane-services/nats-auth-callout/CLAUDE.mdsrc/invocation-plane-services/grpc-proxy/CLAUDE.mdsrc/invocation-plane-services/http-invocation/CLAUDE.mdsrc/invocation-plane-services/llm-api-gateway/CLAUDE.mdsrc/invocation-plane-services/ratelimiter/CLAUDE.mdtools/CLAUDE.md
💤 Files with no reviewable changes (1)
- src/compute-plane-services/nvca/pkg/featureflag/featureflag.go
balajinvda
force-pushed
the
nvca-model-cache-backends-gh
branch
from
July 17, 2026 19:23
0ccbdec to
ba4f845
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go (2)
143-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the established Go application logger.
This introduces controller-runtime
logflogging rather than the requiredlogrusapplication logger. Preserve the same structured fields when switching.As per coding guidelines, use
logrusfor application logging.🤖 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 `@src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go` around lines 143 - 144, Replace the controller-runtime logf logger created in the cache backend initialization with the established logrus application logger, preserving the backend, namespace, and cacheHandle structured fields and using the existing project logging conventions.Source: Coding guidelines
58-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported Secret-name constants.
Lines 60 and 62 have only suppression directives, so generated Go documentation lacks descriptions for these public symbols.
Proposed fix
- //nolint:gosec + // SambaModelCacheReadWriteSecretName is the shared RW SMB credentials Secret. + //nolint:gosec SambaModelCacheReadWriteSecretName = "nvcf-helm-cache-smb-rw-creds" - //nolint:gosec + // SambaModelCacheReadOnlySecretName is the shared RO SMB credentials Secret. + //nolint:gosec SambaModelCacheReadOnlySecretName = "nvcf-helm-cache-smb-ro-creds"As per coding guidelines, add Godoc comments to all exported Go symbols.
🤖 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 `@src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go` around lines 58 - 62, Add Godoc comments describing the purpose of the exported constants SambaModelCacheReadWriteSecretName and SambaModelCacheReadOnlySecretName, placing each comment directly above its corresponding //nolint:gosec directive while preserving the existing constant values and suppressions.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.
Inline comments:
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.go`:
- Around line 73-78: Update the ModelCacheSpec.Backend comment to document every
serialized value emitted by SelectHelmCacheBackend, including the distinct Samba
and emptyDir backends, and describe the behavior for unknown values. Preserve
the existing empty-value backward-compatibility note while ensuring the
documented values match the actual HelmCacheBackend selections exactly.
In `@src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go`:
- Around line 375-382: Configure the DeploymentSpec construction in the Samba
deployment flow to set Strategy.Type to RecreateDeploymentStrategyType. Keep the
existing replicas, selector, template, and pod specification unchanged so the
single RWO-backed deployment replaces the old pod before creating a new one.
---
Nitpick comments:
In `@src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go`:
- Around line 143-144: Replace the controller-runtime logf logger created in the
cache backend initialization with the established logrus application logger,
preserving the backend, namespace, and cacheHandle structured fields and using
the existing project logging conventions.
- Around line 58-62: Add Godoc comments describing the purpose of the exported
constants SambaModelCacheReadWriteSecretName and
SambaModelCacheReadOnlySecretName, placing each comment directly above its
corresponding //nolint:gosec directive while preserving the existing constant
values and suppressions.
🪄 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: 890a9ae2-59f7-412a-b686-991f60318e7a
📒 Files selected for processing (44)
docs/dev/sdd-central-model-cache-service.mdsrc/compute-plane-services/nvca/internal/metrics/METRICS.mdsrc/compute-plane-services/nvca/internal/metrics/metrics.gosrc/compute-plane-services/nvca/internal/metrics/metrics_test.gosrc/compute-plane-services/nvca/internal/metrics/modelcachetypes/types.gosrc/compute-plane-services/nvca/internal/miniservice/BUILD.bazelsrc/compute-plane-services/nvca/internal/miniservice/metadata_configmap.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/conversion.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/conversion_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag.gosrc/compute-plane-services/nvca/pkg/nvca/BUILD.bazelsrc/compute-plane-services/nvca/pkg/nvca/agent_manager.gosrc/compute-plane-services/nvca/pkg/nvca/agent_manager_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_modelcache.gosrc/compute-plane-services/nvca/pkg/storage/BUILD.bazelsrc/compute-plane-services/nvca/pkg/storage/cachebackend.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/BUILD.bazelsrc/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/configmap.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.gosrc/compute-plane-services/nvca/pkg/storage/controller_modelcache.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.gosrc/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.gosrc/compute-plane-services/nvca/pkg/storage/modelcache_test.gosrc/compute-plane-services/nvca/pkg/storage/reconcile.gosrc/compute-plane-services/nvca/pkg/storage/storagerequest.gosrc/compute-plane-services/nvca/pkg/storage/storagerequest_test.gosrc/compute-plane-services/nvca/pkg/storage/types.gosrc/compute-plane-services/nvca/pkg/types/BUILD.bazelsrc/compute-plane-services/nvca/pkg/types/miniservice_types.gosrc/compute-plane-services/nvca/pkg/types/modelcache_types.gosrc/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook.gosrc/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook_test.gosrc/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go
💤 Files with no reviewable changes (1)
- src/compute-plane-services/nvca/pkg/featureflag/featureflag.go
🚧 Files skipped from review as they are similar to previous changes (35)
- src/compute-plane-services/nvca/internal/miniservice/reconcile_modelcache_test.go
- src/compute-plane-services/nvca/pkg/types/BUILD.bazel
- src/compute-plane-services/nvca/pkg/types/modelcache_types.go
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/BUILD.bazel
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/conversion.go
- src/compute-plane-services/nvca/pkg/storage/controller_modelcache.go
- src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go
- src/compute-plane-services/nvca/pkg/storage/types.go
- src/compute-plane-services/nvca/pkg/storage/storagerequest.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go
- src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel
- src/compute-plane-services/nvca/pkg/nvca/agent_manager_test.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.go
- src/compute-plane-services/nvca/pkg/nvca/BUILD.bazel
- src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go
- src/compute-plane-services/nvca/internal/miniservice/metadata_configmap.go
- src/compute-plane-services/nvca/pkg/nvca/agent_manager.go
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/configmap.go
- src/compute-plane-services/nvca/pkg/storage/cachebackend.go
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_modelcache.go
- src/compute-plane-services/nvca/pkg/storage/BUILD.bazel
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.go
- src/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook_test.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.go
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.go
- src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go
- docs/dev/sdd-central-model-cache-service.md
- src/compute-plane-services/nvca/internal/metrics/metrics.go
- src/compute-plane-services/nvca/pkg/webhook/helm_storage_webhook.go
- src/compute-plane-services/nvca/pkg/storage/modelcache_test.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile.go
- src/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.go
- src/compute-plane-services/nvca/internal/metrics/metrics_test.go
- src/compute-plane-services/nvca/pkg/storage/modelcache.go
- select the cache backend only when the launch requests caching, and gate durable StorageRequest creation on the same predicate as the ephemeral path - reject unknown model cache backend values with a terminal validation error instead of routing them to NVMesh - create the Samba writer PV only while population is required, and retain it alongside the writer PVC until the volume detaches - use Recreate strategy for the per-handle Samba server deployment (single RWO backing PVC) - drop the configurable cluster domain suffix from the Samba service DNS name - delete probe leftovers before each probe run so a stale succeeded pod is never accepted as the current result - throttle last-referenced patches on the backing PVC to avoid reconcile fan-out loops - document the serialized backend values on ModelCacheSpec and the sharedfs writer PVC retention and known gaps in the SDD Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/dev/sdd-central-model-cache-service.md (1)
195-211: 📐 Maintainability & Code Quality | 🔵 TrivialRun
./tools/ci/check-docsin an environment that can installfern-api. The script also runsfern check, so it needs a writable npm install location.🤖 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 `@docs/dev/sdd-central-model-cache-service.md` around lines 195 - 211, Update the documentation’s validation instructions to state that ./tools/ci/check-docs must be run in an environment able to install fern-api, with a writable npm installation location because it also invokes fern check.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.
Inline comments:
In `@docs/dev/sdd-central-model-cache-service.md`:
- Around line 209-210: Update the ephemeral init environment configuration in
the model-cache cache service documentation to pass only an explicit allowlist
of variables consumed by the model-cache-init container, removing forwarding of
the full launch environment before production enablement.
- Around line 197-205: The backend selection logic must not choose sharedfs
solely from successful bindability or access-mode validation. Update the
capability probe and selection flow to verify cross-claim data sharing with a
write-through-one-claim/read-through-another test; select sharedfs only when
sharing is proven, otherwise fall back to Samba or ephemeral.
- Around line 206-208: Update the per-handle Samba infrastructure behavior
described in the create-once section to prevent cache-size mismatches on reused
handles: either enforce and reject any requested cache size that differs from
the existing handle’s configured size, or reconcile the change by expanding or
reprovisioning the backing PVC before reuse. Ensure later requests cannot
silently reuse an undersized volume.
---
Nitpick comments:
In `@docs/dev/sdd-central-model-cache-service.md`:
- Around line 195-211: Update the documentation’s validation instructions to
state that ./tools/ci/check-docs must be run in an environment able to install
fern-api, with a writable npm installation location because it also invokes fern
check.
🪄 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: 9b46ce81-84c3-424d-9a8b-89bea8082b2d
📒 Files selected for processing (12)
docs/dev/sdd-central-model-cache-service.mdsrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba.gosrc/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.gosrc/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.gosrc/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go
🚧 Files skipped from review as they are similar to previous changes (11)
- src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1/storage_types.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1/cache_types.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests.go
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/cacheprobe_test.go
- src/compute-plane-services/nvca/pkg/storage/cachebackend_samba.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile.go
- src/compute-plane-services/nvca/pkg/storage/cacheprobe/probe.go
- src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go
- src/compute-plane-services/nvca/pkg/storage/modelcache.go
- src/compute-plane-services/nvca/pkg/storage/cachebackend_samba_test.go
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.
TL;DR
Remove the HelmCachingSupport feature flag and make Helm MiniService model caching work on any cluster topology: gate all caching on CachingSupport and select the cache storage backend from the cluster's storage classes (SelectHelmCacheBackend), instead of assuming NVMesh 3.x.
Additional Details
This folds the full backend selection into NVCA (no new operator, no new CRDs); it extends NVCA's existing model-cache machinery (control namespace, single-writer lease, writer job, RW/RO PVCs, GC, mutating webhook). The design is documented in docs/dev/sdd-central-model-cache-service.md.
This supersedes the pre-migration internal merge request for the same change and folds in the review feedback from that review:
At runtime, when the Samba backend is selected NVCA creates a credentials Secret, a durable nvcf-sc data PVC, a Samba Deployment and Service, and the nvcf-miniservice-sc StorageClass in the model cache control namespace (nvca-modelcache-init).
For the Reviewer
Suggested focus: pkg/storage/cachebackend.go (backend selection), pkg/storage/modelcache.go (per-backend populate paths and the shared-FS partial cleanup), pkg/storage/cacheprobe/ (CSI access-mode probe), internal/miniservice/reconcile.go plus pkg/webhook/helm_storage_webhook.go (ephemeral fallback via miniservice metadata).
For QA
Issues
Closes #226
Checklist
Summary by CodeRabbit
New Features
ModelCacheSpec.backendto select a model-cache storage backend (defaults tonvmesh; invalid values fail validation).Bug Fixes / Improvements
Observability
backendlabel and added new backend-specific counters/gauges.Documentation / Tests