Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use pod namespace from backup when matching PVBs #3475

Merged
merged 2 commits into from
Feb 22, 2021
Merged

Use pod namespace from backup when matching PVBs #3475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77c2bd7
Use pod namespace from backup when matching PVBs
zubron Feb 18, 2021
0d56725
Explain why the namespace mapping can't be used
zubron Feb 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/unreleased/3475-zubron
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a bug where restic volumes would not be restored when using a namespace mapping.
8 changes: 4 additions & 4 deletions pkg/restic/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,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
}

Expand Down
130 changes: 69 additions & 61 deletions pkg/restic/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,78 +47,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"},
},
}

Expand All @@ -128,7 +143,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)
})
}
Expand Down Expand Up @@ -566,17 +581,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{
Expand All @@ -585,14 +597,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{
Expand All @@ -601,14 +611,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{
Expand All @@ -617,14 +625,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{
Expand All @@ -633,13 +639,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)
})

Expand Down
2 changes: 1 addition & 1 deletion pkg/restic/restorer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
11 changes: 10 additions & 1 deletion pkg/restore/restic_restore_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
60 changes: 58 additions & 2 deletions pkg/restore/restic_restore_action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand Down Expand Up @@ -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 {
Expand All @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion pkg/restore/restore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down