Merge pull request #9643 from priyansh17/user/priyansh/issue-#9641

Refactor: simplify VolumeSnapshotContent deletion logic

changelogs/unreleased/9643-priyansh17

@@ -0,0 +1 @@
+Fix issue #9641, Remove redundant ReadyToUse polling in CSI VolumeSnapshotContent delete plugin

internal/delete/actions/csi/volumesnapshotcontent_action.go

@@ -18,7 +18,6 @@ package csi
 
 import (
 	"context"
-	"time"
 
 	"github.com/google/uuid"
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
@@ -27,14 +26,11 @@ import (
 	corev1api "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/wait"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/client"
 	plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
-	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 
@@ -127,37 +123,8 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 	}
 	p.log.Infof("Created temp VolumeSnapshotContent %s with DeletionPolicy=Delete to trigger cloud snapshot cleanup", snapCont.Name)
 
-	// Read resource timeout from backup annotation, if not set, use default value.
-	timeout, err := time.ParseDuration(
-		input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation])
-	if err != nil {
-		p.log.Warnf("fail to parse resource timeout annotation %s: %s",
-			input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation], err.Error())
-		timeout = 10 * time.Minute
-	}
-	p.log.Debugf("resource timeout is set to %s", timeout.String())
-
-	interval := 5 * time.Second
-
-	// Wait until VSC created and ReadyToUse is true.
-	if err := wait.PollUntilContextTimeout(
-		context.Background(),
-		interval,
-		timeout,
-		true,
-		func(ctx context.Context) (bool, error) {
-			return checkVSCReadiness(ctx, &snapCont, p.crClient)
-		},
-	); err != nil {
-		// Clean up the VSC we created since it can't become ready
-		p.log.WithError(err).Warnf("Temp VolumeSnapshotContent %s did not become 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)
-		}
-		return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes 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,
@@ -212,32 +179,6 @@ func (p *volumeSnapshotContentDeleteItemAction) tryDeleteOriginalVSC(
 	return true
 }
 
-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 && boolptr.IsSetToTrue(tmpVSC.Status.ReadyToUse) {
-		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 {

internal/delete/actions/csi/volumesnapshotcontent_action_test.go

@@ -22,7 +22,6 @@ import (
 	"testing"
 
 	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"
@@ -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",
@@ -103,28 +97,8 @@ func TestVSCExecute(t *testing.T) {
 		{
 			name:      "Normal case, VolumeSnapshot should be deleted",
 			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").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).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:      "Error case, deletion fails",
-			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
-			backup:    builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).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").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).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(t.Context(), test.preExistingVSC))
@@ -222,74 +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) {
-			crClient := velerotest.NewFakeControllerRuntimeClient(t)
-			if test.createVSC {
-				require.NoError(t, crClient.Create(t.Context(), test.vsc))
-			}
-
-			ready, err := checkVSCReadiness(t.Context(), 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