From 30575e21539f4b5660b0d4bff7ec164d5fc4c0d5 Mon Sep 17 00:00:00 2001 From: Anand Parthasarathi Date: Fri, 28 Aug 2026 18:41:14 +0530 Subject: [PATCH] fix(nvca): default NVMesh model cache volumes to nvcf-sc-30, not nvcf-sc doModelCacheNVMesh only runs once SelectHelmCacheBackend has confirmed NVMeshStorageClassName ("nvcf-sc-30") exists in the cluster, but the unconfigured storage class default it applied to the PVC was the Samba-oriented DefaultModelCacheStorageClassName ("nvcf-sc"). Both classes share the same NVMesh CSI provisioner, so the volume silently landed on the wrong tier instead of failing loudly. Add nvmeshModelCacheStorageClassName, used only by NewReconciler's one-time resolution of modelCacheStorageClass (consumed exclusively by doModelCacheNVMesh), defaulting to NVMeshStorageClassName. Split off a new sambaModelCacheStorageClass field, resolved from the same override via the original ModelCacheStorageClassName default, so doModelCacheSamba's backing PVC keeps landing on nvcf-sc regardless of the NVMesh-side default change. --- .../nvca/pkg/storage/cachebackend.go | 16 ++++ .../nvca/pkg/storage/cachebackend_test.go | 12 +++ .../nvca/pkg/storage/modelcache.go | 4 +- .../nvca/pkg/storage/modelcache_test.go | 84 +++++++++++++++++-- .../nvca/pkg/storage/reconcile.go | 40 ++++++--- 5 files changed, 134 insertions(+), 22 deletions(-) diff --git a/src/compute-plane-services/nvca/pkg/storage/cachebackend.go b/src/compute-plane-services/nvca/pkg/storage/cachebackend.go index e4f6c4b9d..81c3054b5 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend.go @@ -78,6 +78,22 @@ func ModelCacheStorageClassName(override string) string { return DefaultModelCacheStorageClassName } +// nvmeshModelCacheStorageClassName resolves the storage class NVMesh model +// cache volumes are provisioned on: the override when set, otherwise +// NVMeshStorageClassName. NVCA only reaches the NVMesh reconciliation path +// (doModelCacheNVMesh) once SelectHelmCacheBackend has already confirmed +// NVMeshStorageClassName exists in the cluster, so defaulting to it here +// keeps the volume's storage class aligned with the backend that was +// actually selected, instead of falling back to the Samba-oriented +// DefaultModelCacheStorageClassName. +func nvmeshModelCacheStorageClassName(override string) string { + if override != "" { + return override + } + + return NVMeshStorageClassName +} + // SelectHelmCacheBackend resolves the Helm model-cache storage backend. All // caching is gated on CachingSupport; the mechanism is then chosen by which // storage class the cluster provides, falling back to Samba (when diff --git a/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go b/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go index f2ccff933..d3d96a1f7 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go @@ -172,6 +172,18 @@ func TestModelCacheStorageClassNameResolution(t *testing.T) { assert.Equal(t, "custom-block-sc", ModelCacheStorageClassName("custom-block-sc")) } +// TestNvmeshModelCacheStorageClassNameResolution pins the NVMesh path's +// unconfigured default to NVMeshStorageClassName ("nvcf-sc-30"), not +// DefaultModelCacheStorageClassName ("nvcf-sc"). Regression test for the bug +// where doModelCacheNVMesh provisioned volumes on nvcf-sc even when +// SelectHelmCacheBackend had already confirmed nvcf-sc-30 (NVMesh) was +// available and selected the NVMesh backend. +func TestNvmeshModelCacheStorageClassNameResolution(t *testing.T) { + assert.Equal(t, NVMeshStorageClassName, nvmeshModelCacheStorageClassName("")) + assert.Equal(t, "custom-nvmesh-sc", nvmeshModelCacheStorageClassName("custom-nvmesh-sc")) + assert.NotEqual(t, DefaultModelCacheStorageClassName, nvmeshModelCacheStorageClassName("")) +} + // TestSelectHelmCacheBackend_SambaClassLookupError proves a failed lookup of the // Samba backing class surfaces as an error rather than silently degrading to the // ephemeral cache: a transient API error must be retried, not treated as an diff --git a/src/compute-plane-services/nvca/pkg/storage/modelcache.go b/src/compute-plane-services/nvca/pkg/storage/modelcache.go index 1e44c88cc..26034f2a7 100644 --- a/src/compute-plane-services/nvca/pkg/storage/modelcache.go +++ b/src/compute-plane-services/nvca/pkg/storage/modelcache.go @@ -457,7 +457,7 @@ func (r *Reconciler) doModelCacheSamba(ctx context.Context, func(ctx context.Context) error { var e error infraState, e = EnsureSambaModelCacheInfra(ctx, r.Client, cacheHandle, - r.cfg.Agent.SharedStorage.Server.Image, r.modelCacheStorageClass, smbResources, capacity) + r.cfg.Agent.SharedStorage.Server.Image, r.sambaModelCacheStorageClass, smbResources, capacity) return e }, oteltrace.WithAttributes(otelattr.String("nvcf.modelcache.handle", cacheHandle)), @@ -488,7 +488,7 @@ func (r *Reconciler) doModelCacheSamba(ctx context.Context, return reconcile.Result{}, r.terminalErrorWithMetricErr(modelcachetypes.ReasonSambaInfraTimeout, fmt.Errorf("samba model cache server for handle %s still unavailable after %s, "+ "its backing PVC on storage class %s may be unbindable", - cacheHandle, waited.Round(time.Second), r.modelCacheStorageClass)) + cacheHandle, waited.Round(time.Second), r.sambaModelCacheStorageClass)) } log.V(1).Info("Samba model cache server not ready, requeuing", "waited", waited.Round(time.Second)) return reconcile.Result{RequeueAfter: defaultRequeueDelay}, nil diff --git a/src/compute-plane-services/nvca/pkg/storage/modelcache_test.go b/src/compute-plane-services/nvca/pkg/storage/modelcache_test.go index f508a00bd..b2e4b9ff4 100644 --- a/src/compute-plane-services/nvca/pkg/storage/modelcache_test.go +++ b/src/compute-plane-services/nvca/pkg/storage/modelcache_test.go @@ -566,12 +566,17 @@ func TestGetPrimaryPV(t *testing.T) { } // newMountOptionDefaultsObjects builds the storage class and ConfigMap that the -// reconciler consults to decide which mount option defaults apply. +// reconciler consults to decide which mount option defaults apply. The +// StorageClass is named NVMeshStorageClassName, not +// DefaultModelCacheStorageClassName: newMountOptionsReconciler builds its +// Reconciler with no storage class override, so it resolves the NVMesh +// default (see TestModelCacheStorageClassResolvedOnce) and that is the class +// name modelCacheProvisionerName looks up. func newMountOptionDefaultsObjects(provisioner string, cmData map[string]string) []client.Object { objs := []client.Object{} if provisioner != "" { objs = append(objs, &storagev1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{Name: DefaultModelCacheStorageClassName}, + ObjectMeta: metav1.ObjectMeta{Name: NVMeshStorageClassName}, Provisioner: provisioner, }) } @@ -746,9 +751,15 @@ func TestResolveCacheMountOptions_ConfigMapEditTakesEffect(t *testing.T) { // TestModelCacheStorageClassResolvedOnce covers the storage class NewReconciler // resolves for the life of the reconciler: the option override first (tests), -// then the agent config value, then the default. The config value is the single -// production source, read here and by model cache backend selection, so the -// class that is checked cannot drift from the class volumes are created on. +// then the agent config value, then the NVMesh default. The config value is +// the single production source, read here and by model cache backend +// selection, so the class that is checked cannot drift from the class volumes +// are created on. This reconciler only provisions on the NVMesh path +// (doModelCacheNVMesh), so the unconfigured default must be +// NVMeshStorageClassName ("nvcf-sc-30"), not the Samba-oriented +// DefaultModelCacheStorageClassName ("nvcf-sc") — see +// TestNvmeshModelCacheStorageClassNameResolution for the regression this +// guards against. func TestModelCacheStorageClassResolvedOnce(t *testing.T) { tests := []struct { name string @@ -757,8 +768,8 @@ func TestModelCacheStorageClassResolvedOnce(t *testing.T) { want string }{ { - name: "unset falls back to the default", - want: DefaultModelCacheStorageClassName, + name: "unset falls back to the NVMesh default", + want: NVMeshStorageClassName, }, { name: "option override wins", @@ -788,6 +799,47 @@ func TestModelCacheStorageClassResolvedOnce(t *testing.T) { } } +// TestSambaModelCacheStorageClassResolvedOnce covers the Samba-side sibling of +// modelCacheStorageClass: it must resolve to DefaultModelCacheStorageClassName +// ("nvcf-sc") when unconfigured, independent of modelCacheStorageClass having +// been changed to default to NVMeshStorageClassName ("nvcf-sc-30"). Regression +// test: doModelCacheSamba (modelcache.go) must read +// sambaModelCacheStorageClass, not modelCacheStorageClass, or the Samba +// backend's backing PVC would try to land on the NVMesh class whenever no +// explicit override is configured. +func TestSambaModelCacheStorageClassResolvedOnce(t *testing.T) { + tests := []struct { + name string + override string + agentCfg string + want string + }{ + { + name: "unset falls back to the Samba default", + want: DefaultModelCacheStorageClassName, + }, + { + name: "option override wins", + override: "custom-sc", + want: "custom-sc", + }, + { + name: "agent config value is used when there is no override", + agentCfg: "cfg-sc", + want: "cfg-sc", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := newModelCacheStorageClassReconciler(t, tt.agentCfg, tt.override) + assert.Equal(t, tt.want, r.sambaModelCacheStorageClass) + assert.NotEqual(t, NVMeshStorageClassName, r.sambaModelCacheStorageClass, + "Samba class must never silently resolve to the NVMesh class") + }) + } +} + // newModelCacheStorageClassReconciler builds a Reconciler the way production // does, so the storage class resolution under test is the real one. func newModelCacheStorageClassReconciler(t *testing.T, agentCfg, override string) *Reconciler { @@ -819,10 +871,10 @@ func TestApplyModelCacheStorageClass(t *testing.T) { want string }{ { - name: "spec value is replaced by the default", + name: "spec value is replaced by the NVMesh default", configured: "", specSC: ptr("some-other-sc"), - want: DefaultModelCacheStorageClassName, + want: NVMeshStorageClassName, }, { name: "spec value is replaced by the configured override", @@ -1572,6 +1624,20 @@ func TestReconcile_ModelCacheSamba(t *testing.T) { } require.NoError(t, c.Get(ctx, client.ObjectKey{Name: "samba-rw-pv-" + cacheHandle}, &corev1.PersistentVolume{})) + // The Samba server's own backing data PVC must stay on the Samba-oriented + // default (nvcf-sc), not the NVMesh default (nvcf-sc-30), even though this + // reconciler is built with no explicit override (nvcaCfg above). Regression + // test: doModelCacheSamba used to read the NVMesh-resolved + // r.modelCacheStorageClass for this PVC too, which would have pointed the + // Samba backing storage at nvcf-sc-30 whenever it was unconfigured. + dataPVC := &corev1.PersistentVolumeClaim{} + require.NoError(t, c.Get(ctx, + client.ObjectKey{Name: SambaModelCacheBackingPVCName(cacheHandle), Namespace: ModelCacheInitNamespace}, dataPVC)) + if assert.NotNil(t, dataPVC.Spec.StorageClassName) { + assert.Equal(t, DefaultModelCacheStorageClassName, *dataPVC.Spec.StorageClassName, + "Samba backing data PVC must default to nvcf-sc, not the NVMesh class") + } + // Drive the writer pod to running so the request moves to InitRunning. initJobPod := &corev1.Pod{} initJobPod.Name, initJobPod.Namespace = initJob.Spec.Template.Name+"-foobar", initJob.Namespace diff --git a/src/compute-plane-services/nvca/pkg/storage/reconcile.go b/src/compute-plane-services/nvca/pkg/storage/reconcile.go index 45309cae5..9cd44ffba 100644 --- a/src/compute-plane-services/nvca/pkg/storage/reconcile.go +++ b/src/compute-plane-services/nvca/pkg/storage/reconcile.go @@ -201,7 +201,19 @@ func NewReconciler( // Resolve the model cache storage class once: it cannot change for the life // of the reconciler, so every later use reads the field directly. - reconciler.modelCacheStorageClass = ModelCacheStorageClassName(reconciler.modelCacheStorageClass) + // modelCacheStorageClassOverride is the same unresolved value (option, then + // Agent.ModelCache) used to resolve both the NVMesh and Samba class names + // below, and the value model cache backend selection checks against + // independently. It is deliberately resolved twice, with different + // defaults, because doModelCacheNVMesh and doModelCacheSamba are only ever + // reached for their respective backend and need different unconfigured + // defaults (NVMeshStorageClassName vs. the Samba-oriented + // DefaultModelCacheStorageClassName) — a single shared resolved value would + // hand doModelCacheSamba the NVMesh class (or vice versa) whenever the + // operator has not set an explicit override. + modelCacheStorageClassOverride := reconciler.modelCacheStorageClass + reconciler.modelCacheStorageClass = nvmeshModelCacheStorageClassName(modelCacheStorageClassOverride) + reconciler.sambaModelCacheStorageClass = ModelCacheStorageClassName(modelCacheStorageClassOverride) return reconciler } @@ -220,20 +232,26 @@ type Reconciler struct { k8sTimeConfig *k8sutil.TimeConfig csiVolumeMountOptions []string - // modelCacheStorageClass is the storage class model cache volumes are - // provisioned on and whose provisioner selects the mount option defaults. - // NewReconciler resolves it once (option, then Agent.ModelCache, then the - // default), so it is always a concrete class name. NVCA owns this choice - // rather than taking it from the request spec, so every model cache volume - // in a cluster lands on the same class; model cache backend selection reads - // the same config value before choosing a backend that needs the class. + // modelCacheStorageClass is the storage class NVMesh model cache volumes + // are provisioned on and whose provisioner selects the mount option + // defaults. NewReconciler resolves it once (option, then Agent.ModelCache, + // then NVMeshStorageClassName), so it is always a concrete class name. + // NVCA owns this choice rather than taking it from the request spec, so + // every NVMesh model cache volume in a cluster lands on the same class. + // sambaModelCacheStorageClass is the analogous resolved class for the + // Samba backend's own backing PVC (doModelCacheSamba), resolved from the + // same unconfigured override but defaulting to + // DefaultModelCacheStorageClassName instead — see NewReconciler. Model + // cache backend selection reads the unresolved config value independently, + // before choosing a backend that needs one of these classes. // cacheMountOptionsConfigMap holds the mount option defaults per // provisioner. modelCacheProvisioner is the one time init: a StorageClass // provisioner is immutable, so it is read once and kept. The ConfigMap is // read on each use so operator edits take effect. - modelCacheStorageClass string - cacheMountOptionsConfigMap string - modelCacheProvisioner atomic.Pointer[string] + modelCacheStorageClass string + sambaModelCacheStorageClass string + cacheMountOptionsConfigMap string + modelCacheProvisioner atomic.Pointer[string] tracer oteltrace.Tracer nowFunc func() time.Time