diff --git a/changelogs/unreleased/5894-Lyndon-Li b/changelogs/unreleased/5894-Lyndon-Li new file mode 100644 index 0000000000..62f9a6bade --- /dev/null +++ b/changelogs/unreleased/5894-Lyndon-Li @@ -0,0 +1 @@ +This is to fix issue 5881, enhance the PVB tracker in two modes, Track and Taken \ No newline at end of file diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 24dacce5f5..5b37ff1ba9 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -2624,6 +2624,12 @@ type fakePodVolumeBackupper struct{} // and name "pvb---". 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(), diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index 54d68db1bc..85d2766c79 100644 --- a/pkg/backup/item_backupper.go +++ b/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) } } diff --git a/pkg/backup/pvc_snapshot_tracker.go b/pkg/backup/pvc_snapshot_tracker.go index 3f0cbaeec9..9fd9efe2b1 100644 --- a/pkg/backup/pvc_snapshot_tracker.go +++ b/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, "" }