Remove VolumeSnapshotClass from CSI restore and deletion process.

Remove VolumeSnapshotClass from backup sync process.

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

changelogs/unreleased/9431-blackpiglet

@@ -0,0 +1 @@
+Remove VolumeSnapshotClass from CSI B/R process.

internal/delete/actions/csi/volumesnapshotcontent_action.go

@@ -103,6 +103,14 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 
 	snapCont.ResourceVersion = ""
 
+	if snapCont.Spec.VolumeSnapshotClassName != nil {
+		// Delete VolumeSnapshotClass from the VolumeSnapshotContent.
+		// This is necessary to make the deletion independent of the VolumeSnapshotClass.
+		snapCont.Spec.VolumeSnapshotClassName = nil
+		p.log.Debugf("Deleted VolumeSnapshotClassName from VolumeSnapshotContent %s to make deletion independent of VolumeSnapshotClass",
+			snapCont.Name)
+	}
+
 	if err := p.crClient.Create(context.TODO(), &snapCont); err != nil {
 		return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", snapCont.Name)
 	}

internal/delete/actions/csi/volumesnapshotcontent_action_test.go

@@ -70,7 +70,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"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
+			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(),
 			expectErr: false,
 			function: func(
@@ -82,7 +82,7 @@ func TestVSCExecute(t *testing.T) {
 			},
 		},
 		{
-			name:      "Normal case, VolumeSnapshot should be deleted",
+			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,

pkg/backup/actions/csi/volumesnapshot_action.go

@@ -84,17 +84,6 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		return nil, nil, "", nil, errors.WithStack(err)
 	}
 
-	additionalItems := make([]velero.ResourceIdentifier, 0)
-	if vs.Spec.VolumeSnapshotClassName != nil {
-		additionalItems = append(
-			additionalItems,
-			velero.ResourceIdentifier{
-				GroupResource: kuberesource.VolumeSnapshotClasses,
-				Name:          *vs.Spec.VolumeSnapshotClassName,
-			},
-		)
-	}
-
 	if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
 		backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
 		p.log.
@@ -105,6 +94,24 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		return item, nil, "", nil, nil
 	}
 
+	additionalItems := make([]velero.ResourceIdentifier, 0)
+
+	if vs.Spec.VolumeSnapshotClassName != nil {
+		// This is still needed to add the VolumeSnapshotClass to the backup.
+		// The secret with VolumeSnapshotClass is still relevant to backup.
+		additionalItems = append(
+			additionalItems,
+			velero.ResourceIdentifier{
+				GroupResource: kuberesource.VolumeSnapshotClasses,
+				Name:          *vs.Spec.VolumeSnapshotClassName,
+			},
+		)
+
+		// Because async operation will update VolumeSnapshot during finalizing phase.
+		// No matter what we do, VolumeSnapshotClass cannot be deleted. So skip it.
+		// Just deleting VolumeSnapshotClass during restore and delete is enough.
+	}
+
 	p.log.Infof("Getting VolumesnapshotContent for Volumesnapshot %s/%s",
 		vs.Namespace, vs.Name)
 

pkg/backup/actions/csi/volumesnapshotcontent_action.go

@@ -97,6 +97,10 @@ func (p *volumeSnapshotContentBackupItemAction) Execute(
 		})
 	}
 
