From bab08ed1a61905b12c69e5c20e599f902ee8c7e1 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Wed, 15 Aug 2018 16:27:27 -0700 Subject: [PATCH] backup deletion controller: use backup location for object store Signed-off-by: Steve Kriss --- pkg/cmd/server/server.go | 4 +- pkg/controller/backup_deletion_controller.go | 61 +++++++++++++------ .../backup_deletion_controller_test.go | 61 +++++++++++++++---- 3 files changed, 93 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index d95aed7da..eed216aa4 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -682,13 +682,13 @@ func (s *server) runControllers(config *api.Config) error { s.arkClient.ArkV1(), // deleteBackupRequestClient s.arkClient.ArkV1(), // backupClient s.blockStore, - s.objectStore, - config.BackupStorageProvider.Bucket, s.sharedInformerFactory.Ark().V1().Restores(), s.arkClient.ArkV1(), // restoreClient backupTracker, s.resticManager, s.sharedInformerFactory.Ark().V1().PodVolumeBackups(), + s.sharedInformerFactory.Ark().V1().BackupStorageLocations(), + s.pluginRegistry, ) wg.Add(1) go func() { diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 209f9fec3..3e0e6a9fb 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -28,6 +28,7 @@ import ( arkv1client "github.com/heptio/ark/pkg/generated/clientset/versioned/typed/ark/v1" informers "github.com/heptio/ark/pkg/generated/informers/externalversions/ark/v1" listers "github.com/heptio/ark/pkg/generated/listers/ark/v1" + "github.com/heptio/ark/pkg/plugin" "github.com/heptio/ark/pkg/restic" "github.com/heptio/ark/pkg/util/kube" "github.com/pkg/errors" @@ -50,17 +51,17 @@ type backupDeletionController struct { deleteBackupRequestLister listers.DeleteBackupRequestLister backupClient arkv1client.BackupsGetter blockStore cloudprovider.BlockStore - objectStore cloudprovider.ObjectStore - bucket string restoreLister listers.RestoreLister restoreClient arkv1client.RestoresGetter backupTracker BackupTracker resticMgr restic.RepositoryManager podvolumeBackupLister listers.PodVolumeBackupLister - - deleteBackupDir cloudprovider.DeleteBackupDirFunc - processRequestFunc func(*v1.DeleteBackupRequest) error - clock clock.Clock + backupLocationLister listers.BackupStorageLocationLister + pluginRegistry plugin.Registry + deleteBackupDir cloudprovider.DeleteBackupDirFunc + processRequestFunc func(*v1.DeleteBackupRequest) error + clock clock.Clock + newPluginManager func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager } // NewBackupDeletionController creates a new backup deletion controller. @@ -70,13 +71,13 @@ func NewBackupDeletionController( deleteBackupRequestClient arkv1client.DeleteBackupRequestsGetter, backupClient arkv1client.BackupsGetter, blockStore cloudprovider.BlockStore, - objectStore cloudprovider.ObjectStore, - bucket string, restoreInformer informers.RestoreInformer, restoreClient arkv1client.RestoresGetter, backupTracker BackupTracker, resticMgr restic.RepositoryManager, podvolumeBackupInformer informers.PodVolumeBackupInformer, + backupLocationInformer informers.BackupStorageLocationInformer, + pluginRegistry plugin.Registry, ) Interface { c := &backupDeletionController{ genericController: newGenericController("backup-deletion", logger), @@ -84,16 +85,20 @@ func NewBackupDeletionController( deleteBackupRequestLister: deleteBackupRequestInformer.Lister(), backupClient: backupClient, blockStore: blockStore, - objectStore: objectStore, - bucket: bucket, restoreLister: restoreInformer.Lister(), restoreClient: restoreClient, backupTracker: backupTracker, resticMgr: resticMgr, + podvolumeBackupLister: podvolumeBackupInformer.Lister(), + backupLocationLister: backupLocationInformer.Lister(), + pluginRegistry: pluginRegistry, - podvolumeBackupLister: podvolumeBackupInformer.Lister(), - deleteBackupDir: cloudprovider.DeleteBackupDir, - clock: &clock.RealClock{}, + // use variables to refer to these functions so they can be + // replaced with fakes for testing. + deleteBackupDir: cloudprovider.DeleteBackupDir, + newPluginManager: plugin.NewManager, + + clock: &clock.RealClock{}, } c.syncHandler = c.processQueueItem @@ -102,6 +107,7 @@ func NewBackupDeletionController( deleteBackupRequestInformer.Informer().HasSynced, restoreInformer.Informer().HasSynced, podvolumeBackupInformer.Informer().HasSynced, + backupLocationInformer.Informer().HasSynced, ) c.processRequestFunc = c.processRequest @@ -240,7 +246,6 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e var errs []string - // Try to delete snapshots log.Info("Removing PV snapshots") for _, volumeBackup := range backup.Status.VolumeBackups { log.WithField("snapshotID", volumeBackup.SnapshotID).Info("Removing snapshot associated with backup") @@ -249,7 +254,6 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e } } - // Try to delete restic snapshots log.Info("Removing restic snapshots") if deleteErrs := c.deleteResticSnapshots(backup); len(deleteErrs) > 0 { for _, err := range deleteErrs { @@ -257,13 +261,11 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e } } - // Try to delete backup from backup storage log.Info("Removing backup from backup storage") - if err := c.deleteBackupDir(log, c.objectStore, c.bucket, backup.Name); err != nil { - errs = append(errs, errors.Wrap(err, "error deleting backup from backup storage").Error()) + if err := c.deleteBackupFromStorage(backup, log); err != nil { + errs = append(errs, err.Error()) } - // Try to delete restores 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") @@ -312,6 +314,27 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e return nil } +func (c *backupDeletionController) deleteBackupFromStorage(backup *v1.Backup, log *logrus.Entry) error { + pluginManager := c.newPluginManager(log, log.Level, c.pluginRegistry) + defer pluginManager.CleanupClients() + + backupLocation, err := c.backupLocationLister.BackupStorageLocations(backup.Namespace).Get(backup.Spec.StorageLocation) + if err != nil { + return errors.WithStack(err) + } + + objectStore, err := getObjectStoreForLocation(backupLocation, pluginManager) + if err != nil { + return err + } + + if err := c.deleteBackupDir(log, objectStore, backupLocation.Spec.ObjectStorage.Bucket, backup.Name); err != nil { + return errors.Wrap(err, "error deleting backup from backup storage") + } + + return nil +} + func (c *backupDeletionController) deleteExistingDeletionRequests(req *v1.DeleteBackupRequest, log logrus.FieldLogger) []error { log.Info("Removing existing deletion requests for backup") selector := labels.SelectorFromSet(labels.Set(map[string]string{ diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index dcbe5c736..307f18d81 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -26,10 +26,13 @@ import ( "github.com/heptio/ark/pkg/cloudprovider" "github.com/heptio/ark/pkg/generated/clientset/versioned/fake" informers "github.com/heptio/ark/pkg/generated/informers/externalversions" + "github.com/heptio/ark/pkg/plugin" + pluginmocks "github.com/heptio/ark/pkg/plugin/mocks" arktest "github.com/heptio/ark/pkg/util/test" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -49,13 +52,13 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) { client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient nil, // blockStore - nil, // backupService - "bucket", sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, // restic repository manager sharedInformers.Ark().V1().PodVolumeBackups(), + sharedInformers.Ark().V1().BackupStorageLocations(), + nil, // pluginRegistry ).(*backupDeletionController) // Error splitting key @@ -109,37 +112,51 @@ type backupDeletionControllerTestData struct { client *fake.Clientset sharedInformers informers.SharedInformerFactory blockStore *arktest.FakeBlockStore + objectStore *arktest.ObjectStore controller *backupDeletionController req *v1.DeleteBackupRequest } func setupBackupDeletionControllerTest(objects ...runtime.Object) *backupDeletionControllerTestData { - client := fake.NewSimpleClientset(objects...) - sharedInformers := informers.NewSharedInformerFactory(client, 0) - blockStore := &arktest.FakeBlockStore{SnapshotsTaken: sets.NewString()} - req := pkgbackup.NewDeleteBackupRequest("foo", "uid") + var ( + client = fake.NewSimpleClientset(objects...) + sharedInformers = informers.NewSharedInformerFactory(client, 0) + blockStore = &arktest.FakeBlockStore{SnapshotsTaken: sets.NewString()} + pluginManager = &pluginmocks.Manager{} + objectStore = &arktest.ObjectStore{} + req = pkgbackup.NewDeleteBackupRequest("foo", "uid") + ) data := &backupDeletionControllerTestData{ client: client, sharedInformers: sharedInformers, blockStore: blockStore, + objectStore: objectStore, controller: NewBackupDeletionController( arktest.NewLogger(), sharedInformers.Ark().V1().DeleteBackupRequests(), client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient blockStore, - nil, // objectStore - "bucket", sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, // restic repository manager sharedInformers.Ark().V1().PodVolumeBackups(), + sharedInformers.Ark().V1().BackupStorageLocations(), + nil, // pluginRegistry ).(*backupDeletionController), req: req, } + + data.controller.newPluginManager = func(_ logrus.FieldLogger, _ logrus.Level, _ plugin.Registry) plugin.Manager { + return pluginManager + } + + pluginManager.On("GetObjectStore", "objStoreProvider").Return(objectStore, nil) + pluginManager.On("CleanupClients").Return(nil) + req.Namespace = "heptio-ark" req.Name = "foo-abcde" @@ -347,6 +364,7 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { t.Run("full delete, no errors", func(t *testing.T) { backup := arktest.NewTestBackup().WithName("foo").WithSnapshot("pv-1", "snap-1").Backup backup.UID = "uid" + backup.Spec.StorageLocation = "primary" restore1 := arktest.NewTestRestore("heptio-ark", "restore-1", v1.RestorePhaseCompleted).WithBackup("foo").Restore restore2 := arktest.NewTestRestore("heptio-ark", "restore-2", v1.RestorePhaseCompleted).WithBackup("foo").Restore @@ -358,6 +376,24 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { td.sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore2) td.sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore3) + location := &v1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Spec.StorageLocation, + }, + Spec: v1.BackupStorageLocationSpec{ + Provider: "objStoreProvider", + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "bucket", + }, + }, + }, + } + require.NoError(t, td.sharedInformers.Ark().V1().BackupStorageLocations().Informer().GetStore().Add(location)) + + td.objectStore.On("Init", mock.Anything).Return(nil) + // Clear out req labels to make sure the controller adds them td.req.Labels = make(map[string]string) @@ -374,8 +410,9 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) { return true, backup, nil }) - td.controller.deleteBackupDir = func(_ logrus.FieldLogger, _ cloudprovider.ObjectStore, bucket, backupName string) error { - require.Equal(t, "bucket", bucket) + td.controller.deleteBackupDir = func(_ logrus.FieldLogger, objectStore cloudprovider.ObjectStore, bucket, backupName string) error { + require.NotNil(t, objectStore) + require.Equal(t, location.Spec.ObjectStorage.Bucket, bucket) require.Equal(t, td.req.Spec.BackupName, backupName) return nil } @@ -561,13 +598,13 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) { client.ArkV1(), // deleteBackupRequestClient client.ArkV1(), // backupClient nil, // blockStore - nil, // backupService - "bucket", sharedInformers.Ark().V1().Restores(), client.ArkV1(), // restoreClient NewBackupTracker(), nil, sharedInformers.Ark().V1().PodVolumeBackups(), + sharedInformers.Ark().V1().BackupStorageLocations(), + nil, // pluginRegistry ).(*backupDeletionController) fakeClock := &clock.FakeClock{}