From 4a03370f1d49b5ea897301d3f2853c8f74d83ed8 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Tue, 16 Oct 2018 15:49:37 -0600 Subject: [PATCH] update backup deletion controller for snapshot locations Signed-off-by: Steve Kriss --- pkg/cmd/server/server.go | 2 +- pkg/controller/backup_deletion_controller.go | 81 +++++++++++++------ .../backup_deletion_controller_test.go | 76 ++++++++--------- 3 files changed, 89 insertions(+), 70 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 46b3dd43d..0ff7b591e 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -697,13 +697,13 @@ func (s *server) runControllers(config *api.Config, defaultVolumeSnapshotLocatio s.sharedInformerFactory.Ark().V1().DeleteBackupRequests(), s.arkClient.ArkV1(), // deleteBackupRequestClient s.arkClient.ArkV1(), // backupClient - nil, s.sharedInformerFactory.Ark().V1().Restores(), s.arkClient.ArkV1(), // restoreClient backupTracker, s.resticManager, s.sharedInformerFactory.Ark().V1().PodVolumeBackups(), s.sharedInformerFactory.Ark().V1().BackupStorageLocations(), + s.sharedInformerFactory.Ark().V1().VolumeSnapshotLocations(), newPluginManager, ) wg.Add(1) diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 9a6813161..a1b1e2573 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -51,13 +51,13 @@ type backupDeletionController struct { deleteBackupRequestClient arkv1client.DeleteBackupRequestsGetter deleteBackupRequestLister listers.DeleteBackupRequestLister backupClient arkv1client.BackupsGetter - blockStore cloudprovider.BlockStore restoreLister listers.RestoreLister restoreClient arkv1client.RestoresGetter backupTracker BackupTracker resticMgr restic.RepositoryManager podvolumeBackupLister listers.PodVolumeBackupLister backupLocationLister listers.BackupStorageLocationLister + snapshotLocationLister listers.VolumeSnapshotLocationLister processRequestFunc func(*v1.DeleteBackupRequest) error clock clock.Clock newPluginManager func(logrus.FieldLogger) plugin.Manager @@ -70,13 +70,13 @@ func NewBackupDeletionController( deleteBackupRequestInformer informers.DeleteBackupRequestInformer, deleteBackupRequestClient arkv1client.DeleteBackupRequestsGetter, backupClient arkv1client.BackupsGetter, - blockStore cloudprovider.BlockStore, restoreInformer informers.RestoreInformer, restoreClient arkv1client.RestoresGetter, backupTracker BackupTracker, resticMgr restic.RepositoryManager, podvolumeBackupInformer informers.PodVolumeBackupInformer, backupLocationInformer informers.BackupStorageLocationInformer, + snapshotLocationInformer informers.VolumeSnapshotLocationInformer, newPluginManager func(logrus.FieldLogger) plugin.Manager, ) Interface { c := &backupDeletionController{ @@ -84,13 +84,13 @@ func NewBackupDeletionController( deleteBackupRequestClient: deleteBackupRequestClient, deleteBackupRequestLister: deleteBackupRequestInformer.Lister(), backupClient: backupClient, - blockStore: blockStore, restoreLister: restoreInformer.Lister(), restoreClient: restoreClient, backupTracker: backupTracker, resticMgr: resticMgr, podvolumeBackupLister: podvolumeBackupInformer.Lister(), backupLocationLister: backupLocationInformer.Lister(), + snapshotLocationLister: snapshotLocationInformer.Lister(), // use variables to refer to these functions so they can be // replaced with fakes for testing. @@ -107,6 +107,7 @@ func NewBackupDeletionController( restoreInformer.Informer().HasSynced, podvolumeBackupInformer.Informer().HasSynced, backupLocationInformer.Informer().HasSynced, + snapshotLocationInformer.Informer().HasSynced, ) c.processRequestFunc = c.processRequest @@ -223,17 +224,6 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e } } - // If the backup includes snapshots but we don't currently have a PVProvider, we don't - // want to orphan the snapshots so skip deletion. - if c.blockStore == nil && len(backup.Status.VolumeBackups) > 0 { - req, err = c.patchDeleteBackupRequest(req, func(r *v1.DeleteBackupRequest) { - r.Status.Phase = v1.DeleteBackupRequestPhaseProcessed - r.Status.Errors = []string{"unable to delete backup because it includes PV snapshots and Ark is not configured with a PersistentVolumeProvider"} - }) - - return err - } - // Set backup status to Deleting backup, err = c.patchBackup(backup, func(b *v1.Backup) { b.Status.Phase = v1.BackupPhaseDeleting @@ -245,11 +235,36 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e var errs []string + pluginManager := c.newPluginManager(log) + defer pluginManager.CleanupClients() + + backupStore, backupStoreErr := c.backupStoreForBackup(backup, pluginManager, log) + if backupStoreErr != nil { + errs = append(errs, backupStoreErr.Error()) + // TODO need to not proceed since backupStore will be nil + } + log.Info("Removing PV snapshots") - for _, volumeBackup := range backup.Status.VolumeBackups { - log.WithField("snapshotID", volumeBackup.SnapshotID).Info("Removing snapshot associated with backup") - if err := c.blockStore.DeleteSnapshot(volumeBackup.SnapshotID); err != nil { - errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", volumeBackup.SnapshotID).Error()) + if snapshots, err := backupStore.GetBackupVolumeSnapshots(backup.Name); err != nil { + errs = append(errs, errors.Wrap(err, "error getting backup's volume snapshots").Error()) + } else { + blockStores := make(map[string]cloudprovider.BlockStore) + + for _, snapshot := range snapshots { + log.WithField("providerSnapshotID", snapshot.Status.ProviderSnapshotID).Info("Removing snapshot associated with backup") + + blockStore, ok := blockStores[snapshot.Spec.Location] + if !ok { + if blockStore, err = blockStoreForSnapshotLocation(backup.Namespace, snapshot.Spec.Location, c.snapshotLocationLister, pluginManager); err != nil { + errs = append(errs, err.Error()) + continue + } + blockStores[snapshot.Spec.Location] = blockStore + } + + if err := blockStore.DeleteSnapshot(snapshot.Status.ProviderSnapshotID); err != nil { + errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", snapshot.Status.ProviderSnapshotID).Error()) + } } } @@ -261,14 +276,6 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e } log.Info("Removing backup from backup storage") - pluginManager := c.newPluginManager(log) - defer pluginManager.CleanupClients() - - backupStore, backupStoreErr := c.backupStoreForBackup(backup, pluginManager, log) - if backupStoreErr != nil { - errs = append(errs, backupStoreErr.Error()) - } - if err := backupStore.DeleteBackup(backup.Name); err != nil { errs = append(errs, err.Error()) } @@ -328,6 +335,28 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e return nil } +func blockStoreForSnapshotLocation( + namespace, snapshotLocationName string, + snapshotLocationLister listers.VolumeSnapshotLocationLister, + pluginManager plugin.Manager, +) (cloudprovider.BlockStore, error) { + snapshotLocation, err := snapshotLocationLister.VolumeSnapshotLocations(namespace).Get(snapshotLocationName) + if err != nil { + return nil, errors.Wrapf(err, "error getting volume snapshot location %s", snapshotLocationName) + } + + blockStore, err := pluginManager.GetBlockStore(snapshotLocation.Spec.Provider) + if err != nil { + return nil, errors.Wrapf(err, "error getting block store for provider %s", snapshotLocation.Spec.Provider) + } + + if err = blockStore.Init(snapshotLocation.Spec.Config); err != nil { + return nil, errors.Wrapf(err, "error initializing block store for volume snapshot location %s", snapshotLocationName) + } + + return blockStore, nil +} + func (c *backupDeletionController) backupStoreForBackup(backup *v1.Backup, pluginManager plugin.Manager, log logrus.FieldLogger) (persistence.BackupStore, error) { backupLocation, err := c.backupLocationLister.BackupStorageLocations(backup.Namespace).Get(backup.Spec.StorageLocation) if err != nil { diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 28301056f..425e735b4 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -42,6 +42,7 @@ import ( "github.com/heptio/ark/pkg/plugin" pluginmocks "github.com/heptio/ark/pkg/plugin/mocks" arktest "github.com/heptio/ark/pkg/util/test" + "github.com/heptio/ark/pkg/volume" ) func TestBackupDeletionControllerProcessQueueItem(t *testing.T) { @@ -53,13 +54,13 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) { sharedInformers.Ark().V1().DeleteBackupRequests(), client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient - nil, // blockStore sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, // restic repository manager sharedInformers.Ark().V1().PodVolumeBackups(), sharedInformers.Ark().V1().BackupStorageLocations(), + sharedInformers.Ark().V1().VolumeSnapshotLocations(), nil, // new plugin manager func ).(*backupDeletionController) @@ -139,13 +140,13 @@ func setupBackupDeletionControllerTest(objects ...runtime.Object) *backupDeletio sharedInformers.Ark().V1().DeleteBackupRequests(), client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient - blockStore, sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, // restic repository manager sharedInformers.Ark().V1().PodVolumeBackups(), sharedInformers.Ark().V1().BackupStorageLocations(), + sharedInformers.Ark().V1().VolumeSnapshotLocations(), func(logrus.FieldLogger) plugin.Manager { return pluginManager }, ).(*backupDeletionController), @@ -323,47 +324,8 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { assert.Equal(t, expectedActions, td.client.Actions()) }) - t.Run("no block store, backup has snapshots", func(t *testing.T) { - td := setupBackupDeletionControllerTest() - td.controller.blockStore = nil - - td.client.PrependReactor("get", "backups", func(action core.Action) (bool, runtime.Object, error) { - backup := arktest.NewTestBackup().WithName("backup-1").WithSnapshot("pv-1", "snap-1").Backup - return true, backup, nil - }) - - td.client.PrependReactor("patch", "deletebackuprequests", func(action core.Action) (bool, runtime.Object, error) { - return true, td.req, nil - }) - - err := td.controller.processRequest(td.req) - require.NoError(t, err) - - expectedActions := []core.Action{ - core.NewPatchAction( - v1.SchemeGroupVersion.WithResource("deletebackuprequests"), - td.req.Namespace, - td.req.Name, - []byte(`{"status":{"phase":"InProgress"}}`), - ), - core.NewGetAction( - v1.SchemeGroupVersion.WithResource("backups"), - td.req.Namespace, - td.req.Spec.BackupName, - ), - core.NewPatchAction( - v1.SchemeGroupVersion.WithResource("deletebackuprequests"), - td.req.Namespace, - td.req.Name, - []byte(`{"status":{"errors":["unable to delete backup because it includes PV snapshots and Ark is not configured with a PersistentVolumeProvider"],"phase":"Processed"}}`), - ), - } - - assert.Equal(t, expectedActions, td.client.Actions()) - }) - t.Run("full delete, no errors", func(t *testing.T) { - backup := arktest.NewTestBackup().WithName("foo").WithSnapshot("pv-1", "snap-1").Backup + backup := arktest.NewTestBackup().WithName("foo").Backup backup.UID = "uid" backup.Spec.StorageLocation = "primary" @@ -393,6 +355,17 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { } require.NoError(t, td.sharedInformers.Ark().V1().BackupStorageLocations().Informer().GetStore().Add(location)) + snapshotLocation := &v1.VolumeSnapshotLocation{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: "vsl-1", + }, + Spec: v1.VolumeSnapshotLocationSpec{ + Provider: "provider-1", + }, + } + require.NoError(t, td.sharedInformers.Ark().V1().VolumeSnapshotLocations().Informer().GetStore().Add(snapshotLocation)) + // Clear out req labels to make sure the controller adds them td.req.Labels = make(map[string]string) @@ -409,6 +382,23 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { return true, backup, nil }) + snapshots := []*volume.Snapshot{ + { + Spec: volume.SnapshotSpec{ + Location: "vsl-1", + }, + Status: volume.SnapshotStatus{ + ProviderSnapshotID: "snap-1", + }, + }, + } + + pluginManager := &pluginmocks.Manager{} + pluginManager.On("GetBlockStore", "provider-1").Return(td.blockStore, nil) + pluginManager.On("CleanupClients") + td.controller.newPluginManager = func(logrus.FieldLogger) plugin.Manager { return pluginManager } + + td.backupStore.On("GetBackupVolumeSnapshots", td.req.Spec.BackupName).Return(snapshots, nil) td.backupStore.On("DeleteBackup", td.req.Spec.BackupName).Return(nil) td.backupStore.On("DeleteRestore", "restore-1").Return(nil) td.backupStore.On("DeleteRestore", "restore-2").Return(nil) @@ -593,13 +583,13 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) { sharedInformers.Ark().V1().DeleteBackupRequests(), client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient - nil, // blockStore sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, sharedInformers.Ark().V1().PodVolumeBackups(), sharedInformers.Ark().V1().BackupStorageLocations(), + sharedInformers.Ark().V1().VolumeSnapshotLocations(), nil, // new plugin manager func ).(*backupDeletionController)