From bfb431fcdf3654fea02554d6034465b59c9de53f Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 2 Sep 2025 10:49:59 -0400 Subject: [PATCH 1/8] Clean up stale labels before backing up PVC Signed-off-by: Joseph --- pkg/backup/actions/backup_pv_action.go | 31 ++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/backup/actions/backup_pv_action.go b/pkg/backup/actions/backup_pv_action.go index 4b8a44ef6..618abb572 100644 --- a/pkg/backup/actions/backup_pv_action.go +++ b/pkg/backup/actions/backup_pv_action.go @@ -85,6 +85,37 @@ func (a *PVCAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runti } } + // Clean stale Velero labels from main metadata + if pvc.Labels != nil { + for k, v := range pvc.Labels { + if strings.HasPrefix(k, "velero.io/") { + // Only remove labels that are clearly stale from previous operations + shouldRemove := false + + // Always remove restore-name labels as these are from previous restores + if k == "velero.io/restore-name" { + shouldRemove = true + } + + // Remove backup-name labels that don't match current backup + if k == "velero.io/backup-name" && v != backup.Name { + shouldRemove = true + } + + // Remove volume-snapshot-name labels from previous CSI backups + // Note: If this backup creates new CSI snapshots, the CSI action will add them back + if k == "velero.io/volume-snapshot-name" { + shouldRemove = true + } + + if shouldRemove { + a.log.Infof("Deleting stale Velero label %s=%s from PVC %s", k, v, pvc.Name) + delete(pvc.Labels, k) + } + } + } + } + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc) if err != nil { return nil, nil, errors.Wrap(err, "unable to convert pvc to unstructured item") From e7166fc9e9f8a8752a0028cb2233416359f650e7 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 2 Sep 2025 13:42:48 -0400 Subject: [PATCH 2/8] Add changelog Signed-off-by: Joseph --- changelogs/unreleased/9206-Joeavaikath | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/9206-Joeavaikath diff --git a/changelogs/unreleased/9206-Joeavaikath b/changelogs/unreleased/9206-Joeavaikath new file mode 100644 index 000000000..d080ddf8b --- /dev/null +++ b/changelogs/unreleased/9206-Joeavaikath @@ -0,0 +1 @@ +Remove labels associated with previous backups From 643dd784ea781932d811b59409dc0cc15c80f1e4 Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 4 Sep 2025 14:33:43 -0400 Subject: [PATCH 3/8] 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") + }) + } +} From db2193c53aefefce38be6577af01db3840cf47d7 Mon Sep 17 00:00:00 2001 From: Joseph Date: Mon, 8 Sep 2025 10:00:44 -0400 Subject: [PATCH 4/8] Make update Signed-off-by: Joseph --- pkg/backup/actions/backup_pv_action_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/backup/actions/backup_pv_action_test.go b/pkg/backup/actions/backup_pv_action_test.go index 7b2f9a089..41af9e9bf 100644 --- a/pkg/backup/actions/backup_pv_action_test.go +++ b/pkg/backup/actions/backup_pv_action_test.go @@ -152,10 +152,10 @@ func TestBackupPVAction(t *testing.T) { func TestCleanupStaleVeleroLabels(t *testing.T) { tests := []struct { - name string - inputPVC *corev1api.PersistentVolumeClaim - backup *v1.Backup - expectedLabels map[string]string + name string + inputPVC *corev1api.PersistentVolumeClaim + backup *v1.Backup + expectedLabels map[string]string expectedSelector *metav1.LabelSelector }{ { @@ -239,7 +239,7 @@ func TestCleanupStaleVeleroLabels(t *testing.T) { }, }, }, - backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, + backup: &v1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "current-backup"}}, expectedLabels: nil, expectedSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ @@ -311,10 +311,10 @@ func TestCleanupStaleVeleroLabels(t *testing.T) { 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") From 87dbc16b0aeb7291e44b1affd33b0c8a8dcbb5eb Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 16 Oct 2025 12:29:59 -0400 Subject: [PATCH 5/8] Revert registry local debug value Signed-off-by: Joseph --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 55069d0ae..82129623d 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ BIN ?= velero PKG := github.com/vmware-tanzu/velero # Where to push the docker image. -REGISTRY ?= quay.io/rh_ee_jvaikath/velero-play +REGISTRY ?= velero # 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" From eae5bea469186747c495b8332d3f87c97070b5ae Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 22 Oct 2025 11:20:34 -0400 Subject: [PATCH 6/8] Remove backup.velero.io/must-include-additional-items label Signed-off-by: Joseph --- pkg/backup/actions/backup_pv_action.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/backup/actions/backup_pv_action.go b/pkg/backup/actions/backup_pv_action.go index 608a3f9a4..03baf895f 100644 --- a/pkg/backup/actions/backup_pv_action.go +++ b/pkg/backup/actions/backup_pv_action.go @@ -112,6 +112,10 @@ func (a *PVCAction) cleanupStaleVeleroLabels(pvc *corev1api.PersistentVolumeClai shouldRemove = true } + if k == "backup.velero.io/must-include-additional-items" { + shouldRemove = true + } + // Remove backup-name labels that don't match current backup if k == "velero.io/backup-name" && v != backup.Name { shouldRemove = true From 2d8a87fec43162cc7de19887b5adf627f95eab15 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 22 Oct 2025 11:21:48 -0400 Subject: [PATCH 7/8] Add condition to catch backup.velero.io labels Signed-off-by: Joseph --- pkg/backup/actions/backup_pv_action.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/backup/actions/backup_pv_action.go b/pkg/backup/actions/backup_pv_action.go index 03baf895f..1ba7669d4 100644 --- a/pkg/backup/actions/backup_pv_action.go +++ b/pkg/backup/actions/backup_pv_action.go @@ -103,7 +103,7 @@ func (a *PVCAction) cleanupStaleVeleroLabels(pvc *corev1api.PersistentVolumeClai // Clean stale Velero labels from main metadata if pvc.Labels != nil { for k, v := range pvc.Labels { - if strings.HasPrefix(k, "velero.io/") { + if strings.HasPrefix(k, "velero.io/") || strings.HasPrefix(k, "backup.velero.io/") { // Only remove labels that are clearly stale from previous operations shouldRemove := false From 65eaceee0bcaf6d593b0a2cdac47522ab48e024a Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 19 Dec 2025 20:16:44 +0700 Subject: [PATCH 8/8] Refactor cleanupStaleVeleroLabels to use constants for label keys Signed-off-by: Tiger Kaovilai --- pkg/backup/actions/backup_pv_action.go | 44 ++++++++++++-------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/pkg/backup/actions/backup_pv_action.go b/pkg/backup/actions/backup_pv_action.go index 1ba7669d4..c3f378fac 100644 --- a/pkg/backup/actions/backup_pv_action.go +++ b/pkg/backup/actions/backup_pv_action.go @@ -103,34 +103,32 @@ func (a *PVCAction) cleanupStaleVeleroLabels(pvc *corev1api.PersistentVolumeClai // Clean stale Velero labels from main metadata if pvc.Labels != nil { for k, v := range pvc.Labels { - if strings.HasPrefix(k, "velero.io/") || strings.HasPrefix(k, "backup.velero.io/") { - // Only remove labels that are clearly stale from previous operations - shouldRemove := false + // Only remove labels that are clearly stale from previous operations + shouldRemove := false - // Always remove restore-name labels as these are from previous restores - if k == "velero.io/restore-name" { - shouldRemove = true - } + // Always remove restore-name labels as these are from previous restores + if k == v1.RestoreNameLabel { + shouldRemove = true + } - if k == "backup.velero.io/must-include-additional-items" { - shouldRemove = true - } + if k == v1.MustIncludeAdditionalItemAnnotation { + shouldRemove = true + } - // Remove backup-name labels that don't match current backup - if k == "velero.io/backup-name" && v != backup.Name { - shouldRemove = true - } + // Remove backup-name labels that don't match current backup + if k == v1.BackupNameLabel && v != backup.Name { + shouldRemove = true + } - // Remove volume-snapshot-name labels from previous CSI backups - // Note: If this backup creates new CSI snapshots, the CSI action will add them back - if k == "velero.io/volume-snapshot-name" { - shouldRemove = true - } + // Remove volume-snapshot-name labels from previous CSI backups + // Note: If this backup creates new CSI snapshots, the CSI action will add them back + if k == v1.VolumeSnapshotLabel { + shouldRemove = true + } - if shouldRemove { - a.log.Infof("Deleting stale Velero label %s=%s from PVC %s", k, v, pvc.Name) - delete(pvc.Labels, k) - } + if shouldRemove { + a.log.Infof("Deleting stale Velero label %s=%s from PVC %s", k, v, pvc.Name) + delete(pvc.Labels, k) } } }