Merge pull request #5894 from Lyndon-Li/issue-fix-5881

Fix issue 5881

changelogs/unreleased/5894-Lyndon-Li

@@ -0,0 +1 @@
+This is to fix issue 5881, enhance the PVB tracker in two modes, Track and Taken

pkg/backup/backup_test.go

@@ -2624,6 +2624,12 @@ type fakePodVolumeBackupper struct{}
 // and name "pvb-<pod-namespace>-<pod-name>-<volume-name>".
 func (b *fakePodVolumeBackupper) BackupPodVolumes(backup *velerov1.Backup, pod *corev1.Pod, volumes []string, _ logrus.FieldLogger) ([]*velerov1.PodVolumeBackup, []error) {
 	var res []*velerov1.PodVolumeBackup
+
+	anno := pod.GetAnnotations()
+	if anno != nil && anno["backup.velero.io/bakupper-skip"] != "" {
+		return res, nil
+	}
+
 	for _, vol := range volumes {
 		pvb := builder.ForPodVolumeBackup("velero", fmt.Sprintf("pvb-%s-%s-%s", pod.Namespace, pod.Name, vol)).Volume(vol).Result()
 		res = append(res, pvb)
@@ -2678,6 +2684,27 @@ func TestBackupWithPodVolume(t *testing.T) {
 				builder.ForPodVolumeBackup("velero", "pvb-ns-1-pod-1-foo").Volume("foo").Result(),
 			},
 		},
+		{
+			name:   "when a PVC is used by two pods and annotated for pod volume backup on both, the backup for pod1 gives up the PVC, the backup for pod2 should include it",
+			backup: defaultBackup().Result(),
+			apiResources: []*test.APIResource{
+				test.Pods(
+					builder.ForPod("ns-1", "pod-1").
+						ObjectMeta(builder.WithAnnotations("backup.velero.io/backup-volumes", "foo", "backup.velero.io/bakupper-skip", "foo")).
+						Volumes(builder.ForVolume("foo").PersistentVolumeClaimSource("pvc-1").Result()).
+						Result(),
+				),
+				test.Pods(
+					builder.ForPod("ns-1", "pod-2").
+						ObjectMeta(builder.WithAnnotations("backup.velero.io/backup-volumes", "bar")).
+						Volumes(builder.ForVolume("bar").PersistentVolumeClaimSource("pvc-1").Result()).
+						Result(),
+				),
+			},
+			want: []*velerov1.PodVolumeBackup{
+				builder.ForPodVolumeBackup("velero", "pvb-ns-1-pod-2-bar").Volume("bar").Result(),
+			},
+		},
 		{
 			name:   "when PVC pod volumes are backed up using pod volume backup, their claimed PVs are not also snapshotted",
 			backup: defaultBackup().Result(),

pkg/backup/item_backupper.go

@@ -154,7 +154,12 @@ func (ib *itemBackupper) backupItem(logger logrus.FieldLogger, obj runtime.Unstr
 			// any volumes that use a PVC that we've already backed up (this would be in a read-write-many scenario,
 			// where it's been backed up from another pod), since we don't need >1 backup per PVC.
 			for _, volume := range podvolume.GetVolumesByPod(pod, boolptr.IsSetToTrue(ib.backupRequest.Spec.DefaultVolumesToFsBackup)) {
-				if found, pvcName := ib.podVolumeSnapshotTracker.HasPVCForPodVolume(pod, volume); found {
+				// track the volumes that are PVCs using the PVC snapshot tracker, so that when we backup PVCs/PVs
+				// via an item action in the next step, we don't snapshot PVs that will have their data backed up
+				// with pod volume backup.
+				ib.podVolumeSnapshotTracker.Track(pod, volume)
+
+				if found, pvcName := ib.podVolumeSnapshotTracker.TakenForPodVolume(pod, volume); found {
 					log.WithFields(map[string]interface{}{
 						"podVolume": volume,
 						"pvcName":   pvcName,
@@ -207,11 +212,9 @@ func (ib *itemBackupper) backupItem(logger logrus.FieldLogger, obj runtime.Unstr
 		ib.backupRequest.PodVolumeBackups = append(ib.backupRequest.PodVolumeBackups, podVolumeBackups...)
 		backupErrs = append(backupErrs, errs...)
 
-		// track the volumes that are PVCs using the PVC snapshot tracker, so that when we backup PVCs/PVs
-		// via an item action in the next step, we don't snapshot PVs that will have their data backed up
-		// with pod volume backup.
+		// Mark the volumes that has been processed by pod volume backup as Taken in the tracker.
 		for _, pvb := range podVolumeBackups {
-			ib.podVolumeSnapshotTracker.Track(pod, []string{pvb.Spec.Volume})
+			ib.podVolumeSnapshotTracker.Take(pod, pvb.Spec.Volume)
 		}
 	}
 

pkg/backup/pvc_snapshot_tracker.go

@@ -20,45 +20,60 @@ import (
 	"fmt"
 
 	corev1api "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // pvcSnapshotTracker keeps track of persistent volume claims that have been snapshotted
 // with pod volume backup.
 type pvcSnapshotTracker struct {
-	pvcs sets.String
+	pvcs map[string]pvcSnapshotStatus
+}
+
+type pvcSnapshotStatus struct {
+	taken bool
 }
 
 func newPVCSnapshotTracker() *pvcSnapshotTracker {
 	return &pvcSnapshotTracker{
-		pvcs: sets.NewString(),
+		pvcs: make(map[string]pvcSnapshotStatus),
 	}
 }
 
-// Track takes a pod and a list of volumes from that pod that were snapshotted, and
-// tracks each snapshotted volume that's a PVC.
-func (t *pvcSnapshotTracker) Track(pod *corev1api.Pod, snapshottedVolumes []string) {
-	for _, volumeName := range snapshottedVolumes {
-		// if the volume is a PVC, track it
-		for _, volume := range pod.Spec.Volumes {
-			if volume.Name == volumeName {
-				if volume.PersistentVolumeClaim != nil {
-					t.pvcs.Insert(key(pod.Namespace, volume.PersistentVolumeClaim.ClaimName))
+// Track indicates a volume from a pod should be snapshotted by pod volume backup.
+func (t *pvcSnapshotTracker) Track(pod *corev1api.Pod, volumeName string) {
+	// if the volume is a PVC, track it
+	for _, volume := range pod.Spec.Volumes {
+		if volume.Name == volumeName {
+			if volume.PersistentVolumeClaim != nil {
+				if _, ok := t.pvcs[key(pod.Namespace, volume.PersistentVolumeClaim.ClaimName)]; !ok {
+					t.pvcs[key(pod.Namespace, volume.PersistentVolumeClaim.ClaimName)] = pvcSnapshotStatus{false}
 				}
-				break
 			}
+			break
+		}
+	}
+}
+
+// Take indicates a volume from a pod has been taken by pod volume backup.
+func (t *pvcSnapshotTracker) Take(pod *corev1api.Pod, volumeName string) {
+	for _, volume := range pod.Spec.Volumes {
+		if volume.Name == volumeName {
+			if volume.PersistentVolumeClaim != nil {
+				t.pvcs[key(pod.Namespace, volume.PersistentVolumeClaim.ClaimName)] = pvcSnapshotStatus{true}
+			}
+			break
 		}
 	}
 }
 
 // Has returns true if the PVC with the specified namespace and name has been tracked.
 func (t *pvcSnapshotTracker) Has(namespace, name string) bool {
-	return t.pvcs.Has(key(namespace, name))
+	_, found := t.pvcs[key(namespace, name)]
+	return found
 }
 
-// HasPVCForPodVolume returns true and the PVC's name if the pod volume with the specified name uses a
-// PVC and that PVC has been tracked.
-func (t *pvcSnapshotTracker) HasPVCForPodVolume(pod *corev1api.Pod, volume string) (bool, string) {
+// TakenForPodVolume returns true and the PVC's name if the pod volume with the specified name uses a
+// PVC and that PVC has been taken by pod volume backup.
+func (t *pvcSnapshotTracker) TakenForPodVolume(pod *corev1api.Pod, volume string) (bool, string) {
 	for _, podVolume := range pod.Spec.Volumes {
 		if podVolume.Name != volume {
 			continue
@@ -68,7 +83,12 @@ func (t *pvcSnapshotTracker) HasPVCForPodVolume(pod *corev1api.Pod, volume strin
 			return false, ""
 		}
 
-		if !t.pvcs.Has(key(pod.Namespace, podVolume.PersistentVolumeClaim.ClaimName)) {
+		status, found := t.pvcs[key(pod.Namespace, podVolume.PersistentVolumeClaim.ClaimName)]
+		if !found {
+			return false, ""
+		}
+
+		if !status.taken {
 			return false, ""
 		}