From 557d1705416d7cf84b5ef0f017ab63acc07ba8f4 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Tue, 8 Oct 2019 15:16:35 -0600 Subject: [PATCH] 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 --- changelogs/unreleased/1896-skriss | 1 + pkg/backup/backup_test.go | 56 +++++++++++++++++++++--------- pkg/backup/item_backupper.go | 25 +++++++++---- pkg/backup/pvc_snapshot_tracker.go | 22 ++++++++++++ pkg/restic/backupper.go | 8 ++--- 5 files changed, 84 insertions(+), 28 deletions(-) create mode 100644 changelogs/unreleased/1896-skriss diff --git a/changelogs/unreleased/1896-skriss b/changelogs/unreleased/1896-skriss new file mode 100644 index 000000000..348c3b63f --- /dev/null +++ b/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. diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index a85a3cba0..13632ae18 100644 --- a/pkg/backup/backup_test.go +++ b/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---". +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) diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index 08a2da632..d22cdf781 100644 --- a/pkg/backup/item_backupper.go +++ b/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 } } diff --git a/pkg/backup/pvc_snapshot_tracker.go b/pkg/backup/pvc_snapshot_tracker.go index f5f266a91..d96264dbd 100644 --- a/pkg/backup/pvc_snapshot_tracker.go +++ b/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) } diff --git a/pkg/restic/backupper.go b/pkg/restic/backupper.go index d80d4f79f..25adb50d3 100644 --- a/pkg/restic/backupper.go +++ b/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 }