restic: only backup ReadWriteMany PVC's once per velero backup (#1896)

* restic: only backup ReadWriteMany PVC's once per velero backup

Signed-off-by: Steve Kriss <krisss@vmware.com>

changelogs/unreleased/1896-skriss

@@ -0,0 +1 @@
+restic: only backup read-write-many PVCs at most once, even if they're annotated for backup from multiple pods.

pkg/backup/backup_test.go

@@ -22,6 +22,7 @@ import (
 	"compress/gzip"
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"sort"
@@ -2099,22 +2100,24 @@ func TestBackupWithHooks(t *testing.T) {
 	}
 }
 
-type fakeResticBackupperFactory struct {
-	podVolumeBackups []*velerov1.PodVolumeBackup
-}
+type fakeResticBackupperFactory struct{}
 
 func (f *fakeResticBackupperFactory) NewBackupper(context.Context, *velerov1.Backup) (restic.Backupper, error) {
-	return &fakeResticBackupper{
-		podVolumeBackups: f.podVolumeBackups,
-	}, nil
+	return &fakeResticBackupper{}, nil
 }
 
-type fakeResticBackupper struct {
-	podVolumeBackups []*velerov1.PodVolumeBackup
-}
+type fakeResticBackupper struct{}
 
-func (b *fakeResticBackupper) BackupPodVolumes(backup *velerov1.Backup, pod *corev1.Pod, _ logrus.FieldLogger) ([]*velerov1.PodVolumeBackup, []error) {
-	return b.podVolumeBackups, nil
+// BackupPodVolumes returns one pod volume backup per entry in volumes, with namespace "velero"
+// and name "pvb-<pod-namespace>-<pod-name>-<volume-name>".
+func (b *fakeResticBackupper) BackupPodVolumes(backup *velerov1.Backup, pod *corev1.Pod, volumes []string, _ logrus.FieldLogger) ([]*velerov1.PodVolumeBackup, []error) {
+	var res []*velerov1.PodVolumeBackup
+	for _, vol := range volumes {
+		pvb := builder.ForPodVolumeBackup("velero", fmt.Sprintf("pvb-%s-%s-%s", pod.Namespace, pod.Name, vol)).Result()
+		res = append(res, pvb)
+	}
+
+	return res, nil
 }
 
 // TestBackupWithRestic runs backups of pods that are annotated for restic backup,
@@ -2139,7 +2142,28 @@ func TestBackupWithRestic(t *testing.T) {
 				),
 			},
 			want: []*velerov1.PodVolumeBackup{
-				builder.ForPodVolumeBackup("velero", "pvb-1").Result(),
+				builder.ForPodVolumeBackup("velero", "pvb-ns-1-pod-1-foo").Result(),
+			},
+		},
+		{
+			name:   "when a PVC is used by two pods and annotated for restic backup on both, only one should be backed up",
+			backup: defaultBackup().Result(),
+			apiResources: []*test.APIResource{
+				test.Pods(
+					builder.ForPod("ns-1", "pod-1").
+						ObjectMeta(builder.WithAnnotations("backup.velero.io/backup-volumes", "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-1-foo").Result(),
 			},
 		},
 		{
@@ -2174,8 +2198,8 @@ func TestBackupWithRestic(t *testing.T) {
 					WithVolume("pv-2", "vol-2", "", "type-1", 100, false),
 			},
 			want: []*velerov1.PodVolumeBackup{
-				builder.ForPodVolumeBackup("velero", "pvb-1").Result(),
-				builder.ForPodVolumeBackup("velero", "pvb-2").Result(),
+				builder.ForPodVolumeBackup("velero", "pvb-ns-1-pod-1-vol-1").Result(),
+				builder.ForPodVolumeBackup("velero", "pvb-ns-1-pod-1-vol-2").Result(),
 			},
 		},
 	}
@@ -2188,9 +2212,7 @@ func TestBackupWithRestic(t *testing.T) {
 				backupFile = bytes.NewBuffer([]byte{})
 			)
 
-			h.backupper.resticBackupperFactory = &fakeResticBackupperFactory{
-				podVolumeBackups: tc.want,
-			}
+			h.backupper.resticBackupperFactory = new(fakeResticBackupperFactory)
 
 			for _, resource := range tc.apiResources {
 				h.addItems(t, resource)

pkg/backup/item_backupper.go

@@ -184,11 +184,24 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 			// nil it on error since it's not valid
 			pod = nil
 		} else {
-			// get the volumes to backup using restic, and add any of them that are PVCs to 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 restic.
-			resticVolumesToBackup = restic.GetVolumesToBackup(pod)
+			// Get the list of volumes to back up using restic from the pod's annotations. Remove from this list
+			// 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 restic.GetVolumesToBackup(pod) {
+				if found, pvcName := ib.resticSnapshotTracker.HasPVCForPodVolume(pod, volume); found {
+					log.WithFields(map[string]interface{}{
+						"podVolume": volume,
+						"pvcName":   pvcName,
+					}).Info("Pod volume uses a persistent volume claim which has already been backed up with restic from another pod, skipping.")
+					continue
+				}
 
+				resticVolumesToBackup = append(resticVolumesToBackup, volume)
+			}
+
+			// 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 restic.
 			ib.resticSnapshotTracker.Track(pod, resticVolumesToBackup)
 		}
 	}
