diff --git a/changelogs/unreleased/3007-nrb b/changelogs/unreleased/3007-nrb new file mode 100644 index 000000000..72f4782b4 --- /dev/null +++ b/changelogs/unreleased/3007-nrb @@ -0,0 +1 @@ +Ensure that bound PVCs and PVs remain bound on restore. diff --git a/pkg/restore/pv_restorer.go b/pkg/restore/pv_restorer.go index 2a83378c4..ff8d36b34 100644 --- a/pkg/restore/pv_restorer.go +++ b/pkg/restore/pv_restorer.go @@ -47,20 +47,6 @@ func (r *pvRestorer) executePVAction(obj *unstructured.Unstructured) (*unstructu return nil, errors.New("PersistentVolume is missing its name") } - // It's simpler to just access the spec through the unstructured object than to convert - // to structured and back here, especially since the SetVolumeID(...) call below needs - // the unstructured representation (and does a conversion internally). - res, ok := obj.Object["spec"] - if !ok { - return nil, errors.New("spec not found") - } - spec, ok := res.(map[string]interface{}) - if !ok { - return nil, errors.Errorf("spec was of type %T, expected map[string]interface{}", res) - } - - delete(spec, "claimRef") - if boolptr.IsSetToFalse(r.snapshotVolumes) { // The backup had snapshots disabled, so we can return early return obj, nil diff --git a/pkg/restore/pv_restorer_test.go b/pkg/restore/pv_restorer_test.go index 0ac322442..c4fb0a9db 100644 --- a/pkg/restore/pv_restorer_test.go +++ b/pkg/restore/pv_restorer_test.go @@ -56,19 +56,6 @@ func TestExecutePVAction_NoSnapshotRestores(t *testing.T) { restore: builder.ForRestore(api.DefaultNamespace, "").Result(), expectedErr: true, }, - { - name: "no spec should error", - obj: NewTestUnstructured().WithName("pv-1").Unstructured, - restore: builder.ForRestore(api.DefaultNamespace, "").Result(), - expectedErr: true, - }, - { - name: "ensure spec.claimRef is deleted", - obj: NewTestUnstructured().WithName("pv-1").WithAnnotations("a", "b").WithSpec("claimRef", "someOtherField").Unstructured, - restore: builder.ForRestore(api.DefaultNamespace, "").RestorePVs(false).Result(), - backup: defaultBackup().Phase(api.BackupPhaseInProgress).Result(), - expectedRes: NewTestUnstructured().WithAnnotations("a", "b").WithName("pv-1").WithSpec("someOtherField").Unstructured, - }, { name: "ensure spec.storageClassName is retained", obj: NewTestUnstructured().WithName("pv-1").WithAnnotations("a", "b").WithSpec("storageClassName", "someOtherField").Unstructured, @@ -81,7 +68,7 @@ func TestExecutePVAction_NoSnapshotRestores(t *testing.T) { obj: NewTestUnstructured().WithName("pv-1").WithAnnotations("a", "b").WithSpec("claimRef", "storageClassName", "someOtherField").Unstructured, restore: builder.ForRestore(api.DefaultNamespace, "").RestorePVs(true).Result(), backup: defaultBackup().Phase(api.BackupPhaseInProgress).SnapshotVolumes(false).Result(), - expectedRes: NewTestUnstructured().WithName("pv-1").WithAnnotations("a", "b").WithSpec("storageClassName", "someOtherField").Unstructured, + expectedRes: NewTestUnstructured().WithName("pv-1").WithAnnotations("a", "b").WithSpec("claimRef", "storageClassName", "someOtherField").Unstructured, }, { name: "restore.spec.restorePVs=false, return early", diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 243d080e5..7e056af55 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -62,6 +62,14 @@ import ( "github.com/vmware-tanzu/velero/pkg/volume" ) +// These annotations are taken from the Kubernetes persistent volume/persistent volume claim controller. +// They cannot be directly importing because they are part of the kubernetes/kubernetes package, and importing that package is unsupported. +// Their values are well-known and slow changing. They're duplicated here as constants to provide compile-time checking. +// Originals can be found in kubernetes/kubernetes/pkg/controller/volume/persistentvolume/util/util.go. +const KubeAnnBindCompleted = "pv.kubernetes.io/bind-completed" +const KubeAnnBoundByController = "pv.kubernetes.io/bound-by-controller" +const KubeAnnDynamicallyProvisioned = "pv.kubernetes.io/provisioned-by" + type VolumeSnapshotterGetter interface { GetVolumeSnapshotter(name string) (velero.VolumeSnapshotter, error) } @@ -882,6 +890,13 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs } + // Check to see if the claimRef.namespace field needs to be remapped, and do so if necessary. + _, err = remapClaimRefNS(ctx, obj) + if err != nil { + errs.Add(namespace, err) + return warnings, errs + } + var shouldRestoreSnapshot bool if !shouldRenamePV { // Check if the PV exists in the cluster before attempting to create @@ -899,6 +914,9 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } if shouldRestoreSnapshot { + // reset the PV's binding status so that Kubernetes can properly associate it with the restored PVC. + obj = resetVolumeBindingInfo(obj) + // even if we're renaming the PV, obj still has the old name here, because the pvRestorer // uses the original name to look up metadata about the snapshot. ctx.log.Infof("Restoring persistent volume from snapshot.") @@ -958,8 +976,9 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso default: ctx.log.Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.") - // we call the pvRestorer here to clear out the PV's claimRef, so it can be re-claimed - // when its PVC is restored. + obj = resetVolumeBindingInfo(obj) + // we call the pvRestorer here to clear out the PV's claimRef.UID, so it can be re-claimed + // when its PVC is restored and gets a new UID. updatedObj, err := ctx.pvRestorer.executePVAction(obj) if err != nil { errs.Add(namespace, fmt.Errorf("error executing PVAction for %s: %v", resourceID, err)) @@ -1052,17 +1071,16 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs } - if pvc.Spec.VolumeName != "" && ctx.pvsToProvision.Has(pvc.Spec.VolumeName) { - ctx.log.Infof("Resetting PersistentVolumeClaim %s/%s for dynamic provisioning", namespace, name) + if pvc.Spec.VolumeName != "" { + // This used to only happen with restic volumes, but now always remove this binding metadata + obj = resetVolumeBindingInfo(obj) - // use the unstructured helpers here since we're only deleting and - // the unstructured converter will add back (empty) fields for metadata - // and status that we removed earlier. - unstructured.RemoveNestedField(obj.Object, "spec", "volumeName") - annotations := obj.GetAnnotations() - delete(annotations, "pv.kubernetes.io/bind-completed") - delete(annotations, "pv.kubernetes.io/bound-by-controller") - obj.SetAnnotations(annotations) + // This is the case for restic volumes, where we need to actually have an empty volume created instead of restoring one. + // The assumption is that any PV in pvsToProvision doesn't have an associated snapshot. + if ctx.pvsToProvision.Has(pvc.Spec.VolumeName) { + ctx.log.Infof("Resetting PersistentVolumeClaim %s/%s for dynamic provisioning", namespace, name) + unstructured.RemoveNestedField(obj.Object, "spec", "volumeName") + } } if newName, ok := ctx.renamedPVs[pvc.Spec.VolumeName]; ok { @@ -1215,6 +1233,40 @@ func shouldRenamePV(ctx *restoreContext, obj *unstructured.Unstructured, client return true, nil } +// remapClaimRefNS remaps a PersistentVolume's claimRef.Namespace based on a restore's NamespaceMappings, if necessary. +// Returns true if the namespace was remapped, false if it was not required. +func remapClaimRefNS(ctx *restoreContext, obj *unstructured.Unstructured) (bool, error) { + if len(ctx.restore.Spec.NamespaceMapping) == 0 { + ctx.log.Debug("Persistent volume does not need to have the claimRef.namespace remapped because restore is not remapping any namespaces") + return false, nil + } + + // Conversion to the real type here is more readable than all the error checking involved with reading each field individually. + pv := new(v1.PersistentVolume) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, pv); err != nil { + return false, errors.Wrapf(err, "error converting persistent volume to structured") + } + + if pv.Spec.ClaimRef == nil { + ctx.log.Debugf("Persistent volume does not need to have the claimRef.namepace remapped because it's not claimed") + return false, nil + } + + targetNS, ok := ctx.restore.Spec.NamespaceMapping[pv.Spec.ClaimRef.Namespace] + + if !ok { + ctx.log.Debugf("Persistent volume does not need to have the claimRef.namespace remapped because it's not claimed by a PVC in a namespace that's being remapped") + return false, nil + } + + err := unstructured.SetNestedField(obj.Object, targetNS, "spec", "claimRef", "namespace") + if err != nil { + return false, err + } + ctx.log.Debug("Persistent volume's namespace was updated") + return true, nil +} + // restorePodVolumeBackups restores the PodVolumeBackups for the given restored pod func restorePodVolumeBackups(ctx *restoreContext, createdObj *unstructured.Unstructured, originalNamespace string) { if ctx.resticRestorer == nil { @@ -1329,6 +1381,29 @@ func hasDeleteReclaimPolicy(obj map[string]interface{}) bool { return policy == string(v1.PersistentVolumeReclaimDelete) } +// resetVolumeBindingInfo clears any necessary metadata out of a PersistentVolume or PersistentVolumeClaim that would make it ineligible to be re-bound by Velero. +func resetVolumeBindingInfo(obj *unstructured.Unstructured) *unstructured.Unstructured { + // Clean out ClaimRef UID and resourceVersion, since this information is highly unique. + unstructured.RemoveNestedField(obj.Object, "spec", "claimRef", "uid") + unstructured.RemoveNestedField(obj.Object, "spec", "claimRef", "resourceVersion") + + // Clear out any annotations used by the Kubernetes PV controllers to track bindings. + annotations := obj.GetAnnotations() + + // Upon restore, this new PV will look like a statically provisioned, manually-bound volume rather than one bound by the controller, so remove the annotation that signals that a controller bound it. + delete(annotations, KubeAnnBindCompleted) + // Remove the annotation that signals that the PV is already bound; we want the PV(C) controller to take the two objects and bind them again. + delete(annotations, KubeAnnBoundByController) + + // Remove the provisioned-by annotation which signals that the persistent volume was dynamically provisioned; it is now statically provisioned. + delete(annotations, KubeAnnDynamicallyProvisioned) + + // GetAnnotations returns a copy, so we have to set them again + obj.SetAnnotations(annotations) + + return obj +} + func resetMetadataAndStatus(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { res, ok := obj.Object["metadata"] if !ok { diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 8e8d3b3fb..a29331076 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -1830,7 +1830,7 @@ func TestRestorePersistentVolumes(t *testing.T) { }, }, { - name: "when a PV with a reclaim policy of retain has no snapshot and does not exist in-cluster, it gets restored, without its claim ref", + name: "when a PV with a reclaim policy of retain has no snapshot and does not exist in-cluster, it gets restored, with its claim ref", restore: defaultRestore().Result(), backup: defaultBackup().Result(), tarball: test.NewTarWriter(t). @@ -1849,6 +1849,7 @@ func TestRestorePersistentVolumes(t *testing.T) { ObjectMeta( builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), ). + ClaimRef("ns-1", "pvc-1"). Result(), ), }, @@ -2096,13 +2097,12 @@ func TestRestorePersistentVolumes(t *testing.T) { want: []*test.APIResource{ test.PVs( builder.ForPersistentVolume("source-pv").AWSEBSVolumeID("source-volume").ClaimRef("source-ns", "pvc-1").Result(), - // note that the renamed PV is not expected to have a claimRef in this test; that would be - // added after creation by the Kubernetes PV/PVC controller when it does a bind. builder.ForPersistentVolume("renamed-source-pv"). ObjectMeta( builder.WithAnnotations("velero.io/original-pv-name", "source-pv"), builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), - ). + // the namespace for this PV's claimRef should be the one that the PVC was remapped into. + ).ClaimRef("target-ns", "pvc-1"). AWSEBSVolumeID("new-volume"). Result(), ), @@ -2161,6 +2161,7 @@ func TestRestorePersistentVolumes(t *testing.T) { ObjectMeta( builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), ). + ClaimRef("target-ns", "pvc-1"). AWSEBSVolumeID("new-volume"). Result(), ), @@ -2221,6 +2222,7 @@ func TestRestorePersistentVolumes(t *testing.T) { builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), builder.WithAnnotations("velero.io/original-pv-name", "source-pv"), ). + ClaimRef("target-ns", "pvc-1"). AWSEBSVolumeID("new-pvname"). Result(), ), @@ -2340,13 +2342,12 @@ func TestRestorePersistentVolumes(t *testing.T) { want: []*test.APIResource{ test.PVs( builder.ForPersistentVolume("source-pv").AWSEBSVolumeID("source-volume").ClaimRef("source-ns", "pvc-1").Result(), - // note that the renamed PV is not expected to have a claimRef in this test; that would be - // added after creation by the Kubernetes PV/PVC controller when it does a bind. builder.ForPersistentVolume("volumesnapshotter-renamed-source-pv"). ObjectMeta( builder.WithAnnotations("velero.io/original-pv-name", "source-pv"), builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), ). + ClaimRef("target-ns", "pvc-1"). AWSEBSVolumeID("new-volume"). Result(), ), @@ -2846,3 +2847,49 @@ func (h *harness) AddItems(t *testing.T, resource *test.APIResource) { require.NoError(t, err) } } + +func Test_resetVolumeBindingInfo(t *testing.T) { + tests := []struct { + name string + obj *unstructured.Unstructured + expected *unstructured.Unstructured + }{ + { + name: "PVs that are bound have their binding and dynamic provisioning annotations removed", + obj: NewTestUnstructured().WithMetadataField("kind", "persistentVolume"). + WithName("pv-1").WithAnnotations( + KubeAnnBindCompleted, + KubeAnnBoundByController, + KubeAnnDynamicallyProvisioned, + ).WithSpecField("claimRef", map[string]interface{}{ + "namespace": "ns-1", + "name": "pvc-1", + "uid": "abc", + "resourceVersion": "1"}).Unstructured, + expected: NewTestUnstructured().WithMetadataField("kind", "persistentVolume"). + WithName("pv-1"). + WithAnnotations(). + WithSpecField("claimRef", map[string]interface{}{ + "namespace": "ns-1", "name": "pvc-1"}).Unstructured, + }, + { + name: "PVCs that are bound have their binding annotations removed, but the volume name stays", + obj: NewTestUnstructured().WithMetadataField("kind", "persistentVolumeClaim"). + WithName("pvc-1").WithAnnotations( + KubeAnnBindCompleted, + KubeAnnBoundByController, + KubeAnnDynamicallyProvisioned, + ).WithSpecField("volumeName", "pv-1").Unstructured, + expected: NewTestUnstructured().WithMetadataField("kind", "persistentVolumeClaim"). + WithName("pvc-1").WithAnnotations(). + WithSpecField("volumeName", "pv-1").Unstructured, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actual := resetVolumeBindingInfo(tc.obj) + assert.Equal(t, tc.expected, actual) + }) + } +}