From 4f9e4451422b14a8c337d3ccf7f9402e27948e84 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Thu, 31 Mar 2022 18:09:35 +0800 Subject: [PATCH] Make the vsc created by backup sync controller deletable Fixes #4760 This commit make changes in 2 parts: 1) When a volumesnapshotcontent is persisted during backup, velero will reset its `Source` field to remove the VolumeHandle, so that the csi-snapshotter will not try to call `CreateSnapshot` when its synced to another cluster with a backup. 2) Make sure the referenced volumesnapshotclasses are persisted and synced with the backup, so that when the volumesnapshotcontent is deleted the storage snapshot is also removed. Signed-off-by: Daniel Jiang --- changelogs/unreleased/4832-reasonerjt | 1 + pkg/cmd/server/server.go | 9 +++-- pkg/controller/backup_controller.go | 34 +++++++++++++++++-- pkg/controller/backup_sync_controller.go | 16 +++++++++ pkg/persistence/mocks/backup_store.go | 5 +++ pkg/persistence/object_store.go | 24 +++++++++++++- pkg/persistence/object_store_layout.go | 4 +++ pkg/util/csi/reset.go | 42 ++++++++++++++++++++++++ 8 files changed, 129 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/4832-reasonerjt create mode 100644 pkg/util/csi/reset.go diff --git a/changelogs/unreleased/4832-reasonerjt b/changelogs/unreleased/4832-reasonerjt new file mode 100644 index 000000000..07f9a3499 --- /dev/null +++ b/changelogs/unreleased/4832-reasonerjt @@ -0,0 +1 @@ +Make the vsc created by backup sync controller deletable \ No newline at end of file diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 0a911cae9..fd7e11bce 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -535,10 +535,11 @@ func (s *server) initRestic() error { return nil } -func (s *server) getCSISnapshotListers() (snapshotv1listers.VolumeSnapshotLister, snapshotv1listers.VolumeSnapshotContentLister) { +func (s *server) getCSISnapshotListers() (snapshotv1listers.VolumeSnapshotLister, snapshotv1listers.VolumeSnapshotContentLister, snapshotv1listers.VolumeSnapshotClassLister) { // Make empty listers that will only be populated if CSI is properly enabled. var vsLister snapshotv1listers.VolumeSnapshotLister var vscLister snapshotv1listers.VolumeSnapshotContentLister + var vsClassLister snapshotv1listers.VolumeSnapshotClassLister var err error // If CSI is enabled, check for the CSI groups and generate the listers @@ -556,11 +557,12 @@ func (s *server) getCSISnapshotListers() (snapshotv1listers.VolumeSnapshotLister // Access the wrapped factory directly here since we've already done the feature flag check above to know it's safe. vsLister = s.csiSnapshotterSharedInformerFactory.factory.Snapshot().V1().VolumeSnapshots().Lister() vscLister = s.csiSnapshotterSharedInformerFactory.factory.Snapshot().V1().VolumeSnapshotContents().Lister() + vsClassLister = s.csiSnapshotterSharedInformerFactory.factory.Snapshot().V1().VolumeSnapshotClasses().Lister() case err != nil: cmd.CheckError(err) } } - return vsLister, vscLister + return vsLister, vscLister, vsClassLister } func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string) error { @@ -587,7 +589,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string backupStoreGetter := persistence.NewObjectBackupStoreGetter(s.credentialFileStore) - csiVSLister, csiVSCLister := s.getCSISnapshotListers() + csiVSLister, csiVSCLister, csiVSClassLister := s.getCSISnapshotListers() backupSyncControllerRunInfo := func() controllerRunInfo { backupSyncContoller := controller.NewBackupSyncController( @@ -645,6 +647,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.config.formatFlag.Parse(), csiVSLister, csiVSCLister, + csiVSClassLister, backupStoreGetter, ) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 56c744fe8..76d5375be 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -36,8 +36,11 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/clock" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" + "github.com/vmware-tanzu/velero/pkg/util/csi" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" @@ -85,6 +88,7 @@ type backupController struct { formatFlag logging.Format volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister volumeSnapshotContentLister snapshotv1listers.VolumeSnapshotContentLister + volumeSnapshotClassLister snapshotv1listers.VolumeSnapshotClassLister } func NewBackupController( @@ -106,6 +110,7 @@ func NewBackupController( formatFlag logging.Format, volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, volumeSnapshotContentLister snapshotv1listers.VolumeSnapshotContentLister, + volumesnapshotClassLister snapshotv1listers.VolumeSnapshotClassLister, backupStoreGetter persistence.ObjectBackupStoreGetter, ) Interface { c := &backupController{ @@ -128,6 +133,7 @@ func NewBackupController( formatFlag: formatFlag, volumeSnapshotLister: volumeSnapshotLister, volumeSnapshotContentLister: volumeSnapshotContentLister, + volumeSnapshotClassLister: volumesnapshotClassLister, backupStoreGetter: backupStoreGetter, } @@ -604,9 +610,9 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { // This way, we only make the Lister call if the feature flag's on. var volumeSnapshots []*snapshotv1api.VolumeSnapshot var volumeSnapshotContents []*snapshotv1api.VolumeSnapshotContent + var volumeSnapshotClasses []*snapshotv1api.VolumeSnapshotClass if features.IsEnabled(velerov1api.CSIFeatureFlag) { selector := label.NewSelectorForBackup(backup.Name) - // Listers are wrapped in a nil check out of caution, since they may not be populated based on the // EnableCSI feature flag. This is more to guard against programmer error, as they shouldn't be nil // when EnableCSI is on. @@ -623,6 +629,23 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { backupLog.Error(err) } } + vsClassSet := sets.NewString() + for _, vsc := range volumeSnapshotContents { + // persist the volumesnapshotclasses referenced by vsc + if c.volumeSnapshotClassLister != nil && + vsc.Spec.VolumeSnapshotClassName != nil && + !vsClassSet.Has(*vsc.Spec.VolumeSnapshotClassName) { + if vsClass, err := c.volumeSnapshotClassLister.Get(*vsc.Spec.VolumeSnapshotClassName); err != nil { + backupLog.Error(err) + } else { + vsClassSet.Insert(*vsc.Spec.VolumeSnapshotClassName) + volumeSnapshotClasses = append(volumeSnapshotClasses, vsClass) + } + } + if err := csi.ResetVolumeSnapshotContent(vsc); err != nil { + backupLog.Error(err) + } + } } // Mark completion timestamp before serializing and uploading. @@ -666,7 +689,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { return err } - if errs := persistBackup(backup, backupFile, logFile, backupStore, c.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)), volumeSnapshots, volumeSnapshotContents); len(errs) > 0 { + if errs := persistBackup(backup, backupFile, logFile, backupStore, c.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)), volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses); len(errs) > 0 { fatalErrs = append(fatalErrs, errs...) } @@ -706,6 +729,7 @@ func persistBackup(backup *pkgbackup.Request, log logrus.FieldLogger, csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot, csiVolumeSnapshotContents []*snapshotv1api.VolumeSnapshotContent, + csiVolumesnapshotClasses []*snapshotv1api.VolumeSnapshotClass, ) []error { persistErrs := []error{} backupJSON := new(bytes.Buffer) @@ -734,6 +758,10 @@ func persistBackup(backup *pkgbackup.Request, if errs != nil { persistErrs = append(persistErrs, errs...) } + csiSnapshotClassesJSON, errs := encodeToJSONGzip(csiVolumesnapshotClasses, "csi volume snapshot classes list") + if errs != nil { + persistErrs = append(persistErrs, errs...) + } backupResourceList, errs := encodeToJSONGzip(backup.BackupResourceList(), "backup resources list") if errs != nil { @@ -748,6 +776,7 @@ func persistBackup(backup *pkgbackup.Request, backupResourceList = nil csiSnapshotJSON = nil csiSnapshotContentsJSON = nil + csiSnapshotClassesJSON = nil } backupInfo := persistence.BackupInfo{ @@ -760,6 +789,7 @@ func persistBackup(backup *pkgbackup.Request, BackupResourceList: backupResourceList, CSIVolumeSnapshots: csiSnapshotJSON, CSIVolumeSnapshotContents: csiSnapshotContentsJSON, + CSIVolumeSnapshotClasses: csiSnapshotClassesJSON, } if err := backupStore.PutBackup(backupInfo); err != nil { persistErrs = append(persistErrs, err) diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 9107449cc..6a1f7c562 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -283,6 +283,22 @@ func (c *backupSyncController) run() { 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 = "" + created, err := c.csiSnapshotClient.SnapshotV1().VolumeSnapshotClasses().Create(context.TODO(), vsClass, metav1.CreateOptions{}) + if err != nil { + log.WithError(errors.WithStack(err)).Errorf("Error syncing volumesnapshotclass %s into cluster", vsClass.Name) + continue + } + log.Infof("Created CSI volumesnapshotclass %s", created.Name) + } + log.Info("Syncing CSI volumesnapshotcontents in backup") snapConts, err := backupStore.GetCSIVolumeSnapshotContents(backupName) if err != nil { diff --git a/pkg/persistence/mocks/backup_store.go b/pkg/persistence/mocks/backup_store.go index 9a6eefacf..449e86fa7 100644 --- a/pkg/persistence/mocks/backup_store.go +++ b/pkg/persistence/mocks/backup_store.go @@ -284,6 +284,11 @@ func (_m *BackupStore) GetCSIVolumeSnapshotContents(backup string) ([]*snapshotv return nil, nil } +func (_m *BackupStore) GetCSIVolumeSnapshotClasses(backup string) ([]*snapshotv1api.VolumeSnapshotClass, error) { + panic("Not implemented") + return nil, nil +} + func (_m *BackupStore) GetItemSnapshots(name string) ([]*volume.ItemSnapshot, error) { panic("implement me") } diff --git a/pkg/persistence/object_store.go b/pkg/persistence/object_store.go index b60b9491b..20bf9328e 100644 --- a/pkg/persistence/object_store.go +++ b/pkg/persistence/object_store.go @@ -47,7 +47,8 @@ type BackupInfo struct { ItemSnapshots, BackupResourceList, CSIVolumeSnapshots, - CSIVolumeSnapshotContents io.Reader + CSIVolumeSnapshotContents, + CSIVolumeSnapshotClasses io.Reader } // BackupStore defines operations for creating, retrieving, and deleting @@ -65,6 +66,7 @@ type BackupStore interface { GetBackupContents(name string) (io.ReadCloser, error) GetCSIVolumeSnapshots(name string) ([]*snapshotv1api.VolumeSnapshot, error) GetCSIVolumeSnapshotContents(name string) ([]*snapshotv1api.VolumeSnapshotContent, error) + GetCSIVolumeSnapshotClasses(name string) ([]*snapshotv1api.VolumeSnapshotClass, error) // BackupExists checks if the backup metadata file exists in object storage. BackupExists(bucket, backupName string) (bool, error) @@ -252,6 +254,7 @@ func (s *objectBackupStore) PutBackup(info BackupInfo) error { s.layout.getBackupResourceListKey(info.Name): info.BackupResourceList, s.layout.getCSIVolumeSnapshotKey(info.Name): info.CSIVolumeSnapshots, s.layout.getCSIVolumeSnapshotContentsKey(info.Name): info.CSIVolumeSnapshotContents, + s.layout.getCSIVolumeSnapshotClassesKey(info.Name): info.CSIVolumeSnapshotClasses, } for key, reader := range backupObjs { @@ -371,6 +374,25 @@ func decode(jsongzReader io.Reader, into interface{}) error { return nil } +func (s *objectBackupStore) GetCSIVolumeSnapshotClasses(name string) ([]*snapshotv1api.VolumeSnapshotClass, error) { + res, err := tryGet(s.objectStore, s.bucket, s.layout.getCSIVolumeSnapshotClassesKey(name)) + if err != nil { + return nil, err + } + if res == nil { + // this indicates that the no CSI volumesnapshots were prensent in the backup + return nil, nil + } + defer res.Close() + + var csiVSClasses []*snapshotv1api.VolumeSnapshotClass + if err := decode(res, &csiVSClasses); err != nil { + return nil, err + } + return csiVSClasses, nil + +} + func (s *objectBackupStore) GetCSIVolumeSnapshots(name string) ([]*snapshotv1api.VolumeSnapshot, error) { res, err := tryGet(s.objectStore, s.bucket, s.layout.getCSIVolumeSnapshotKey(name)) if err != nil { diff --git a/pkg/persistence/object_store_layout.go b/pkg/persistence/object_store_layout.go index 046b99a68..cad7479e0 100644 --- a/pkg/persistence/object_store_layout.go +++ b/pkg/persistence/object_store_layout.go @@ -111,3 +111,7 @@ func (l *ObjectStoreLayout) getCSIVolumeSnapshotKey(backup string) string { func (l *ObjectStoreLayout) getCSIVolumeSnapshotContentsKey(backup string) string { return path.Join(l.subdirs["backups"], backup, fmt.Sprintf("%s-csi-volumesnapshotcontents.json.gz", backup)) } + +func (l *ObjectStoreLayout) getCSIVolumeSnapshotClassesKey(backup string) string { + return path.Join(l.subdirs["backups"], backup, fmt.Sprintf("%s-csi-volumesnapshotclasses.json.gz", backup)) +} diff --git a/pkg/util/csi/reset.go b/pkg/util/csi/reset.go new file mode 100644 index 000000000..efe10ad01 --- /dev/null +++ b/pkg/util/csi/reset.go @@ -0,0 +1,42 @@ +/* +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 ( + "fmt" + + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + corev1 "k8s.io/api/core/v1" +) + +// ResetVolumeSnapshotContent make changes to the volumesnapshot content before it's persisted +// It will move the snapshot Handle to the source to avoid the snapshot-controller creating a snapshot when it's +// synced by the backup sync controller. +// It will return an error if the snapshot handle is not set, which should not happen when this func is called. +func ResetVolumeSnapshotContent(snapCont *snapshotv1api.VolumeSnapshotContent) error { + if snapCont.Status != nil && snapCont.Status.SnapshotHandle != nil && len(*snapCont.Status.SnapshotHandle) > 0 { + v := *snapCont.Status.SnapshotHandle + snapCont.Spec.Source = snapshotv1api.VolumeSnapshotContentSource{ + SnapshotHandle: &v, + } + } else { + return fmt.Errorf("the volumesnapshotcontent '%s' does not have snapshothandle set", snapCont.Name) + } + + snapCont.Spec.VolumeSnapshotRef = corev1.ObjectReference{} + return nil +}