diff --git a/internal/volumehelper/volume_policy_helper.go b/internal/volumehelper/volume_policy_helper.go index 63115cba2..160a2005e 100644 --- a/internal/volumehelper/volume_policy_helper.go +++ b/internal/volumehelper/volume_policy_helper.go @@ -41,6 +41,11 @@ type volumeHelperImpl struct { pvcPodCache *podvolumeutil.PVCPodCache } +// NewVolumeHelperImpl creates a VolumeHelper without PVC-to-Pod caching. +// +// Deprecated: Use NewVolumeHelperImplWithNamespaces or NewVolumeHelperImplWithCache instead +// for better performance. These functions provide PVC-to-Pod caching which avoids O(N*M) +// complexity when there are many PVCs and pods. See issue #9179 for details. func NewVolumeHelperImpl( volumePolicy *resourcepolicies.Policies, snapshotVolumes *bool, diff --git a/pkg/plugin/utils/volumehelper/volume_policy_helper.go b/pkg/plugin/utils/volumehelper/volume_policy_helper.go index 715db15d9..8fda7f5af 100644 --- a/pkg/plugin/utils/volumehelper/volume_policy_helper.go +++ b/pkg/plugin/utils/volumehelper/volume_policy_helper.go @@ -17,10 +17,7 @@ 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" @@ -36,6 +33,11 @@ import ( // up on demand. On the other hand, the volumeHelperImpl assume there // is a VolumeHelper instance initialized before calling the // ShouldPerformXXX functions. +// +// Deprecated: Use ShouldPerformSnapshotWithVolumeHelper instead for better performance. +// ShouldPerformSnapshotWithVolumeHelper allows passing a pre-created VolumeHelper with +// an internal PVC-to-Pod cache, which avoids O(N*M) complexity when there are many PVCs and pods. +// See issue #9179 for details. func ShouldPerformSnapshotWithBackup( unstructured runtime.Unstructured, groupResource schema.GroupResource, @@ -92,92 +94,3 @@ func ShouldPerformSnapshotWithVolumeHelper( 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 -} diff --git a/pkg/plugin/utils/volumehelper/volume_policy_helper_test.go b/pkg/plugin/utils/volumehelper/volume_policy_helper_test.go index b4d835a76..08b23ae04 100644 --- a/pkg/plugin/utils/volumehelper/volume_policy_helper_test.go +++ b/pkg/plugin/utils/volumehelper/volume_policy_helper_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "github.com/vmware-tanzu/velero/internal/volumehelper" 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" @@ -290,8 +291,16 @@ func TestShouldPerformSnapshotWithNonNilVolumeHelper(t *testing.T) { logger := logrus.New() - // Create VolumeHelper using the factory function - vh, err := NewVolumeHelperForBackup(*backup, client, logger, []string{"default"}) + // Create VolumeHelper using the internal function with namespace caching + vh, err := volumehelper.NewVolumeHelperImplWithNamespaces( + nil, // no resource policies for this test + nil, // snapshotVolumes not set + logger, + client, + false, // defaultVolumesToFSBackup + true, // backupExcludePVC + []string{"default"}, + ) require.NoError(t, err) require.NotNil(t, vh) @@ -313,128 +322,3 @@ func TestShouldPerformSnapshotWithNonNilVolumeHelper(t *testing.T) { 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) - }) - } -} diff --git a/pkg/util/podvolume/pod_volume.go b/pkg/util/podvolume/pod_volume.go index 33d67dc05..7c7e0f9c4 100644 --- a/pkg/util/podvolume/pod_volume.go +++ b/pkg/util/podvolume/pod_volume.go @@ -253,19 +253,8 @@ func GetVolumesToExclude(obj metav1.Object) []string { return strings.Split(annotations[velerov1api.VolumesToExcludeAnnotation], ",") } -// IsPVCDefaultToFSBackup checks if a PVC should default to fs-backup based on pod annotations. -// Deprecated: Use IsPVCDefaultToFSBackupWithCache for better performance. -func IsPVCDefaultToFSBackup(pvcNamespace, pvcName string, crClient crclient.Client, defaultVolumesToFsBackup bool) (bool, error) { - pods, err := GetPodsUsingPVC(pvcNamespace, pvcName, crClient) - if err != nil { - return false, errors.WithStack(err) - } - - return checkPodsForFSBackup(pods, pvcName, defaultVolumesToFsBackup) -} - -// IsPVCDefaultToFSBackupWithCache is the cached version of IsPVCDefaultToFSBackup. -// If cache is nil or not built, it falls back to the non-cached version. +// IsPVCDefaultToFSBackupWithCache checks if a PVC should default to fs-backup based on pod annotations. +// If cache is nil or not built, it falls back to listing pods directly. // Note: In the main backup path, the cache is always built (via NewVolumeHelperImplWithNamespaces), // so the fallback is only used by plugins that don't need cache optimization. func IsPVCDefaultToFSBackupWithCache( @@ -281,7 +270,7 @@ func IsPVCDefaultToFSBackupWithCache( if cache != nil && cache.IsBuilt() { pods = cache.GetPodsUsingPVC(pvcNamespace, pvcName) } else { - pods, err = GetPodsUsingPVC(pvcNamespace, pvcName, crClient) + pods, err = getPodsUsingPVCDirect(pvcNamespace, pvcName, crClient) if err != nil { return false, errors.WithStack(err) } @@ -318,10 +307,32 @@ func getPodVolumeNameForPVC(pod corev1api.Pod, pvcName string) (string, error) { return "", errors.Errorf("Pod %s/%s does not use PVC %s/%s", pod.Namespace, pod.Name, pod.Namespace, pvcName) } -// GetPodsUsingPVC returns all pods in the given namespace that use the specified PVC. -// This function lists all pods in the namespace and filters them, which can be slow -// when there are many pods. Consider using GetPodsUsingPVCWithCache for better performance. -func GetPodsUsingPVC( +// GetPodsUsingPVCWithCache returns all pods that use the specified PVC. +// If cache is available and built, it uses the cache for O(1) lookup. +// Otherwise, it falls back to listing pods directly. +// Note: In the main backup path, the cache is always built (via NewVolumeHelperImplWithNamespaces), +// so the fallback is only used by plugins that don't need cache optimization. +func GetPodsUsingPVCWithCache( + pvcNamespace, pvcName string, + crClient crclient.Client, + cache *PVCPodCache, +) ([]corev1api.Pod, error) { + // Use cache if available + if cache != nil && cache.IsBuilt() { + pods := cache.GetPodsUsingPVC(pvcNamespace, pvcName) + if pods == nil { + return []corev1api.Pod{}, nil + } + return pods, nil + } + + // Fall back to direct lookup (for plugins without cache) + return getPodsUsingPVCDirect(pvcNamespace, pvcName, crClient) +} + +// getPodsUsingPVCDirect returns all pods in the given namespace that use the specified PVC. +// This is an internal function that lists all pods in the namespace and filters them. +func getPodsUsingPVCDirect( pvcNamespace, pvcName string, crClient crclient.Client, ) ([]corev1api.Pod, error) { @@ -346,29 +357,6 @@ func GetPodsUsingPVC( return podsUsingPVC, nil } -// GetPodsUsingPVCWithCache returns all pods that use the specified PVC. -// If cache is available and built, it uses the cache for O(1) lookup. -// Otherwise, it falls back to the original GetPodsUsingPVC function. -// Note: In the main backup path, the cache is always built (via NewVolumeHelperImplWithNamespaces), -// so the fallback is only used by plugins that don't need cache optimization. -func GetPodsUsingPVCWithCache( - pvcNamespace, pvcName string, - crClient crclient.Client, - cache *PVCPodCache, -) ([]corev1api.Pod, error) { - // Use cache if available - if cache != nil && cache.IsBuilt() { - pods := cache.GetPodsUsingPVC(pvcNamespace, pvcName) - if pods == nil { - return []corev1api.Pod{}, nil - } - return pods, nil - } - - // Fall back to direct lookup - return GetPodsUsingPVC(pvcNamespace, pvcName, crClient) -} - func GetVolumesToProcess(volumes []corev1api.Volume, volsToProcessByLegacyApproach []string) []corev1api.Volume { volsToProcess := make([]corev1api.Volume, 0) diff --git a/pkg/util/podvolume/pod_volume_test.go b/pkg/util/podvolume/pod_volume_test.go index 3ba2a05e9..a3484c2e3 100644 --- a/pkg/util/podvolume/pod_volume_test.go +++ b/pkg/util/podvolume/pod_volume_test.go @@ -382,196 +382,6 @@ func TestGetVolumesByPod(t *testing.T) { } } -func TestIsPVCDefaultToFSBackup(t *testing.T) { - objs := []runtime.Object{ - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod1", - Namespace: "default", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod2", - Namespace: "default", - Annotations: map[string]string{ - "backup.velero.io/backup-volumes": "csi-vol1", - }, - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod3", - Namespace: "default", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - EmptyDir: &corev1api.EmptyDirVolumeSource{}, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "awesome-pod-1", - Namespace: "awesome-ns", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "awesome-csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "awesome-pod-2", - Namespace: "awesome-ns", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "awesome-csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod1", - Namespace: "uploader-ns", - Annotations: map[string]string{ - "backup.velero.io/backup-volumes": "csi-vol1", - }, - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod2", - Namespace: "uploader-ns", - Annotations: map[string]string{ - "backup.velero.io/backup-volumes": "csi-vol1", - }, - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - } - fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...) - - testCases := []struct { - name string - inPVCNamespace string - inPVCName string - expectedIsFSUploaderUsed bool - defaultVolumesToFSBackup bool - }{ - { - name: "2 pods using PVC, 1 pod using uploader", - inPVCNamespace: "default", - inPVCName: "csi-pvc1", - expectedIsFSUploaderUsed: true, - defaultVolumesToFSBackup: false, - }, - { - name: "2 pods using PVC, 2 pods using uploader", - inPVCNamespace: "uploader-ns", - inPVCName: "csi-pvc1", - expectedIsFSUploaderUsed: true, - defaultVolumesToFSBackup: false, - }, - { - name: "2 pods using PVC, 0 pods using uploader", - inPVCNamespace: "awesome-ns", - inPVCName: "awesome-csi-pvc1", - expectedIsFSUploaderUsed: false, - defaultVolumesToFSBackup: false, - }, - { - name: "0 pods using PVC", - inPVCNamespace: "default", - inPVCName: "does-not-exist", - expectedIsFSUploaderUsed: false, - defaultVolumesToFSBackup: false, - }, - { - name: "2 pods using PVC, using uploader by default", - inPVCNamespace: "awesome-ns", - inPVCName: "awesome-csi-pvc1", - expectedIsFSUploaderUsed: true, - defaultVolumesToFSBackup: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - actualIsFSUploaderUsed, _ := IsPVCDefaultToFSBackup(tc.inPVCNamespace, tc.inPVCName, fakeClient, tc.defaultVolumesToFSBackup) - assert.Equal(t, tc.expectedIsFSUploaderUsed, actualIsFSUploaderUsed) - }) - } -} - func TestGetPodVolumeNameForPVC(t *testing.T) { testCases := []struct { name string @@ -677,122 +487,6 @@ func TestGetPodVolumeNameForPVC(t *testing.T) { } } -func TestGetPodsUsingPVC(t *testing.T) { - objs := []runtime.Object{ - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod1", - Namespace: "default", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod2", - Namespace: "default", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod3", - Namespace: "default", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - EmptyDir: &corev1api.EmptyDirVolumeSource{}, - }, - }, - }, - }, - }, - &corev1api.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod1", - Namespace: "awesome-ns", - }, - Spec: corev1api.PodSpec{ - Volumes: []corev1api.Volume{ - { - Name: "csi-vol1", - VolumeSource: corev1api.VolumeSource{ - PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ - ClaimName: "csi-pvc1", - }, - }, - }, - }, - }, - }, - } - fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...) - - testCases := []struct { - name string - pvcNamespace string - pvcName string - expectedPodCount int - }{ - { - name: "should find exactly 2 pods using the PVC", - pvcNamespace: "default", - pvcName: "csi-pvc1", - expectedPodCount: 2, - }, - { - name: "should find exactly 1 pod using the PVC", - pvcNamespace: "awesome-ns", - pvcName: "csi-pvc1", - expectedPodCount: 1, - }, - { - name: "should find 0 pods using the PVC", - pvcNamespace: "default", - pvcName: "unused-pvc", - expectedPodCount: 0, - }, - { - name: "should find 0 pods in non-existent namespace", - pvcNamespace: "does-not-exist", - pvcName: "csi-pvc1", - expectedPodCount: 0, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - actualPods, err := GetPodsUsingPVC(tc.pvcNamespace, tc.pvcName, fakeClient) - require.NoErrorf(t, err, "Want error=nil; Got error=%v", err) - assert.Lenf(t, actualPods, tc.expectedPodCount, "unexpected number of pods in result; Want: %d; Got: %d", tc.expectedPodCount, len(actualPods)) - }) - } -} - func TestGetVolumesToProcess(t *testing.T) { testCases := []struct { name string