Modify CSI related DeleteItemActions.

Remove the VS DIA.
Modify the VSC DIA: create then delete the VSC.

Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>

internal/delete/actions/csi/volumesnapshot_action.go

@@ -1,120 +0,0 @@
-/*
-Copyright the Velero contributors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package csi
-
-import (
-	"context"
-	"fmt"
-
-	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/runtime"
-	crclient "sigs.k8s.io/controller-runtime/pkg/client"
-
-	"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/csi"
-	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
-)
-
-// volumeSnapshotDeleteItemAction is a backup item action plugin for Velero.
-type volumeSnapshotDeleteItemAction struct {
-	log      logrus.FieldLogger
-	crClient crclient.Client
-}
-
-// AppliesTo returns information indicating that the
-// VolumeSnapshotBackupItemAction should be invoked to backup
-// VolumeSnapshots.
-func (p *volumeSnapshotDeleteItemAction) AppliesTo() (velero.ResourceSelector, error) {
-	p.log.Debug("VolumeSnapshotBackupItemAction AppliesTo")
-
-	return velero.ResourceSelector{
-		IncludedResources: []string{"volumesnapshots.snapshot.storage.k8s.io"},
-	}, nil
-}
-
-func (p *volumeSnapshotDeleteItemAction) Execute(
-	input *velero.DeleteItemActionExecuteInput,
-) error {
-	p.log.Info("Starting VolumeSnapshotDeleteItemAction for volumeSnapshot")
-
-	var vs snapshotv1api.VolumeSnapshot
-
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(
-		input.Item.UnstructuredContent(),
-		&vs,
-	); err != nil {
-		return errors.Wrapf(err, "failed to convert input.Item from unstructured")
-	}
-
-	// We don't want this DeleteItemAction plugin to delete VolumeSnapshot
-	// taken outside of Velero.  So skip deleting VolumeSnapshot objects
-	// that were not created in the process of creating the Velero
-	// backup being deleted.
-	if !kubeutil.HasBackupLabel(&vs.ObjectMeta, input.Backup.Name) {
-		p.log.Info(
-			"VolumeSnapshot %s/%s was not taken by backup %s, skipping deletion",
-			vs.Namespace, vs.Name, input.Backup.Name,
-		)
-		return nil
-	}
-
-	p.log.Infof("Deleting VolumeSnapshot %s/%s", vs.Namespace, vs.Name)
-	if vs.Status != nil && vs.Status.BoundVolumeSnapshotContentName != nil {
-		// we patch the DeletionPolicy of the VolumeSnapshotContent
-		// to set it to Delete.  This ensures that the volume snapshot
-		// in the storage provider is also deleted.
-		err := csi.SetVolumeSnapshotContentDeletionPolicy(
-			*vs.Status.BoundVolumeSnapshotContentName,
-			p.crClient,
-		)
-		if err != nil && !apierrors.IsNotFound(err) {
-			return errors.Wrapf(
-				err,
-				fmt.Sprintf("failed to patch DeletionPolicy of volume snapshot %s/%s",
-					vs.Namespace, vs.Name),
-			)
-		}
-
-		if apierrors.IsNotFound(err) {
-			return nil
-		}
-	}
-	err := p.crClient.Delete(context.TODO(), &vs)
-	if err != nil && !apierrors.IsNotFound(err) {
-		return err
-	}
-	return nil
-}
-
-func NewVolumeSnapshotDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer {
-	return func(logger logrus.FieldLogger) (any, error) {
-		crClient, err := f.KubebuilderClient()
-		if err != nil {
-			return nil, errors.WithStack(err)
-		}
-
-		return &volumeSnapshotDeleteItemAction{
-			log:      logger,
-			crClient: crClient,
-		}, nil
-	}
-}

internal/delete/actions/csi/volumesnapshot_action_test.go

@@ -1,151 +0,0 @@
-/*
-Copyright the Velero contributors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package csi
-
-import (
-	"context"
-	"fmt"
-	"testing"
-
-	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
-	"github.com/sirupsen/logrus"
-	"github.com/stretchr/testify/require"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
-
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	"github.com/vmware-tanzu/velero/pkg/builder"
-	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"
-)
-
-func TestVSExecute(t *testing.T) {
-	tests := []struct {
-		name      string
-		item      runtime.Unstructured
-		vs        *snapshotv1api.VolumeSnapshot
-		backup    *velerov1api.Backup
-		createVS  bool
-		expectErr bool
-	}{
-		{
-			name: "VolumeSnapshot doesn't have backup label",
-			item: velerotest.UnstructuredOrDie(
-				`
-				{
-					"apiVersion": "snapshot.storage.k8s.io/v1",
-					"kind": "VolumeSnapshot",
-					"metadata": {
-						"namespace": "ns",
-						"name": "foo"
-					}
-				}
-				`,
-			),
-			backup:    builder.ForBackup("velero", "backup").Result(),
-			expectErr: false,
-		},
-		{
-			name: "VolumeSnapshot doesn't exist in the cluster",
-			vs: builder.ForVolumeSnapshot("foo", "bar").
-				ObjectMeta(builder.WithLabelsMap(
-					map[string]string{velerov1api.BackupNameLabel: "backup"},
-				)).Status().
-				BoundVolumeSnapshotContentName("vsc").
-				Result(),
-			backup:    builder.ForBackup("velero", "backup").Result(),
-			expectErr: true,
-		},
-		{
-			name: "Normal case, VolumeSnapshot should be deleted",
-			vs: builder.ForVolumeSnapshot("foo", "bar").
-				ObjectMeta(builder.WithLabelsMap(
-					map[string]string{velerov1api.BackupNameLabel: "backup"},
-				)).Status().
-				BoundVolumeSnapshotContentName("vsc").
-				Result(),
-			backup:    builder.ForBackup("velero", "backup").Result(),
-			expectErr: false,
-			createVS:  true,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			crClient := velerotest.NewFakeControllerRuntimeClient(t)
-			logger := logrus.StandardLogger()
-
-			p := volumeSnapshotDeleteItemAction{log: logger, crClient: crClient}
-
-			if test.vs != nil {
-				vsMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(test.vs)
-				require.NoError(t, err)
-				test.item = &unstructured.Unstructured{Object: vsMap}
-			}
-
-			if test.createVS {
-				require.NoError(t, crClient.Create(context.TODO(), test.vs))
-			}
-
-			err := p.Execute(
-				&velero.DeleteItemActionExecuteInput{
-					Item:   test.item,
-					Backup: test.backup,
-				},
-			)
-
-			if test.expectErr == false {
-				require.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestVSAppliesTo(t *testing.T) {
-	p := volumeSnapshotDeleteItemAction{
-		log: logrus.StandardLogger(),
-	}
-	selector, err := p.AppliesTo()
-
-	require.NoError(t, err)
-
-	require.Equal(
-		t,
-		velero.ResourceSelector{
-			IncludedResources: []string{"volumesnapshots.snapshot.storage.k8s.io"},
-		},
-		selector,
-	)
-}
-
-func TestNewVolumeSnapshotDeleteItemAction(t *testing.T) {
-	logger := logrus.StandardLogger()
-	crClient := velerotest.NewFakeControllerRuntimeClient(t)
-
-	f := &factorymocks.Factory{}
-	f.On("KubebuilderClient").Return(nil, fmt.Errorf(""))
-	plugin := NewVolumeSnapshotDeleteItemAction(f)
-	_, err := plugin(logger)
-	require.Error(t, err)
-
-	f1 := &factorymocks.Factory{}
-	f1.On("KubebuilderClient").Return(crClient, nil)
-	plugin1 := NewVolumeSnapshotDeleteItemAction(f1)
-	_, err1 := plugin1(logger)
-	require.NoError(t, err1)
-}

internal/delete/actions/csi/volumesnapshotcontent_action.go

@@ -18,19 +18,22 @@ package csi
 
 import (
 	"context"
-	"fmt"
+	"time"
 
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	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/csi"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 
@@ -77,25 +80,48 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 
 	p.log.Infof("Deleting VolumeSnapshotContent %s", snapCont.Name)
 
-	if err := csi.SetVolumeSnapshotContentDeletionPolicy(
-		snapCont.Name,
-		p.crClient,
+	snapCont.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentDelete
+
+	snapCont.Spec.Source = snapshotv1api.VolumeSnapshotContentSource{
+		SnapshotHandle: snapCont.Status.SnapshotHandle,
+	}
+
+	snapCont.Spec.VolumeSnapshotRef = corev1api.ObjectReference{
+		APIVersion: snapshotv1api.SchemeGroupVersion.String(),
+		Kind:       "VolumeSnapshot",
+		Namespace:  "ns-" + string(snapCont.UID),
+		Name:       "name-" + string(snapCont.UID),
+	}
+
+	snapCont.ResourceVersion = ""
+
+	if err := p.crClient.Create(context.TODO(), &snapCont); err != nil {
+		return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", 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 {
-		// #4764: Leave a warning when VolumeSnapshotContent cannot be found for deletion.
-		// Manual deleting VolumeSnapshotContent can cause this.
-		// It's tricky for Velero to handle this inconsistency.
-		// Even if Velero restores the VolumeSnapshotContent, CSI snapshot controller
-		// may not delete it correctly due to the snapshot represented by VolumeSnapshotContent
-		// already deleted on cloud provider.
-		if apierrors.IsNotFound(err) {
-			p.log.Warnf(
-				"VolumeSnapshotContent %s of backup %s cannot be found. May leave orphan snapshot %s on cloud provider.",
-				snapCont.Name, input.Backup.Name, *snapCont.Status.SnapshotHandle)
-			return nil
-		}
-		return errors.Wrapf(err, fmt.Sprintf(
-			"failed to set DeletionPolicy on volumesnapshotcontent %s. Skipping deletion",
-			snapCont.Name))
+		return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes ready.", snapCont.Name)
 	}
 
 	if err := p.crClient.Delete(
@@ -109,6 +135,25 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 	return nil
 }
 
+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
+	}
+
+	return false, nil
+}
+
 func NewVolumeSnapshotContentDeleteItemAction(
 	f client.Factory,
 ) plugincommon.HandlerInitializer {

internal/delete/actions/csi/volumesnapshotcontent_action_test.go

@@ -22,10 +22,12 @@ import (
 	"testing"
 
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
@@ -37,11 +39,15 @@ import (
 func TestVSCExecute(t *testing.T) {
 	snapshotHandleStr := "test"
 	tests := []struct {
-		name      string
-		item      runtime.Unstructured
-		vsc       *snapshotv1api.VolumeSnapshotContent
-		backup    *velerov1api.Backup
-		createVSC bool
+		name     string
+		item     runtime.Unstructured
+		vsc      *snapshotv1api.VolumeSnapshotContent
+		backup   *velerov1api.Backup
+		function func(
+			ctx context.Context,
+			vsc *snapshotv1api.VolumeSnapshotContent,
+			client crclient.Client,
+		) (bool, error)
 		expectErr bool
 	}{
 		{
@@ -62,17 +68,30 @@ func TestVSCExecute(t *testing.T) {
 			expectErr: false,
 		},
 		{
-			name:      "VolumeSnapshotContent doesn't exist in the cluster, no error",
+			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(),
+			backup:    builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).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: false,
-			createVSC: true,
+			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")
+			},
 		},
 	}
 
@@ -80,6 +99,7 @@ func TestVSCExecute(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			crClient := velerotest.NewFakeControllerRuntimeClient(t)
 			logger := logrus.StandardLogger()
+			checkVSCReadiness = test.function
 
 			p := volumeSnapshotContentDeleteItemAction{log: logger, crClient: crClient}
 
@@ -89,10 +109,6 @@ func TestVSCExecute(t *testing.T) {
 				test.item = &unstructured.Unstructured{Object: vscMap}
 			}
 
-			if test.createVSC {
-				require.NoError(t, crClient.Create(context.TODO(), test.vsc))
-			}
-
 			err := p.Execute(
 				&velero.DeleteItemActionExecuteInput{
 					Item:   test.item,

pkg/cmd/server/plugin/plugin.go

@@ -138,10 +138,6 @@ func NewCommand(f client.Factory) *cobra.Command {
 					"velero.io/dataupload-delete",
 					newDateUploadDeleteItemAction(f),
 				).
-				RegisterDeleteItemAction(
-					"velero.io/csi-volumesnapshot-delete",
-					newVolumeSnapshotDeleteItemAction(f),
-				).
 				RegisterDeleteItemAction(
 					"velero.io/csi-volumesnapshotcontent-delete",
 					newVolumeSnapshotContentDeleteItemAction(f),
@@ -432,10 +428,6 @@ func newVolumeSnapshotClassBackupItemAction(logger logrus.FieldLogger) (any, err
 
 // DeleteItemAction plugins
 
-func newVolumeSnapshotDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer {
-	return dia.NewVolumeSnapshotDeleteItemAction(f)
-}
-
 func newVolumeSnapshotContentDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer {
 	return dia.NewVolumeSnapshotContentDeleteItemAction(f)
 }