From 0246a32ad0897053fbc444d86a0bec65762bc714 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 22 Feb 2021 14:16:00 -0500 Subject: [PATCH] Use pod namespace from backup when matching PVBs (#3475) * Use pod namespace from backup when matching PVBs In #3051, we introduced an additional check to ensure that a PVB matched a particular pod by checking both the name and the namespace of the pod. This caused an issue when using a namespace mapping on restore. In the case where a namespace mapping is being used, the check for whether a PVB matches a particular pod will fail as the PVB was created for the original pod namespace and is not aware of the new namespace mapping being used. This resulted in PVRs not being created for pods that were being restored into new namespaces. The restic init containers were being created to wait on the volume restore, however this would cause the restored pods to block indefinitely as they would be waiting for a volume restore that was not scheduled. To fix this, we use the original namespace of the pod from the backup to match the PVB to the pod being restored, not the new namespace where the pod is being restored into. Fixes #3467. Signed-off-by: Bridget McErlean * Explain why the namespace mapping can't be used Signed-off-by: Bridget McErlean --- changelogs/unreleased/3475-zubron | 1 + pkg/restic/common.go | 8 +- pkg/restic/common_test.go | 130 ++++++++++++---------- pkg/restic/restorer.go | 2 +- pkg/restore/restic_restore_action.go | 11 +- pkg/restore/restic_restore_action_test.go | 60 +++++++++- pkg/restore/restore.go | 2 +- 7 files changed, 144 insertions(+), 70 deletions(-) create mode 100644 changelogs/unreleased/3475-zubron diff --git a/changelogs/unreleased/3475-zubron b/changelogs/unreleased/3475-zubron new file mode 100644 index 000000000..ed00ab9ca --- /dev/null +++ b/changelogs/unreleased/3475-zubron @@ -0,0 +1 @@ +Fixed a bug where restic volumes would not be restored when using a namespace mapping. \ No newline at end of file diff --git a/pkg/restic/common.go b/pkg/restic/common.go index 592f3a3b7..d248fe922 100644 --- a/pkg/restic/common.go +++ b/pkg/restic/common.go @@ -96,17 +96,17 @@ func getPodSnapshotAnnotations(obj metav1.Object) map[string]string { return res } -func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, pod metav1.Object) bool { - return pod.GetName() == pvb.Spec.Pod.Name && pod.GetNamespace() == pvb.Spec.Pod.Namespace +func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, podName string, namespace string) bool { + return podName == pvb.Spec.Pod.Name && namespace == pvb.Spec.Pod.Namespace } // 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) map[string]string { +func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod metav1.Object, sourcePodNs string) map[string]string { volumes := make(map[string]string) for _, pvb := range podVolumeBackups { - if !isPVBMatchPod(pvb, pod) { + if !isPVBMatchPod(pvb, pod.GetName(), sourcePodNs) { continue } diff --git a/pkg/restic/common_test.go b/pkg/restic/common_test.go index cde5e9d37..25e4d8ba4 100644 --- a/pkg/restic/common_test.go +++ b/pkg/restic/common_test.go @@ -40,78 +40,93 @@ func TestGetVolumeBackupsForPod(t *testing.T) { podVolumeBackups []*velerov1api.PodVolumeBackup podAnnotations map[string]string podName string + sourcePodNs string expected map[string]string }{ { - name: "nil annotations", + name: "nil annotations results in no volume backups returned", podAnnotations: nil, expected: nil, }, { - name: "empty annotations", + name: "empty annotations results in no volume backups returned", podAnnotations: make(map[string]string), expected: nil, }, { - name: "non-empty map, no snapshot annotation", + name: "pod annotations with no snapshot annotation prefix results in no volume backups returned", podAnnotations: map[string]string{"foo": "bar"}, expected: nil, }, { - name: "has snapshot annotation only, no suffix", - podAnnotations: map[string]string{podAnnotationPrefix: "bar"}, - expected: map[string]string{"": "bar"}, + name: "pod annotation with only snapshot annotation prefix, results in volume backup with empty volume key", + podAnnotations: map[string]string{podAnnotationPrefix: "snapshotID"}, + expected: map[string]string{"": "snapshotID"}, }, { - name: "has snapshot annotation only, with suffix", - podAnnotations: map[string]string{podAnnotationPrefix + "foo": "bar"}, - expected: map[string]string{"foo": "bar"}, + name: "pod annotation with snapshot annotation prefix results in volume backup with volume name and snapshot ID", + podAnnotations: map[string]string{podAnnotationPrefix + "volume": "snapshotID"}, + expected: map[string]string{"volume": "snapshotID"}, }, { - name: "has snapshot annotation, with suffix", - podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - expected: map[string]string{"foo": "bar", "abc": "123"}, + name: "only pod annotations with snapshot annotation prefix are considered", + podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "volume1": "snapshot1", podAnnotationPrefix + "volume2": "snapshot2"}, + expected: map[string]string{"volume1": "snapshot1", "volume2": "snapshot2"}, }, { - name: "has snapshot annotation, with suffix, and also PVBs", + name: "pod annotations are not considered if PVBs are provided", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), }, podName: "TestPod", + sourcePodNs: "TestNS", podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "no snapshot annotation, but with PVBs", + name: "volume backups are returned even if no pod annotations are present", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), }, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "no snapshot annotation, but with PVBs, some of which have snapshot IDs and some of which don't", + name: "only volumes from PVBs with snapshot IDs are returned", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), - builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").Volume("pvbtest3-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-4").PodName("TestPod").Volume("pvbtest4-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").PodNamespace("TestNS").Volume("pvbtest3-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-4").PodName("TestPod").PodNamespace("TestNS").Volume("pvbtest4-abc").Result(), }, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "has snapshot annotation, with suffix, and with PVBs from current pod and a PVB from another pod", + name: "only volumes from PVBs for the given pod are returned", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), - builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestAnotherPod").SnapshotID("xyz").Volume("pvbtest3-xyz").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestAnotherPod").SnapshotID("snapshot3").Volume("pvbtest3-xyz").Result(), }, - podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, + }, + { + name: "only volumes from PVBs which match the pod name and source pod namespace are returned", + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestAnotherPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").PodNamespace("TestAnotherNS").SnapshotID("snapshot3").Volume("pvbtest3-xyz").Result(), + }, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1"}, }, } @@ -121,7 +136,7 @@ func TestGetVolumeBackupsForPod(t *testing.T) { pod.Annotations = test.podAnnotations pod.Name = test.podName - res := GetVolumeBackupsForPod(test.podVolumeBackups, pod) + res := GetVolumeBackupsForPod(test.podVolumeBackups, pod, test.sourcePodNs) assert.Equal(t, test.expected, res) }) } @@ -558,17 +573,14 @@ func TestGetPodVolumesUsingRestic(t *testing.T) { func TestIsPVBMatchPod(t *testing.T) { testCases := []struct { - name string - pod metav1.Object - pvb velerov1api.PodVolumeBackup - expected bool + name string + pvb velerov1api.PodVolumeBackup + podName string + sourcePodNs string + expected bool }{ { name: "should match PVB and pod", - pod: &metav1.ObjectMeta{ - Name: "matching-pod", - Namespace: "matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -577,14 +589,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: true, + podName: "matching-pod", + sourcePodNs: "matching-namespace", + expected: true, }, { name: "should not match PVB and pod, pod name mismatch", - pod: &metav1.ObjectMeta{ - Name: "not-matching-pod", - Namespace: "matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -593,14 +603,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "not-matching-pod", + sourcePodNs: "matching-namespace", + expected: false, }, { name: "should not match PVB and pod, pod namespace mismatch", - pod: &metav1.ObjectMeta{ - Name: "matching-pod", - Namespace: "not-matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -609,14 +617,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "matching-pod", + sourcePodNs: "not-matching-namespace", + expected: false, }, { name: "should not match PVB and pod, pod name and namespace mismatch", - pod: &metav1.ObjectMeta{ - Name: "not-matching-pod", - Namespace: "not-matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -625,13 +631,15 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "not-matching-pod", + sourcePodNs: "not-matching-namespace", + expected: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - actual := isPVBMatchPod(&tc.pvb, tc.pod) + actual := isPVBMatchPod(&tc.pvb, tc.podName, tc.sourcePodNs) assert.Equal(t, tc.expected, actual) }) diff --git a/pkg/restic/restorer.go b/pkg/restic/restorer.go index a17cfeea2..049d734e7 100644 --- a/pkg/restic/restorer.go +++ b/pkg/restic/restorer.go @@ -92,7 +92,7 @@ func newRestorer( } func (r *restorer) RestorePodVolumes(data RestoreData) []error { - volumesToRestore := GetVolumeBackupsForPod(data.PodVolumeBackups, data.Pod) + volumesToRestore := GetVolumeBackupsForPod(data.PodVolumeBackups, data.Pod, data.SourceNamespace) if len(volumesToRestore) == 0 { return nil } diff --git a/pkg/restore/restic_restore_action.go b/pkg/restore/restic_restore_action.go index 38aebd72c..42c755a93 100644 --- a/pkg/restore/restic_restore_action.go +++ b/pkg/restore/restic_restore_action.go @@ -76,6 +76,15 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu return nil, errors.Wrap(err, "unable to convert pod from runtime.Unstructured") } + // At the point when this function is called, the namespace mapping for the restore + // has not yet been applied to `input.Item` so we can't perform a reverse-lookup in + // the namespace mapping in the restore spec. Instead, use the pod from the backup + // so that if the mapping is applied earlier, we still use the correct namespace. + var podFromBackup corev1.Pod + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(input.ItemFromBackup.UnstructuredContent(), &podFromBackup); err != nil { + return nil, errors.Wrap(err, "unable to convert source pod from runtime.Unstructured") + } + log := a.logger.WithField("pod", kube.NamespaceAndName(&pod)) opts := label.NewListOptionsForBackup(input.Restore.Spec.BackupName) @@ -88,7 +97,7 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu for i := range podVolumeBackupList.Items { podVolumeBackups = append(podVolumeBackups, &podVolumeBackupList.Items[i]) } - volumeSnapshots := restic.GetVolumeBackupsForPod(podVolumeBackups, &pod) + volumeSnapshots := restic.GetVolumeBackupsForPod(podVolumeBackups, &pod, podFromBackup.Namespace) if len(volumeSnapshots) == 0 { log.Debug("No restic backups found for pod") return velero.NewRestoreItemActionExecuteOutput(input.Item), nil diff --git a/pkg/restore/restic_restore_action_test.go b/pkg/restore/restic_restore_action_test.go index 0211cdcba..dc1c65acc 100644 --- a/pkg/restore/restic_restore_action_test.go +++ b/pkg/restore/restic_restore_action_test.go @@ -122,6 +122,7 @@ func TestResticRestoreActionExecute(t *testing.T) { tests := []struct { name string pod *corev1api.Pod + podFromBackup *corev1api.Pod podVolumeBackups []*velerov1api.PodVolumeBackup want *corev1api.Pod }{ @@ -202,6 +203,49 @@ func TestResticRestoreActionExecute(t *testing.T) { builder.ForContainer("first-container", "").Result()). Result(), }, + { + name: "Restoring pod in another namespace adds the restic initContainer and uses the namespace of the backup pod for matching PVBs", + pod: builder.ForPod("new-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + Result(), + podFromBackup: builder.ForPod("original-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup(veleroNs, "pvb-1"). + PodName("my-pod"). + PodNamespace("original-ns"). + Volume("vol-1"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, backupName)). + SnapshotID("foo"). + Result(), + builder.ForPodVolumeBackup(veleroNs, "pvb-2"). + PodName("my-pod"). + PodNamespace("original-ns"). + Volume("vol-2"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, backupName)). + SnapshotID("foo"). + Result(), + }, + want: builder.ForPod("new-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + InitContainers( + newResticInitContainerBuilder(initContainerImage(defaultImageBase), ""). + Resources(&resourceReqs). + SecurityContext(&securityContext). + VolumeMounts(builder.ForVolumeMount("vol-1", "/restores/vol-1").Result(), builder.ForVolumeMount("vol-2", "/restores/vol-2").Result()). + Command([]string{"/velero-restic-restore-helper"}).Result()). + Result(), + }, } for _, tc := range tests { @@ -214,12 +258,24 @@ func TestResticRestoreActionExecute(t *testing.T) { require.NoError(t, err) } - unstructuredMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pod) + unstructuredPod, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pod) require.NoError(t, err) + // Default to using the same pod for both Item and ItemFromBackup if podFromBackup not provided + var unstructuredPodFromBackup map[string]interface{} + if tc.podFromBackup != nil { + unstructuredPodFromBackup, err = runtime.DefaultUnstructuredConverter.ToUnstructured(tc.podFromBackup) + require.NoError(t, err) + } else { + unstructuredPodFromBackup = unstructuredPod + } + input := &velero.RestoreItemActionExecuteInput{ Item: &unstructured.Unstructured{ - Object: unstructuredMap, + Object: unstructuredPod, + }, + ItemFromBackup: &unstructured.Unstructured{ + Object: unstructuredPodFromBackup, }, Restore: builder.ForRestore(veleroNs, restoreName). Backup(backupName). diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 89605bd51..c6ab9dd9e 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1197,7 +1197,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs } - if groupResource == kuberesource.Pods && len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, obj)) > 0 { + if groupResource == kuberesource.Pods && len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, obj, originalNamespace)) > 0 { restorePodVolumeBackups(ctx, createdObj, originalNamespace) }