From 687dcf69e7949e77e93a67c23698437083a58a00 Mon Sep 17 00:00:00 2001 From: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> Date: Sun, 15 Jun 2025 09:31:52 +0000 Subject: [PATCH] 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> --- changelogs/unreleased/9024-amastbau | 1 + pkg/backup/actions/csi/pvc_action.go | 82 +++++++-- pkg/backup/actions/csi/pvc_action_test.go | 171 ++++++++++++++++-- pkg/restore/actions/csi/pvc_action.go | 128 ++++--------- pkg/restore/actions/csi/pvc_action_test.go | 104 +---------- .../actions/csi/volumesnapshot_action.go | 2 +- 6 files changed, 252 insertions(+), 236 deletions(-) create mode 100644 changelogs/unreleased/9024-amastbau diff --git a/changelogs/unreleased/9024-amastbau b/changelogs/unreleased/9024-amastbau new file mode 100644 index 000000000..1122267b9 --- /dev/null +++ b/changelogs/unreleased/9024-amastbau @@ -0,0 +1 @@ +Fix Issue 8816 When specifying LabelSelector on restore, related items such as PVC and VolumeSnapshot are not included diff --git a/pkg/backup/actions/csi/pvc_action.go b/pkg/backup/actions/csi/pvc_action.go index 907e62b95..c91d7a2b4 100644 --- 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()) + } + } +} diff --git a/pkg/backup/actions/csi/pvc_action_test.go b/pkg/backup/actions/csi/pvc_action_test.go index 051c0174c..795e3c038 100644 --- 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()) + }) + } +} diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index 4e7074315..dcf33f5fc 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -22,10 +22,10 @@ import ( "fmt" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "github.com/pkg/errors" "github.com/sirupsen/logrus" corev1api "k8s.io/api/core/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" @@ -36,6 +36,7 @@ import ( 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" + kuberesource "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/label" plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common" "github.com/vmware-tanzu/velero/pkg/plugin/velero" @@ -65,39 +66,6 @@ func (p *pvcRestoreItemAction) AppliesTo() (velero.ResourceSelector, error) { }, nil } -func resetPVCSpec(pvc *corev1api.PersistentVolumeClaim, vsName string) { - // Restore operation for the PVC will use the VolumeSnapshot as the data source. - // So clear out the volume name, which is a ref to the PV - pvc.Spec.VolumeName = "" - dataSource := &corev1api.TypedLocalObjectReference{ - APIGroup: &snapshotv1api.SchemeGroupVersion.Group, - Kind: "VolumeSnapshot", - Name: vsName, - } - pvc.Spec.DataSource = dataSource - pvc.Spec.DataSourceRef = nil -} - -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()) - } - } -} - // Execute modifies the PVC's spec to use the VolumeSnapshot object as the // data source ensuring that the newly provisioned volume can be pre-populated // with data from the VolumeSnapshot. @@ -139,6 +107,7 @@ func (p *pvcRestoreItemAction) Execute( operationID := "" + additionalItems := []velero.ResourceIdentifier{} if boolptr.IsSetToFalse(input.Restore.Spec.RestorePVs) { logger.Info("Restore did not request for PVs to be restored from snapshot") pvc.Spec.VolumeName = "" @@ -186,24 +155,27 @@ func (p *pvcRestoreItemAction) Execute( logger.Infof("DataDownload %s/%s is created successfully.", dataDownload.Namespace, dataDownload.Name) } else { - targetVSName := "" - if vsName, nameOK := pvcFromBackup.Annotations[velerov1api.VolumeSnapshotLabel]; nameOK { - targetVSName = util.GenerateSha256FromRestoreUIDAndVsName(string(input.Restore.UID), vsName) - } else { - logger.Info("Skipping PVCRestoreItemAction for PVC,", - "PVC does not have a CSI VolumeSnapshot.") - // Make no change in the input PVC. + //CSI restore + vsName, nameOK := pvcFromBackup.Annotations[velerov1api.VolumeSnapshotLabel] + if !nameOK { + logger.Info("Skipping PVCRestoreItemAction for PVC, PVC does not have a CSI VolumeSnapshot.") return &velero.RestoreItemActionExecuteOutput{ UpdatedItem: input.Item, }, nil } - if err := restoreFromVolumeSnapshot( - &pvc, newNamespace, p.crClient, targetVSName, logger, - ); err != nil { - logger.Errorf("Failed to restore PVC from VolumeSnapshot.") - return nil, errors.WithStack(err) - } + //To avoid confilcs, vs and vsc get a new uniq name based in restore UID + // and vs name old name + newVSName := util.GenerateSha256FromRestoreUIDAndVsName(string(input.Restore.UID), vsName) + + p.log.Debugf("Setting PVC source to VolumeSnapshot new name: %s", newVSName) + resetPVCSourceToVolumeSnapshot(&pvc, newVSName) + + additionalItems = append(additionalItems, velero.ResourceIdentifier{ + GroupResource: kuberesource.VolumeSnapshots, + Name: vsName, + Namespace: pvc.Namespace, + }) } } @@ -214,11 +186,25 @@ func (p *pvcRestoreItemAction) Execute( logger.Info("Returning from PVCRestoreItemAction for PVC") return &velero.RestoreItemActionExecuteOutput{ - UpdatedItem: &unstructured.Unstructured{Object: pvcMap}, - OperationID: operationID, + UpdatedItem: &unstructured.Unstructured{Object: pvcMap}, + OperationID: operationID, + AdditionalItems: additionalItems, }, nil } +func resetPVCSourceToVolumeSnapshot(pvc *corev1api.PersistentVolumeClaim, vsName string) { + // Restore operation for the PVC will use the VolumeSnapshot as the data source. + // So clear out the volume name, which is a ref to the PV + pvc.Spec.VolumeName = "" + dataSource := &corev1api.TypedLocalObjectReference{ + APIGroup: &snapshotv1api.SchemeGroupVersion.Group, + Kind: "VolumeSnapshot", + Name: vsName, + } + pvc.Spec.DataSource = dataSource + pvc.Spec.DataSourceRef = nil +} + func (p *pvcRestoreItemAction) Name() string { return "PVCRestoreItemAction" } @@ -456,50 +442,6 @@ func newDataDownload( return dataDownload } -func restoreFromVolumeSnapshot( - pvc *corev1api.PersistentVolumeClaim, - newNamespace string, - crClient crclient.Client, - volumeSnapshotName string, - logger logrus.FieldLogger, -) error { - vs := new(snapshotv1api.VolumeSnapshot) - if err := crClient.Get(context.TODO(), - crclient.ObjectKey{ - Namespace: newNamespace, - Name: volumeSnapshotName, - }, - vs, - ); err != nil { - return errors.Wrapf(err, "Failed to get Volumesnapshot %s/%s to restore PVC %s/%s", - newNamespace, volumeSnapshotName, newNamespace, pvc.Name) - } - - if _, exists := vs.Annotations[velerov1api.VolumeSnapshotRestoreSize]; exists { - restoreSize, err := resource.ParseQuantity( - vs.Annotations[velerov1api.VolumeSnapshotRestoreSize]) - if err != nil { - return errors.Wrapf(err, - "Failed to parse %s from annotation on Volumesnapshot %s/%s into restore size", - vs.Annotations[velerov1api.VolumeSnapshotRestoreSize], vs.Namespace, vs.Name) - } - // 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) - } - - resetPVCSpec(pvc, volumeSnapshotName) - - return nil -} - func restoreFromDataUploadResult( ctx context.Context, restore *velerov1api.Restore, diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index ad9271562..bc7f66d89 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -154,7 +154,7 @@ func TestResetPVCSpec(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { before := tc.pvc.DeepCopy() - resetPVCSpec(&tc.pvc, tc.vsName) + resetPVCSourceToVolumeSnapshot(&tc.pvc, tc.vsName) assert.Equalf(t, tc.pvc.Name, before.Name, "unexpected change to Object.Name, Want: %s; Got %s", before.Name, tc.pvc.Name) assert.Equalf(t, tc.pvc.Namespace, before.Namespace, "unexpected change to Object.Namespace, Want: %s; Got %s", before.Namespace, tc.pvc.Namespace) @@ -170,101 +170,6 @@ func TestResetPVCSpec(t *testing.T) { } } -func TestResetPVCResourceRequest(t *testing.T) { - var storageReq50Mi, storageReq1Gi, cpuQty resource.Quantity - - storageReq50Mi, err := resource.ParseQuantity("50Mi") - require.NoError(t, err) - storageReq1Gi, err = resource.ParseQuantity("1Gi") - require.NoError(t, err) - cpuQty, err = resource.ParseQuantity("100m") - require.NoError(t, err) - - testCases := []struct { - name string - pvc corev1api.PersistentVolumeClaim - restoreSize resource.Quantity - expectedStorageRequestQty string - }{ - { - name: "should set storage resource request from volumesnapshot, pvc has nil resource requests", - pvc: corev1api.PersistentVolumeClaim{ - Spec: corev1api.PersistentVolumeClaimSpec{ - Resources: corev1api.VolumeResourceRequirements{ - Requests: nil, - }, - }, - }, - restoreSize: storageReq50Mi, - expectedStorageRequestQty: "50Mi", - }, - { - name: "should set storage resource request from volumesnapshot, pvc has empty resource requests", - pvc: corev1api.PersistentVolumeClaim{ - Spec: corev1api.PersistentVolumeClaimSpec{ - Resources: corev1api.VolumeResourceRequirements{ - Requests: corev1api.ResourceList{}, - }, - }, - }, - restoreSize: storageReq50Mi, - expectedStorageRequestQty: "50Mi", - }, - { - name: "should merge resource requests from volumesnapshot into pvc with no storage resource requests", - pvc: corev1api.PersistentVolumeClaim{ - Spec: corev1api.PersistentVolumeClaimSpec{ - Resources: corev1api.VolumeResourceRequirements{ - Requests: corev1api.ResourceList{ - corev1api.ResourceCPU: cpuQty, - }, - }, - }, - }, - restoreSize: storageReq50Mi, - expectedStorageRequestQty: "50Mi", - }, - { - name: "should set storage resource request from volumesnapshot, pvc requests less storage", - pvc: corev1api.PersistentVolumeClaim{ - Spec: corev1api.PersistentVolumeClaimSpec{ - Resources: corev1api.VolumeResourceRequirements{ - Requests: corev1api.ResourceList{ - corev1api.ResourceStorage: storageReq50Mi, - }, - }, - }, - }, - restoreSize: storageReq1Gi, - expectedStorageRequestQty: "1Gi", - }, - { - name: "should not set storage resource request from volumesnapshot, pvc requests more storage", - pvc: corev1api.PersistentVolumeClaim{ - Spec: corev1api.PersistentVolumeClaimSpec{ - Resources: corev1api.VolumeResourceRequirements{ - Requests: corev1api.ResourceList{ - corev1api.ResourceStorage: storageReq1Gi, - }, - }, - }, - }, - restoreSize: storageReq50Mi, - expectedStorageRequestQty: "1Gi", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - log := logrus.New().WithField("unit-test", tc.name) - setPVCStorageResourceRequest(&tc.pvc, tc.restoreSize, log) - expected, err := resource.ParseQuantity(tc.expectedStorageRequestQty) - require.NoError(t, err) - assert.Equal(t, expected, tc.pvc.Spec.Resources.Requests[corev1api.ResourceStorage]) - }) - } -} - func TestProgress(t *testing.T) { currentTime := time.Now() tests := []struct { @@ -485,13 +390,6 @@ func TestExecute(t *testing.T) { pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").Result(), expectedErr: "fail to get backup for restore: backups.velero.io \"testBackup\" not found", }, - { - name: "VolumeSnapshot cannot be found", - backup: builder.ForBackup("velero", "testBackup").Result(), - restore: builder.ForRestore("velero", "testRestore").ObjectMeta(builder.WithUID("restoreUID")).Backup("testBackup").Result(), - pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName")).Result(), - expectedErr: fmt.Sprintf("Failed to get Volumesnapshot velero/%s to restore PVC velero/testPVC: volumesnapshots.snapshot.storage.k8s.io \"%s\" not found", vsName, vsName), - }, { name: "Restore from VolumeSnapshot", backup: builder.ForBackup("velero", "testBackup").Result(), diff --git a/pkg/restore/actions/csi/volumesnapshot_action.go b/pkg/restore/actions/csi/volumesnapshot_action.go index 49e346bac..50fb3f0ed 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action.go +++ b/pkg/restore/actions/csi/volumesnapshot_action.go @@ -121,7 +121,7 @@ func (p *volumeSnapshotRestoreItemAction) Execute( } p.log.Infof(`Returning from VolumeSnapshotRestoreItemAction with - no additionalItems`) + VolumeSnapshotContent in additionalItems`) return &velero.RestoreItemActionExecuteOutput{ UpdatedItem: &unstructured.Unstructured{Object: vsMap},