Add global VolumeHelper caching in CSI PVC BIA plugin

Address review feedback to have a global VolumeHelper instance per plugin process instead of creating one on each ShouldPerformSnapshot call. Changes: - Add volumeHelper, cachedForBackup, and mu fields to pvcBackupItemAction struct for caching the VolumeHelper per backup - Add getOrCreateVolumeHelper() method for thread-safe lazy initialization - Update Execute() to use cached VolumeHelper via ShouldPerformSnapshotWithVolumeHelper() - Update filterPVCsByVolumePolicy() to accept VolumeHelper parameter - Add ShouldPerformSnapshotWithVolumeHelper() that accepts optional VolumeHelper for reuse across multiple calls - Add NewVolumeHelperForBackup() factory function for BIA plugins - Add comprehensive unit tests for both nil and non-nil VolumeHelper paths This completes the fix for issue #9179 by ensuring the PVC-to-Pod cache is built once per backup and reused across all PVC processing, avoiding O(N*M) complexity. Fixes #9179 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
2026-09-19 06:26:44 +00:00 · 2025-12-15 14:18:05 -08:00
parent 99e821a870
commit 987edf5037
4 changed files with 855 additions and 5 deletions
@@ -17,7 +17,10 @@ limitations under the License.
 package volumehelper

 import (
+	"context"
+
 	"github.com/sirupsen/logrus"
+	corev1api "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -40,6 +43,35 @@ func ShouldPerformSnapshotWithBackup(
 	crClient crclient.Client,
 	logger logrus.FieldLogger,
 ) (bool, error) {
+	return ShouldPerformSnapshotWithVolumeHelper(
+		unstructured,
+		groupResource,
+		backup,
+		crClient,
+		logger,
+		nil, // no cached VolumeHelper, will create one
+	)
+}
+
+// ShouldPerformSnapshotWithVolumeHelper is like ShouldPerformSnapshotWithBackup
+// but accepts an optional VolumeHelper. If vh is non-nil, it will be used directly,
+// avoiding the overhead of creating a new VolumeHelper on each call.
+// This is useful for BIA plugins that process multiple PVCs during a single backup
+// and want to reuse the same VolumeHelper (with its internal cache) across calls.
+func ShouldPerformSnapshotWithVolumeHelper(
+	unstructured runtime.Unstructured,
+	groupResource schema.GroupResource,
+	backup velerov1api.Backup,
+	crClient crclient.Client,
+	logger logrus.FieldLogger,
+	vh volumehelper.VolumeHelper,
+) (bool, error) {
+	// If a VolumeHelper is provided, use it directly
+	if vh != nil {
+		return vh.ShouldPerformSnapshot(unstructured, groupResource)
+	}
+
+	// Otherwise, create a new VolumeHelper (original behavior for third-party plugins)
 	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(
 		backup,
 		crClient,
@@ -60,3 +92,92 @@ func ShouldPerformSnapshotWithBackup(

 	return volumeHelperImpl.ShouldPerformSnapshot(unstructured, groupResource)
 }
+
+// NewVolumeHelperForBackup creates a VolumeHelper for the given backup with a PVC-to-Pod cache.
+// The cache is built for the provided namespaces list to avoid O(N*M) complexity when there
+// are many PVCs and pods. See issue #9179 for details.
+//
+// This function is intended for BIA plugins to create a VolumeHelper once and reuse it
+// across multiple Execute() calls for the same backup.
+//
+// If namespaces is nil or empty, the function will resolve the namespace list from the backup spec.
+// If backup.Spec.IncludedNamespaces is empty (meaning all namespaces), it will list all namespaces
+// from the cluster.
+func NewVolumeHelperForBackup(
+	backup velerov1api.Backup,
+	crClient crclient.Client,
+	logger logrus.FieldLogger,
+	namespaces []string,
+) (volumehelper.VolumeHelper, error) {
+	// If no namespaces provided, resolve from backup spec
+	if len(namespaces) == 0 {
+		var err error
+		namespaces, err = resolveNamespacesForBackup(backup, crClient)
+		if err != nil {
+			logger.WithError(err).Warn("Failed to resolve namespaces for cache, proceeding without cache")
+			namespaces = nil
+		}
+	}
+
+	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(
+		backup,
+		crClient,
+		logger,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	return volumehelper.NewVolumeHelperImplWithNamespaces(
+		resourcePolicies,
+		backup.Spec.SnapshotVolumes,
+		logger,
+		crClient,
+		boolptr.IsSetToTrue(backup.Spec.DefaultVolumesToFsBackup),
+		true,
+		namespaces,
+	)
+}
+
+// resolveNamespacesForBackup determines which namespaces will be backed up.
+// If IncludedNamespaces is specified, it returns those (excluding any in ExcludedNamespaces).
+// If IncludedNamespaces is empty (meaning all namespaces), it lists all namespaces from the cluster.
+func resolveNamespacesForBackup(backup velerov1api.Backup, crClient crclient.Client) ([]string, error) {
+	// If specific namespaces are included, use those
+	if len(backup.Spec.IncludedNamespaces) > 0 {
+		// Filter out excluded namespaces
+		excludeSet := make(map[string]bool)
+		for _, ns := range backup.Spec.ExcludedNamespaces {
+			excludeSet[ns] = true
+		}
+
+		var namespaces []string
+		for _, ns := range backup.Spec.IncludedNamespaces {
+			if !excludeSet[ns] {
+				namespaces = append(namespaces, ns)
+			}
+		}
+		return namespaces, nil
+	}
+
+	// IncludedNamespaces is empty, meaning all namespaces - list from cluster
+	nsList := &corev1api.NamespaceList{}
+	if err := crClient.List(context.Background(), nsList); err != nil {
+		return nil, err
+	}
+
+	// Filter out excluded namespaces
+	excludeSet := make(map[string]bool)
+	for _, ns := range backup.Spec.ExcludedNamespaces {
+		excludeSet[ns] = true
+	}
+
+	var namespaces []string
+	for _, ns := range nsList.Items {
+		if !excludeSet[ns.Name] {
+			namespaces = append(namespaces, ns.Name)
+		}
+	}
+
+	return namespaces, nil
+}
@@ -0,0 +1,440 @@
+/*
+Copyright the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package volumehelper
+
+import (
+	"testing"
+
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+	corev1api "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/kuberesource"
+	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+)
+
+func TestShouldPerformSnapshotWithBackup(t *testing.T) {
+	tests := []struct {
+		name         string
+		pvc          *corev1api.PersistentVolumeClaim
+		pv           *corev1api.PersistentVolume
+		backup       *velerov1api.Backup
+		wantSnapshot bool
+		wantError    bool
+	}{
+		{
+			name: "Returns true when snapshotVolumes not set",
+			pvc: &corev1api.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-pvc",
+					Namespace: "default",
+				},
+				Spec: corev1api.PersistentVolumeClaimSpec{
+					VolumeName: "test-pv",
+				},
+				Status: corev1api.PersistentVolumeClaimStatus{
+					Phase: corev1api.ClaimBound,
+				},
+			},
+			pv: &corev1api.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-pv",
+				},
+				Spec: corev1api.PersistentVolumeSpec{
+					PersistentVolumeSource: corev1api.PersistentVolumeSource{
+						CSI: &corev1api.CSIPersistentVolumeSource{
+							Driver: "test-driver",
+						},
+					},
+					ClaimRef: &corev1api.ObjectReference{
+						Namespace: "default",
+						Name:      "test-pvc",
+					},
+				},
+			},
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+			},
+			wantSnapshot: true,
+			wantError:    false,
+		},
+		{
+			name: "Returns false when snapshotVolumes is false",
+			pvc: &corev1api.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-pvc",
+					Namespace: "default",
+				},
+				Spec: corev1api.PersistentVolumeClaimSpec{
+					VolumeName: "test-pv",
+				},
+				Status: corev1api.PersistentVolumeClaimStatus{
+					Phase: corev1api.ClaimBound,
+				},
+			},
+			pv: &corev1api.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-pv",
+				},
+				Spec: corev1api.PersistentVolumeSpec{
+					PersistentVolumeSource: corev1api.PersistentVolumeSource{
+						CSI: &corev1api.CSIPersistentVolumeSource{
+							Driver: "test-driver",
+						},
+					},
+					ClaimRef: &corev1api.ObjectReference{
+						Namespace: "default",
+						Name:      "test-pvc",
+					},
+				},
+			},
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{
+					SnapshotVolumes: boolPtr(false),
+				},
+			},
+			wantSnapshot: false,
+			wantError:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create fake client with PV and PVC
+			client := velerotest.NewFakeControllerRuntimeClient(t, tt.pv, tt.pvc)
+
+			// Convert PVC to unstructured
+			pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.pvc)
+			require.NoError(t, err)
+			unstructuredPVC := &unstructured.Unstructured{Object: pvcMap}
+
+			logger := logrus.New()
+
+			// Call the function under test - this is the wrapper for third-party plugins
+			result, err := ShouldPerformSnapshotWithBackup(
+				unstructuredPVC,
+				kuberesource.PersistentVolumeClaims,
+				*tt.backup,
+				client,
+				logger,
+			)
+
+			if tt.wantError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.wantSnapshot, result)
+			}
+		})
+	}
+}
+
+func boolPtr(b bool) *bool {
+	return &b
+}
+
+func TestShouldPerformSnapshotWithVolumeHelper(t *testing.T) {
+	tests := []struct {
+		name         string
+		pvc          *corev1api.PersistentVolumeClaim
+		pv           *corev1api.PersistentVolume
+		backup       *velerov1api.Backup
+		wantSnapshot bool
+		wantError    bool
+	}{
+		{
+			name: "Returns true with nil VolumeHelper when snapshotVolumes not set",
+			pvc: &corev1api.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-pvc",
+					Namespace: "default",
+				},
+				Spec: corev1api.PersistentVolumeClaimSpec{
+					VolumeName: "test-pv",
+				},
+				Status: corev1api.PersistentVolumeClaimStatus{
+					Phase: corev1api.ClaimBound,
+				},
+			},
+			pv: &corev1api.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-pv",
+				},
+				Spec: corev1api.PersistentVolumeSpec{
+					PersistentVolumeSource: corev1api.PersistentVolumeSource{
+						CSI: &corev1api.CSIPersistentVolumeSource{
+							Driver: "test-driver",
+						},
+					},
+					ClaimRef: &corev1api.ObjectReference{
+						Namespace: "default",
+						Name:      "test-pvc",
+					},
+				},
+			},
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+			},
+			wantSnapshot: true,
+			wantError:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create fake client with PV
+			client := velerotest.NewFakeControllerRuntimeClient(t, tt.pv, tt.pvc)
+
+			// Convert PVC to unstructured
+			pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.pvc)
+			require.NoError(t, err)
+			unstructuredPVC := &unstructured.Unstructured{Object: pvcMap}
+
+			logger := logrus.New()
+
+			// Call the function under test with nil VolumeHelper
+			// This exercises the fallback path that creates a new VolumeHelper per call
+			result, err := ShouldPerformSnapshotWithVolumeHelper(
+				unstructuredPVC,
+				kuberesource.PersistentVolumeClaims,
+				*tt.backup,
+				client,
+				logger,
+				nil, // Pass nil for VolumeHelper - exercises fallback path
+			)
+
+			if tt.wantError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.wantSnapshot, result)
+			}
+		})
+	}
+}
+
+// TestShouldPerformSnapshotWithNonNilVolumeHelper tests the ShouldPerformSnapshotWithVolumeHelper
+// function when a pre-created VolumeHelper is passed. This exercises the cached path used
+// by BIA plugins for better performance.
+func TestShouldPerformSnapshotWithNonNilVolumeHelper(t *testing.T) {
+	pvc := &corev1api.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-pvc",
+			Namespace: "default",
+		},
+		Spec: corev1api.PersistentVolumeClaimSpec{
+			VolumeName: "test-pv",
+		},
+		Status: corev1api.PersistentVolumeClaimStatus{
+			Phase: corev1api.ClaimBound,
+		},
+	}
+
+	pv := &corev1api.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-pv",
+		},
+		Spec: corev1api.PersistentVolumeSpec{
+			PersistentVolumeSource: corev1api.PersistentVolumeSource{
+				CSI: &corev1api.CSIPersistentVolumeSource{
+					Driver: "test-driver",
+				},
+			},
+			ClaimRef: &corev1api.ObjectReference{
+				Namespace: "default",
+				Name:      "test-pvc",
+			},
+		},
+	}
+
+	backup := &velerov1api.Backup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-backup",
+			Namespace: "velero",
+		},
+		Spec: velerov1api.BackupSpec{
+			IncludedNamespaces: []string{"default"},
+		},
+	}
+
+	// Create fake client with PV and PVC
+	client := velerotest.NewFakeControllerRuntimeClient(t, pv, pvc)
+
+	logger := logrus.New()
+
+	// Create VolumeHelper using the factory function
+	vh, err := NewVolumeHelperForBackup(*backup, client, logger, []string{"default"})
+	require.NoError(t, err)
+	require.NotNil(t, vh)
+
+	// Convert PVC to unstructured
+	pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvc)
+	require.NoError(t, err)
+	unstructuredPVC := &unstructured.Unstructured{Object: pvcMap}
+
+	// Call with non-nil VolumeHelper - exercises the cached path
+	result, err := ShouldPerformSnapshotWithVolumeHelper(
+		unstructuredPVC,
+		kuberesource.PersistentVolumeClaims,
+		*backup,
+		client,
+		logger,
+		vh, // Pass non-nil VolumeHelper - exercises cached path
+	)
+
+	require.NoError(t, err)
+	require.True(t, result, "Should return true for snapshot when snapshotVolumes not set")
+}
+
+func TestNewVolumeHelperForBackup(t *testing.T) {
+	tests := []struct {
+		name       string
+		backup     *velerov1api.Backup
+		namespaces []string
+		wantError  bool
+	}{
+		{
+			name: "Creates VolumeHelper with explicit namespaces",
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{
+					IncludedNamespaces: []string{"ns1", "ns2"},
+				},
+			},
+			namespaces: []string{"ns1", "ns2"},
+			wantError:  false,
+		},
+		{
+			name: "Creates VolumeHelper with namespaces from backup spec",
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{
+					IncludedNamespaces: []string{"ns1", "ns2"},
+				},
+			},
+			namespaces: nil, // Will be resolved from backup spec
+			wantError:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := velerotest.NewFakeControllerRuntimeClient(t)
+			logger := logrus.New()
+
+			vh, err := NewVolumeHelperForBackup(*tt.backup, client, logger, tt.namespaces)
+
+			if tt.wantError {
+				require.Error(t, err)
+				require.Nil(t, vh)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, vh)
+			}
+		})
+	}
+}
+
+func TestResolveNamespacesForBackup(t *testing.T) {
+	tests := []struct {
+		name           string
+		backup         *velerov1api.Backup
+		existingNS     []string
+		expectedResult []string
+	}{
+		{
+			name: "Returns included namespaces",
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{
+					IncludedNamespaces: []string{"ns1", "ns2", "ns3"},
+				},
+			},
+			existingNS:     []string{"ns1", "ns2", "ns3", "ns4"},
+			expectedResult: []string{"ns1", "ns2", "ns3"},
+		},
+		{
+			name: "Excludes specified namespaces from included list",
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{
+					IncludedNamespaces: []string{"ns1", "ns2", "ns3"},
+					ExcludedNamespaces: []string{"ns2"},
+				},
+			},
+			existingNS:     []string{"ns1", "ns2", "ns3", "ns4"},
+			expectedResult: []string{"ns1", "ns3"},
+		},
+		{
+			name: "Returns all namespaces when IncludedNamespaces is empty",
+			backup: &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: "velero",
+				},
+				Spec: velerov1api.BackupSpec{},
+			},
+			existingNS:     []string{"default", "kube-system", "app-ns"},
+			expectedResult: []string{"default", "kube-system", "app-ns"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create fake client with namespaces
+			var objects []runtime.Object
+			for _, ns := range tt.existingNS {
+				objects = append(objects, &corev1api.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: ns,
+					},
+				})
+			}
+			client := velerotest.NewFakeControllerRuntimeClient(t, objects...)
+
+			result, err := resolveNamespacesForBackup(*tt.backup, client)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tt.expectedResult, result)
+		})
+	}
+}