diff --git a/changelogs/CHANGELOG-1.5.md b/changelogs/CHANGELOG-1.5.md index 7013a85e7..324d0f810 100644 --- a/changelogs/CHANGELOG-1.5.md +++ b/changelogs/CHANGELOG-1.5.md @@ -1,3 +1,23 @@ +## v1.5.2 +### 2020-10-20 +### Download +https://github.com/vmware-tanzu/velero/releases/tag/v1.5.2 + +### Container Image +`velero/velero:v1.5.2` + +### Documentation +https://velero.io/docs/v1.5/ + +### Upgrading +https://velero.io/docs/v1.5/upgrade-to-1.5/ + +### All Changes + * Fix BSL controller to avoid invoking init() on all BSLs regardless of ValidationFrequency (#2992, @betta1) + * cli: allow creating multiple instances of Velero across two different namespaces (#2886, @alaypatel07) + * Restore CRD Resource name to fix CRD wait functionality. (#2949, @sseago) + * Ensure that bound PVCs and PVs remain bound on restore. (#3007, @nrb) + ## v1.5.1 ### 2020-09-16 @@ -80,3 +100,4 @@ Displays the Timestamps when issued a print or describe (#2748, @thejasbabu) * when creating new backup from schedule from cli, allow backup name to be automatically generated (#2569, @cblecker) * Convert manifests + BSL api client to kubebuilder (#2561, @carlisia) * backup/restore: reinstantiate backup store just before uploading artifacts to ensure credentials are up-to-date (#2550, @skriss) + diff --git a/pkg/backup/remap_crd_version_action.go b/pkg/backup/remap_crd_version_action.go index eaced726e..b282e876e 100644 --- a/pkg/backup/remap_crd_version_action.go +++ b/pkg/backup/remap_crd_version_action.go @@ -31,7 +31,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" ) @@ -111,7 +110,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/client/factory.go b/pkg/client/factory.go index e46b81dc1..6ecc63be7 100644 --- a/pkg/client/factory.go +++ b/pkg/client/factory.go @@ -155,6 +155,10 @@ func (f *factory) KubebuilderClient() (kbclient.Client, error) { Scheme: scheme, }) + if err != nil { + return nil, err + } + return kubebuilderClient, nil } diff --git a/pkg/controller/backupstoragelocation_controller.go b/pkg/controller/backupstoragelocation_controller.go index fef15545b..8075bf058 100644 --- a/pkg/controller/backupstoragelocation_controller.go +++ b/pkg/controller/backupstoragelocation_controller.go @@ -77,14 +77,14 @@ func (r *BackupStorageLocationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu defaultFound = true } - backupStore, err := r.NewBackupStore(location, pluginManager, log) - if err != nil { - log.WithError(err).Error("Error getting a backup store") + if !storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo, log) { + log.Debug("Backup location not ready to be validated") continue } - if !storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo, log) { - log.Debug("Backup location not ready to be validated") + backupStore, err := r.NewBackupStore(location, pluginManager, log) + if err != nil { + log.WithError(err).Error("Error getting a backup store") continue } diff --git a/pkg/install/resources.go b/pkg/install/resources.go index cbf78c5a4..9ea7ccbaf 100644 --- a/pkg/install/resources.go +++ b/pkg/install/resources.go @@ -51,6 +51,7 @@ var ( DefaultResticPodMemRequest = "512Mi" DefaultResticPodCPULimit = "1000m" DefaultResticPodMemLimit = "1Gi" + DefaultVeleroNamespace = "velero" ) func labels() map[string]string { @@ -105,8 +106,12 @@ func ServiceAccount(namespace string, annotations map[string]string) *corev1.Ser } func ClusterRoleBinding(namespace string) *rbacv1beta1.ClusterRoleBinding { + crbName := "velero" + if namespace != DefaultVeleroNamespace { + crbName = "velero-" + namespace + } crb := &rbacv1beta1.ClusterRoleBinding{ - ObjectMeta: objectMeta("", "velero"), + ObjectMeta: objectMeta("", crbName), TypeMeta: metav1.TypeMeta{ Kind: "ClusterRoleBinding", APIVersion: rbacv1beta1.SchemeGroupVersion.String(), diff --git a/pkg/install/resources_test.go b/pkg/install/resources_test.go index e2c296493..748d70def 100644 --- a/pkg/install/resources_test.go +++ b/pkg/install/resources_test.go @@ -23,7 +23,7 @@ import ( ) func TestResources(t *testing.T) { - bsl := BackupStorageLocation("velero", "test", "test", "", make(map[string]string), []byte("test")) + bsl := BackupStorageLocation(DefaultVeleroNamespace, "test", "test", "", make(map[string]string), []byte("test")) assert.Equal(t, "velero", bsl.ObjectMeta.Namespace) assert.Equal(t, "test", bsl.Spec.Provider) @@ -31,7 +31,7 @@ func TestResources(t *testing.T) { assert.Equal(t, make(map[string]string), bsl.Spec.Config) assert.Equal(t, []byte("test"), bsl.Spec.ObjectStorage.CACert) - vsl := VolumeSnapshotLocation("velero", "test", make(map[string]string)) + vsl := VolumeSnapshotLocation(DefaultVeleroNamespace, "test", make(map[string]string)) assert.Equal(t, "velero", vsl.ObjectMeta.Namespace) assert.Equal(t, "test", vsl.Spec.Provider) @@ -41,12 +41,19 @@ func TestResources(t *testing.T) { assert.Equal(t, "velero", ns.Name) - crb := ClusterRoleBinding("velero") + crb := ClusterRoleBinding(DefaultVeleroNamespace) // The CRB is a cluster-scoped resource assert.Equal(t, "", crb.ObjectMeta.Namespace) + assert.Equal(t, "velero", crb.ObjectMeta.Name) assert.Equal(t, "velero", crb.Subjects[0].Namespace) - sa := ServiceAccount("velero", map[string]string{"abcd": "cbd"}) + customNamespaceCRB := ClusterRoleBinding("foo") + // The CRB is a cluster-scoped resource + assert.Equal(t, "", customNamespaceCRB.ObjectMeta.Namespace) + assert.Equal(t, "velero-foo", customNamespaceCRB.ObjectMeta.Name) + assert.Equal(t, "foo", customNamespaceCRB.Subjects[0].Namespace) + + sa := ServiceAccount(DefaultVeleroNamespace, map[string]string{"abcd": "cbd"}) assert.Equal(t, "velero", sa.ObjectMeta.Namespace) assert.Equal(t, "cbd", sa.ObjectMeta.Annotations["abcd"]) } 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 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) + }) + } +}