Merge pull request #9206 from Joeavaikath/label-cleanup

Remove labels associated with previous backups

changelogs/unreleased/9206-Joeavaikath

@@ -0,0 +1 @@
+Remove labels associated with previous backups

pkg/backup/actions/backup_pv_action.go

@@ -76,14 +76,8 @@ 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
-	if pvc.Spec.Selector != nil && pvc.Spec.Selector.MatchLabels != nil {
-		for k := range pvc.Spec.Selector.MatchLabels {
-			if strings.HasPrefix(k, "velero.io/") {
-				delete(pvc.Spec.Selector.MatchLabels, k)
-			}
-		}
-	}
+	// Clean stale Velero labels from PVC metadata and selector
+	a.cleanupStaleVeleroLabels(pvc, backup)
 
 	pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc)
 	if err != nil {
@@ -92,3 +86,50 @@ func (a *PVCAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runti
 
 	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)
+			}
+		}
+	}
+
+	// Clean stale Velero labels from main metadata
+	if pvc.Labels != nil {
+		for k, v := range pvc.Labels {
+			// 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 == v1.RestoreNameLabel {
+				shouldRemove = true
+			}
+
+			if k == v1.MustIncludeAdditionalItemAnnotation {
+				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 == 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)
+			}
+		}
+	}
+}

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")
+		})
+	}
+}