diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index 21f133081..7a4d6baa5 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action.go @@ -115,25 +115,8 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( } p.log.Infof("Created temp VolumeSnapshotContent %s with DeletionPolicy=Delete to trigger cloud snapshot cleanup", snapCont.Name) - // Check if the VSC is ready before proceeding to deletion. - ready, err := checkVSCReadiness(context.TODO(), &snapCont, p.crClient) - if err != nil || !ready { - // Clean up the VSC we created since it isn't ready - if err != nil { - p.log.WithError(err).Warnf("Temp VolumeSnapshotContent %s is not ready, cleaning up", snapCont.Name) - } else { - p.log.Warnf("Temp VolumeSnapshotContent %s is not ready, cleaning up", snapCont.Name) - } - if deleteErr := p.crClient.Delete(context.TODO(), &snapCont); deleteErr != nil && !apierrors.IsNotFound(deleteErr) { - p.log.WithError(deleteErr).Errorf("Failed to clean up temp VolumeSnapshotContent %s", snapCont.Name) - } - if err != nil { - return errors.Wrapf(err, "VolumeSnapshotContent %s is not ready", snapCont.Name) - } - return errors.Errorf("VolumeSnapshotContent %s is not ready", snapCont.Name) - } - - p.log.Infof("Temp VolumeSnapshotContent %s is ready, deleting to trigger cloud snapshot removal", snapCont.Name) + // Delete the temp VSC immediately to trigger cloud snapshot removal. + // The CSI driver will handle the actual cloud snapshot deletion. if err := p.crClient.Delete( context.TODO(), &snapCont, @@ -188,35 +171,6 @@ func (p *volumeSnapshotContentDeleteItemAction) tryDeleteOriginalVSC( return true } -// checkVSCReadiness checks if the given VolumeSnapshotContent has a SnapshotHandle in its status, -// which indicates that the CSI driver has processed the VSC and it's ready for deletion. -// It also checks for any permanent errors reported by the CSI driver and fails fast if such an error is found. -var checkVSCReadiness = func( - ctx context.Context, - vsc *snapshotv1api.VolumeSnapshotContent, - client crclient.Client, -) (bool, error) { - tmpVSC := new(snapshotv1api.VolumeSnapshotContent) - if err := client.Get(ctx, crclient.ObjectKeyFromObject(vsc), tmpVSC); err != nil { - return false, errors.Wrapf( - err, "failed to get VolumeSnapshotContent %s", vsc.Name, - ) - } - - if tmpVSC.Status != nil && tmpVSC.Status.SnapshotHandle != nil { - return true, nil - } - - // Fail fast on permanent CSI driver errors (e.g., InvalidSnapshot.NotFound) - if tmpVSC.Status != nil && tmpVSC.Status.Error != nil && tmpVSC.Status.Error.Message != nil { - return false, errors.Errorf( - "VolumeSnapshotContent %s has error: %s", vsc.Name, *tmpVSC.Status.Error.Message, - ) - } - - return false, nil -} - func NewVolumeSnapshotContentDeleteItemAction( f client.Factory, ) plugincommon.HandlerInitializer { diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index d39d7dd52..a6e5fd14a 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go @@ -21,8 +21,7 @@ import ( "fmt" "testing" - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" - "github.com/pkg/errors" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" @@ -76,12 +75,7 @@ func TestVSCExecute(t *testing.T) { vsc *snapshotv1api.VolumeSnapshotContent backup *velerov1api.Backup preExistingVSC *snapshotv1api.VolumeSnapshotContent - function func( - ctx context.Context, - vsc *snapshotv1api.VolumeSnapshotContent, - client crclient.Client, - ) (bool, error) - expectErr bool + expectErr bool }{ { name: "VolumeSnapshotContent doesn't have backup label", @@ -105,26 +99,6 @@ func TestVSCExecute(t *testing.T) { vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).VolumeSnapshotClassName("volumesnapshotclass").Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), backup: builder.ForBackup("velero", "backup").Result(), expectErr: false, - function: func( - ctx context.Context, - vsc *snapshotv1api.VolumeSnapshotContent, - client crclient.Client, - ) (bool, error) { - return true, nil - }, - }, - { - name: "Normal case, VolumeSnapshot should be deleted", - vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: true, - function: func( - ctx context.Context, - vsc *snapshotv1api.VolumeSnapshotContent, - client crclient.Client, - ) (bool, error) { - return false, errors.Errorf("test error case") - }, }, { name: "Original VSC exists in cluster, cleaned up directly", @@ -141,26 +115,12 @@ func TestVSCExecute(t *testing.T) { }, }, }, - { - name: "Error case with CSI error, dangling VSC should be cleaned up", - vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: true, - function: func( - ctx context.Context, - vsc *snapshotv1api.VolumeSnapshotContent, - client crclient.Client, - ) (bool, error) { - return false, errors.Errorf("VolumeSnapshotContent %s has error: InvalidSnapshot.NotFound", vsc.Name) - }, - }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { crClient := velerotest.NewFakeControllerRuntimeClient(t) logger := logrus.StandardLogger() - checkVSCReadiness = test.function if test.preExistingVSC != nil { require.NoError(t, crClient.Create(context.Background(), test.preExistingVSC)) @@ -222,75 +182,6 @@ func TestNewVolumeSnapshotContentDeleteItemAction(t *testing.T) { require.NoError(t, err1) } -func TestCheckVSCReadiness(t *testing.T) { - tests := []struct { - name string - vsc *snapshotv1api.VolumeSnapshotContent - createVSC bool - expectErr bool - ready bool - }{ - { - name: "VSC not exist", - vsc: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vsc-1", - Namespace: "velero", - }, - }, - createVSC: false, - expectErr: true, - ready: false, - }, - { - name: "VSC not ready", - vsc: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vsc-1", - Namespace: "velero", - }, - }, - createVSC: true, - expectErr: false, - ready: false, - }, - { - name: "VSC has error from CSI driver", - vsc: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vsc-1", - Namespace: "velero", - }, - Status: &snapshotv1api.VolumeSnapshotContentStatus{ - ReadyToUse: boolPtr(false), - Error: &snapshotv1api.VolumeSnapshotError{ - Message: stringPtr("InvalidSnapshot.NotFound: The snapshot 'snap-0abc123' does not exist."), - }, - }, - }, - createVSC: true, - expectErr: true, - ready: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ctx := context.TODO() - crClient := velerotest.NewFakeControllerRuntimeClient(t) - if test.createVSC { - require.NoError(t, crClient.Create(ctx, test.vsc)) - } - - ready, err := checkVSCReadiness(ctx, test.vsc, crClient) - require.Equal(t, test.ready, ready) - if test.expectErr { - require.Error(t, err) - } - }) - } -} - func TestTryDeleteOriginalVSC(t *testing.T) { tests := []struct { name string