From 74a0b39e3e4cd5115bef42e1d582ef28166fcc55 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Thu, 17 Jun 2021 14:00:37 -0400 Subject: [PATCH] Skip volume restores from projected sources (#3877) In #3863, it was discovered that volumes from projected sources were being backed up by restic when they should have been skipped. Restoring these volumes triggers a known bug in restic. In #3866, we started skipping volumes from a projected source, however there will exist backups that were taken before this fix was introduced. This change modifies the restore logic to skip the restore of any volume that came from a projected source, allowing backups taken before #3866 to be restored successfully. Signed-off-by: Bridget McErlean --- changelogs/unreleased/3877-zubron | 1 + pkg/restic/common.go | 20 +++++- pkg/restic/common_test.go | 101 ++++++++++++++++++++++++++++++ pkg/restore/restore.go | 12 +++- 4 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/3877-zubron diff --git a/changelogs/unreleased/3877-zubron b/changelogs/unreleased/3877-zubron new file mode 100644 index 000000000..c4020a4bc --- /dev/null +++ b/changelogs/unreleased/3877-zubron @@ -0,0 +1 @@ +Skip the restore of volumes that originally came from a projected volume when using restic. \ No newline at end of file diff --git a/pkg/restic/common.go b/pkg/restic/common.go index 5ce15e32e..e122ff4bb 100644 --- a/pkg/restic/common.go +++ b/pkg/restic/common.go @@ -100,9 +100,20 @@ func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, podName string, namespace s return podName == pvb.Spec.Pod.Name && namespace == pvb.Spec.Pod.Namespace } +// volumeIsProjected checks if the given volume exists in the list of podVolumes +// and returns true if the volume has a projected source +func volumeIsProjected(volumeName string, podVolumes []corev1api.Volume) bool { + for _, volume := range podVolumes { + if volume.Name == volumeName && volume.Projected != nil { + return true + } + } + return false +} + // GetVolumeBackupsForPod returns a map, of volume name -> snapshot id, // of the PodVolumeBackups that exist for the provided pod. -func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod metav1.Object, sourcePodNs string) map[string]string { +func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod *corev1api.Pod, sourcePodNs string) map[string]string { volumes := make(map[string]string) for _, pvb := range podVolumeBackups { @@ -116,6 +127,13 @@ func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod continue } + // If the volume came from a projected source, skip its restore. + // This allows backups affected by https://github.com/vmware-tanzu/velero/issues/3863 + // to be restored successfully. + if volumeIsProjected(pvb.Spec.Volume, pod.Spec.Volumes) { + continue + } + volumes[pvb.Spec.Volume] = pvb.Status.SnapshotID } diff --git a/pkg/restic/common_test.go b/pkg/restic/common_test.go index b5e0f1ff9..7e102ca97 100644 --- a/pkg/restic/common_test.go +++ b/pkg/restic/common_test.go @@ -37,6 +37,7 @@ func TestGetVolumeBackupsForPod(t *testing.T) { tests := []struct { name string podVolumeBackups []*velerov1api.PodVolumeBackup + podVolumes []corev1api.Volume podAnnotations map[string]string podName string sourcePodNs string @@ -127,6 +128,30 @@ func TestGetVolumeBackupsForPod(t *testing.T) { sourcePodNs: "TestNS", expected: map[string]string{"pvbtest1-foo": "snapshot1"}, }, + { + name: "volumes from PVBs that correspond to a pod volume from a projected source are not returned", + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvb-non-projected").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvb-projected").Result(), + }, + podVolumes: []corev1api.Volume{ + { + Name: "pvb-non-projected", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{}, + }, + }, + { + Name: "pvb-projected", + VolumeSource: corev1api.VolumeSource{ + Projected: &corev1api.ProjectedVolumeSource{}, + }, + }, + }, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvb-non-projected": "snapshot1"}, + }, } for _, test := range tests { @@ -134,6 +159,7 @@ func TestGetVolumeBackupsForPod(t *testing.T) { pod := &corev1api.Pod{} pod.Annotations = test.podAnnotations pod.Name = test.podName + pod.Spec.Volumes = test.podVolumes res := GetVolumeBackupsForPod(test.podVolumeBackups, pod, test.sourcePodNs) assert.Equal(t, test.expected, res) @@ -629,3 +655,78 @@ func TestIsPVBMatchPod(t *testing.T) { } } + +func TestVolumeIsProjected(t *testing.T) { + testCases := []struct { + name string + volumeName string + podVolumes []corev1api.Volume + expected bool + }{ + { + name: "volume name not in list of volumes", + volumeName: "missing-volume", + podVolumes: []corev1api.Volume{ + { + Name: "non-projected", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{}, + }, + }, + { + Name: "projected", + VolumeSource: corev1api.VolumeSource{ + Projected: &corev1api.ProjectedVolumeSource{}, + }, + }, + }, + expected: false, + }, + { + name: "volume name in list of volumes but not projected", + volumeName: "non-projected", + podVolumes: []corev1api.Volume{ + { + Name: "non-projected", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{}, + }, + }, + { + Name: "projected", + VolumeSource: corev1api.VolumeSource{ + Projected: &corev1api.ProjectedVolumeSource{}, + }, + }, + }, + expected: false, + }, + { + name: "volume name in list of volumes and projected", + volumeName: "projected", + podVolumes: []corev1api.Volume{ + { + Name: "non-projected", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{}, + }, + }, + { + Name: "projected", + VolumeSource: corev1api.VolumeSource{ + Projected: &corev1api.ProjectedVolumeSource{}, + }, + }, + }, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual := volumeIsProjected(tc.volumeName, tc.podVolumes) + assert.Equal(t, tc.expected, actual) + }) + + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 4817dca38..41aee1938 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1232,8 +1232,16 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs } - if groupResource == kuberesource.Pods && len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, obj, originalNamespace)) > 0 { - restorePodVolumeBackups(ctx, createdObj, originalNamespace) + if groupResource == kuberesource.Pods { + pod := new(v1.Pod) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), pod); err != nil { + errs.Add(namespace, err) + return warnings, errs + } + + if len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, pod, originalNamespace)) > 0 { + restorePodVolumeBackups(ctx, createdObj, originalNamespace) + } } if groupResource == kuberesource.Pods {