diff --git a/changelogs/CHANGELOG-1.4.md b/changelogs/CHANGELOG-1.4.md index 354e68514..e681c1857 100644 --- a/changelogs/CHANGELOG-1.4.md +++ b/changelogs/CHANGELOG-1.4.md @@ -1,3 +1,23 @@ +## v1.4.3 +### 2020-10-20 + +### Download +https://github.com/vmware-tanzu/velero/releases/tag/v1.4.3 + +### Container Image +`velero/velero:v1.4.3` + +### Documentation +https://velero.io/docs/v1.4/ + +### Upgrading +https://velero.io/docs/v1.4/upgrade-to-1.4/ + +### All Changes + * Restore CRD Resource name to fix CRD wait functionality. (#2949, @sseago) + * rename the PV if VolumeSnapshotter has modified the PV name (#2835, @pawanpraka1) + * Ensure that bound PVCs and PVs remain bound on restore. (#3007, @nrb) + ## v1.4.2 ### 2020-07-13 diff --git a/pkg/backup/remap_crd_version_action.go b/pkg/backup/remap_crd_version_action.go index be7b8f3d6..320b882c1 100644 --- a/pkg/backup/remap_crd_version_action.go +++ b/pkg/backup/remap_crd_version_action.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" - "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/plugin/velero" ) @@ -110,7 +109,7 @@ func fetchV1beta1CRD(name string, betaCRDClient apiextv1beta1client.CustomResour // See https://github.com/kubernetes/kubernetes/issues/3030. Unsure why this is happening here and not in main Velero; // probably has to do with List calls and Dynamic client vs typed client // Set these all the time, since they shouldn't ever be different, anyway - betaCRD.Kind = kuberesource.CustomResourceDefinitions.Resource + betaCRD.Kind = "CustomResourceDefinition" betaCRD.APIVersion = apiextv1beta1.SchemeGroupVersion.String() m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&betaCRD) diff --git a/pkg/kuberesource/kuberesource.go b/pkg/kuberesource/kuberesource.go index d3daafe35..a515a70ff 100644 --- a/pkg/kuberesource/kuberesource.go +++ b/pkg/kuberesource/kuberesource.go @@ -23,7 +23,7 @@ import ( var ( ClusterRoleBindings = schema.GroupResource{Group: "rbac.authorization.k8s.io", Resource: "clusterrolebindings"} ClusterRoles = schema.GroupResource{Group: "rbac.authorization.k8s.io", Resource: "clusterroles"} - CustomResourceDefinitions = schema.GroupResource{Group: "apiextensions.k8s.io", Resource: "CustomResourceDefinition"} + CustomResourceDefinitions = schema.GroupResource{Group: "apiextensions.k8s.io", Resource: "customresourcedefinitions"} Jobs = schema.GroupResource{Group: "batch", Resource: "jobs"} Namespaces = schema.GroupResource{Group: "", Resource: "namespaces"} PersistentVolumeClaims = schema.GroupResource{Group: "", Resource: "persistentvolumeclaims"} 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 299f1ac3f..0d6ea4787 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -60,6 +60,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) } @@ -868,6 +876,13 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc 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 @@ -885,6 +900,9 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc } 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.") @@ -894,6 +912,11 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc return warnings, errs } obj = updatedObj + + // VolumeSnapshotter has modified the PV name, we should rename the PV + if oldName != obj.GetName() { + shouldRenamePV = true + } } if shouldRenamePV { @@ -939,8 +962,9 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc 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)) @@ -1033,17 +1057,16 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc 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 { @@ -1192,6 +1215,40 @@ func shouldRenamePV(ctx *context, obj *unstructured.Unstructured, client 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 *context, 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 *context, createdObj *unstructured.Unstructured, originalNamespace string) { if ctx.resticRestorer == nil { @@ -1269,6 +1326,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 57ee63b31..71f065829 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -1834,7 +1834,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: newTarWriter(t). @@ -1853,6 +1853,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(), ), }, @@ -2100,13 +2101,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(), ), @@ -2165,6 +2165,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(), ), @@ -2178,6 +2179,67 @@ func TestRestorePersistentVolumes(t *testing.T) { ), }, }, + { + name: "when a PV is renamed and the original PV does not exist in-cluster, the PV should be renamed", + restore: defaultRestore().NamespaceMappings("source-ns", "target-ns").Result(), + backup: defaultBackup().Result(), + tarball: newTarWriter(t). + addItems( + "persistentvolumes", + builder.ForPersistentVolume("source-pv").AWSEBSVolumeID("source-volume").ClaimRef("source-ns", "pvc-1").Result(), + ). + addItems( + "persistentvolumeclaims", + builder.ForPersistentVolumeClaim("source-ns", "pvc-1").VolumeName("source-pv").Result(), + ). + done(), + apiResources: []*test.APIResource{ + test.PVs(), + test.PVCs(), + }, + volumeSnapshots: []*volume.Snapshot{ + { + Spec: volume.SnapshotSpec{ + BackupName: "backup-1", + Location: "default", + PersistentVolumeName: "source-pv", + }, + Status: volume.SnapshotStatus{ + Phase: volume.SnapshotPhaseCompleted, + ProviderSnapshotID: "snapshot-1", + }, + }, + }, + volumeSnapshotLocations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "default").Provider("provider-1").Result(), + }, + volumeSnapshotterGetter: map[string]velero.VolumeSnapshotter{ + "provider-1": &volumeSnapshotter{ + snapshotVolumes: map[string]string{"snapshot-1": "new-pvname"}, + pvName: map[string]string{"new-pvname": "new-pvname"}, + }, + }, + want: []*test.APIResource{ + test.PVs( + builder.ForPersistentVolume("new-pvname"). + ObjectMeta( + 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(), + ), + test.PVCs( + builder.ForPersistentVolumeClaim("target-ns", "pvc-1"). + ObjectMeta( + builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"), + ). + VolumeName("new-pvname"). + Result(), + ), + }, + }, { name: "when a PV with a reclaim policy of retain has a snapshot and exists in-cluster, neither the snapshot nor the PV are restored", restore: defaultRestore().Result(), @@ -2284,13 +2346,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(), ), @@ -2870,3 +2931,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) + }) + } +}