From 3abd3932de865ae27bcce8d080248bb93253e3aa Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Fri, 28 Aug 2026 15:42:32 -0700 Subject: [PATCH] feat(nvca): gate helm model caching behind a default-off feature flag (3.2) Backport of #1332 to the nvca 3.2 release line. Helm model caching was gated only by the broad CachingSupport flag, so a cluster could not enable caching support while keeping the Helm model-cache path off, and the path could not be turned off on its own. Add a HelmModelCaching feature flag, default off, as a sub-gate of CachingSupport. SelectHelmCacheBackend now returns HelmCacheBackendNone unless both are enabled, which covers the durable path (no ModelCacheRequest in makeStorageRequests) and the ephemeral path (no per-pod model-cache-init container injection), since both branch on the backend it selects. Default off means existing clusters see no behavior change until the flag is added to their feature gate list. Re-applied by hand rather than cherry-picked: this branch predates the modelCacheStorageClass parameter and the Samba block-class check on main, so SelectHelmCacheBackend has a different signature here and main's TestSelectHelmCacheBackend_SambaClassLookupError does not exist. Relates to #1331 Co-Authored-By: Balaji Ganesan --- .../reconcile_storagerequests_test.go | 18 ++++++++++ .../nvca/pkg/featureflag/featureflag.go | 9 +++-- .../nvca/pkg/featureflag/featureflag_test.go | 34 +++++++++++++++++++ .../nvca/pkg/storage/cachebackend.go | 17 ++++++---- .../nvca/pkg/storage/cachebackend_test.go | 20 ++++++++++- 5 files changed, 88 insertions(+), 10 deletions(-) diff --git a/src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go b/src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go index 646b4063f..29de4835c 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go +++ b/src/compute-plane-services/nvca/internal/miniservice/reconcile_storagerequests_test.go @@ -97,6 +97,24 @@ func TestMakeStorageRequests_BackendHandling(t *testing.T) { assert.Empty(t, sts, "cacheLaunchRequested gates durable backends like the ephemeral path") }) + t.Run("none backend emits no ModelCacheRequest", func(t *testing.T) { + // SelectHelmCacheBackend returns None when CachingSupport or the + // HelmModelCaching sub-gate is off; a valid cache spec must still + // produce no ModelCacheRequest. + icmsReq := &nvcav2beta1.ICMSRequest{} + icmsReq.Spec.Action = common.FunctionCreationAction + icmsReq.Spec.CreationMsgInfo.FunctionLaunchSpecification = &function.LaunchSpecification{ + CacheLaunchSpecification: &common.CacheLaunchSpecification{ + CacheArtifacts: true, + CacheHandle: "h1", + CacheSize: 262144000, + }, + } + sts, err := r.makeStorageRequests(icmsReq, nil, job, pvc, nvcastorage.HelmCacheBackendNone) + require.NoError(t, err) + assert.Empty(t, sts) + }) + t.Run("ephemeral backend emits no ModelCacheRequest", func(t *testing.T) { icmsReq := &nvcav2beta1.ICMSRequest{} icmsReq.Spec.Action = common.FunctionCreationAction diff --git a/src/compute-plane-services/nvca/pkg/featureflag/featureflag.go b/src/compute-plane-services/nvca/pkg/featureflag/featureflag.go index 6f226c840..79a8623ea 100644 --- a/src/compute-plane-services/nvca/pkg/featureflag/featureflag.go +++ b/src/compute-plane-services/nvca/pkg/featureflag/featureflag.go @@ -34,8 +34,13 @@ var ( ) var ( - LogPosting = newFeatureFlag("LogPosting", newBool(false)) - CachingSupport = newFeatureFlag("CachingSupport", newBool(false)) + LogPosting = newFeatureFlag("LogPosting", newBool(false)) + CachingSupport = newFeatureFlag("CachingSupport", newBool(false)) + // HelmModelCaching gates model caching for Helm-based workloads. It is a + // sub-gate of CachingSupport: both must be on before a backend is selected + // in storage.SelectHelmCacheBackend. When off, no ModelCacheRequest is + // created and no ephemeral model-cache-init container is injected. + HelmModelCaching = newFeatureFlag("HelmModelCaching", newBool(false)) NVMeshEncryption = newFeatureFlag("NVMeshEncryption", newBool(false)) PeriodicInstanceStatusUpdate = newFeatureFlag("PeriodicInstanceStatusUpdate", newBool(true)) HelmRBACEnforcement = newFeatureFlag("HelmRBACEnforcement", newBool(true)) diff --git a/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go b/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go index e5a80d517..2d6f37225 100644 --- a/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go +++ b/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go @@ -103,6 +103,40 @@ func TestMaintenanceModeFeatureFlags(t *testing.T) { assert.Equal(t, CordonAndDrainMaintenance, flag) } +func TestHelmModelCachingFeatureFlag(t *testing.T) { + // parseFlags mutates package globals. Restore them via Cleanup so state + // cannot leak into other tests even if an assertion aborts this one. + origCaching, origHelm := CachingSupport.enabled, HelmModelCaching.enabled + t.Cleanup(func() { + CachingSupport.enabled, HelmModelCaching.enabled = origCaching, origHelm + }) + + // Helm model caching must be opted into. Enabled() prefers the override, so + // clear it to assert the declared default is what an unset flag reports. + HelmModelCaching.enabled = nil + require.NotNil(t, HelmModelCaching.defaultValue) + assert.False(t, *HelmModelCaching.defaultValue, "HelmModelCaching must default to off") + assert.False(t, HelmModelCaching.Enabled(), "the declared default must reach Enabled()") + + _ = parseFlags(fmt.Sprintf("+%s", HelmModelCaching.Key)) + assert.True(t, HelmModelCaching.Enabled()) + + _ = parseFlags(fmt.Sprintf("-%s", HelmModelCaching.Key)) + assert.False(t, HelmModelCaching.Enabled()) + + // CachingSupport is the parent gate, but it must not imply the Helm + // sub-gate. Clear the child override first, so this covers an unset child + // rather than one explicitly disabled just above. + HelmModelCaching.enabled = nil + _ = parseFlags(fmt.Sprintf("+%s", CachingSupport.Key)) + assert.True(t, CachingSupport.Enabled()) + assert.False(t, HelmModelCaching.Enabled()) + + flag, err := Get(HelmModelCaching.Key) + require.NoError(t, err) + assert.Equal(t, HelmModelCaching, flag) +} + func TestHelmAllowCPUNodesFeatureFlag(t *testing.T) { // Reset flags to defaults _ = parseFlags("-HelmAllowCPUNodes,-HelmResourceConstraints") diff --git a/src/compute-plane-services/nvca/pkg/storage/cachebackend.go b/src/compute-plane-services/nvca/pkg/storage/cachebackend.go index 2db6b7d5d..0201723b9 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend.go @@ -30,14 +30,15 @@ import ( ) // HelmCacheBackend identifies the storage backend selected for the Helm model -// cache. The backend is resolved from the CachingSupport gate plus the storage -// classes available in the cluster, replacing the old HelmCachingSupport flag. +// cache. The backend is resolved from the CachingSupport and HelmModelCaching +// gates plus the storage classes available in the cluster. // The type and values live in pkg/types so metrics can share them; they are // aliased here for the storage-facing API. type HelmCacheBackend = nvcatypes.HelmCacheBackend const ( - // HelmCacheBackendNone means caching is disabled (CachingSupport off). + // HelmCacheBackendNone means caching is disabled (CachingSupport or + // HelmModelCaching off). HelmCacheBackendNone = nvcatypes.HelmCacheBackendNone // HelmCacheBackendNVMesh uses NVMesh 3.x cross-namespace PV sharing // (the existing doModelCacheNVMesh path). @@ -64,9 +65,10 @@ const ( ) // 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 -// HelmSharedStorage is enabled) and finally to a per-pod ephemeral cache: +// caching is gated on CachingSupport plus the HelmModelCaching sub-gate; the +// mechanism is then chosen by which storage class the cluster provides, +// falling back to Samba (when HelmSharedStorage is enabled) and finally to a +// per-pod ephemeral cache: // // 1. nvcf-sc-30 present -> NVMesh 3.x installed -> NVMesh // 2. nvcf-miniservice-sc present -> operator shared storage -> SharedFS @@ -77,7 +79,8 @@ func SelectHelmCacheBackend( c client.Client, ff featureflag.Fetcher, ) (HelmCacheBackend, error) { - if !ff.IsFeatureFlagEnabled(featureflag.CachingSupport) { + if !ff.IsFeatureFlagEnabled(featureflag.CachingSupport) || + !ff.IsFeatureFlagEnabled(featureflag.HelmModelCaching) { return HelmCacheBackendNone, nil } 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 a3809a631..6546be934 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go @@ -47,9 +47,13 @@ func cacheBackendClient(t *testing.T, scs ...*storagev1.StorageClass) *fake.Clie } func TestSelectHelmCacheBackend(t *testing.T) { - cachingOnly := []*featureflag.FeatureFlag{featureflag.CachingSupport} + cachingOnly := []*featureflag.FeatureFlag{ + featureflag.CachingSupport, + featureflag.HelmModelCaching, + } cachingAndSamba := []*featureflag.FeatureFlag{ featureflag.CachingSupport, + featureflag.HelmModelCaching, &featureflag.HelmSharedStorage.FeatureFlag, } @@ -66,6 +70,20 @@ func TestSelectHelmCacheBackend(t *testing.T) { storageClasses: []*storagev1.StorageClass{storageClass(NVMeshStorageClassName)}, want: HelmCacheBackendNone, }, + { + name: "HelmModelCaching off -> none", + flags: []*featureflag.FeatureFlag{featureflag.CachingSupport}, + // CachingSupport on and nvcf-sc-30 present, but the Helm sub-gate + // is off: no backend is selected. + storageClasses: []*storagev1.StorageClass{storageClass(NVMeshStorageClassName)}, + want: HelmCacheBackendNone, + }, + { + name: "CachingSupport off, HelmModelCaching on -> none", + flags: []*featureflag.FeatureFlag{featureflag.HelmModelCaching}, + storageClasses: []*storagev1.StorageClass{storageClass(NVMeshStorageClassName)}, + want: HelmCacheBackendNone, + }, { name: "nvcf-sc-30 present -> nvmesh", flags: cachingOnly,