From e9d312c27e130262543e1fb9f2547feecdcb2cf0 Mon Sep 17 00:00:00 2001 From: Priyansh Choudhary Date: Tue, 24 Mar 2026 20:34:26 +0530 Subject: [PATCH 1/3] refactor: simplify VolumeSnapshotContent deletion logic and remove unused timeout handling Signed-off-by: Priyansh Choudhary --- .../csi/volumesnapshotcontent_action.go | 47 +++++++------------ .../csi/volumesnapshotcontent_action_test.go | 6 +-- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index a8b43d456..51ab49455 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/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,34 +123,22 @@ 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) + // 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) } - return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes ready.", 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) @@ -212,6 +196,9 @@ 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, @@ -224,7 +211,7 @@ var checkVSCReadiness = func( ) } - if tmpVSC.Status != nil && boolptr.IsSetToTrue(tmpVSC.Status.ReadyToUse) { + if tmpVSC.Status != nil && tmpVSC.Status.SnapshotHandle != nil { return true, nil } diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index ee96018fe..3f0df2120 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go @@ -103,7 +103,7 @@ 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, @@ -116,7 +116,7 @@ func TestVSCExecute(t *testing.T) { { 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(), + backup: builder.ForBackup("velero", "backup").Result(), expectErr: true, function: func( ctx context.Context, @@ -144,7 +144,7 @@ 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(), + backup: builder.ForBackup("velero", "backup").Result(), expectErr: true, function: func( ctx context.Context, From 905a561c84b4e390ca03db5ce745a09c6c3ae36f Mon Sep 17 00:00:00 2001 From: Priyansh Choudhary Date: Tue, 24 Mar 2026 20:49:08 +0530 Subject: [PATCH 2/3] added changelog Signed-off-by: Priyansh Choudhary --- changelogs/unreleased/9643-priyansh17 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/9643-priyansh17 diff --git a/changelogs/unreleased/9643-priyansh17 b/changelogs/unreleased/9643-priyansh17 new file mode 100644 index 000000000..10870c871 --- /dev/null +++ b/changelogs/unreleased/9643-priyansh17 @@ -0,0 +1 @@ +Fix issue #9641, Remove redundant ReadyToUse polling in CSI VolumeSnapshotContent delete plugin \ No newline at end of file From c74d5e7aba5c765a37aa96a6379f369e1c847638 Mon Sep 17 00:00:00 2001 From: Priyansh Choudhary Date: Wed, 25 Mar 2026 15:23:11 +0530 Subject: [PATCH 3/3] refactor: streamline VolumeSnapshotContent deletion process and remove readiness checks Signed-off-by: Priyansh Choudhary --- .../csi/volumesnapshotcontent_action.go | 50 +------- .../csi/volumesnapshotcontent_action_test.go | 110 +----------------- 2 files changed, 3 insertions(+), 157 deletions(-) diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index 51ab49455..98e0fc03b 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action.go @@ -123,25 +123,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, @@ -196,35 +179,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 3f0df2120..114bd752f 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/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", @@ -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: "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").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(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