From 643dd784ea781932d811b59409dc0cc15c80f1e4 Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 4 Sep 2025 14:33:43 -0400 Subject: [PATCH] Refactor into func and add tests Signed-off-by: Joseph --- Makefile | 2 +- pkg/backup/actions/backup_pv_action.go | 24 ++- pkg/backup/actions/backup_pv_action_test.go | 173 ++++++++++++++++++++ 3 files changed, 190 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 82129623d..55069d0ae 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ BIN ?= velero PKG := github.com/vmware-tanzu/velero # Where to push the docker image. -REGISTRY ?= velero +REGISTRY ?= quay.io/rh_ee_jvaikath/velero-play # In order to push images to an insecure registry, follow the two steps: # 1. Set "INSECURE_REGISTRY=true" # 2. Provide your own buildx builder instance by setting "BUILDX_INSTANCE=your-own-builder-instance" diff --git a/pkg/backup/actions/backup_pv_action.go b/pkg/backup/actions/backup_pv_action.go index 618abb572..608a3f9a4 100644 --- a/pkg/backup/actions/backup_pv_action.go +++ b/pkg/backup/actions/backup_pv_action.go @@ -76,10 +76,25 @@ func (a *PVCAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runti pvc.Spec.Selector = nil } - // remove label selectors with "velero.io/" prefixing in the key which is left by Velero restore + // Clean stale Velero labels from PVC metadata and selector + a.cleanupStaleVeleroLabels(pvc, backup) + + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc) + if err != nil { + return nil, nil, errors.Wrap(err, "unable to convert pvc to unstructured item") + } + + return &unstructured.Unstructured{Object: pvcMap}, actionhelpers.RelatedItemsForPVC(pvc, a.log), nil +} + +// cleanupStaleVeleroLabels removes stale Velero labels from both the PVC metadata +// and the selector's match labels to ensure clean backups +func (a *PVCAction) cleanupStaleVeleroLabels(pvc *corev1api.PersistentVolumeClaim, backup *v1.Backup) { + // Clean stale Velero labels from selector match labels if pvc.Spec.Selector != nil && pvc.Spec.Selector.MatchLabels != nil { for k := range pvc.Spec.Selector.MatchLabels { if strings.HasPrefix(k, "velero.io/") { + a.log.Infof("Deleting stale Velero label %s from PVC %s selector", k, pvc.Name) delete(pvc.Spec.Selector.MatchLabels, k) } } @@ -115,11 +130,4 @@ func (a *PVCAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runti } } } - - pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc) - if err != nil { - return nil, nil, errors.Wrap(err, "unable to convert pvc to unstructured item") - } - - return &unstructured.Unstructured{Object: pvcMap}, actionhelpers.RelatedItemsForPVC(pvc, a.log), nil } diff --git a/pkg/backup/actions/backup_pv_action_test.go b/pkg/backup/actions/backup_pv_action_test.go index 7c8f596b3..7b2f9a089 100644 --- a/pkg/backup/actions/backup_pv_action_test.go +++ b/pkg/backup/actions/backup_pv_action_test.go @@ -149,3 +149,176 @@ func TestBackupPVAction(t *testing.T) { require.NoError(t, err) assert.Empty(t, additional) } + +func TestCleanupStaleVeleroLabels(t *testing.T) { + tests := []struct { + name string + inputPVC *corev1api.PersistentVolumeClaim + backup *v1.Backup + expectedLabels map[string]string + expectedSelector *metav1.LabelSelector + }{ + { + name: "removes restore-name labels", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "velero.io/restore-name": "old-restore", + "app": "myapp", + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "app": "myapp", + }, + }, + { + name: "removes backup-name labels that don't match current backup", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "velero.io/backup-name": "old-backup", + "app": "myapp", + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "app": "myapp", + }, + }, + { + name: "keeps backup-name labels that match current backup", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "velero.io/backup-name": "current-backup", + "app": "myapp", + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "velero.io/backup-name": "current-backup", + "app": "myapp", + }, + }, + { + name: "removes volume-snapshot-name labels", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "velero.io/volume-snapshot-name": "old-snapshot", + "app": "myapp", + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "app": "myapp", + }, + }, + { + name: "removes velero labels from selector match labels", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "velero.io/restore-name": "old-restore", + "velero.io/backup-name": "old-backup", + "app": "myapp", + }, + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: nil, + expectedSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "myapp", + }, + }, + }, + { + name: "handles PVC with no labels", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: nil, + }, + { + name: "handles PVC with no selector", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "app": "myapp", + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "app": "myapp", + }, + expectedSelector: nil, + }, + { + name: "removes multiple stale velero labels", + inputPVC: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvc", + Labels: map[string]string{ + "velero.io/restore-name": "old-restore", + "velero.io/backup-name": "old-backup", + "velero.io/volume-snapshot-name": "old-snapshot", + "app": "myapp", + "env": "prod", + }, + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "velero.io/restore-name": "old-restore", + "app": "myapp", + }, + }, + }, + }, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + expectedLabels: map[string]string{ + "app": "myapp", + "env": "prod", + }, + expectedSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "myapp", + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + action := NewPVCAction(velerotest.NewLogger()) + + // Create a copy of the input PVC to avoid modifying the test case + pvcCopy := tc.inputPVC.DeepCopy() + + action.cleanupStaleVeleroLabels(pvcCopy, tc.backup) + + assert.Equal(t, tc.expectedLabels, pvcCopy.Labels, "Labels should match expected values") + assert.Equal(t, tc.expectedSelector, pvcCopy.Spec.Selector, "Selector should match expected values") + }) + } +}