From 096436507e6d8fdd31a7c937fc57e50db238899c Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Wed, 26 Nov 2025 14:08:01 +0800 Subject: [PATCH] Remove VolumeSnapshotClass from CSI restore and deletion process. Remove VolumeSnapshotClass from backup sync process. Signed-off-by: Xun Jiang --- changelogs/unreleased/9431-blackpiglet | 1 + .../csi/volumesnapshotcontent_action.go | 8 +++ .../csi/volumesnapshotcontent_action_test.go | 4 +- .../actions/csi/volumesnapshot_action.go | 29 ++++++---- .../csi/volumesnapshotcontent_action.go | 4 ++ .../csi/volumesnapshotcontent_action_test.go | 2 +- pkg/controller/backup_sync_controller.go | 57 ------------------- pkg/controller/backup_sync_controller_test.go | 3 - pkg/persistence/object_store.go | 13 ++--- .../actions/csi/volumesnapshot_action.go | 8 +++ .../actions/csi/volumesnapshot_action_test.go | 22 ++++--- .../csi/volumesnapshotcontent_action.go | 8 +++ .../csi/volumesnapshotcontent_action_test.go | 34 ++++++----- 13 files changed, 90 insertions(+), 103 deletions(-) create mode 100644 changelogs/unreleased/9431-blackpiglet diff --git a/changelogs/unreleased/9431-blackpiglet b/changelogs/unreleased/9431-blackpiglet new file mode 100644 index 000000000..f82eb459c --- /dev/null +++ b/changelogs/unreleased/9431-blackpiglet @@ -0,0 +1 @@ +Remove VolumeSnapshotClass from CSI B/R process. diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index 582041533..d12c7c43a 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/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) } diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index 1c84cf7bf..24baccdb2 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/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, diff --git a/pkg/backup/actions/csi/volumesnapshot_action.go b/pkg/backup/actions/csi/volumesnapshot_action.go index 69f881181..b7283b628 100644 --- a/pkg/backup/actions/csi/volumesnapshot_action.go +++ b/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) diff --git a/pkg/backup/actions/csi/volumesnapshotcontent_action.go b/pkg/backup/actions/csi/volumesnapshotcontent_action.go index c2ab334e0..d4cd6d46c 100644 --- a/pkg/backup/actions/csi/volumesnapshotcontent_action.go +++ b/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) diff --git a/pkg/backup/actions/csi/volumesnapshotcontent_action_test.go b/pkg/backup/actions/csi/volumesnapshotcontent_action_test.go index 120eebdb4..e24623d9a 100644 --- a/pkg/backup/actions/csi/volumesnapshotcontent_action_test.go +++ b/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( ` { diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 69a6b2ac1..b84ae6f0b 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/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 { diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index 0aa7f2818..fe440ff09 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/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) } diff --git a/pkg/persistence/object_store.go b/pkg/persistence/object_store.go index 44ad96118..edcd2fa49 100644 --- a/pkg/persistence/object_store.go +++ b/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 { diff --git a/pkg/restore/actions/csi/volumesnapshot_action.go b/pkg/restore/actions/csi/volumesnapshot_action.go index 50fb3f0ed..6d4bc1eda 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action.go +++ b/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) diff --git a/pkg/restore/actions/csi/volumesnapshot_action_test.go b/pkg/restore/actions/csi/volumesnapshot_action_test.go index bec4f9582..4fb37e301 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action_test.go +++ b/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(), diff --git a/pkg/restore/actions/csi/volumesnapshotcontent_action.go b/pkg/restore/actions/csi/volumesnapshotcontent_action.go index 9b7d5c878..0a268eab2 100644 --- a/pkg/restore/actions/csi/volumesnapshotcontent_action.go +++ b/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, diff --git a/pkg/restore/actions/csi/volumesnapshotcontent_action_test.go b/pkg/restore/actions/csi/volumesnapshotcontent_action_test.go index 85fd1a092..83b79bd5b 100644 --- a/pkg/restore/actions/csi/volumesnapshotcontent_action_test.go +++ b/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",