+	// Because async operation will update VolumeSnapshotContent during finalizing phase.
+	// No matter what we do, VolumeSnapshotClass cannot be deleted. So skip it.
+	// Just deleting VolumeSnapshotClass during restore and delete is enough.
+
 	snapContMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&snapCont)
 	if err != nil {
 		return nil, nil, "", nil, errors.WithStack(err)

pkg/backup/actions/csi/volumesnapshotcontent_action_test.go

@@ -42,7 +42,7 @@ func TestVSCExecute(t *testing.T) {
 		expectedItems []velero.ResourceIdentifier
 	}{
 		{
-			name: "Invalid VolumeSnapshotClass",
+			name: "Invalid VolumeSnapshotContent",
 			item: velerotest.UnstructuredOrDie(
 				`
 				{

pkg/controller/backup_sync_controller.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"time"
 
-	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -36,7 +35,6 @@ import (
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/constant"
-	"github.com/vmware-tanzu/velero/pkg/features"
 	"github.com/vmware-tanzu/velero/pkg/label"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
@@ -243,31 +241,6 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				log.Debug("Synced pod volume backup into cluster")
 			}
 		}
-
-		if features.IsEnabled(velerov1api.CSIFeatureFlag) {
-			// we are syncing these objects only to ensure that the storage snapshots are cleaned up
-			// on backup deletion or expiry.
-			log.Info("Syncing CSI VolumeSnapshotClasses in backup")
-			vsClasses, err := backupStore.GetCSIVolumeSnapshotClasses(backupName)
-			if err != nil {
-				log.WithError(errors.WithStack(err)).Error("Error getting CSI VolumeSnapClasses for this backup from backup store")
-				continue
-			}
-			for _, vsClass := range vsClasses {
-				vsClass.ResourceVersion = ""
-				err := b.client.Create(ctx, vsClass, &client.CreateOptions{})
-				switch {
-				case err != nil && apierrors.IsAlreadyExists(err):
-					log.Debugf("VolumeSnapshotClass %s already exists in cluster", vsClass.Name)
-					continue
-				case err != nil && !apierrors.IsAlreadyExists(err):
-					log.WithError(errors.WithStack(err)).Errorf("Error syncing VolumeSnapshotClass %s into cluster", vsClass.Name)
-					continue
-				default:
-					log.Infof("Created CSI VolumeSnapshotClass %s", vsClass.Name)
-				}
-			}
-		}
 	}
 
 	b.deleteOrphanedBackups(ctx, location.Name, backupStoreBackups, log)
@@ -364,40 +337,10 @@ func (b *backupSyncReconciler) deleteOrphanedBackups(ctx context.Context, locati
 
 		if err := b.client.Delete(ctx, &backupList.Items[i], &client.DeleteOptions{}); err != nil {
 			log.WithError(errors.WithStack(err)).Error("Error deleting orphaned backup from cluster")
-		} else {
-			log.Debug("Deleted orphaned backup from cluster")
-			b.deleteCSISnapshotsByBackup(ctx, backup.Name, log)
 		}
 	}
 }
 
-func (b *backupSyncReconciler) deleteCSISnapshotsByBackup(ctx context.Context, backupName string, log logrus.FieldLogger) {
-	if !features.IsEnabled(velerov1api.CSIFeatureFlag) {
-		return
-	}
-	m := client.MatchingLabels{velerov1api.BackupNameLabel: label.GetValidName(backupName)}
-	var vsList snapshotv1api.VolumeSnapshotList
-	listOptions := &client.ListOptions{
-		LabelSelector: label.NewSelectorForBackup(label.GetValidName(backupName)),
-	}
-	if err := b.client.List(ctx, &vsList, listOptions); err != nil {
-		log.WithError(err).Warnf("Failed to list volumesnapshots for backup: %s, the deletion will be skipped", backupName)
-	} else {
-		for i, vs := range vsList.Items {
-			name := kube.NamespaceAndName(vs.GetObjectMeta())
-			log.Debugf("Deleting volumesnapshot %s", name)
-			if err := b.client.Delete(context.TODO(), &vsList.Items[i]); err != nil {
-				log.WithError(err).Warnf("Failed to delete volumesnapshot %s", name)
-			}
-		}
-	}
-	vsc := &snapshotv1api.VolumeSnapshotContent{}
-	log.Debugf("Deleting volumesnapshotcontents for backup: %s", backupName)
-	if err := b.client.DeleteAllOf(context.TODO(), vsc, m); err != nil {
-		log.WithError(err).Warnf("Failed to delete volumesnapshotcontents for backup: %s", backupName)
-	}
-}
-
 // backupSyncSourceOrderFunc returns a new slice with the default backup location first (if it exists),
 // followed by the rest of the locations in no particular order.
 func backupSyncSourceOrderFunc(objList client.ObjectList) client.ObjectList {

pkg/controller/backup_sync_controller_test.go

@@ -24,7 +24,6 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
-	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -451,8 +450,6 @@ var _ = Describe("Backup Sync Reconciler", func() {
 					backupStore.On("GetBackupMetadata", backup.backup.Name).Return(backup.backup, nil)
 					backupStore.On("GetPodVolumeBackups", backup.backup.Name).Return(backup.podVolumeBackups, nil)
 					backupStore.On("BackupExists", "bucket-1", backup.backup.Name).Return(true, nil)
-					backupStore.On("GetCSIVolumeSnapshotClasses", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotClass{}, nil)
-					backupStore.On("GetCSIVolumeSnapshotContents", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotContent{}, nil)
 				}
 				backupStore.On("ListBackups").Return(backupNames, nil)
 			}

pkg/persistence/object_store.go

@@ -266,13 +266,12 @@ func (s *objectBackupStore) PutBackup(info BackupInfo) error {
 	// Since the logic for all of these files is the exact same except for the name and the contents,
 	// use a map literal to iterate through them and write them to the bucket.
 	var backupObjs = map[string]io.Reader{
-		s.layout.getPodVolumeBackupsKey(info.Name):         info.PodVolumeBackups,
-		s.layout.getBackupVolumeSnapshotsKey(info.Name):    info.VolumeSnapshots,
-		s.layout.getBackupItemOperationsKey(info.Name):     info.BackupItemOperations,
-		s.layout.getBackupResourceListKey(info.Name):       info.BackupResourceList,
-		s.layout.getCSIVolumeSnapshotClassesKey(info.Name): info.CSIVolumeSnapshotClasses,
-		s.layout.getBackupResultsKey(info.Name):            info.BackupResults,
-		s.layout.getBackupVolumeInfoKey(info.Name):         info.BackupVolumeInfo,
+		s.layout.getPodVolumeBackupsKey(info.Name):      info.PodVolumeBackups,
+		s.layout.getBackupVolumeSnapshotsKey(info.Name): info.VolumeSnapshots,
+		s.layout.getBackupItemOperationsKey(info.Name):  info.BackupItemOperations,
+		s.layout.getBackupResourceListKey(info.Name):    info.BackupResourceList,
+		s.layout.getBackupResultsKey(info.Name):         info.BackupResults,
+		s.layout.getBackupVolumeInfoKey(info.Name):      info.BackupVolumeInfo,
 	}
 
 	for key, reader := range backupObjs {

pkg/restore/actions/csi/volumesnapshot_action.go

@@ -103,6 +103,14 @@ func (p *volumeSnapshotRestoreItemAction) Execute(
 	// DeletionPolicy to Retain.
 	resetVolumeSnapshotAnnotation(&vs)
 
+	if vs.Spec.VolumeSnapshotClassName != nil {
+		// Delete VolumeSnapshotClass from the VolumeSnapshot.
+		// This is necessary to make the restore independent of the VolumeSnapshotClass.
+		vs.Spec.VolumeSnapshotClassName = nil
+		p.log.Debugf("Deleted VolumeSnapshotClassName from VolumeSnapshot %s/%s to make restore independent of VolumeSnapshotClass",
+			vs.Namespace, vs.Name)
+	}
+
 	vsMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&vs)
 	if err != nil {
 		p.log.Errorf("Fail to convert VS %s to unstructured", vs.Namespace+"/"+vs.Name)

pkg/restore/actions/csi/volumesnapshot_action_test.go

@@ -124,14 +124,20 @@ func TestVSExecute(t *testing.T) {
 		},
 		{
 			name: "Normal case, VSC should be created",
-			vs: builder.ForVolumeSnapshot("ns", "vsName").ObjectMeta(
-				builder.WithAnnotationsMap(
-					map[string]string{
-						velerov1api.VolumeSnapshotHandleAnnotation: "vsc",
-						velerov1api.DriverNameAnnotation:           "pd.csi.storage.gke.io",
-					},
-				),
-			).SourceVolumeSnapshotContentName(newVscName).Status().BoundVolumeSnapshotContentName("vscName").Result(),
+			vs: builder.ForVolumeSnapshot("ns", "vsName").
+				ObjectMeta(
+					builder.WithAnnotationsMap(
+						map[string]string{
+							velerov1api.VolumeSnapshotHandleAnnotation: "vsc",
+							velerov1api.DriverNameAnnotation:           "pd.csi.storage.gke.io",
+						},
+					),
+				).
+				SourceVolumeSnapshotContentName(newVscName).
+				VolumeSnapshotClass("vscClass").
+				Status().
+				BoundVolumeSnapshotContentName("vscName").
+				Result(),
 			restore:    builder.ForRestore("velero", "restore").ObjectMeta(builder.WithUID("restoreUID")).Result(),
 			expectErr:  false,
 			expectedVS: builder.ForVolumeSnapshot("ns", "test").SourceVolumeSnapshotContentName(newVscName).Result(),

pkg/restore/actions/csi/volumesnapshotcontent_action.go

@@ -108,6 +108,14 @@ func (p *volumeSnapshotContentRestoreItemAction) Execute(
 		return nil, errors.Errorf("fail to get snapshot handle from VSC %s status", vsc.Name)
 	}
 
+	if vsc.Spec.VolumeSnapshotClassName != nil {
+		// Delete VolumeSnapshotClass from the VolumeSnapshotContent.
+		// This is necessary to make the restore independent of the VolumeSnapshotClass.
+		vsc.Spec.VolumeSnapshotClassName = nil
+		p.log.Debugf("Deleted VolumeSnapshotClassName from VolumeSnapshotContent %s to make restore independent of VolumeSnapshotClass",
+			vsc.Name)
+	}
+
 	additionalItems := []velero.ResourceIdentifier{}
 	if csi.IsVolumeSnapshotContentHasDeleteSecret(&vsc) {
 		additionalItems = append(additionalItems,

pkg/restore/actions/csi/volumesnapshotcontent_action_test.go

@@ -55,13 +55,17 @@ func TestVSCExecute(t *testing.T) {
 		},
 		{
 			name: "Normal case, additional items should return    ",
-			vsc: builder.ForVolumeSnapshotContent("test").ObjectMeta(builder.WithAnnotationsMap(
-				map[string]string{
-					velerov1api.PrefixedSecretNameAnnotation:      "name",
-					velerov1api.PrefixedSecretNamespaceAnnotation: "namespace",
-				},
-			)).VolumeSnapshotRef("velero", "vsName", "vsUID").
-				Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleName}).Result(),
+			vsc: builder.ForVolumeSnapshotContent("test").
+				ObjectMeta(builder.WithAnnotationsMap(
+					map[string]string{
+						velerov1api.PrefixedSecretNameAnnotation:      "name",
+						velerov1api.PrefixedSecretNamespaceAnnotation: "namespace",
+					},
+				)).
+				VolumeSnapshotRef("velero", "vsName", "vsUID").
+				VolumeSnapshotClassName("vsClass").
+				Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleName}).
+				Result(),
 			restore: builder.ForRestore("velero", "restore").ObjectMeta(builder.WithUID("restoreUID")).
 				NamespaceMappings("velero", "restore").Result(),
 			expectErr: false,
@@ -72,15 +76,17 @@ func TestVSCExecute(t *testing.T) {
 					Name:          "name",
 				},
 			},
-			expectedVSC: builder.ForVolumeSnapshotContent(newVscName).ObjectMeta(builder.WithAnnotationsMap(
-				map[string]string{
-					velerov1api.PrefixedSecretNameAnnotation:      "name",
-					velerov1api.PrefixedSecretNamespaceAnnotation: "namespace",
-				},
-			)).VolumeSnapshotRef("restore", newVscName, "").
+			expectedVSC: builder.ForVolumeSnapshotContent(newVscName).
+				ObjectMeta(builder.WithAnnotationsMap(
+					map[string]string{
+						velerov1api.PrefixedSecretNameAnnotation:      "name",
+						velerov1api.PrefixedSecretNamespaceAnnotation: "namespace",
+					},
+				)).VolumeSnapshotRef("restore", newVscName, "").
 				Source(snapshotv1api.VolumeSnapshotContentSource{SnapshotHandle: &snapshotHandleName}).
 				DeletionPolicy(snapshotv1api.VolumeSnapshotContentRetain).
-				Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleName}).Result(),
+				Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleName}).
+				Result(),
 		},
 		{
 			name: "VSC exists in cluster, same as the normal case",