Fix #6589

1. Only return Async operation for VS and VSC created for current backup. 2. Add DataUpload name annotaion 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 <[email protected]>
vmware-tanzu · Aug 4, 2023 · 6b66740 · 6b66740
1 parent 2662f83
commit 6b66740
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 23 deletions.
diff --git 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
@@ -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,13 @@ 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)
+				fmt.Printf("debug: %+v\n", *resultPVC)
+				require.Equal(t, *tc.expectedPVC, *resultPVC)
+			}
 		})
 	}
 }

diff --git 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 <namespace>/<volumesnapshot-name>/<started-time>
-	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 <namespace>/<volumesnapshot-name>/<started-time>
+		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
@@ -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 <volumesnapshotcontent-name>/<started-time>
-	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 <volumesnapshotcontent-name>/<started-time>
+		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
@@ -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.Infof("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
@@ -578,15 +578,15 @@ 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",
 		},
 		{
 			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
@@ -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"
 )