From 97a4d62d3c09ec10082945301a50e3807eeb932c Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Fri, 16 May 2025 12:11:34 -0700 Subject: [PATCH] Extend PVCAction itemblock plugin to support grouping PVCs under VolumeGroupSnapshot label Signed-off-by: Shubham Pampattiwar Add changelog file Signed-off-by: Shubham Pampattiwar Update VGS label key and address PR feedback Signed-off-by: Shubham Pampattiwar update log level to debug for edge cases Signed-off-by: Shubham Pampattiwar Change VGS label key constant location Signed-off-by: Shubham Pampattiwar run make update Signed-off-by: Shubham Pampattiwar --- .../unreleased/8944-shubham-pampattiwar | 1 + pkg/apis/velero/v1/labels_annotations.go | 3 + pkg/cmd/server/config/config.go | 9 +- pkg/controller/backup_controller_test.go | 6 +- pkg/itemblock/actions/pvc_action.go | 60 +++++++++- pkg/itemblock/actions/pvc_action_test.go | 109 ++++++++++++++++++ 6 files changed, 178 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/8944-shubham-pampattiwar diff --git a/changelogs/unreleased/8944-shubham-pampattiwar b/changelogs/unreleased/8944-shubham-pampattiwar new file mode 100644 index 000000000..f59ba9844 --- /dev/null +++ b/changelogs/unreleased/8944-shubham-pampattiwar @@ -0,0 +1 @@ +Extend PVCAction itemblock plugin to support grouping PVCs under VGS label key \ No newline at end of file diff --git a/pkg/apis/velero/v1/labels_annotations.go b/pkg/apis/velero/v1/labels_annotations.go index 78d231ba9..ca3f91b6d 100644 --- a/pkg/apis/velero/v1/labels_annotations.go +++ b/pkg/apis/velero/v1/labels_annotations.go @@ -101,6 +101,9 @@ const ( // ExcludeFromBackupLabel is the label to exclude k8s resource from backup, // even if the resource contains a matching selector label. ExcludeFromBackupLabel = "velero.io/exclude-from-backup" + + // defaultVGSLabelKey is the default label key used to group PVCs under a VolumeGroupSnapshot + DefaultVGSLabelKey = "velero.io/volume-group" ) type AsyncOperationIDPrefix string diff --git a/pkg/cmd/server/config/config.go b/pkg/cmd/server/config/config.go index b80a5c458..b9540bf2c 100644 --- a/pkg/cmd/server/config/config.go +++ b/pkg/cmd/server/config/config.go @@ -5,6 +5,8 @@ import ( "strings" "time" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/sirupsen/logrus" "github.com/spf13/pflag" @@ -37,9 +39,6 @@ const ( // the default TTL for a backup defaultBackupTTL = 30 * 24 * time.Hour - // defaultVGSLabelKey is the default label key used to group PVCs under a VolumeGroupSnapshot - defaultVGSLabelKey = "velero.io/volume-group-snapshot" - defaultCSISnapshotTimeout = 10 * time.Minute defaultItemOperationTimeout = 4 * time.Hour @@ -193,7 +192,7 @@ func GetDefaultConfig() *Config { DefaultVolumeSnapshotLocations: flag.NewMap().WithKeyValueDelimiter(':'), BackupSyncPeriod: defaultBackupSyncPeriod, DefaultBackupTTL: defaultBackupTTL, - DefaultVGSLabelKey: defaultVGSLabelKey, + DefaultVGSLabelKey: velerov1api.DefaultVGSLabelKey, DefaultCSISnapshotTimeout: defaultCSISnapshotTimeout, DefaultItemOperationTimeout: defaultItemOperationTimeout, ResourceTimeout: resourceTimeout, @@ -245,7 +244,7 @@ func (c *Config) BindFlags(flags *pflag.FlagSet) { flags.StringVar(&c.ProfilerAddress, "profiler-address", c.ProfilerAddress, "The address to expose the pprof profiler.") flags.DurationVar(&c.ResourceTerminatingTimeout, "terminating-resource-timeout", c.ResourceTerminatingTimeout, "How long to wait on persistent volumes and namespaces to terminate during a restore before timing out.") flags.DurationVar(&c.DefaultBackupTTL, "default-backup-ttl", c.DefaultBackupTTL, "How long to wait by default before backups can be garbage collected.") - flags.StringVar(&c.DefaultVGSLabelKey, "volume-group-snapshot-label-key", c.DefaultVGSLabelKey, "Label key for grouping PVCs into VolumeGroupSnapshot. Default value is 'velero.io/volume-group-snapshot'") + flags.StringVar(&c.DefaultVGSLabelKey, "volume-group-snapshot-label-key", c.DefaultVGSLabelKey, "Label key for grouping PVCs into VolumeGroupSnapshot. Default value is 'velero.io/volume-group'") flags.DurationVar(&c.RepoMaintenanceFrequency, "default-repo-maintain-frequency", c.RepoMaintenanceFrequency, "How often 'maintain' is run for backup repositories by default.") flags.DurationVar(&c.GarbageCollectionFrequency, "garbage-collection-frequency", c.GarbageCollectionFrequency, "How often garbage collection is run for expired backups.") flags.DurationVar(&c.ItemOperationSyncFrequency, "item-operation-sync-frequency", c.ItemOperationSyncFrequency, "How often to check status on backup/restore operations after backup/restore processing. Default is 10 seconds") diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index e8a28282d..8136010b3 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -490,8 +490,6 @@ func TestPrepareBackupRequest_SetsVGSLabelKey(t *testing.T) { require.NoError(t, err) now = now.Local() - defaultVGSLabelKey := "velero.io/volume-group-snapshot" - tests := []struct { name string backup *velerov1api.Backup @@ -515,8 +513,8 @@ func TestPrepareBackupRequest_SetsVGSLabelKey(t *testing.T) { { name: "backup with no spec or server flag, uses default", backup: builder.ForBackup("velero", "backup-3").Result(), - serverFlagKey: defaultVGSLabelKey, - expectedLabelKey: defaultVGSLabelKey, + serverFlagKey: velerov1api.DefaultVGSLabelKey, + expectedLabelKey: velerov1api.DefaultVGSLabelKey, }, } diff --git a/pkg/itemblock/actions/pvc_action.go b/pkg/itemblock/actions/pvc_action.go index eee4e2fb3..b5d7074af 100644 --- a/pkg/itemblock/actions/pvc_action.go +++ b/pkg/itemblock/actions/pvc_action.go @@ -105,9 +105,67 @@ func (a *PVCAction) GetRelatedItems(item runtime.Unstructured, backup *v1.Backup } } + // Gather groupedPVCs based on VGS label provided in the backup + groupedPVCs, err := a.getGroupedPVCs(context.Background(), pvc, backup) + if err != nil { + return nil, err + } + + // Add the groupedPVCs to relatedItems so that they processed in a single item block + relatedItems = append(relatedItems, groupedPVCs...) + return relatedItems, nil } func (a *PVCAction) Name() string { - return "PodItemBlockAction" + return "PVCItemBlockAction" +} + +// getGroupedPVCs returns other PVCs in the same group based on the VGS label key in the Backup spec. +func (a *PVCAction) getGroupedPVCs(ctx context.Context, pvc *corev1api.PersistentVolumeClaim, backup *v1.Backup) ([]velero.ResourceIdentifier, error) { + var related []velero.ResourceIdentifier + + vgsLabelKey := backup.Spec.VolumeGroupSnapshotLabelKey + if vgsLabelKey == "" { + a.log.Debug("No VolumeGroupSnapshotLabelKey provided in backup spec; skipping PVC grouping") + return nil, nil + } + + groupID, ok := pvc.Labels[vgsLabelKey] + if !ok || groupID == "" { + // PVC does not belong to any VGS group or groupID has empty value + a.log.Debug("PVC does not belong to any PVC group or group label value is empty; skipping PVC grouping") + return nil, nil + } + + pvcList := new(corev1api.PersistentVolumeClaimList) + if err := a.crClient.List( + ctx, + pvcList, + crclient.InNamespace(pvc.Namespace), + crclient.MatchingLabels{vgsLabelKey: groupID}, + ); err != nil { + return nil, errors.Wrapf(err, "failed to list PVCs for VGS grouping with label %s=%s in namespace %s", vgsLabelKey, groupID, pvc.Namespace) + } + + if len(pvcList.Items) <= 1 { + // Only the current PVC exists in this group + return nil, nil + } + + for _, groupPVC := range pvcList.Items { + if groupPVC.Name == pvc.Name { + continue + } + + a.log.Infof("Adding grouped PVC %s (group %s) to relatedItems for PVC %s", groupPVC.Name, groupID, pvc.Name) + + related = append(related, velero.ResourceIdentifier{ + GroupResource: kuberesource.PersistentVolumeClaims, + Namespace: groupPVC.Namespace, + Name: groupPVC.Name, + }) + } + + return related, nil } diff --git a/pkg/itemblock/actions/pvc_action_test.go b/pkg/itemblock/actions/pvc_action_test.go index c485dcd80..fcd54b022 100644 --- a/pkg/itemblock/actions/pvc_action_test.go +++ b/pkg/itemblock/actions/pvc_action_test.go @@ -124,6 +124,22 @@ func TestBackupPVAction(t *testing.T) { {GroupResource: kuberesource.Pods, Namespace: "velero", Name: "testPod2"}, }, }, + { + name: "Test with PVC grouping via VGS label", + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC-1").ObjectMeta(builder.WithLabels("velero.io/group", "db")).VolumeName("testPV-1").Phase(corev1api.ClaimBound).Result(), + pods: []*corev1api.Pod{ + builder.ForPod("velero", "testPod-1"). + Volumes(builder.ForVolume("testPV-1").PersistentVolumeClaimSource("testPVC-1").Result()). + NodeName("node"). + Phase(corev1api.PodRunning).Result(), + }, + expectedErr: nil, + expectedRelated: []velero.ResourceIdentifier{ + {GroupResource: kuberesource.PersistentVolumes, Name: "testPV-1"}, + {GroupResource: kuberesource.Pods, Namespace: "velero", Name: "testPod-1"}, + {GroupResource: kuberesource.PersistentVolumeClaims, Namespace: "velero", Name: "groupedPVC"}, + }, + }, } backup := &v1.Backup{} @@ -152,6 +168,12 @@ func TestBackupPVAction(t *testing.T) { require.NoError(t, crClient.Create(context.Background(), pod)) } + if tc.name == "Test with PVC grouping via VGS label" { + groupedPVC := builder.ForPersistentVolumeClaim("velero", "groupedPVC").ObjectMeta(builder.WithLabels("velero.io/group", "db")).VolumeName("groupedPV").Phase(corev1api.ClaimBound).Result() + require.NoError(t, crClient.Create(context.Background(), groupedPVC)) + backup.Spec.VolumeGroupSnapshotLabelKey = "velero.io/group" + } + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.pvc) require.NoError(t, err) @@ -165,3 +187,90 @@ func TestBackupPVAction(t *testing.T) { }) } } + +// Test_getGroupedPVCs verifies the PVC grouping logic for VolumeGroupSnapshots. +// This ensures only same-namespace PVCs with the same label key and value are included. +func Test_getGroupedPVCs(t *testing.T) { + tests := []struct { + name string + labelKey string + groupValue string + existingPVCs []*corev1api.PersistentVolumeClaim + targetPVC *corev1api.PersistentVolumeClaim + expectedRelated []velero.ResourceIdentifier + expectError bool + }{ + { + name: "No label key in spec", + labelKey: "", + targetPVC: builder.ForPersistentVolumeClaim("ns", "pvc-1").Result(), + expectError: false, + }, + { + name: "No group value", + labelKey: "velero.io/group", + groupValue: "", + targetPVC: builder.ForPersistentVolumeClaim("ns", "pvc-1").Result(), + expectError: false, + }, + { + name: "Target PVC does not have the label", + labelKey: "velero.io/group", + targetPVC: builder.ForPersistentVolumeClaim("ns", "pvc-1").Result(), + expectError: false, + }, + { + name: "Target PVC has label, but no group matches", + labelKey: "velero.io/group", + groupValue: "group-1", + targetPVC: builder.ForPersistentVolumeClaim("ns", "pvc-1").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + existingPVCs: []*corev1api.PersistentVolumeClaim{ + builder.ForPersistentVolumeClaim("ns", "pvc-1").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + }, + expectError: false, + expectedRelated: nil, + }, + { + name: "Multiple PVCs in the same group", + labelKey: "velero.io/group", + groupValue: "group-1", + targetPVC: builder.ForPersistentVolumeClaim("ns", "pvc-1").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + existingPVCs: []*corev1api.PersistentVolumeClaim{ + builder.ForPersistentVolumeClaim("ns", "pvc-1").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + builder.ForPersistentVolumeClaim("ns", "pvc-2").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + builder.ForPersistentVolumeClaim("ns", "pvc-3").ObjectMeta(builder.WithLabels("velero.io/group", "group-1")).Result(), + }, + expectError: false, + expectedRelated: []velero.ResourceIdentifier{ + {GroupResource: kuberesource.PersistentVolumeClaims, Namespace: "ns", Name: "pvc-2"}, + {GroupResource: kuberesource.PersistentVolumeClaims, Namespace: "ns", Name: "pvc-3"}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + crClient := velerotest.NewFakeControllerRuntimeClient(t) + for _, pvc := range tc.existingPVCs { + require.NoError(t, crClient.Create(context.Background(), pvc)) + } + + logger := logrus.New() + a := &PVCAction{ + log: logger, + crClient: crClient, + } + + backup := builder.ForBackup("ns", "bkp").VolumeGroupSnapshotLabelKey(tc.labelKey).Result() + + related, err := a.getGroupedPVCs(context.Background(), tc.targetPVC, backup) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + assert.ElementsMatch(t, tc.expectedRelated, related) + }) + } +}