From 21b4cac8fabcb477d90f86bdc3a0053f07007472 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Fri, 4 Aug 2023 11:33:11 +0800 Subject: [PATCH] Fix #6589 1. Only return Async operation for VS and VSC created for current backup. 2. Add DataUpload name annotation in PVC during backup if DataUpload is created successfully. 3. Check whether PVC has DataUpload name annotation in restore. If it has, create DataDownload. If not, return. The annotation is cleaned from PVC. Signed-off-by: Xun Jiang --- internal/backup/pvc_action.go | 10 +++++--- internal/backup/pvc_action_test.go | 24 ++++++++++++++++++- internal/backup/volumesnapshot_action.go | 22 ++++++++++------- .../backup/volumesnapshotcontent_action.go | 23 ++++++++++++------ internal/restore/pvc_action.go | 13 +++++++++- internal/restore/pvc_action_test.go | 12 +++++++--- internal/util/labels_annotations.go | 3 +++ 7 files changed, 84 insertions(+), 23 deletions(-) diff --git a/internal/backup/pvc_action.go b/internal/backup/pvc_action.go index 49f8fbbf..5fd9b2cc 100644 --- a/internal/backup/pvc_action.go +++ b/internal/backup/pvc_action.go @@ -99,6 +99,7 @@ func (p *PVCBackupItemAction) Execute(item runtime.Unstructured, backup *velerov } if pv.Spec.PersistentVolumeSource.CSI == nil { p.Log.Infof("Skipping PVC %s/%s, associated PV %s is not a CSI volume", pvc.Namespace, pvc.Name, pv.Name) + util.AddAnnotations(&pvc.ObjectMeta, map[string]string{ util.SkippedNoCSIPVAnnotation: "true", }) @@ -168,9 +169,6 @@ func (p *PVCBackupItemAction) Execute(item runtime.Unstructured, backup *velerov annotations := labels annotations[util.MustIncludeAdditionalItemAnnotation] = "true" - util.AddAnnotations(&pvc.ObjectMeta, annotations) - util.AddLabels(&pvc.ObjectMeta, labels) - var additionalItems []velero.ResourceIdentifier operationID := "" var itemToUpdate []velero.ResourceIdentifier @@ -203,6 +201,9 @@ func (p *PVCBackupItemAction) Execute(item runtime.Unstructured, backup *velerov Name: dataUpload.Name, }, } + // Set the DataUploadNameLabel, which is used for restore to let CSI plugin check whether + // it should handle the volume. If volume is CSI migration, PVC doesn't have the annotation. + annotations[util.DataUploadNameAnnotation] = dataUpload.Namespace + "/" + dataUpload.Name dataUploadLog.Info("DataUpload is submitted successfully.") } @@ -216,6 +217,9 @@ func (p *PVCBackupItemAction) Execute(item runtime.Unstructured, backup *velerov } } + util.AddAnnotations(&pvc.ObjectMeta, annotations) + util.AddLabels(&pvc.ObjectMeta, labels) + p.Log.Infof("Returning from PVCBackupItemAction with %d additionalItems to backup", len(additionalItems)) for _, ai := range additionalItems { p.Log.Debugf("%s: %s", ai.GroupResource.String(), ai.Name) diff --git a/internal/backup/pvc_action_test.go b/internal/backup/pvc_action_test.go index 82ec64a3..0c0b019b 100644 --- a/internal/backup/pvc_action_test.go +++ b/internal/backup/pvc_action_test.go @@ -56,6 +56,7 @@ func TestExecute(t *testing.T) { expectedErr error expectedBackup *velerov1api.Backup expectedDataUpload *velerov2alpha1.DataUpload + expectedPVC *corev1.PersistentVolumeClaim }{ { name: "Skip PVC handling if SnapshotVolume set to false", @@ -113,6 +114,21 @@ func TestExecute(t *testing.T) { }, }, }, + { + name: "", + backup: builder.ForBackup("velero", "test").SnapshotMoveData(true).Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").StorageClass("testSC").Phase(corev1.ClaimBound).Result(), + pv: builder.ForPersistentVolume("testPV").CSI("hostpath", "testVolume").Result(), + sc: builder.ForStorageClass("testSC").Provisioner("hostpath").Result(), + vs: builder.ForVolumeSnapshot("velero", "testVS").Result(), + vsClass: builder.ForVolumeSnapshotClass("tescVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(util.VolumeSnapshotClassSelectorLabel, "")).Result(), + operationID: ".", + expectedErr: nil, + expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC"). + ObjectMeta(builder.WithAnnotations(util.MustIncludeAdditionalItemAnnotation, "true", velerov1api.BackupNameLabel, "test", util.DataUploadNameAnnotation, "velero/", util.VolumeSnapshotLabel, ""), + builder.WithLabels(util.MustIncludeAdditionalItemAnnotation, "true", velerov1api.BackupNameLabel, "test", util.DataUploadNameAnnotation, "velero/", util.VolumeSnapshotLabel, "")). + VolumeName("testPV").StorageClass("testSC").Phase(corev1.ClaimBound).Result(), + }, } for _, tc := range tests { @@ -150,7 +166,7 @@ func TestExecute(t *testing.T) { pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.pvc) require.NoError(t, err) - _, _, _, _, err = pvcBIA.Execute(&unstructured.Unstructured{Object: pvcMap}, tc.backup) + resultUnstructed, _, _, _, err := pvcBIA.Execute(&unstructured.Unstructured{Object: pvcMap}, tc.backup) if tc.expectedErr != nil { require.Equal(t, err, tc.expectedErr) } @@ -161,6 +177,12 @@ func TestExecute(t *testing.T) { require.Equal(t, 1, len(dataUploadList.Items)) require.Equal(t, *tc.expectedDataUpload, dataUploadList.Items[0]) } + + if tc.expectedPVC != nil { + resultPVC := new(corev1.PersistentVolumeClaim) + runtime.DefaultUnstructuredConverter.FromUnstructured(resultUnstructed.UnstructuredContent(), resultPVC) + require.Equal(t, *tc.expectedPVC, *resultPVC) + } }) } } diff --git a/internal/backup/volumesnapshot_action.go b/internal/backup/volumesnapshot_action.go index caac4f39..8a63f579 100644 --- a/internal/backup/volumesnapshot_action.go +++ b/internal/backup/volumesnapshot_action.go @@ -184,14 +184,20 @@ func (p *VolumeSnapshotBackupItemAction) Execute(item runtime.Unstructured, back p.Log.Debugf("%s: %s", ai.GroupResource.String(), ai.Name) } - // The operationID is of the form // - operationID := vs.Namespace + "/" + vs.Name + "/" + time.Now().Format(time.RFC3339) - itemToUpdate := []velero.ResourceIdentifier{ - { - GroupResource: kuberesource.VolumeSnapshots, - Namespace: vs.Namespace, - Name: vs.Name, - }, + operationID := "" + var itemToUpdate []velero.ResourceIdentifier + + // Only return Async operation for VSC created for this backup. + if backupOngoing { + // The operationID is of the form // + operationID = vs.Namespace + "/" + vs.Name + "/" + time.Now().Format(time.RFC3339) + itemToUpdate = []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.VolumeSnapshots, + Namespace: vs.Namespace, + Name: vs.Name, + }, + } } return &unstructured.Unstructured{Object: vsMap}, additionalItems, operationID, itemToUpdate, nil diff --git a/internal/backup/volumesnapshotcontent_action.go b/internal/backup/volumesnapshotcontent_action.go index 81b9b112..2298ba00 100644 --- a/internal/backup/volumesnapshotcontent_action.go +++ b/internal/backup/volumesnapshotcontent_action.go @@ -33,6 +33,7 @@ import ( "github.com/vmware-tanzu/velero-plugin-for-csi/internal/util" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/kuberesource" + "github.com/vmware-tanzu/velero/pkg/label" "github.com/vmware-tanzu/velero/pkg/plugin/velero" biav2 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v2" "github.com/vmware-tanzu/velero/pkg/util/boolptr" @@ -90,13 +91,21 @@ func (p *VolumeSnapshotContentBackupItemAction) Execute(item runtime.Unstructure return nil, nil, "", nil, errors.WithStack(err) } - // The operationID is of the form / - operationID := snapCont.Name + "/" + time.Now().Format(time.RFC3339) - itemToUpdate := []velero.ResourceIdentifier{ - { - GroupResource: kuberesource.VolumeSnapshotContents, - Name: snapCont.Name, - }, + backupOnGoing := snapCont.GetLabels()[velerov1api.BackupNameLabel] == label.GetValidName(backup.Name) + operationID := "" + var itemToUpdate []velero.ResourceIdentifier + + // Only return Async operation for VSC created for this backup. + if backupOnGoing { + // The operationID is of the form / + operationID = snapCont.Name + "/" + time.Now().Format(time.RFC3339) + itemToUpdate = []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.VolumeSnapshotContents, + Name: snapCont.Name, + }, + } + } p.Log.Infof("Returning from VolumeSnapshotContentBackupItemAction with %d additionalItems to backup", len(additionalItems)) diff --git a/internal/restore/pvc_action.go b/internal/restore/pvc_action.go index 986278da..b01c7398 100644 --- a/internal/restore/pvc_action.go +++ b/internal/restore/pvc_action.go @@ -143,7 +143,8 @@ func (p *PVCRestoreItemAction) Execute(input *velero.RestoreItemActionExecuteInp operationID := "" // remove the volumesnapshot name annotation as well - removePVCAnnotations(&pvc, []string{util.VolumeSnapshotLabel}) + // clean the DataUploadNameLabel for snapshot data mover case. + removePVCAnnotations(&pvc, []string{util.VolumeSnapshotLabel, util.DataUploadNameAnnotation}) if boolptr.IsSetToFalse(input.Restore.Spec.RestorePVs) { logger.Info("Restore did not request for PVs to be restored from snapshot") @@ -161,6 +162,16 @@ func (p *PVCRestoreItemAction) Execute(input *velero.RestoreItemActionExecuteInp if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { logger.Info("Start DataMover restore.") + // If PVC doesn't have a DataUploadNameLabel, which should be created + // during backup, then CSI cannot handle the volume during to restore, + // so return early to let Velero tries to fall back to Velero native snapshot. + if _, ok := pvcFromBackup.Annotations[util.DataUploadNameAnnotation]; !ok { + logger.Warnf("PVC doesn't have a DataUpload for data mover. Return.") + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: input.Item, + }, nil + } + operationID = label.GetValidName(string(velerov1api.AsyncOperationIDPrefixDataDownload) + string(input.Restore.UID) + "." + string(pvcFromBackup.UID)) dataDownload, err := restoreFromDataUploadResult(context.Background(), input.Restore, &pvc, operationID, pvcFromBackup.Namespace, p.Client, p.VeleroClient) diff --git a/internal/restore/pvc_action_test.go b/internal/restore/pvc_action_test.go index afc37724..33440884 100644 --- a/internal/restore/pvc_action_test.go +++ b/internal/restore/pvc_action_test.go @@ -578,7 +578,7 @@ func TestExecute(t *testing.T) { name: "DataUploadResult cannot be found", backup: builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result(), restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").Result(), - pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi")).Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi", util.DataUploadNameAnnotation, "velero/")).Result(), expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").Result(), expectedErr: "fail get DataUploadResult for restore: testRestore: no DataUpload result cm found with labels velero.io/pvc-namespace-name=velero.testPVC,velero.io/restore-uid=,velero.io/resource-usage=DataUpload", }, @@ -586,7 +586,7 @@ func TestExecute(t *testing.T) { name: "Restore from DataUploadResult", backup: builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result(), restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").ObjectMeta(builder.WithUID("uid")).Result(), - pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi")).Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi", util.DataUploadNameAnnotation, "velero/")).Result(), dataUploadResult: builder.ForConfigMap("velero", "testCM").Data("uid", "{}").ObjectMeta(builder.WithLabels(velerov1api.RestoreUIDLabel, "uid", velerov1api.PVCNamespaceNameLabel, "velero.testPVC", velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)))).Result(), expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations("velero.io/vsi-volumesnapshot-restore-size", "10Gi")).Result(), expectedDataDownload: builder.ForDataDownload("velero", "").TargetVolume(velerov2alpha1.TargetVolumeSpec{PVC: "testPVC", Namespace: "velero"}). @@ -598,10 +598,16 @@ func TestExecute(t *testing.T) { name: "Restore from DataUploadResult with long source PVC namespace and name", backup: builder.ForBackup("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "testBackup").SnapshotMoveData(true).Result(), restore: builder.ForRestore("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "testRestore").Backup("testBackup").ObjectMeta(builder.WithUID("uid")).Result(), - pvc: builder.ForPersistentVolumeClaim("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "kibishii-data-kibishii-deployment-0").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi")).Result(), + pvc: builder.ForPersistentVolumeClaim("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "kibishii-data-kibishii-deployment-0").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi", util.DataUploadNameAnnotation, "velero/")).Result(), dataUploadResult: builder.ForConfigMap("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "testCM").Data("uid", "{}").ObjectMeta(builder.WithLabels(velerov1api.RestoreUIDLabel, "uid", velerov1api.PVCNamespaceNameLabel, "migre209d0da-49c7-45ba-8d5a-3e59fd591ec1.kibishii-data-ki152333", velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)))).Result(), expectedPVC: builder.ForPersistentVolumeClaim("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "kibishii-data-kibishii-deployment-0").ObjectMeta(builder.WithAnnotations("velero.io/vsi-volumesnapshot-restore-size", "10Gi")).Result(), }, + { + name: "PVC had no DataUploadNameLabel annotation", + backup: builder.ForBackup("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "testBackup").SnapshotMoveData(true).Result(), + restore: builder.ForRestore("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "testRestore").Backup("testBackup").ObjectMeta(builder.WithUID("uid")).Result(), + pvc: builder.ForPersistentVolumeClaim("migre209d0da-49c7-45ba-8d5a-3e59fd591ec1", "kibishii-data-kibishii-deployment-0").ObjectMeta(builder.WithAnnotations(util.VolumeSnapshotRestoreSize, "10Gi")).Result(), + }, } for _, tc := range tests { diff --git a/internal/util/labels_annotations.go b/internal/util/labels_annotations.go index d24d915c..bb9f4b89 100644 --- a/internal/util/labels_annotations.go +++ b/internal/util/labels_annotations.go @@ -49,4 +49,7 @@ const ( // DynamicPVRestoreLabel is the label key for dynamic PV restore DynamicPVRestoreLabel = "velero.io/dynamic-pv-restore" + + // DataUploadNameAnnotation is the label key for the DataUpload name + DataUploadNameAnnotation = "velero.io/data-upload-name" )