✨ ⚠️ Remove CSI volumesnapshot artifact deletion (#3734)

This change is incompatible with velero-plugin-for-csi
releases <= v0.1.2

Remove special casing of CSI volumesnapshot artifacts
from backup deletion logic as this has been moved to
a DeleteItemAction plugin in the velero-plugin-for-csi repo

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

changelogs/unreleased/3734-ashish-amarnath

@@ -0,0 +1,4 @@
+✨ ⚠️ Remove CSI volumesnapshot artifact deletion
+
+This change requires https://github.com/vmware-tanzu/velero-plugin-for-csi/pull/86 for Velero to continue
+deleting of CSI volumesnapshots when the corresponding backups are deleted.

pkg/controller/backup_deletion_controller.go

@@ -24,7 +24,6 @@ import (
 
 	jsonpatch "github.com/evanphx/json-patch"
 	snapshotterClientSet "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned"
-	snapshotter "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/typed/volumesnapshot/v1beta1"
 	snapshotv1beta1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1beta1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -40,7 +39,6 @@ import (
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/vmware-tanzu/velero/pkg/backup"
 	"github.com/vmware-tanzu/velero/pkg/discovery"
-	"github.com/vmware-tanzu/velero/pkg/features"
 	velerov1client "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/typed/velero/v1"
 	velerov1informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions/velero/v1"
 	velerov1listers "github.com/vmware-tanzu/velero/pkg/generated/listers/velero/v1"
@@ -370,22 +368,6 @@ func (c *backupDeletionController) processRequest(req *velerov1api.DeleteBackupR
 		}
 	}
 
-	if features.IsEnabled(velerov1api.CSIFeatureFlag) {
-		log.Info("Removing CSI volumesnapshots")
-		if csiErrs := deleteCSIVolumeSnapshots(backup.Name, c.csiSnapshotLister, c.csiSnapshotClient.SnapshotV1beta1(), log); len(csiErrs) > 0 {
-			for _, err := range csiErrs {
-				errs = append(errs, err.Error())
-			}
-		}
-
-		log.Info("Removing CSI volumesnapshotcontents")
-		if csiErrs := deleteCSIVolumeSnapshotContents(backup.Name, c.csiSnapshotContentLister, c.csiSnapshotClient.SnapshotV1beta1(), log); len(csiErrs) > 0 {
-			for _, err := range csiErrs {
-				errs = append(errs, err.Error())
-			}
-		}
-	}
-
 	log.Info("Removing restores")
 	if restores, err := c.restoreLister.Restores(backup.Namespace).List(labels.Everything()); err != nil {
 		log.WithError(errors.WithStack(err)).Error("Error listing restore API objects")
@@ -514,82 +496,6 @@ func (c *backupDeletionController) deleteResticSnapshots(backup *velerov1api.Bac
 	return errs
 }
 
-func setVolumeSnapshotContentDeletionPolicy(vscName string, csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) error {
-	log.Infof("Setting DeletionPolicy of CSI volumesnapshotcontent %s to Delete", vscName)
-	pb := []byte(`{"spec":{"deletionPolicy":"Delete"}}`)
-	_, err := csiClient.VolumeSnapshotContents().Patch(context.TODO(), vscName, types.MergePatchType, pb, metav1.PatchOptions{})
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func deleteCSIVolumeSnapshots(backupName string, csiSnapshotLister snapshotv1beta1listers.VolumeSnapshotLister,
-	csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error {
-	errs := []error{}
-
-	selector := label.NewSelectorForBackup(backupName)
-	csiVolSnaps, err := csiSnapshotLister.List(selector)
-	if err != nil {
-		return []error{err}
-	}
-
-	log.Infof("Deleting %d CSI volumesnapshots", len(csiVolSnaps))
-	for _, csiVS := range csiVolSnaps {
-		log.Infof("Deleting CSI volumesnapshot %s/%s", csiVS.Namespace, csiVS.Name)
-		if csiVS.Status != nil && csiVS.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 := setVolumeSnapshotContentDeletionPolicy(*csiVS.Status.BoundVolumeSnapshotContentName, csiClient, log)
-			if err != nil && !apierrors.IsNotFound(err) {
-				log.Errorf("Skipping deletion of volumesnapshot %s/%s", csiVS.Namespace, csiVS.Name)
-				errs = append(errs, err)
-				continue
-			}
-		}
-		err := csiClient.VolumeSnapshots(csiVS.Namespace).Delete(context.TODO(), csiVS.Name, metav1.DeleteOptions{})
-		if err != nil {
-			errs = append(errs, err)
-		}
-	}
-	return errs
-}
-
-func deleteCSIVolumeSnapshotContents(backupName string, csiVSCLister snapshotv1beta1listers.VolumeSnapshotContentLister,
-	csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error {
-	errs := []error{}
-	selector := label.NewSelectorForBackup(backupName)
-	csiVolSnapConts, err := csiVSCLister.List(selector)
-	if err != nil {
-		return []error{err}
-	}
-	// It is possible that by the time deleteCSIVolumeSnapshotContents is called after deleteCSIVolumeSnapshots
-	// that deletion of VSCs hasn't been completed, by the snapshot-controller (one of the CSI components).
-	// For that reason the csiVSCLister returned VSCs that are yet to be deleted. To handle this scenario,
-	// we swallow `IsNotFound` errors from the setVolumeSnapshotContentDeletionPolicy function and the
-	// csiClient.VolumeSnapshotContents().Delete(...)
-	log.Infof("Deleting %d CSI volumesnapshotcontents", len(csiVolSnapConts))
-	for _, snapCont := range csiVolSnapConts {
-		err := setVolumeSnapshotContentDeletionPolicy(snapCont.Name, csiClient, log)
-		if err != nil && !apierrors.IsNotFound(err) {
-			log.Errorf("Failed to set DeletionPolicy on volumesnapshotcontent %s. Skipping deletion", snapCont.Name)
-			errs = append(errs, err)
-			continue
-		}
-		if apierrors.IsNotFound(err) {
-			log.Infof("volumesnapshotcontent %s not found", snapCont.Name)
-			continue
-		}
-		log.Infof("Deleting volumesnapshotcontent %s", snapCont.Name)
-		err = csiClient.VolumeSnapshotContents().Delete(context.TODO(), snapCont.Name, metav1.DeleteOptions{})
-		if err != nil && !apierrors.IsNotFound(err) {
-			errs = append(errs, err)
-		}
-	}
-
-	return errs
-}
-
 const deleteBackupRequestMaxAge = 24 * time.Hour
 
 func (c *backupDeletionController) deleteExpiredRequests() {

pkg/controller/backup_deletion_controller_test.go

@@ -24,9 +24,6 @@ import (
 	"testing"
 	"time"
 
-	snapshotv1beta1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1beta1"
-	snapshotFake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake"
-	snapshotv1beta1informers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
@@ -1154,342 +1151,3 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) {
 		})
 	}
 }
-
-func TestSetVolumeSnapshotContentDeletionPolicy(t *testing.T) {
-	testCases := []struct {
-		name         string
-		inputVSCName string
-		objs         []runtime.Object
-		expectError  bool
-	}{
-		{
-			name:         "should update DeletionPolicy of a VSC from retain to delete",
-			inputVSCName: "retainVSC",
-			objs: []runtime.Object{
-				&snapshotv1beta1api.VolumeSnapshotContent{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "retainVSC",
-					},
-					Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-						DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-					},
-				},
-			},
-			expectError: false,
-		},
-		{
-			name:         "should be a no-op updating if DeletionPolicy of a VSC is already Delete",
-			inputVSCName: "deleteVSC",
-			objs: []runtime.Object{
-				&snapshotv1beta1api.VolumeSnapshotContent{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "deleteVSC",
-					},
-					Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-						DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentDelete,
-					},
-				},
-			},
-			expectError: false,
-		},
-		{
-			name:         "should update DeletionPolicy of a VSC with no DeletionPolicy",
-			inputVSCName: "nothingVSC",
-			objs: []runtime.Object{
-				&snapshotv1beta1api.VolumeSnapshotContent{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "nothingVSC",
-					},
-					Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{},
-				},
-			},
-			expectError: false,
-		},
-		{
-			name:         "should return not found error if supplied VSC does not exist",
-			inputVSCName: "does-not-exist",
-			objs:         []runtime.Object{},
-			expectError:  true,
-		},
-	}
-
-	log := velerotest.NewLogger().WithFields(
-		logrus.Fields{
-			"unit-test": "unit-test",
-		},
-	)
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeClient := snapshotFake.NewSimpleClientset(tc.objs...)
-			err := setVolumeSnapshotContentDeletionPolicy(tc.inputVSCName, fakeClient.SnapshotV1beta1(), log)
-			if tc.expectError {
-				assert.NotNil(t, err)
-			} else {
-				assert.Nil(t, err)
-				actual, err := fakeClient.SnapshotV1beta1().VolumeSnapshotContents().Get(context.TODO(), tc.inputVSCName, metav1.GetOptions{})
-				assert.Nil(t, err)
-				assert.Equal(t, snapshotv1beta1api.VolumeSnapshotContentDelete, actual.Spec.DeletionPolicy)
-			}
-		})
-	}
-}
-
-func TestDeleteCSIVolumeSnapshots(t *testing.T) {
-	//Backup1
-	ns1VS1VSCName := "ns1vs1vsc"
-	ns1VS1VSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ns1VS1VSCName,
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-		},
-	}
-	ns1VS1 := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "vs1",
-			Namespace: "ns1",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup1",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: &ns1VS1VSCName,
-		},
-	}
-
-	ns1VS2VSCName := "ns1vs2vsc"
-	ns1VS2VSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ns1VS2VSCName,
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-		},
-	}
-	ns1VS2 := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "vs2",
-			Namespace: "ns1",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup1",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: &ns1VS2VSCName,
-		},
-	}
-
-	ns2VS1VSCName := "ns2vs1vsc"
-	ns2VS1VSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ns2VS1VSCName,
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-		},
-	}
-	ns2VS1 := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "vs1",
-			Namespace: "ns2",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup1",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: &ns2VS1VSCName,
-		},
-	}
-
-	ns2VS2VSCName := "ns2vs2vsc"
-	ns2VS2VSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ns2VS2VSCName,
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-		},
-	}
-	ns2VS2 := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "vs2",
-			Namespace: "ns2",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup1",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: &ns2VS2VSCName,
-		},
-	}
-
-	// Backup2
-	ns1NilStatusVS := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "ns1NilStatusVS",
-			Namespace: "ns2",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup2",
-			},
-		},
-		Status: nil,
-	}
-
-	// Backup3
-	ns1NilBoundVSCVS := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "ns1NilBoundVSCVS",
-			Namespace: "ns2",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup3",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: nil,
-		},
-	}
-
-	// Backup4
-	notFound := "not-found"
-	ns1NonExistentVSCVS := snapshotv1beta1api.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "ns1NonExistentVSCVS",
-			Namespace: "ns2",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup3",
-			},
-		},
-		Status: &snapshotv1beta1api.VolumeSnapshotStatus{
-			BoundVolumeSnapshotContentName: &notFound,
-		},
-	}
-
-	testCases := []struct {
-		name       string
-		backupName string
-		objs       []runtime.Object
-	}{
-		{
-			name:       "should delete volumesnapshots bound to existing volumesnapshotcontent",
-			backupName: "backup1",
-			objs:       []runtime.Object{&ns1VS1VSC, &ns1VS1, &ns1VS2VSC, &ns1VS2, &ns2VS1VSC, &ns2VS1, &ns2VS2VSC, &ns2VS2},
-		},
-		{
-			name:       "should delete volumesnapshots with nil status",
-			backupName: "backup2",
-			objs:       []runtime.Object{&ns1NilStatusVS},
-		},
-		{
-			name:       "should delete volumesnapshots with nil BoundVolumeSnapshotContentName",
-			backupName: "backup3",
-			objs:       []runtime.Object{&ns1NilBoundVSCVS},
-		},
-		{
-			name:       "should delete volumesnapshots bound to non-existent volumesnapshotcontents",
-			backupName: "backup4",
-			objs:       []runtime.Object{&ns1NonExistentVSCVS},
-		},
-		{
-			name:       "should be a no-op when there are no volumesnapshots to delete",
-			backupName: "backup-no-vs",
-			objs:       []runtime.Object{},
-		},
-	}
-
-	log := velerotest.NewLogger().WithFields(
-		logrus.Fields{
-			"unit-test": "unit-test",
-		},
-	)
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeClient := snapshotFake.NewSimpleClientset(tc.objs...)
-			fakeSharedInformer := snapshotv1beta1informers.NewSharedInformerFactoryWithOptions(fakeClient, 0)
-			for _, o := range tc.objs {
-				fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshots().Informer().GetStore().Add(o)
-			}
-			errs := deleteCSIVolumeSnapshots(tc.backupName, fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshots().Lister(), fakeClient.SnapshotV1beta1(), log)
-			assert.Empty(t, errs)
-		})
-	}
-}
-
-func TestDeleteCSIVolumeSnapshotContents(t *testing.T) {
-	retainVSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "retainVSC",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup1",
-			},
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain,
-		},
-	}
-	deleteVSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "deleteVSC",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup2",
-			},
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{
-			DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentDelete,
-		},
-	}
-
-	nothingVSC := snapshotv1beta1api.VolumeSnapshotContent{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "nothingVSC",
-			Labels: map[string]string{
-				velerov1api.BackupNameLabel: "backup3",
-			},
-		},
-		Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{},
-	}
-
-	testCases := []struct {
-		name       string
-		backupName string
-		objs       []runtime.Object
-	}{
-		{
-			name:       "should delete volumesnapshotcontent with DeletionPolicy Retain",
-			backupName: "backup1",
-			objs:       []runtime.Object{&retainVSC},
-		},
-		{
-			name:       "should delete volumesnapshotcontent with DeletionPolicy Delete",
-			backupName: "backup3",
-			objs:       []runtime.Object{&deleteVSC},
-		},
-		{
-			name:       "should delete volumesnapshotcontent with No DeletionPolicy",
-			backupName: "backup3",
-			objs:       []runtime.Object{&nothingVSC},
-		},
-		{
-			name:       "should return no error when backup has no volumesnapshotconents",
-			backupName: "backup-with-no-vsc",
-			objs:       []runtime.Object{},
-		},
-	}
-	log := velerotest.NewLogger().WithFields(
-		logrus.Fields{
-			"unit-test": "unit-test",
-		},
-	)
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeClient := snapshotFake.NewSimpleClientset(tc.objs...)
-			fakeSharedInformer := snapshotv1beta1informers.NewSharedInformerFactoryWithOptions(fakeClient, 0)
-			for _, o := range tc.objs {
-				fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshotContents().Informer().GetStore().Add(o)
-			}
-
-			errs := deleteCSIVolumeSnapshotContents(tc.backupName, fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshotContents().Lister(), fakeClient.SnapshotV1beta1(), log)
-			assert.Empty(t, errs)
-		})
-	}
-}