@@ -280,7 +293,7 @@ func (ib *defaultItemBackupper) backupPodVolumes(log logrus.FieldLogger, pod *co
 		return nil, nil
 	}
 
-	return ib.resticBackupper.BackupPodVolumes(ib.backupRequest.Backup, pod, log)
+	return ib.resticBackupper.BackupPodVolumes(ib.backupRequest.Backup, pod, volumes, log)
 }
 
 func (ib *defaultItemBackupper) executeActions(
@@ -394,7 +407,7 @@ func (ib *defaultItemBackupper) takePVSnapshot(obj runtime.Unstructured, log log
 	// of this PV. If so, don't take a snapshot.
 	if pv.Spec.ClaimRef != nil {
 		if ib.resticSnapshotTracker.Has(pv.Spec.ClaimRef.Namespace, pv.Spec.ClaimRef.Name) {
-			log.Info("Skipping persistent volume snapshot because volume has already been backed up with restic.")
+			log.Info("Skipping snapshot of persistent volume because volume is being backed up with restic.")
 			return nil
 		}
 	}

pkg/backup/pvc_snapshot_tracker.go

@@ -56,6 +56,28 @@ func (t *pvcSnapshotTracker) Has(namespace, name string) bool {
 	return t.pvcs.Has(key(namespace, name))
 }
 
+// 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) {
+	for _, podVolume := range pod.Spec.Volumes {
+		if podVolume.Name != volume {
+			continue
+		}
+
+		if podVolume.PersistentVolumeClaim == nil {
+			return false, ""
+		}
+
+		if !t.pvcs.Has(key(pod.Namespace, podVolume.PersistentVolumeClaim.ClaimName)) {
+			return false, ""
+		}
+
+		return true, podVolume.PersistentVolumeClaim.ClaimName
+	}
+
+	return false, ""
+}
+
 func key(namespace, name string) string {
 	return fmt.Sprintf("%s/%s", namespace, name)
 }

pkg/restic/backupper.go

@@ -35,8 +35,8 @@ import (
 
 // Backupper can execute restic backups of volumes in a pod.
 type Backupper interface {
-	// BackupPodVolumes backs up all annotated volumes in a pod.
-	BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, []error)
+	// BackupPodVolumes backs up all specified volumes in a pod.
+	BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, volumesToBackup []string, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, []error)
 }
 
 type backupper struct {
@@ -96,9 +96,7 @@ func resultsKey(ns, name string) string {
 	return fmt.Sprintf("%s/%s", ns, name)
 }
 
-func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, []error) {
-	// get volumes to backup from pod's annotations
-	volumesToBackup := GetVolumesToBackup(pod)
+func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, volumesToBackup []string, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, []error) {
 	if len(volumesToBackup) == 0 {
 		return nil, nil
 	}