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 diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index a8b43d456..98e0fc03b 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,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 { diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index ee96018fe..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", @@ -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