From fb8dd420b6665dd01827652dcc8ca8ada4f6a9ae Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Fri, 28 Aug 2026 13:41:46 -0700 Subject: [PATCH 1/2] feat(nvca): gate helm model caching behind a default-off feature flag 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. Closes #1331 Co-Authored-By: Balaji Ganesan --- .../reconcile_storagerequests_test.go | 18 +++++++++++++ .../nvca/pkg/featureflag/featureflag.go | 9 +++++-- .../nvca/pkg/featureflag/featureflag_test.go | 27 +++++++++++++++++++ .../nvca/pkg/storage/cachebackend.go | 18 +++++++------ .../nvca/pkg/storage/cachebackend_test.go | 21 ++++++++++++++- 5 files changed, 82 insertions(+), 11 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 9496ec555..7bac81f6c 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..bd54f58a9 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,33 @@ func TestMaintenanceModeFeatureFlags(t *testing.T) { assert.Equal(t, CordonAndDrainMaintenance, flag) } +func TestHelmModelCachingFeatureFlag(t *testing.T) { + // Helm model caching must be opted into: the declared default is off. + require.NotNil(t, HelmModelCaching.defaultValue) + assert.False(t, *HelmModelCaching.defaultValue, "HelmModelCaching must default to off") + + _ = parseFlags(fmt.Sprintf("-%s,-%s", CachingSupport.Key, HelmModelCaching.Key)) + assert.False(t, HelmModelCaching.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. + _ = 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) + + // Reset for other tests. + _ = parseFlags(fmt.Sprintf("-%s,-%s", CachingSupport.Key, HelmModelCaching.Key)) +} + 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 e4f6c4b9d..1dcdde29e 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend.go @@ -31,14 +31,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). @@ -79,10 +80,10 @@ func ModelCacheStorageClassName(override string) string { } // 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 the block class Samba needs exists) 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 the block class +// Samba needs exists) 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 @@ -98,7 +99,8 @@ func SelectHelmCacheBackend( ff featureflag.Fetcher, modelCacheStorageClass string, ) (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 f2ccff933..9c9dc0a1f 100644 --- a/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go +++ b/src/compute-plane-services/nvca/pkg/storage/cachebackend_test.go @@ -51,9 +51,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, } @@ -72,6 +76,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, @@ -192,6 +210,7 @@ func TestSelectHelmCacheBackend_SambaClassLookupError(t *testing.T) { }).Build() ff := &featureflagmock.Fetcher{EnabledFFs: []*featureflag.FeatureFlag{ featureflag.CachingSupport, + featureflag.HelmModelCaching, &featureflag.HelmSharedStorage.FeatureFlag, }} From 6ec85a0e2d56cd5dc13e22b0ea5470243dfc7119 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Fri, 28 Aug 2026 14:20:04 -0700 Subject: [PATCH 2/2] test(nvca): harden the HelmModelCaching flag test against global state Enabled() prefers the enabled override over defaultValue, so the parent-gate assertion ran with the child explicitly disabled and only proved that a disabled flag stays disabled. Clear the override before the default and parent-gate assertions so both cover an unset flag, and assert the declared default actually reaches Enabled(). Restore the mutated globals with t.Cleanup instead of a trailing parseFlags call, which is skipped when a require assertion aborts the test. Relates to #1331 Co-Authored-By: Balaji Ganesan --- .../nvca/pkg/featureflag/featureflag_test.go | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) 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 bd54f58a9..2d6f37225 100644 --- a/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go +++ b/src/compute-plane-services/nvca/pkg/featureflag/featureflag_test.go @@ -104,12 +104,19 @@ func TestMaintenanceModeFeatureFlags(t *testing.T) { } func TestHelmModelCachingFeatureFlag(t *testing.T) { - // Helm model caching must be opted into: the declared default is off. + // 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") - - _ = parseFlags(fmt.Sprintf("-%s,-%s", CachingSupport.Key, HelmModelCaching.Key)) - assert.False(t, HelmModelCaching.Enabled()) + assert.False(t, HelmModelCaching.Enabled(), "the declared default must reach Enabled()") _ = parseFlags(fmt.Sprintf("+%s", HelmModelCaching.Key)) assert.True(t, HelmModelCaching.Enabled()) @@ -117,7 +124,10 @@ func TestHelmModelCachingFeatureFlag(t *testing.T) { _ = 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. + // 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()) @@ -125,9 +135,6 @@ func TestHelmModelCachingFeatureFlag(t *testing.T) { flag, err := Get(HelmModelCaching.Key) require.NoError(t, err) assert.Equal(t, HelmModelCaching, flag) - - // Reset for other tests. - _ = parseFlags(fmt.Sprintf("-%s,-%s", CachingSupport.Key, HelmModelCaching.Key)) } func TestHelmAllowCPUNodesFeatureFlag(t *testing.T) {