From 1a1372550d7ab12bc1f9a5fe5ccb4f09d2a4969d Mon Sep 17 00:00:00 2001 From: Mayank <33252549+mynktl@users.noreply.github.com> Date: Tue, 11 Feb 2020 01:47:15 +0530 Subject: [PATCH] Use PV name returned from volumesnapshotter while creating a PV (#2216) * Using PV name returned from volumesnapshotter while creating a PV Signed-off-by: mayank --- changelogs/unreleased/2216-mynktl | 1 + pkg/restore/restore.go | 22 +++++---- pkg/restore/restore_test.go | 77 ++++++++++++++++++++++++++++++- 3 files changed, 91 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/2216-mynktl diff --git a/changelogs/unreleased/2216-mynktl b/changelogs/unreleased/2216-mynktl new file mode 100644 index 000000000..92e1162cc --- /dev/null +++ b/changelogs/unreleased/2216-mynktl @@ -0,0 +1 @@ +Added support of using PV name from volumesnapshotter('SetVolumeID') in case of PV renaming during the restore diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 2875c0830..aced1d887 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -927,6 +927,7 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc if groupResource == kuberesource.PersistentVolumes { switch { case hasSnapshot(name, ctx.volumeSnapshots): + oldName := obj.GetName() shouldRenamePV, err := shouldRenamePV(ctx, obj, resourceClient) if err != nil { errs.Add(namespace, err) @@ -962,16 +963,21 @@ func (ctx *context) restoreItem(obj *unstructured.Unstructured, groupResource sc } if shouldRenamePV { - // give obj a new name, and record the mapping between the old and new names - oldName := obj.GetName() - newName, err := ctx.pvRenamer(oldName) - if err != nil { - errs.Add(namespace, errors.Wrapf(err, "error renaming PV")) - return warnings, errs + var pvName string + if oldName == obj.GetName() { + // pvRestorer hasn't modified the PV name, we need to rename the PV + pvName, err = ctx.pvRenamer(oldName) + if err != nil { + errs.Add(namespace, errors.Wrapf(err, "error renaming PV")) + return warnings, errs + } + } else { + // VolumeSnapshotter could have modified the PV name through function `SetVolumeID`, + pvName = obj.GetName() } - ctx.renamedPVs[oldName] = newName - obj.SetName(newName) + ctx.renamedPVs[oldName] = pvName + obj.SetName(pvName) // add the original PV name as an annotation annotations := obj.GetAnnotations() diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 1c9bff12e..154365f32 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -1731,6 +1731,9 @@ func (vsg volumeSnapshotterGetter) GetVolumeSnapshotter(name string) (velero.Vol type volumeSnapshotter struct { // a map from snapshotID to volumeID snapshotVolumes map[string]string + + // a map from volumeID to new pv name + pvName map[string]string } // Init is a no-op. @@ -1751,8 +1754,15 @@ func (vs *volumeSnapshotter) CreateVolumeFromSnapshot(snapshotID, volumeType, vo // SetVolumeID sets the persistent volume's spec.awsElasticBlockStore.volumeID field // with the provided volumeID. -func (*volumeSnapshotter) SetVolumeID(pv runtime.Unstructured, volumeID string) (runtime.Unstructured, error) { +func (vs *volumeSnapshotter) SetVolumeID(pv runtime.Unstructured, volumeID string) (runtime.Unstructured, error) { unstructured.SetNestedField(pv.UnstructuredContent(), volumeID, "spec", "awsElasticBlockStore", "volumeID") + + newPVName, ok := vs.pvName[volumeID] + if !ok { + return pv, nil + } + + unstructured.SetNestedField(pv.UnstructuredContent(), newPVName, "metadata", "name") return pv, nil } @@ -2231,6 +2241,71 @@ func TestRestorePersistentVolumes(t *testing.T) { ), }, }, + { + name: "when a PV with a snapshot is used by a PVC in a namespace that's being remapped, and the original PV exists in-cluster, the PV is renamed by volumesnapshotter", + 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( + builder.ForPersistentVolume("source-pv").AWSEBSVolumeID("source-volume").ClaimRef("source-ns", "pvc-1").Result(), + ), + 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-volume"}, + pvName: map[string]string{"new-volume": "volumesnapshotter-renamed-source-pv"}, + }, + }, + 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"), + ). + AWSEBSVolumeID("new-volume"). + 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("volumesnapshotter-renamed-source-pv"). + Result(), + ), + }, + }, } for _, tc := range tests {