csi pvc backup action

Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> Update pvc_action.go Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> Update pvc_action.go Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> Adding missing test covarage + log mesasgae as suggested Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> Adding missing test covarage + log mesasgae as suggested Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com>
2026-01-07 13:55:20 +00:00 · 2025-06-15 09:31:52 +00:00
parent 21fa637f17
commit 687dcf69e7
6 changed files with 252 additions and 236 deletions
--- a/pkg/backup/actions/csi/pvc_action.go
+++ b/pkg/backup/actions/csi/pvc_action.go
@@ -42,9 +42,11 @@ import (
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

+	"k8s.io/apimachinery/pkg/api/resource"
+
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	velerov2alpha1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1"
-	"github.com/vmware-tanzu/velero/pkg/client"
+	veleroclient "github.com/vmware-tanzu/velero/pkg/client"
 	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/label"
 	plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common"
@@ -267,6 +269,21 @@ func (p *pvcBackupItemAction) Execute(
 		return nil, nil, "", nil, err
 	}

+	// Wait until VS associated VSC snapshot handle created before
+	// continue.we later require the vsc restore size
+	vsc, err := csi.WaitUntilVSCHandleIsReady(
+		vs,
+		p.crClient,
+		p.log,
+		backup.Spec.CSISnapshotTimeout.Duration,
+	)
+	if err != nil {
+		p.log.Errorf("Failed to wait for VolumeSnapshot %s/%s to become ReadyToUse within timeout %v: %s",
+			vs.Namespace, vs.Name, backup.Spec.CSISnapshotTimeout.Duration, err.Error())
+		csi.CleanupVolumeSnapshot(vs, p.crClient, p.log)
+		return nil, nil, "", nil, errors.WithStack(err)
+	}
+
 	labels := map[string]string{
 		velerov1api.VolumeSnapshotLabel: vs.Name,
 		velerov1api.BackupNameLabel:     backup.Name,
@@ -294,23 +311,6 @@ func (p *pvcBackupItemAction) Execute(
 			"Backup":         backup.Name,
 		})

-		// Wait until VS associated VSC snapshot handle created before
-		// returning with the Async operation for data mover.
-		vsc, err := csi.WaitUntilVSCHandleIsReady(
-			vs,
-			p.crClient,
-			p.log,
-			backup.Spec.CSISnapshotTimeout.Duration,
-		)
-		if err != nil {
-			dataUploadLog.Errorf(
-				"Fail to wait VolumeSnapshot turned to ReadyToUse: %s",
-				err.Error(),
-			)
-			csi.CleanupVolumeSnapshot(vs, p.crClient, p.log)
-			return nil, nil, "", nil, errors.WithStack(err)
-		}
-
 		dataUploadLog.Info("Starting data upload of backup")

 		dataUpload, err := createDataUpload(
@@ -355,6 +355,8 @@ func (p *pvcBackupItemAction) Execute(
 			dataUploadLog.Info("DataUpload is submitted successfully.")
 		}
 	} else {
+		setPVCRequestSizeToVSRestoreSize(&pvc, vsc, p.log)
+
 		additionalItems = []velero.ResourceIdentifier{
 			{
 				GroupResource: kuberesource.VolumeSnapshots,
@@ -571,7 +573,7 @@ func cancelDataUpload(
 	return nil
 }

-func NewPvcBackupItemAction(f client.Factory) plugincommon.HandlerInitializer {
+func NewPvcBackupItemAction(f veleroclient.Factory) plugincommon.HandlerInitializer {
 	return func(logger logrus.FieldLogger) (any, error) {
 		crClient, err := f.KubebuilderClient()
 		if err != nil {
@@ -1036,3 +1038,45 @@ func (p *pvcBackupItemAction) getVGSByLabels(ctx context.Context, namespace stri

 	return &vgsList.Items[0], nil
 }
+
+func setPVCRequestSizeToVSRestoreSize(
+	pvc *corev1api.PersistentVolumeClaim,
+	vsc *snapshotv1api.VolumeSnapshotContent,
+	logger logrus.FieldLogger,
+) {
+	if vsc.Status.RestoreSize != nil {
+		logger.Debugf("Patching PVC request size to fit the volumesnapshot restore size %d", vsc.Status.RestoreSize)
+		restoreSize := *resource.NewQuantity(*vsc.Status.RestoreSize, resource.BinarySI)
+
+		// It is possible that the volume provider allocated a larger
+		// capacity volume than what was requested in the backed up PVC.
+		// In this scenario the volumesnapshot of the PVC will end being
+		// larger than its requested storage size.  Such a PVC, on restore
+		// as-is, will be stuck attempting to use a VolumeSnapshot as a
+		// data source for a PVC that is not large enough.
+		// To counter that, here we set the storage request on the PVC
+		// to the larger of the PVC's storage request and the size of the
+		// VolumeSnapshot
+		setPVCStorageResourceRequest(pvc, restoreSize, logger)
+	}
+}
+
+func setPVCStorageResourceRequest(
+	pvc *corev1api.PersistentVolumeClaim,
+	restoreSize resource.Quantity,
+	log logrus.FieldLogger,
+) {
+	{
+		if pvc.Spec.Resources.Requests == nil {
+			pvc.Spec.Resources.Requests = corev1api.ResourceList{}
+		}
+
+		storageReq, exists := pvc.Spec.Resources.Requests[corev1api.ResourceStorage]
+		if !exists || storageReq.Cmp(restoreSize) < 0 {
+			pvc.Spec.Resources.Requests[corev1api.ResourceStorage] = restoreSize
+			rs := pvc.Spec.Resources.Requests[corev1api.ResourceStorage]
+			log.Infof("Resetting storage requests for PVC %s/%s to %s",
+				pvc.Namespace, pvc.Name, rs.String())
+		}
+	}
+}
--- a/pkg/backup/actions/csi/pvc_action_test.go
+++ b/pkg/backup/actions/csi/pvc_action_test.go
@@ -36,10 +36,13 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
+
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	corev1api "k8s.io/api/core/v1"
 	storagev1api "k8s.io/api/storage/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -54,11 +57,27 @@ import (
 	factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
-	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 )

 const testDriver = "csi.example.com"

+// errorInjectingClient is a wrapper around a normal client that injects an error
+// when a specific resource type (VolumeSnapshot) is created.
+type errorInjectingClient struct {
+	crclient.Client
+}
+
+// Create overrides the embedded client's Create method.
+func (c *errorInjectingClient) Create(ctx context.Context, obj crclient.Object, opts ...crclient.CreateOption) error {
+	// Check if the object being created is a VolumeSnapshot.
+	if _, ok := obj.(*snapshotv1api.VolumeSnapshot); ok {
+		// If it is, return our injected error instead of proceeding.
+		return errors.New("injected error on create")
+	}
+	// For all other object types, call the original, embedded Create method.
+	return c.Client.Create(ctx, obj, opts...)
+}
+
 func TestExecute(t *testing.T) {
 	boolTrue := true
 	tests := []struct {
@@ -70,15 +89,37 @@ func TestExecute(t *testing.T) {
 		vsClass            *snapshotv1api.VolumeSnapshotClass
 		operationID        string
 		expectedErr        error
+		expectErr          bool // Use bool for cases where we just need to check for any error
 		expectedBackup     *velerov1api.Backup
 		expectedDataUpload *velerov2alpha1.DataUpload
 		expectedPVC        *corev1api.PersistentVolumeClaim
 		resourcePolicy     *corev1api.ConfigMap
+		failVSCreate       bool
+		skipVSReadyUpdate  bool // New flag to control VS readiness
 	}{
 		{
-			name:        "Skip PVC BIA when backup is in finalizing phase",
-			backup:      builder.ForBackup("velero", "test").Phase(velerov1api.BackupPhaseFinalizing).Result(),
-			expectedErr: nil,
+			name:   "Skip PVC BIA when backup is in finalizing phase",
+			backup: builder.ForBackup("velero", "test").Phase(velerov1api.BackupPhaseFinalizing).Result(),
+		},
+		{
+			name:         "Fail when creating volumesnapshot returns error",
+			backup:       builder.ForBackup("velero", "test").CSISnapshotTimeout(1 * time.Minute).Result(),
+			pvc:          builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").StorageClass("testSC").Phase(corev1api.ClaimBound).Result(),
+			pv:           builder.ForPersistentVolume("testPV").CSI("hostpath", "testVolume").Result(),
+			sc:           builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
+			vsClass:      builder.ForVolumeSnapshotClass("testVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
+			failVSCreate: true,
+			expectedErr:  errors.New("error creating volume snapshot: injected error on create"),
+		},
+		{
+			name:              "Fail when waiting for VolumeSnapshot to be ready times out",
+			backup:            builder.ForBackup("velero", "test").CSISnapshotTimeout(20 * time.Millisecond).Result(), // Short timeout
+			pvc:               builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").StorageClass("testSC").Phase(corev1api.ClaimBound).Result(),
+			pv:                builder.ForPersistentVolume("testPV").CSI("hostpath", "testVolume").Result(),
+			sc:                builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
+			vsClass:           builder.ForVolumeSnapshotClass("testVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
+			skipVSReadyUpdate: true, // This will cause the timeout
+			expectErr:         true, // Expect an error, but the exact message can vary
 		},
 		{
 			name:        "Test SnapshotMoveData",
@@ -88,7 +129,6 @@ func TestExecute(t *testing.T) {
 			sc:          builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
 			vsClass:     builder.ForVolumeSnapshotClass("testVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
 			operationID: ".",
-			expectedErr: nil,
 			expectedDataUpload: &velerov2alpha1.DataUpload{
 				TypeMeta: metav1.TypeMeta{
 					Kind:       "DataUpload",
@@ -134,7 +174,6 @@ func TestExecute(t *testing.T) {
 			sc:          builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
 			vsClass:     builder.ForVolumeSnapshotClass("tescVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
 			operationID: ".",
-			expectedErr: nil,
 			expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").
 				ObjectMeta(builder.WithAnnotations(velerov1api.MustIncludeAdditionalItemAnnotation, "true", velerov1api.DataUploadNameAnnotation, "velero/"),
 					builder.WithLabels(velerov1api.BackupNameLabel, "test")).
@@ -142,18 +181,17 @@ func TestExecute(t *testing.T) {
 		},
 		{
 			name:           "Test ResourcePolicy",
-			backup:         builder.ForBackup("velero", "test").ResourcePolicies("resourcePolicy").SnapshotVolumes(false).Result(),
+			backup:         builder.ForBackup("velero", "test").ResourcePolicies("resourcePolicy").SnapshotVolumes(false).CSISnapshotTimeout(time.Duration(3600) * time.Second).Result(),
 			resourcePolicy: builder.ForConfigMap("velero", "resourcePolicy").Data("policy", "{\"version\":\"v1\", \"volumePolicies\":[{\"conditions\":{\"csi\": {}},\"action\":{\"type\":\"snapshot\"}}]}").Result(),
 			pvc:            builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").StorageClass("testSC").Phase(corev1api.ClaimBound).Result(),
 			pv:             builder.ForPersistentVolume("testPV").CSI("hostpath", "testVolume").Result(),
 			sc:             builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
 			vsClass:        builder.ForVolumeSnapshotClass("tescVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
-			expectedErr:    nil,
 		},
 	}

 	for _, tc := range tests {
-		t.Run(tc.name, func(*testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			logger := logrus.New()
 			logger.Level = logrus.DebugLevel
 			objects := make([]runtime.Object, 0)
@@ -173,7 +211,13 @@ func TestExecute(t *testing.T) {
 				objects = append(objects, tc.resourcePolicy)
 			}

-			crClient := velerotest.NewFakeControllerRuntimeClient(t, objects...)
+			var crClient crclient.Client
+			if tc.failVSCreate {
+				realFakeClient := velerotest.NewFakeControllerRuntimeClient(t, objects...)
+				crClient = &errorInjectingClient{Client: realFakeClient}
+			} else {
+				crClient = velerotest.NewFakeControllerRuntimeClient(t, objects...)
+			}

 			pvcBIA := pvcBackupItemAction{
 				log:      logger,
@@ -183,7 +227,7 @@ func TestExecute(t *testing.T) {
 			pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.pvc)
 			require.NoError(t, err)

-			if boolptr.IsSetToTrue(tc.backup.Spec.SnapshotMoveData) == true {
+			if tc.pvc != nil && !tc.failVSCreate && !tc.skipVSReadyUpdate {
 				go func() {
 					var vsList snapshotv1api.VolumeSnapshotList
 					err := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(ctx context.Context) (bool, error) {
@@ -191,8 +235,7 @@ func TestExecute(t *testing.T) {

 						require.NoError(t, err)
 						if err != nil || len(vsList.Items) == 0 {
-							//lint:ignore nilerr reason
-							return false, nil // ignore
+							return false, err
 						}
 						return true, nil
 					})
@@ -215,8 +258,18 @@ func TestExecute(t *testing.T) {
 			}

 			resultUnstructed, _, _, _, err := pvcBIA.Execute(&unstructured.Unstructured{Object: pvcMap}, tc.backup)
+
 			if tc.expectedErr != nil {
 				require.EqualError(t, err, tc.expectedErr.Error())
+			} else if tc.expectErr {
+				require.Error(t, err)
+				// On timeout failure, check that the cleanup logic was called
+				if tc.skipVSReadyUpdate {
+					vsList := new(snapshotv1api.VolumeSnapshotList)
+					errList := crClient.List(t.Context(), vsList, &crclient.ListOptions{Namespace: tc.pvc.Namespace})
+					require.NoError(t, errList)
+					require.Empty(t, vsList.Items, "VolumeSnapshot should have been cleaned up after readiness check failed")
+				}
 			} else {
 				require.NoError(t, err)
 			}
@@ -232,7 +285,6 @@ func TestExecute(t *testing.T) {
 			if tc.expectedPVC != nil {
 				resultPVC := new(corev1api.PersistentVolumeClaim)
 				runtime.DefaultUnstructuredConverter.FromUnstructured(resultUnstructed.UnstructuredContent(), resultPVC)
-
 				require.True(t, cmp.Equal(tc.expectedPVC, resultPVC, cmpopts.IgnoreFields(corev1api.PersistentVolumeClaim{}, "ResourceVersion", "Annotations", "Labels")))
 			}
 		})
@@ -296,7 +348,7 @@ func TestProgress(t *testing.T) {
 	}

 	for _, tc := range tests {
-		t.Run(tc.name, func(*testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			crClient := velerotest.NewFakeControllerRuntimeClient(t)
 			logger := logrus.New()

@@ -345,7 +397,6 @@ func TestCancel(t *testing.T) {
 				},
 			},
 			operationID: "testing",
-			expectedErr: nil,
 			expectedDataUpload: velerov2alpha1.DataUpload{
 				TypeMeta: metav1.TypeMeta{
 					Kind:       "DataUpload",
@@ -366,7 +417,7 @@ func TestCancel(t *testing.T) {
 	}

 	for _, tc := range tests {
-		t.Run(tc.name, func(*testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			crClient := velerotest.NewFakeControllerRuntimeClient(t)
 			logger := logrus.New()

@@ -379,9 +430,7 @@ func TestCancel(t *testing.T) {
 			require.NoError(t, err)

 			err = pvcBIA.Cancel(tc.operationID, tc.backup)
-			if tc.expectedErr != nil {
-				require.EqualError(t, err, tc.expectedErr.Error())
-			}
+			require.NoError(t, err)

 			du := new(velerov2alpha1.DataUpload)
 			err = crClient.Get(t.Context(), crclient.ObjectKey{Namespace: tc.dataUpload.Namespace, Name: tc.dataUpload.Name}, du)
@@ -1554,3 +1603,85 @@ func TestHasOwnerReference(t *testing.T) {
 		})
 	}
 }
+
+func TestPVCRequestSize(t *testing.T) {
+	logger := logrus.New()
+
+	tests := []struct {
+		name         string
+		pvcInitial   *corev1api.PersistentVolumeClaim // Use full PVC to allow for nil Requests
+		restoreSize  string
+		expectedSize string
+	}{
+		{
+			name: "UpdateRequired: PVC request is lower than restore size",
+			pvcInitial: func() *corev1api.PersistentVolumeClaim {
+				pvc := builder.ForPersistentVolumeClaim("velero", "testPVC").Result()
+				pvc.Spec.Resources.Requests = corev1api.ResourceList{
+					corev1api.ResourceStorage: resource.MustParse("1Gi"),
+				}
+				return pvc
+			}(),
+			restoreSize:  "2Gi",
+			expectedSize: "2Gi",
+		},
+		{
+			name: "NoUpdateRequired: PVC request is larger than restore size",
+			pvcInitial: func() *corev1api.PersistentVolumeClaim {
+				pvc := builder.ForPersistentVolumeClaim("velero", "testPVC").Result()
+				pvc.Spec.Resources.Requests = corev1api.ResourceList{
+					corev1api.ResourceStorage: resource.MustParse("3Gi"),
+				}
+				return pvc
+			}(),
+			restoreSize:  "2Gi",
+			expectedSize: "3Gi",
+		},
+		{
+			name: "PVC has no initial storage request",
+			pvcInitial: func() *corev1api.PersistentVolumeClaim {
+				pvc := builder.ForPersistentVolumeClaim("velero", "testPVC").Result()
+				pvc.Spec.Resources.Requests = corev1api.ResourceList{} // Empty request list
+				return pvc
+			}(),
+			restoreSize:  "2Gi",
+			expectedSize: "2Gi",
+		},
+		{
+			name: "PVC has no initial Resources.Requests map",
+			pvcInitial: func() *corev1api.PersistentVolumeClaim {
+				pvc := builder.ForPersistentVolumeClaim("velero", "testPVC").Result()
+				pvc.Spec.Resources.Requests = nil // This will trigger the line to be covered
+				return pvc
+			}(),
+			restoreSize:  "2Gi",
+			expectedSize: "2Gi",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Create a VolumeSnapshotContent with restore size
+			rsQty := resource.MustParse(tc.restoreSize)
+
+			vsc := &snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "testVSC",
+				},
+				Status: &snapshotv1api.VolumeSnapshotContentStatus{
+					RestoreSize: pointer.Int64(rsQty.Value()),
+				},
+			}
+
+			// Call the function under test
+			pvc := tc.pvcInitial
+			setPVCRequestSizeToVSRestoreSize(pvc, vsc, logger)
+
+			// Verify that the PVC storage request is updated as expected.
+			updatedSize := pvc.Spec.Resources.Requests[corev1api.ResourceStorage]
+			expected := resource.MustParse(tc.expectedSize)
+			// Corrected line below:
+			require.Equal(t, 0, expected.Cmp(updatedSize), "Expected size %s, but got %s", expected.String(), updatedSize.String())
+		})
+	}
+}