From b7052c2cb1a81a0b1a7bf8faca9335c150cbc41a Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 16 Dec 2025 12:28:47 -0800 Subject: [PATCH] Implement lazy per-namespace PVC-to-Pod caching for plugin path This commit addresses reviewer feedback on PR #9441 regarding concurrent backup caching concerns. Key changes: 1. Added lazy per-namespace caching for the CSI PVC BIA plugin path: - Added IsNamespaceBuilt() method to check if namespace is cached - Added BuildCacheForNamespace() for lazy, per-namespace cache building - Plugin builds cache incrementally as namespaces are encountered 2. Added NewVolumeHelperImplWithCache constructor for plugins: - Accepts externally-managed PVC-to-Pod cache - Follows pattern from PR #9226 (Scott Seago's design) 3. Plugin instance lifecycle clarification: - Plugin instances are unique per backup (created via newPluginManager) - Cleaned up via CleanupClients at backup completion - No mutex or backup UID tracking needed 4. Test coverage: - Added tests for IsNamespaceBuilt and BuildCacheForNamespace - Added tests for NewVolumeHelperImplWithCache constructor - Added test verifying cache usage for fs-backup determination This maintains the O(N+M) complexity improvement from issue #9179 while addressing architectural concerns about concurrent access. Signed-off-by: Shubham Pampattiwar --- internal/volumehelper/volume_policy_helper.go | 27 +++ .../volumehelper/volume_policy_helper_test.go | 181 ++++++++++++++ pkg/backup/actions/csi/pvc_action.go | 90 ++++--- pkg/backup/actions/csi/pvc_action_test.go | 132 ++--------- pkg/util/podvolume/pod_volume.go | 60 +++++ pkg/util/podvolume/pod_volume_test.go | 221 ++++++++++++++++++ 6 files changed, 573 insertions(+), 138 deletions(-) diff --git a/internal/volumehelper/volume_policy_helper.go b/internal/volumehelper/volume_policy_helper.go index 22a88a146..63115cba2 100644 --- a/internal/volumehelper/volume_policy_helper.go +++ b/internal/volumehelper/volume_policy_helper.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/pkg/errors" "github.com/sirupsen/logrus" corev1api "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -12,6 +13,7 @@ import ( crclient "sigs.k8s.io/controller-runtime/pkg/client" "github.com/vmware-tanzu/velero/internal/resourcepolicies" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/util/boolptr" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -95,6 +97,31 @@ func NewVolumeHelperImplWithNamespaces( }, nil } +// NewVolumeHelperImplWithCache creates a VolumeHelper using an externally managed PVC-to-Pod cache. +// This is used by plugins that build the cache lazily per-namespace (following the pattern from PR #9226). +// The cache can be nil, in which case PVC-to-Pod lookups will fall back to direct API calls. +func NewVolumeHelperImplWithCache( + backup velerov1api.Backup, + client crclient.Client, + logger logrus.FieldLogger, + pvcPodCache *podvolumeutil.PVCPodCache, +) (VolumeHelper, error) { + resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(backup, client, logger) + if err != nil { + return nil, errors.Wrap(err, "failed to get volume policies from backup") + } + + return &volumeHelperImpl{ + volumePolicy: resourcePolicies, + snapshotVolumes: backup.Spec.SnapshotVolumes, + logger: logger, + client: client, + defaultVolumesToFSBackup: boolptr.IsSetToTrue(backup.Spec.DefaultVolumesToFsBackup), + backupExcludePVC: boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData), + pvcPodCache: pvcPodCache, + }, nil +} + func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error) { // check if volume policy exists and also check if the object(pv/pvc) fits a volume policy criteria and see if the associated action is snapshot // if it is not snapshot then skip the code path for snapshotting the PV/PVC diff --git a/internal/volumehelper/volume_policy_helper_test.go b/internal/volumehelper/volume_policy_helper_test.go index 57c99e862..c0f93f94f 100644 --- a/internal/volumehelper/volume_policy_helper_test.go +++ b/internal/volumehelper/volume_policy_helper_test.go @@ -34,6 +34,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/kuberesource" velerotest "github.com/vmware-tanzu/velero/pkg/test" + podvolumeutil "github.com/vmware-tanzu/velero/pkg/util/podvolume" ) func TestVolumeHelperImpl_ShouldPerformSnapshot(t *testing.T) { @@ -1053,3 +1054,183 @@ func TestVolumeHelperImplWithCache_ShouldPerformFSBackup(t *testing.T) { }) } } + +// TestNewVolumeHelperImplWithCache tests the NewVolumeHelperImplWithCache constructor +// which is used by plugins that build the cache lazily per-namespace. +func TestNewVolumeHelperImplWithCache(t *testing.T) { + testCases := []struct { + name string + backup velerov1api.Backup + resourcePolicyConfigMap *corev1api.ConfigMap + pvcPodCache bool // whether to pass a cache + expectError bool + }{ + { + name: "creates VolumeHelper with nil cache", + backup: velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "velero", + }, + Spec: velerov1api.BackupSpec{ + SnapshotVolumes: ptr.To(true), + DefaultVolumesToFsBackup: ptr.To(false), + }, + }, + pvcPodCache: false, + expectError: false, + }, + { + name: "creates VolumeHelper with non-nil cache", + backup: velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "velero", + }, + Spec: velerov1api.BackupSpec{ + SnapshotVolumes: ptr.To(true), + DefaultVolumesToFsBackup: ptr.To(true), + SnapshotMoveData: ptr.To(true), + }, + }, + pvcPodCache: true, + expectError: false, + }, + { + name: "creates VolumeHelper with resource policies", + backup: velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "velero", + }, + Spec: velerov1api.BackupSpec{ + SnapshotVolumes: ptr.To(true), + ResourcePolicy: &corev1api.TypedLocalObjectReference{ + Kind: "ConfigMap", + Name: "resource-policy", + }, + }, + }, + resourcePolicyConfigMap: &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "resource-policy", + Namespace: "velero", + }, + Data: map[string]string{ + "policy": `version: v1 +volumePolicies: +- conditions: + storageClass: + - gp2-csi + action: + type: snapshot`, + }, + }, + pvcPodCache: true, + expectError: false, + }, + { + name: "fails when resource policy ConfigMap not found", + backup: velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "velero", + }, + Spec: velerov1api.BackupSpec{ + ResourcePolicy: &corev1api.TypedLocalObjectReference{ + Kind: "ConfigMap", + Name: "non-existent-policy", + }, + }, + }, + pvcPodCache: false, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var objs []runtime.Object + if tc.resourcePolicyConfigMap != nil { + objs = append(objs, tc.resourcePolicyConfigMap) + } + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...) + + var cache *podvolumeutil.PVCPodCache + if tc.pvcPodCache { + cache = podvolumeutil.NewPVCPodCache() + } + + vh, err := NewVolumeHelperImplWithCache( + tc.backup, + fakeClient, + logrus.StandardLogger(), + cache, + ) + + if tc.expectError { + require.Error(t, err) + require.Nil(t, vh) + } else { + require.NoError(t, err) + require.NotNil(t, vh) + } + }) + } +} + +// TestNewVolumeHelperImplWithCache_UsesCache verifies that the VolumeHelper created +// via NewVolumeHelperImplWithCache actually uses the provided cache for lookups. +func TestNewVolumeHelperImplWithCache_UsesCache(t *testing.T) { + // Create a pod that uses a PVC via opt-out (defaultVolumesToFsBackup=true) + pod := builder.ForPod("ns", "pod-1").Volumes( + &corev1api.Volume{ + Name: "volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-1", + }, + }, + }, + ).Result() + + pvc := &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "pvc-1", + }, + } + + pv := builder.ForPersistentVolume("example-pv").StorageClass("gp2-csi").ClaimRef("ns", "pvc-1").Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, pvc, pv, pod) + + // Build cache for the namespace + cache := podvolumeutil.NewPVCPodCache() + err := cache.BuildCacheForNamespace(t.Context(), "ns", fakeClient) + require.NoError(t, err) + + backup := velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "velero", + }, + Spec: velerov1api.BackupSpec{ + SnapshotVolumes: ptr.To(true), + DefaultVolumesToFsBackup: ptr.To(true), // opt-out mode + }, + } + + vh, err := NewVolumeHelperImplWithCache(backup, fakeClient, logrus.StandardLogger(), cache) + require.NoError(t, err) + + // Convert PV to unstructured + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pv) + require.NoError(t, err) + + // ShouldPerformSnapshot should return false because the volume is selected for fs-backup + // This relies on the cache to find the pod using the PVC + shouldSnapshot, err := vh.ShouldPerformSnapshot(&unstructured.Unstructured{Object: obj}, kuberesource.PersistentVolumes) + require.NoError(t, err) + require.False(t, shouldSnapshot, "Expected snapshot to be skipped due to fs-backup selection via cache") +} diff --git a/pkg/backup/actions/csi/pvc_action.go b/pkg/backup/actions/csi/pvc_action.go index c5a83c07e..8e2e77316 100644 --- a/pkg/backup/actions/csi/pvc_action.go +++ b/pkg/backup/actions/csi/pvc_action.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "strconv" - "sync" "time" "k8s.io/client-go/util/retry" @@ -59,6 +58,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/csi" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" + podvolumeutil "github.com/vmware-tanzu/velero/pkg/util/podvolume" ) // TODO: Replace hardcoded VolumeSnapshot finalizer strings with constants from @@ -75,15 +75,13 @@ type pvcBackupItemAction struct { log logrus.FieldLogger crClient crclient.Client - // volumeHelper caches the VolumeHelper instance for the current backup. + // pvcPodCache provides lazy per-namespace caching of PVC-to-Pod mappings. + // Since plugin instances are unique per backup (created via newPluginManager and + // cleaned up via CleanupClients at backup completion), we can safely cache this + // without mutex or backup UID tracking. // This avoids the O(N*M) performance issue when there are many PVCs and pods. - // See issue #9179 for details. - volumeHelper internalvolumehelper.VolumeHelper - // cachedForBackup tracks which backup the volumeHelper was built for. - // If the backup UID changes, we need to rebuild the cache. - cachedForBackup types.UID - // mu protects volumeHelper and cachedForBackup for concurrent access. - mu sync.Mutex + // See issue #9179 and PR #9226 for details. + pvcPodCache *podvolumeutil.PVCPodCache } // AppliesTo returns information indicating that the PVCBackupItemAction @@ -109,29 +107,57 @@ func (p *pvcBackupItemAction) validateBackup(backup velerov1api.Backup) (valid b return true } -// getOrCreateVolumeHelper returns a cached VolumeHelper for the given backup. -// If the backup UID has changed or no VolumeHelper exists, a new one is created. -// This avoids the O(N*M) performance issue when there are many PVCs and pods. -// See issue #9179 for details. -func (p *pvcBackupItemAction) getOrCreateVolumeHelper(backup *velerov1api.Backup) (internalvolumehelper.VolumeHelper, error) { - p.mu.Lock() - defer p.mu.Unlock() - - // Check if we already have a VolumeHelper for this backup - if p.volumeHelper != nil && p.cachedForBackup == backup.UID { - return p.volumeHelper, nil +// ensurePVCPodCacheForNamespace ensures the PVC-to-Pod cache is built for the given namespace. +// This uses lazy per-namespace caching following the pattern from PR #9226. +// Since plugin instances are unique per backup, we can safely cache without mutex or backup UID tracking. +func (p *pvcBackupItemAction) ensurePVCPodCacheForNamespace(ctx context.Context, namespace string) error { + // Initialize cache if needed + if p.pvcPodCache == nil { + p.pvcPodCache = podvolumeutil.NewPVCPodCache() } - // Build a new VolumeHelper with cache for this backup - p.log.Infof("Building VolumeHelper with PVC-to-Pod cache for backup %s/%s", backup.Namespace, backup.Name) - vh, err := volumehelper.NewVolumeHelperForBackup(*backup, p.crClient, p.log, nil) + // Build cache for namespace if not already done + if !p.pvcPodCache.IsNamespaceBuilt(namespace) { + p.log.Debugf("Building PVC-to-Pod cache for namespace %s", namespace) + if err := p.pvcPodCache.BuildCacheForNamespace(ctx, namespace, p.crClient); err != nil { + return errors.Wrapf(err, "failed to build PVC-to-Pod cache for namespace %s", namespace) + } + } + return nil +} + +// getVolumeHelperWithCache creates a VolumeHelper using the pre-built PVC-to-Pod cache. +// The cache should be ensured for the relevant namespace(s) before calling this. +func (p *pvcBackupItemAction) getVolumeHelperWithCache(backup *velerov1api.Backup) (internalvolumehelper.VolumeHelper, error) { + // Create VolumeHelper with our lazy-built cache + vh, err := internalvolumehelper.NewVolumeHelperImplWithCache( + *backup, + p.crClient, + p.log, + p.pvcPodCache, + ) if err != nil { - return nil, errors.Wrap(err, "failed to create VolumeHelper for backup") + return nil, errors.Wrap(err, "failed to create VolumeHelper") + } + return vh, nil +} + +// getOrCreateVolumeHelper returns a VolumeHelper with lazy per-namespace caching. +// The VolumeHelper uses the pvcPodCache which is populated lazily as namespaces are encountered. +// Callers should use ensurePVCPodCacheForNamespace before calling methods that need +// PVC-to-Pod lookups for a specific namespace. +// Since plugin instances are unique per backup (created via newPluginManager and +// cleaned up via CleanupClients at backup completion), we can safely cache this. +// See issue #9179 and PR #9226 for details. +func (p *pvcBackupItemAction) getOrCreateVolumeHelper(backup *velerov1api.Backup) (internalvolumehelper.VolumeHelper, error) { + // Initialize the PVC-to-Pod cache if needed + if p.pvcPodCache == nil { + p.pvcPodCache = podvolumeutil.NewPVCPodCache() } - p.volumeHelper = vh - p.cachedForBackup = backup.UID - return vh, nil + // Return the VolumeHelper with our lazily-built cache + // The cache will be populated incrementally as namespaces are encountered + return p.getVolumeHelperWithCache(backup) } func (p *pvcBackupItemAction) validatePVCandPV( @@ -285,6 +311,11 @@ func (p *pvcBackupItemAction) Execute( return item, nil, "", nil, nil } + // Ensure PVC-to-Pod cache is built for this namespace (lazy per-namespace caching) + if err := p.ensurePVCPodCacheForNamespace(context.TODO(), pvc.Namespace); err != nil { + return nil, nil, "", nil, err + } + // Get or create the cached VolumeHelper for this backup vh, err := p.getOrCreateVolumeHelper(backup) if err != nil { @@ -665,6 +696,11 @@ func (p *pvcBackupItemAction) getVolumeSnapshotReference( return nil, errors.Wrapf(err, "failed to list PVCs in VolumeGroupSnapshot group %q in namespace %q", group, pvc.Namespace) } + // Ensure PVC-to-Pod cache is built for this namespace (lazy per-namespace caching) + if err := p.ensurePVCPodCacheForNamespace(ctx, pvc.Namespace); err != nil { + return nil, errors.Wrapf(err, "failed to build PVC-to-Pod cache for namespace %s", pvc.Namespace) + } + // Get the cached VolumeHelper for filtering PVCs by volume policy vh, err := p.getOrCreateVolumeHelper(backup) if err != nil { diff --git a/pkg/backup/actions/csi/pvc_action_test.go b/pkg/backup/actions/csi/pvc_action_test.go index 8fad3821a..b94d63701 100644 --- a/pkg/backup/actions/csi/pvc_action_test.go +++ b/pkg/backup/actions/csi/pvc_action_test.go @@ -2067,87 +2067,11 @@ func TestPVCRequestSize(t *testing.T) { } } -// TestGetOrCreateVolumeHelper tests the VolumeHelper caching behavior +// TestGetOrCreateVolumeHelper tests the VolumeHelper and PVC-to-Pod cache behavior. +// Since plugin instances are unique per backup (created via newPluginManager and +// cleaned up via CleanupClients at backup completion), we verify that the pvcPodCache +// is properly initialized and reused across calls. func TestGetOrCreateVolumeHelper(t *testing.T) { - tests := []struct { - name string - setup func() (*pvcBackupItemAction, *velerov1api.Backup, *velerov1api.Backup) - wantSameCache bool - }{ - { - name: "Returns same VolumeHelper for same backup UID", - setup: func() (*pvcBackupItemAction, *velerov1api.Backup, *velerov1api.Backup) { - client := velerotest.NewFakeControllerRuntimeClient(t) - action := &pvcBackupItemAction{ - log: velerotest.NewLogger(), - crClient: client, - } - backup := &velerov1api.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-backup", - Namespace: "velero", - UID: types.UID("test-uid-1"), - }, - } - return action, backup, backup // Same backup instance - }, - wantSameCache: true, - }, - { - name: "Returns new VolumeHelper for different backup UID", - setup: func() (*pvcBackupItemAction, *velerov1api.Backup, *velerov1api.Backup) { - client := velerotest.NewFakeControllerRuntimeClient(t) - action := &pvcBackupItemAction{ - log: velerotest.NewLogger(), - crClient: client, - } - backup1 := &velerov1api.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-backup-1", - Namespace: "velero", - UID: types.UID("test-uid-1"), - }, - } - backup2 := &velerov1api.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-backup-2", - Namespace: "velero", - UID: types.UID("test-uid-2"), - }, - } - return action, backup1, backup2 // Different backup instances - }, - wantSameCache: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - action, backup1, backup2 := tt.setup() - - // Get VolumeHelper for first backup - vh1, err := action.getOrCreateVolumeHelper(backup1) - require.NoError(t, err) - require.NotNil(t, vh1) - - // Get VolumeHelper for second backup - vh2, err := action.getOrCreateVolumeHelper(backup2) - require.NoError(t, err) - require.NotNil(t, vh2) - - if tt.wantSameCache { - // Same backup UID should return same VolumeHelper pointer - require.Same(t, vh1, vh2, "Expected same VolumeHelper instance for same backup UID") - } else { - // Different backup UID should return different VolumeHelper pointer - require.NotSame(t, vh1, vh2, "Expected different VolumeHelper instance for different backup UID") - } - }) - } -} - -// TestGetOrCreateVolumeHelperConcurrency tests thread-safety of VolumeHelper caching -func TestGetOrCreateVolumeHelperConcurrency(t *testing.T) { client := velerotest.NewFakeControllerRuntimeClient(t) action := &pvcBackupItemAction{ log: velerotest.NewLogger(), @@ -2157,41 +2081,27 @@ func TestGetOrCreateVolumeHelperConcurrency(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "test-backup", Namespace: "velero", - UID: types.UID("test-uid"), + UID: types.UID("test-uid-1"), }, } - // Run multiple goroutines concurrently to get VolumeHelper - const numGoroutines = 10 - results := make(chan any, numGoroutines) - errors := make(chan error, numGoroutines) + // Initially, pvcPodCache should be nil + require.Nil(t, action.pvcPodCache, "pvcPodCache should be nil initially") - for i := 0; i < numGoroutines; i++ { - go func() { - vh, err := action.getOrCreateVolumeHelper(backup) - if err != nil { - errors <- err - return - } - results <- vh - }() - } + // Get VolumeHelper first time - should create new cache and VolumeHelper + vh1, err := action.getOrCreateVolumeHelper(backup) + require.NoError(t, err) + require.NotNil(t, vh1) - // Collect all results - var volumeHelpers []any - for i := 0; i < numGoroutines; i++ { - select { - case vh := <-results: - volumeHelpers = append(volumeHelpers, vh) - case err := <-errors: - t.Fatalf("Unexpected error: %v", err) - } - } + // pvcPodCache should now be initialized + require.NotNil(t, action.pvcPodCache, "pvcPodCache should be initialized after first call") + cache1 := action.pvcPodCache - // All goroutines should get the same VolumeHelper instance - require.Len(t, volumeHelpers, numGoroutines) - firstVH := volumeHelpers[0] - for i := 1; i < len(volumeHelpers); i++ { - require.Same(t, firstVH, volumeHelpers[i], "All goroutines should get the same VolumeHelper instance") - } + // Get VolumeHelper second time - should reuse the same cache + vh2, err := action.getOrCreateVolumeHelper(backup) + require.NoError(t, err) + require.NotNil(t, vh2) + + // The pvcPodCache should be the same instance + require.Same(t, cache1, action.pvcPodCache, "Expected same pvcPodCache instance on repeated calls") } diff --git a/pkg/util/podvolume/pod_volume.go b/pkg/util/podvolume/pod_volume.go index 1bf87a5fa..33d67dc05 100644 --- a/pkg/util/podvolume/pod_volume.go +++ b/pkg/util/podvolume/pod_volume.go @@ -113,6 +113,66 @@ func (c *PVCPodCache) IsBuilt() bool { return c.built } +// IsNamespaceBuilt returns true if the cache has been built for the given namespace. +func (c *PVCPodCache) IsNamespaceBuilt(namespace string) bool { + c.mu.RLock() + defer c.mu.RUnlock() + _, ok := c.cache[namespace] + return ok +} + +// BuildCacheForNamespace builds the cache for a single namespace lazily. +// This is used by plugins where namespaces are encountered one at a time. +// If the namespace is already cached, this is a no-op. +func (c *PVCPodCache) BuildCacheForNamespace( + ctx context.Context, + namespace string, + crClient crclient.Client, +) error { + // Check if already built (read lock first for performance) + c.mu.RLock() + if _, ok := c.cache[namespace]; ok { + c.mu.RUnlock() + return nil + } + c.mu.RUnlock() + + // Need to build - acquire write lock + c.mu.Lock() + defer c.mu.Unlock() + + // Double-check after acquiring write lock + if _, ok := c.cache[namespace]; ok { + return nil + } + + podList := new(corev1api.PodList) + if err := crClient.List( + ctx, + podList, + &crclient.ListOptions{Namespace: namespace}, + ); err != nil { + return errors.Wrapf(err, "failed to list pods in namespace %s", namespace) + } + + c.cache[namespace] = make(map[string][]corev1api.Pod) + + // Build mapping from PVC name to pods + for i := range podList.Items { + pod := podList.Items[i] + for _, v := range pod.Spec.Volumes { + if v.PersistentVolumeClaim != nil { + pvcName := v.PersistentVolumeClaim.ClaimName + c.cache[namespace][pvcName] = append(c.cache[namespace][pvcName], pod) + } + } + } + + // Mark as built for GetPodsUsingPVCWithCache fallback logic + c.built = true + return nil +} + // GetVolumesByPod returns a list of volume names to backup for the provided pod. func GetVolumesByPod(pod *corev1api.Pod, defaultVolumesToFsBackup, backupExcludePVC bool, volsToProcessByLegacyApproach []string) ([]string, []string) { // tracks the volumes that have been explicitly opted out of backup via the annotation in the pod diff --git a/pkg/util/podvolume/pod_volume_test.go b/pkg/util/podvolume/pod_volume_test.go index 6732904ef..af28ff9ef 100644 --- a/pkg/util/podvolume/pod_volume_test.go +++ b/pkg/util/podvolume/pod_volume_test.go @@ -1252,3 +1252,224 @@ func TestIsPVCDefaultToFSBackupWithCache(t *testing.T) { }) } } + +// TestIsNamespaceBuilt tests the IsNamespaceBuilt method for lazy per-namespace caching. +func TestIsNamespaceBuilt(t *testing.T) { + cache := NewPVCPodCache() + + // Initially no namespace should be built + assert.False(t, cache.IsNamespaceBuilt("ns1"), "namespace should not be built initially") + assert.False(t, cache.IsNamespaceBuilt("ns2"), "namespace should not be built initially") + + // Create a fake client with a pod in ns1 + pod := &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "ns1", + }, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }, + }, + }, + }, + }, + } + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, pod) + + // Build cache for ns1 + err := cache.BuildCacheForNamespace(t.Context(), "ns1", fakeClient) + require.NoError(t, err) + + // ns1 should be built, ns2 should not + assert.True(t, cache.IsNamespaceBuilt("ns1"), "namespace ns1 should be built") + assert.False(t, cache.IsNamespaceBuilt("ns2"), "namespace ns2 should not be built") + + // Build cache for ns2 (empty namespace) + err = cache.BuildCacheForNamespace(t.Context(), "ns2", fakeClient) + require.NoError(t, err) + + // Both should now be built + assert.True(t, cache.IsNamespaceBuilt("ns1"), "namespace ns1 should still be built") + assert.True(t, cache.IsNamespaceBuilt("ns2"), "namespace ns2 should now be built") +} + +// TestBuildCacheForNamespace tests the lazy per-namespace cache building. +func TestBuildCacheForNamespace(t *testing.T) { + tests := []struct { + name string + pods []runtime.Object + namespace string + expectedPVCs map[string]int // pvcName -> expected pod count + expectError bool + }{ + { + name: "build cache for namespace with pods using PVCs", + namespace: "ns1", + pods: []runtime.Object{ + &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }, + }, + }, + }, + }, + }, + &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod2", Namespace: "ns1"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }, + }, + }, + }, + }, + }, + }, + expectedPVCs: map[string]int{"pvc1": 2}, + }, + { + name: "build cache for empty namespace", + namespace: "empty-ns", + pods: []runtime.Object{}, + expectedPVCs: map[string]int{}, + }, + { + name: "build cache ignores pods without PVCs", + namespace: "ns1", + pods: []runtime.Object{ + &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "config-vol", + VolumeSource: corev1api.VolumeSource{ + ConfigMap: &corev1api.ConfigMapVolumeSource{ + LocalObjectReference: corev1api.LocalObjectReference{ + Name: "my-config", + }, + }, + }, + }, + }, + }, + }, + }, + expectedPVCs: map[string]int{}, + }, + { + name: "build cache only for specified namespace", + namespace: "ns1", + pods: []runtime.Object{ + &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }, + }, + }, + }, + }, + }, + &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod2", Namespace: "ns2"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc2", + }, + }, + }, + }, + }, + }, + }, + expectedPVCs: map[string]int{"pvc1": 1}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, tc.pods...) + cache := NewPVCPodCache() + + // Build cache for the namespace + err := cache.BuildCacheForNamespace(t.Context(), tc.namespace, fakeClient) + if tc.expectError { + require.Error(t, err) + return + } + require.NoError(t, err) + + // Verify namespace is marked as built + assert.True(t, cache.IsNamespaceBuilt(tc.namespace)) + + // Verify PVC to pod mappings + for pvcName, expectedCount := range tc.expectedPVCs { + pods := cache.GetPodsUsingPVC(tc.namespace, pvcName) + assert.Len(t, pods, expectedCount, "unexpected pod count for PVC %s", pvcName) + } + + // Calling BuildCacheForNamespace again should be a no-op + err = cache.BuildCacheForNamespace(t.Context(), tc.namespace, fakeClient) + require.NoError(t, err) + }) + } +} + +// TestBuildCacheForNamespaceIdempotent verifies that building cache multiple times is safe. +func TestBuildCacheForNamespaceIdempotent(t *testing.T) { + pod := &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "vol1", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }, + }, + }, + }, + }, + } + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, pod) + cache := NewPVCPodCache() + + // Build cache multiple times - should be idempotent + for i := 0; i < 3; i++ { + err := cache.BuildCacheForNamespace(t.Context(), "ns1", fakeClient) + require.NoError(t, err) + assert.True(t, cache.IsNamespaceBuilt("ns1")) + + pods := cache.GetPodsUsingPVC("ns1", "pvc1") + assert.Len(t, pods, 1, "should have exactly 1 pod using pvc1") + } +}