From 50a95d052ed75b9ec5ceaf29a67bfabcc4a8e62a Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Wed, 6 Sep 2017 16:58:43 -0400 Subject: [PATCH] Delete all objects in backup dir Delete all objects in backup "dir" when deleting a backup, instead of hard-coding individual file names/types. This way, we'll be able to delete log files and anything else we add without having to update our deletion code. Signed-off-by: Andy Goldstein --- pkg/cloudprovider/backup_service.go | 31 +- pkg/cloudprovider/backup_service_test.go | 436 +++++---------- pkg/controller/backup_controller_test.go | 2 +- pkg/controller/backup_sync_controller_test.go | 42 +- pkg/controller/gc_controller.go | 21 +- pkg/controller/gc_controller_test.go | 500 +++++------------- pkg/controller/restore_controller_test.go | 113 ++-- pkg/util/test/backup_service.go | 49 +- 8 files changed, 393 insertions(+), 801 deletions(-) diff --git a/pkg/cloudprovider/backup_service.go b/pkg/cloudprovider/backup_service.go index be4290bed..4414209c6 100644 --- a/pkg/cloudprovider/backup_service.go +++ b/pkg/cloudprovider/backup_service.go @@ -45,11 +45,8 @@ type BackupService interface { // downloading or reading the file from the cloud API. DownloadBackup(bucket, name string) (io.ReadCloser, error) - // DeleteBackup deletes the backup content in object storage for the given backup. - DeleteBackupFile(bucket, backupName string) error - - // DeleteBackup deletes the backup metadata file in object storage for the given backup. - DeleteBackupMetadataFile(bucket, backupName string) error + // DeleteBackupDir deletes all files in object storage for the given backup. + DeleteBackupDir(bucket, backupName string) error // GetBackup gets the specified api.Backup from the given bucket in object storage. GetBackup(bucket, name string) (*api.Backup, error) @@ -180,16 +177,22 @@ func (br *backupService) GetBackup(bucket, name string) (*api.Backup, error) { return backup, nil } -func (br *backupService) DeleteBackupFile(bucket, backupName string) error { - key := getBackupKey(backupName) - glog.V(4).Infof("Trying to delete bucket=%s, key=%s", bucket, key) - return br.objectStorage.DeleteObject(bucket, key) -} +func (br *backupService) DeleteBackupDir(bucket, backupName string) error { + objects, err := br.objectStorage.ListObjects(bucket, backupName+"/") + if err != nil { + return err + } -func (br *backupService) DeleteBackupMetadataFile(bucket, backupName string) error { - key := getMetadataKey(backupName) - glog.V(4).Infof("Trying to delete bucket=%s, key=%s", bucket, key) - return br.objectStorage.DeleteObject(bucket, key) + var errs []error + for _, key := range objects { + glog.V(4).Infof("Trying to delete bucket=%s, key=%s", bucket, key) + fmt.Printf("Trying to delete bucket=%s, key=%s\n", bucket, key) + if err := br.objectStorage.DeleteObject(bucket, key); err != nil { + errs = append(errs, err) + } + } + + return errors.NewAggregate(errs) } func (br *backupService) CreateBackupLogSignedURL(bucket, backupName string, ttl time.Duration) (string, error) { diff --git a/pkg/cloudprovider/backup_service_test.go b/pkg/cloudprovider/backup_service_test.go index 37645e4fc..86da45cc4 100644 --- a/pkg/cloudprovider/backup_service_test.go +++ b/pkg/cloudprovider/backup_service_test.go @@ -24,13 +24,13 @@ import ( "io/ioutil" "strings" "testing" - "time" + testutil "github.com/heptio/ark/pkg/util/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/sets" api "github.com/heptio/ark/pkg/apis/ark/v1" "github.com/heptio/ark/pkg/util/encode" @@ -38,261 +38,140 @@ import ( func TestUploadBackup(t *testing.T) { tests := []struct { - name string - bucket string - bucketExists bool - backupName string - metadata io.ReadSeeker - backup io.ReadSeeker - log io.ReadSeeker - objectStoreErrs map[string]map[string]interface{} - expectedErr bool - expectedRes map[string][]byte + name string + metadata io.ReadSeeker + metadataError error + expectMetadataDelete bool + backup io.ReadSeeker + backupError error + log io.ReadSeeker + logError error + expectedErr string }{ { - name: "normal case", - bucket: "test-bucket", - bucketExists: true, - backupName: "test-backup", - metadata: newStringReadSeeker("foo"), - backup: newStringReadSeeker("bar"), - log: newStringReadSeeker("baz"), - expectedErr: false, - expectedRes: map[string][]byte{ - "test-backup/ark-backup.json": []byte("foo"), - "test-backup/test-backup.tar.gz": []byte("bar"), - "test-backup/test-backup.log.gz": []byte("baz"), - }, + name: "normal case", + metadata: newStringReadSeeker("foo"), + backup: newStringReadSeeker("bar"), + log: newStringReadSeeker("baz"), }, { - name: "no such bucket causes error", - bucket: "test-bucket", - bucketExists: false, - backupName: "test-backup", - expectedErr: true, + name: "error on metadata upload does not upload data or log", + metadata: newStringReadSeeker("foo"), + metadataError: errors.New("md"), + expectedErr: "md", }, { - name: "error on metadata upload does not upload data or log", - bucket: "test-bucket", - bucketExists: true, - backupName: "test-backup", - metadata: newStringReadSeeker("foo"), - backup: newStringReadSeeker("bar"), - log: newStringReadSeeker("baz"), - objectStoreErrs: map[string]map[string]interface{}{ - "putobject": map[string]interface{}{ - "test-bucket||test-backup/ark-backup.json": true, - }, - }, - expectedErr: true, - expectedRes: make(map[string][]byte), + name: "error on data upload deletes metadata", + metadata: newStringReadSeeker("foo"), + backup: newStringReadSeeker("bar"), + backupError: errors.New("backup"), + expectMetadataDelete: true, + expectedErr: "backup", }, { - name: "error on data upload deletes metadata", - bucket: "test-bucket", - bucketExists: true, - backupName: "test-backup", - metadata: newStringReadSeeker("foo"), - backup: newStringReadSeeker("bar"), - log: newStringReadSeeker("baz"), - objectStoreErrs: map[string]map[string]interface{}{ - "putobject": map[string]interface{}{ - "test-bucket||test-backup/test-backup.tar.gz": true, - }, - }, - expectedErr: true, - expectedRes: make(map[string][]byte), - }, - { - name: "error on log upload is ok", - bucket: "test-bucket", - bucketExists: true, - backupName: "test-backup", - metadata: newStringReadSeeker("foo"), - backup: newStringReadSeeker("bar"), - log: newStringReadSeeker("baz"), - objectStoreErrs: map[string]map[string]interface{}{ - "putobject": map[string]interface{}{ - "test-bucket||test-backup/test-backup.log.gz": true, - }, - }, - expectedErr: false, - expectedRes: map[string][]byte{ - "test-backup/ark-backup.json": []byte("foo"), - "test-backup/test-backup.tar.gz": []byte("bar"), - }, + name: "error on log upload is ok", + metadata: newStringReadSeeker("foo"), + backup: newStringReadSeeker("bar"), + log: newStringReadSeeker("baz"), + logError: errors.New("log"), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - objStore := &fakeObjectStorage{ - returnErrors: test.objectStoreErrs, - storage: make(map[string]map[string][]byte), + objStore := &testutil.ObjectStorageAdapter{} + + bucket := "test-bucket" + backupName := "test-backup" + + if test.metadata != nil { + objStore.On("PutObject", bucket, backupName+"/ark-backup.json", test.metadata).Return(test.metadataError) } - if test.bucketExists { - objStore.storage[test.bucket] = make(map[string][]byte) + if test.backup != nil { + objStore.On("PutObject", bucket, backupName+"/"+backupName+".tar.gz", test.backup).Return(test.backupError) + } + if test.log != nil { + objStore.On("PutObject", bucket, backupName+"/"+backupName+".log.gz", test.log).Return(test.logError) + } + if test.expectMetadataDelete { + objStore.On("DeleteObject", bucket, backupName+"/ark-backup.json").Return(nil) } backupService := NewBackupService(objStore) - err := backupService.UploadBackup(test.bucket, test.backupName, test.metadata, test.backup, test.log) + err := backupService.UploadBackup(bucket, backupName, test.metadata, test.backup, test.log) - assert.Equal(t, test.expectedErr, err != nil, "got error %v", err) - - assert.Equal(t, test.expectedRes, objStore.storage[test.bucket]) + if test.expectedErr != "" { + assert.EqualError(t, err, test.expectedErr) + } else { + assert.NoError(t, err) + } + objStore.AssertExpectations(t) }) } } func TestDownloadBackup(t *testing.T) { + o := &testutil.ObjectStorageAdapter{} + bucket := "b" + backup := "bak" + o.On("GetObject", bucket, backup+"/"+backup+".tar.gz").Return(ioutil.NopCloser(strings.NewReader("foo")), nil) + s := NewBackupService(o) + rc, err := s.DownloadBackup(bucket, backup) + require.NoError(t, err) + require.NotNil(t, rc) + data, err := ioutil.ReadAll(rc) + require.NoError(t, err) + assert.Equal(t, "foo", string(data)) + o.AssertExpectations(t) +} + +func TestDeleteBackup(t *testing.T) { tests := []struct { - name string - bucket string - backupName string - storage map[string]map[string][]byte - expectedErr bool - expectedRes []byte + name string + listObjectsError error + deleteErrors []error + expectedErr string }{ { - name: "normal case", - bucket: "test-bucket", - backupName: "test-backup", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{ - "test-backup/test-backup.tar.gz": []byte("foo"), - }, - }, - expectedErr: false, - expectedRes: []byte("foo"), + name: "normal case", }, { - name: "no such bucket causes error", - bucket: "test-bucket", - backupName: "test-backup", - storage: map[string]map[string][]byte{}, - expectedErr: true, - }, - { - name: "no such key causes error", - bucket: "test-bucket", - backupName: "test-backup", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{}, - }, - expectedErr: true, + name: "some delete errors, do as much as we can", + deleteErrors: []error{errors.New("a"), nil, errors.New("c")}, + expectedErr: "[a, c]", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - objStore := &fakeObjectStorage{storage: test.storage} - backupService := NewBackupService(objStore) + bucket := "bucket" + backup := "bak" + objects := []string{"bak/ark-backup.json", "bak/bak.tar.gz", "bak/bak.log.gz"} - rdr, err := backupService.DownloadBackup(test.bucket, test.backupName) + objStore := &testutil.ObjectStorageAdapter{} + objStore.On("ListObjects", bucket, backup+"/").Return(objects, test.listObjectsError) + for i, o := range objects { + var err error + if i < len(test.deleteErrors) { + err = test.deleteErrors[i] + } - assert.Equal(t, test.expectedErr, err != nil, "got error %v", err) - - if err == nil { - res, err := ioutil.ReadAll(rdr) - assert.Nil(t, err) - assert.Equal(t, test.expectedRes, res) + objStore.On("DeleteObject", bucket, o).Return(err) } - }) - } -} -func TestDeleteBackupFile(t *testing.T) { - tests := []struct { - name string - bucket string - backupName string - storage map[string]map[string][]byte - expectedErr bool - expectedRes map[string][]byte - }{ - { - name: "normal case", - bucket: "test-bucket", - backupName: "bak", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{ - "bak/bak.tar.gz": nil, - }, - }, - expectedErr: false, - expectedRes: make(map[string][]byte), - }, - { - name: "failed delete of backup returns error", - bucket: "test-bucket", - backupName: "bak", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{}, - }, - expectedErr: true, - expectedRes: make(map[string][]byte), - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - objStore := &fakeObjectStorage{storage: test.storage} backupService := NewBackupService(objStore) - res := backupService.DeleteBackupFile(test.bucket, test.backupName) + err := backupService.DeleteBackupDir(bucket, backup) - assert.Equal(t, test.expectedErr, res != nil, "got error %v", res) + if test.expectedErr != "" { + assert.EqualError(t, err, test.expectedErr) + } else { + assert.NoError(t, err) + } - assert.Equal(t, test.expectedRes, objStore.storage[test.bucket]) - }) - } -} - -func TestDeleteBackupMetadataFile(t *testing.T) { - tests := []struct { - name string - bucket string - backupName string - storage map[string]map[string][]byte - expectedErr bool - expectedRes map[string][]byte - }{ - { - name: "normal case", - bucket: "test-bucket", - backupName: "bak", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{ - "bak/ark-backup.json": nil, - }, - }, - expectedErr: false, - expectedRes: make(map[string][]byte), - }, - { - name: "failed delete of file returns error", - bucket: "test-bucket", - backupName: "bak", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{}, - }, - expectedErr: true, - expectedRes: make(map[string][]byte), - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - objStore := &fakeObjectStorage{storage: test.storage} - backupService := NewBackupService(objStore) - - res := backupService.DeleteBackupMetadataFile(test.bucket, test.backupName) - - assert.Equal(t, test.expectedErr, res != nil, "got error %v", res) - - assert.Equal(t, test.expectedRes, objStore.storage[test.bucket]) + objStore.AssertExpectations(t) }) } } @@ -300,21 +179,16 @@ func TestDeleteBackupMetadataFile(t *testing.T) { func TestGetAllBackups(t *testing.T) { tests := []struct { name string - bucket string - storage map[string]map[string][]byte + storageData map[string][]byte expectedRes []*api.Backup - expectedErr bool + expectedErr string }{ { - name: "normal case", - bucket: "test-bucket", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{ - "backup-1/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), - "backup-2/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-2"}}), - }, + name: "normal case", + storageData: map[string][]byte{ + "backup-1/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), + "backup-2/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-2"}}), }, - expectedErr: false, expectedRes: []*api.Backup{ &api.Backup{ TypeMeta: metav1.TypeMeta{Kind: "Backup", APIVersion: "ark.heptio.com/v1"}, @@ -327,13 +201,10 @@ func TestGetAllBackups(t *testing.T) { }, }, { - name: "backup that can't be decoded is ignored", - bucket: "test-bucket", - storage: map[string]map[string][]byte{ - "test-bucket": map[string][]byte{ - "backup-1/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), - "backup-2/ark-backup.json": []byte("this is not valid backup JSON"), - }, + name: "backup that can't be decoded is ignored", + storageData: map[string][]byte{ + "backup-1/ark-backup.json": encodeToBytes(&api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), + "backup-2/ark-backup.json": []byte("this is not valid backup JSON"), }, expectedRes: []*api.Backup{ &api.Backup{ @@ -346,14 +217,26 @@ func TestGetAllBackups(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - objStore := &fakeObjectStorage{storage: test.storage} + bucket := "bucket" + + objStore := &testutil.ObjectStorageAdapter{} + objStore.On("ListCommonPrefixes", bucket, "/").Return([]string{"backup-1", "backup-2"}, nil) + objStore.On("GetObject", bucket, "backup-1/ark-backup.json").Return(ioutil.NopCloser(bytes.NewReader(test.storageData["backup-1/ark-backup.json"])), nil) + objStore.On("GetObject", bucket, "backup-2/ark-backup.json").Return(ioutil.NopCloser(bytes.NewReader(test.storageData["backup-2/ark-backup.json"])), nil) + backupService := NewBackupService(objStore) - res, err := backupService.GetAllBackups(test.bucket) + res, err := backupService.GetAllBackups(bucket) - assert.Equal(t, test.expectedErr, err != nil, "got error %v", err) + if test.expectedErr != "" { + assert.EqualError(t, err, test.expectedErr) + } else { + assert.NoError(t, err) + } assert.Equal(t, test.expectedRes, res) + + objStore.AssertExpectations(t) }) } } @@ -387,86 +270,3 @@ func newStringReadSeeker(s string) *stringReadSeeker { func (srs *stringReadSeeker) Seek(offset int64, whence int) (int64, error) { panic("not implemented") } - -type fakeObjectStorage struct { - storage map[string]map[string][]byte - returnErrors map[string]map[string]interface{} -} - -func (os *fakeObjectStorage) PutObject(bucket string, key string, body io.ReadSeeker) error { - if os.returnErrors["putobject"] != nil && os.returnErrors["putobject"][bucket+"||"+key] != nil { - return errors.New("error") - } - - if os.storage[bucket] == nil { - return errors.New("bucket not found") - } - - data, err := ioutil.ReadAll(body) - if err != nil { - return err - } - - os.storage[bucket][key] = data - - return nil -} - -func (os *fakeObjectStorage) GetObject(bucket string, key string) (io.ReadCloser, error) { - if os.storage == nil { - return nil, errors.New("storage not initialized") - } - if os.storage[bucket] == nil { - return nil, errors.New("bucket not found") - } - - if os.storage[bucket][key] == nil { - return nil, errors.New("key not found") - } - - return ioutil.NopCloser(bytes.NewReader(os.storage[bucket][key])), nil -} - -func (os *fakeObjectStorage) ListCommonPrefixes(bucket string, delimiter string) ([]string, error) { - if os.storage == nil { - return nil, errors.New("storage not initialized") - } - if os.storage[bucket] == nil { - return nil, errors.New("bucket not found") - } - - prefixes := sets.NewString() - - for key := range os.storage[bucket] { - delimIdx := strings.LastIndex(key, delimiter) - - if delimIdx == -1 { - prefixes.Insert(key) - } - - prefixes.Insert(key[0:delimIdx]) - } - - return prefixes.List(), nil -} - -func (os *fakeObjectStorage) DeleteObject(bucket string, key string) error { - if os.storage == nil { - return errors.New("storage not initialized") - } - if os.storage[bucket] == nil { - return errors.New("bucket not found") - } - - if _, exists := os.storage[bucket][key]; !exists { - return errors.New("key not found") - } - - delete(os.storage[bucket], key) - - return nil -} - -func (os *fakeObjectStorage) CreateSignedURL(bucket, key string, ttl time.Duration) (string, error) { - panic("not implemented") -} diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 048edb87e..938e0e07b 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -155,7 +155,7 @@ func TestProcessBackup(t *testing.T) { backupper := &fakeBackupper{} - cloudBackups := &fakeBackupService{} + cloudBackups := &BackupService{} sharedInformers := informers.NewSharedInformerFactory(client, 0) diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index fafe0ff91..f75c694a1 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "errors" "testing" "time" @@ -24,46 +25,38 @@ import ( core "k8s.io/client-go/testing" - api "github.com/heptio/ark/pkg/apis/ark/v1" + "github.com/heptio/ark/pkg/apis/ark/v1" "github.com/heptio/ark/pkg/generated/clientset/fake" . "github.com/heptio/ark/pkg/util/test" ) -func TestRun(t *testing.T) { +func TestBackupSyncControllerRun(t *testing.T) { tests := []struct { - name string - cloudBackups map[string][]*api.Backup - backupSvcErr error + name string + getAllBackupsError error + cloudBackups []*v1.Backup }{ { name: "no cloud backups", }, { - name: "backup service returns error on GetAllBackups", - cloudBackups: map[string][]*api.Backup{ - "nonexistent-bucket": []*api.Backup{ - NewTestBackup().WithNamespace("ns-1").WithName("backup-1").Backup, - }, - }, + name: "backup service returns error on GetAllBackups", + getAllBackupsError: errors.New("getAllBackups"), }, { name: "normal case", - cloudBackups: map[string][]*api.Backup{ - "bucket": []*api.Backup{ - NewTestBackup().WithNamespace("ns-1").WithName("backup-1").Backup, - NewTestBackup().WithNamespace("ns-1").WithName("backup-2").Backup, - NewTestBackup().WithNamespace("ns-2").WithName("backup-3").Backup, - }, + cloudBackups: []*v1.Backup{ + NewTestBackup().WithNamespace("ns-1").WithName("backup-1").Backup, + NewTestBackup().WithNamespace("ns-1").WithName("backup-2").Backup, + NewTestBackup().WithNamespace("ns-2").WithName("backup-3").Backup, }, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - var ( - bs = &fakeBackupService{backupsByBucket: test.cloudBackups} - client = fake.NewSimpleClientset() - ) + bs := &BackupService{} + client := fake.NewSimpleClientset() c := NewBackupSyncController( client.ArkV1(), @@ -72,14 +65,16 @@ func TestRun(t *testing.T) { time.Duration(0), ).(*backupSyncController) + bs.On("GetAllBackups", "bucket").Return(test.cloudBackups, test.getAllBackupsError) + c.run() expectedActions := make([]core.Action, 0) // we only expect creates for items within the target bucket - for _, cloudBackup := range test.cloudBackups["bucket"] { + for _, cloudBackup := range test.cloudBackups { action := core.NewCreateAction( - api.SchemeGroupVersion.WithResource("backups"), + v1.SchemeGroupVersion.WithResource("backups"), cloudBackup.Namespace, cloudBackup, ) @@ -88,6 +83,7 @@ func TestRun(t *testing.T) { } assert.Equal(t, expectedActions, client.Actions()) + bs.AssertExpectations(t) }) } } diff --git a/pkg/controller/gc_controller.go b/pkg/controller/gc_controller.go index 1eca1a59c..b9870a741 100644 --- a/pkg/controller/gc_controller.go +++ b/pkg/controller/gc_controller.go @@ -104,9 +104,9 @@ func (c *gcController) run() { } // garbageCollectBackup removes an expired backup by deleting any associated backup files (if -// deleteBackupFile = true), volume snapshots, restore API objects, and the backup API object +// deleteBackupFiles = true), volume snapshots, restore API objects, and the backup API object // itself. -func (c *gcController) garbageCollectBackup(backup *api.Backup, deleteBackupFile bool) { +func (c *gcController) garbageCollectBackup(backup *api.Backup, deleteBackupFiles bool) { // if the backup includes snapshots but we don't currently have a PVProvider, we don't // want to orphan the snapshots so skip garbage-collection entirely. if c.snapshotService == nil && len(backup.Status.VolumeBackups) > 0 { @@ -128,23 +128,14 @@ func (c *gcController) garbageCollectBackup(backup *api.Backup, deleteBackupFile } } - // If applicable, delete backup & metadata file from object storage *before* deleting the API object + // If applicable, delete everything in the backup dir in object storage *before* deleting the API object // because otherwise the backup sync controller could re-sync the backup from object storage. - if deleteBackupFile { - glog.Infof("Removing backup %s", kube.NamespaceAndName(backup)) - if err := c.backupService.DeleteBackupFile(c.bucket, backup.Name); err != nil { + if deleteBackupFiles { + glog.Infof("Removing backup %s from object storage", kube.NamespaceAndName(backup)) + if err := c.backupService.DeleteBackupDir(c.bucket, backup.Name); err != nil { glog.Errorf("error deleting backup %s: %v", kube.NamespaceAndName(backup), err) deletionFailure = true } - - if deletionFailure { - glog.Warningf("Backup %s will not be deleted due to errors deleting related object storage files(s) and/or volume snapshots", kube.NamespaceAndName(backup)) - } else { - if err := c.backupService.DeleteBackupMetadataFile(c.bucket, backup.Name); err != nil { - glog.Errorf("error deleting backup metadata file for %s: %v", kube.NamespaceAndName(backup), err) - deletionFailure = true - } - } } glog.Infof("Getting restore API objects referencing backup %s", kube.NamespaceAndName(backup)) diff --git a/pkg/controller/gc_controller_test.go b/pkg/controller/gc_controller_test.go index 3d7277396..382760664 100644 --- a/pkg/controller/gc_controller_test.go +++ b/pkg/controller/gc_controller_test.go @@ -17,18 +17,11 @@ limitations under the License. package controller import ( - "bytes" - "errors" - "fmt" - "io" - "io/ioutil" "testing" "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" "k8s.io/apimachinery/pkg/util/sets" core "k8s.io/client-go/testing" @@ -42,12 +35,11 @@ import ( type gcTest struct { name string - bucket string - backups map[string][]*api.Backup + backups []*api.Backup snapshots sets.String nilSnapshotService bool - expectedBackupsRemaining map[string]sets.String + expectedDeletions sets.String expectedSnapshotsRemaining sets.String } @@ -56,130 +48,96 @@ func TestGarbageCollect(t *testing.T) { tests := []gcTest{ gcTest{ - name: "basic-expired", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(-1*time.Second)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - }, + name: "basic-expired", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(-1*time.Second)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, }, snapshots: sets.NewString("snapshot-1", "snapshot-2"), - expectedBackupsRemaining: make(map[string]sets.String), + expectedDeletions: sets.NewString("backup-1"), expectedSnapshotsRemaining: sets.NewString(), }, gcTest{ - name: "basic-unexpired", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(1*time.Minute)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - }, - }, - snapshots: sets.NewString("snapshot-1", "snapshot-2"), - expectedBackupsRemaining: map[string]sets.String{ - "bucket-1": sets.NewString("backup-1"), + name: "basic-unexpired", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(1*time.Minute)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, }, + snapshots: sets.NewString("snapshot-1", "snapshot-2"), + expectedDeletions: sets.NewString(), expectedSnapshotsRemaining: sets.NewString("snapshot-1", "snapshot-2"), }, gcTest{ - name: "one expired, one unexpired", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(-1*time.Minute)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - NewTestBackup().WithName("backup-2"). - WithExpiration(fakeClock.Now().Add(1*time.Minute)). - WithSnapshot("pv-3", "snapshot-3"). - WithSnapshot("pv-4", "snapshot-4"). - Backup, - }, - }, - snapshots: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), - expectedBackupsRemaining: map[string]sets.String{ - "bucket-1": sets.NewString("backup-2"), + name: "one expired, one unexpired", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(-1*time.Minute)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, + NewTestBackup().WithName("backup-2"). + WithExpiration(fakeClock.Now().Add(1*time.Minute)). + WithSnapshot("pv-3", "snapshot-3"). + WithSnapshot("pv-4", "snapshot-4"). + Backup, }, + snapshots: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), + expectedDeletions: sets.NewString("backup-1"), expectedSnapshotsRemaining: sets.NewString("snapshot-3", "snapshot-4"), }, gcTest{ - name: "none expired in target bucket", - bucket: "bucket-2", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(-1*time.Minute)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - }, - "bucket-2": []*api.Backup{ - NewTestBackup().WithName("backup-2"). - WithExpiration(fakeClock.Now().Add(1*time.Minute)). - WithSnapshot("pv-3", "snapshot-3"). - WithSnapshot("pv-4", "snapshot-4"). - Backup, - }, - }, - snapshots: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), - expectedBackupsRemaining: map[string]sets.String{ - "bucket-1": sets.NewString("backup-1"), - "bucket-2": sets.NewString("backup-2"), + name: "none expired in target bucket", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-2"). + WithExpiration(fakeClock.Now().Add(1*time.Minute)). + WithSnapshot("pv-3", "snapshot-3"). + WithSnapshot("pv-4", "snapshot-4"). + Backup, }, + snapshots: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), + expectedDeletions: sets.NewString(), expectedSnapshotsRemaining: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), }, gcTest{ - name: "orphan snapshots", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(-1*time.Minute)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - }, + name: "orphan snapshots", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(-1*time.Minute)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, }, snapshots: sets.NewString("snapshot-1", "snapshot-2", "snapshot-3", "snapshot-4"), - expectedBackupsRemaining: make(map[string]sets.String), + expectedDeletions: sets.NewString("backup-1"), expectedSnapshotsRemaining: sets.NewString("snapshot-3", "snapshot-4"), }, gcTest{ - name: "no snapshot service only GC's backups without snapshots", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(-1*time.Second)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - NewTestBackup().WithName("backup-2"). - WithExpiration(fakeClock.Now().Add(-1 * time.Second)). - Backup, - }, + name: "no snapshot service only GC's backups without snapshots", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(-1*time.Second)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, + NewTestBackup().WithName("backup-2"). + WithExpiration(fakeClock.Now().Add(-1 * time.Second)). + Backup, }, snapshots: sets.NewString("snapshot-1", "snapshot-2"), nilSnapshotService: true, - expectedBackupsRemaining: map[string]sets.String{ - "bucket-1": sets.NewString("backup-1"), - }, + expectedDeletions: sets.NewString("backup-2"), }, } for _, test := range tests { var ( - backupService = &fakeBackupService{} + backupService = &BackupService{} snapshotService *FakeSnapshotService ) @@ -188,23 +146,11 @@ func TestGarbageCollect(t *testing.T) { } t.Run(test.name, func(t *testing.T) { - backupService.backupsByBucket = make(map[string][]*api.Backup) - backupService.backupMetadataByBucket = make(map[string][]*api.Backup) - - for bucket, backups := range test.backups { - data := make([]*api.Backup, 0, len(backups)) - for _, backup := range backups { - data = append(data, backup) - } - - backupService.backupsByBucket[bucket] = data - backupService.backupMetadataByBucket[bucket] = data - } - var ( client = fake.NewSimpleClientset() sharedInformers = informers.NewSharedInformerFactory(client, 0) snapSvc cloudprovider.SnapshotService + bucket = "bucket" ) if snapshotService != nil { @@ -214,7 +160,7 @@ func TestGarbageCollect(t *testing.T) { controller := NewGCController( backupService, snapSvc, - test.bucket, + bucket, 1*time.Millisecond, sharedInformers.Ark().V1().Backups(), client.ArkV1(), @@ -223,161 +169,75 @@ func TestGarbageCollect(t *testing.T) { ).(*gcController) controller.clock = fakeClock - controller.processBackups() - - // verify every bucket has the backups we expect - for bucket, backups := range backupService.backupsByBucket { - // if actual and expected are both empty, no further verification needed - if len(backups) == 0 && len(test.expectedBackupsRemaining[bucket]) == 0 { - continue - } - - // get all the actual backups remaining in this bucket - backupNames := sets.NewString() - for _, backup := range backupService.backupsByBucket[bucket] { - backupNames.Insert(backup.Name) - } - - assert.Equal(t, test.expectedBackupsRemaining[bucket], backupNames) + backupService.On("GetAllBackups", bucket).Return(test.backups, nil) + for _, b := range test.expectedDeletions.List() { + backupService.On("DeleteBackupDir", bucket, b).Return(nil) } + controller.processBackups() + if !test.nilSnapshotService { assert.Equal(t, test.expectedSnapshotsRemaining, snapshotService.SnapshotsTaken) } + + backupService.AssertExpectations(t) }) } } func TestGarbageCollectBackup(t *testing.T) { tests := []struct { - name string - backup *api.Backup - deleteBackupFile bool - snapshots sets.String - backupFiles sets.String - backupMetadataFiles sets.String - restores []*api.Restore - expectedRestoreDeletes []string - expectedBackupDelete string - expectedSnapshots sets.String - expectedBackupFiles sets.String - expectedMetadataFiles sets.String + name string + backup *api.Backup + deleteBackupFile bool + snapshots sets.String + backupFiles sets.String + backupMetadataFiles sets.String + restores []*api.Restore + expectedRestoreDeletes []string + expectedBackupDelete string + expectedSnapshots sets.String + expectedObjectStorageDeletions sets.String }{ { - name: "failed snapshot deletion shouldn't delete backup metadata file", + name: "deleteBackupFile=false, snapshot deletion fails, don't delete kube backup", backup: NewTestBackup().WithName("backup-1"). WithSnapshot("pv-1", "snapshot-1"). WithSnapshot("pv-2", "snapshot-2"). Backup, - deleteBackupFile: true, - snapshots: sets.NewString("snapshot-1"), - backupFiles: sets.NewString("backup-1"), - backupMetadataFiles: sets.NewString("backup-1"), - restores: nil, - expectedBackupDelete: "", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString(), - expectedMetadataFiles: sets.NewString("backup-1"), + deleteBackupFile: false, + snapshots: sets.NewString("snapshot-1"), + expectedSnapshots: sets.NewString(), + expectedObjectStorageDeletions: sets.NewString(), }, { - name: "failed backup file deletion shouldn't delete backup metadata file", - backup: NewTestBackup().WithName("backup-1"). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - deleteBackupFile: true, - snapshots: sets.NewString("snapshot-1", "snapshot-2"), - backupFiles: sets.NewString("doesn't-match-backup-name"), - backupMetadataFiles: sets.NewString("backup-1"), - restores: nil, - expectedBackupDelete: "", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString("doesn't-match-backup-name"), - expectedMetadataFiles: sets.NewString("backup-1"), - }, - { - name: "missing backup metadata file still deletes snapshots & backup file", - backup: NewTestBackup().WithName("backup-1"). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - deleteBackupFile: true, - snapshots: sets.NewString("snapshot-1", "snapshot-2"), - backupFiles: sets.NewString("backup-1"), - backupMetadataFiles: sets.NewString("doesn't-match-backup-name"), - restores: nil, - expectedBackupDelete: "", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString(), - expectedMetadataFiles: sets.NewString("doesn't-match-backup-name"), - }, - { - name: "deleteBackupFile=false shouldn't error if no backup file exists", - backup: NewTestBackup().WithName("backup-1"). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - deleteBackupFile: false, - snapshots: sets.NewString("snapshot-1", "snapshot-2"), - backupFiles: sets.NewString("non-matching-backup"), - backupMetadataFiles: sets.NewString("non-matching-backup"), - restores: nil, - expectedBackupDelete: "backup-1", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString("non-matching-backup"), - expectedMetadataFiles: sets.NewString("non-matching-backup"), - }, - { - name: "deleteBackupFile=false should error if snapshot delete fails", - backup: NewTestBackup().WithName("backup-1"). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - deleteBackupFile: false, - snapshots: sets.NewString("snapshot-1"), - backupFiles: sets.NewString("non-matching-backup"), - backupMetadataFiles: sets.NewString("non-matching-backup"), - restores: nil, - expectedBackupDelete: "", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString("non-matching-backup"), - expectedMetadataFiles: sets.NewString("non-matching-backup"), - }, - { - name: "related restores should be deleted", - backup: NewTestBackup().WithName("backup-1").Backup, - deleteBackupFile: false, - snapshots: sets.NewString(), - backupFiles: sets.NewString("non-matching-backup"), - backupMetadataFiles: sets.NewString("non-matching-backup"), + name: "related restores should be deleted", + backup: NewTestBackup().WithName("backup-1").Backup, + deleteBackupFile: true, + snapshots: sets.NewString(), restores: []*api.Restore{ NewTestRestore(api.DefaultNamespace, "restore-1", api.RestorePhaseCompleted).WithBackup("backup-1").Restore, NewTestRestore(api.DefaultNamespace, "restore-2", api.RestorePhaseCompleted).WithBackup("backup-2").Restore, }, - expectedRestoreDeletes: []string{"restore-1"}, - expectedBackupDelete: "backup-1", - expectedSnapshots: sets.NewString(), - expectedBackupFiles: sets.NewString("non-matching-backup"), - expectedMetadataFiles: sets.NewString("non-matching-backup"), + expectedRestoreDeletes: []string{"restore-1"}, + expectedBackupDelete: "backup-1", + expectedSnapshots: sets.NewString(), + expectedObjectStorageDeletions: sets.NewString("backup-1"), }, } for _, test := range tests { - var () - t.Run(test.name, func(t *testing.T) { var ( - backupService = &fakeBackupService{ - backupsByBucket: make(map[string][]*api.Backup), - backupMetadataByBucket: make(map[string][]*api.Backup), - } + backupService = &BackupService{} snapshotService = &FakeSnapshotService{SnapshotsTaken: test.snapshots} client = fake.NewSimpleClientset() sharedInformers = informers.NewSharedInformerFactory(client, 0) + bucket = "bucket-1" controller = NewGCController( backupService, snapshotService, - "bucket-1", + bucket, 1*time.Millisecond, sharedInformers.Ark().V1().Backups(), client.ArkV1(), @@ -386,20 +246,15 @@ func TestGarbageCollectBackup(t *testing.T) { ).(*gcController) ) - for file := range test.backupFiles { - backup := &api.Backup{ObjectMeta: metav1.ObjectMeta{Name: file}} - backupService.backupsByBucket["bucket-1"] = append(backupService.backupsByBucket["bucket-1"], backup) - } - for file := range test.backupMetadataFiles { - backup := &api.Backup{ObjectMeta: metav1.ObjectMeta{Name: file}} - backupService.backupMetadataByBucket["bucket-1"] = append(backupService.backupMetadataByBucket["bucket-1"], backup) - } - sharedInformers.Ark().V1().Backups().Informer().GetStore().Add(test.backup) for _, restore := range test.restores { sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore) } + for _, b := range test.expectedObjectStorageDeletions.List() { + backupService.On("DeleteBackupDir", bucket, b).Return(nil) + } + // METHOD UNDER TEST controller.garbageCollectBackup(test.backup, test.deleteBackupFile) @@ -408,22 +263,6 @@ func TestGarbageCollectBackup(t *testing.T) { // remaining snapshots assert.Equal(t, test.expectedSnapshots, snapshotService.SnapshotsTaken) - // remaining object storage backup files - expectedBackups := make([]*api.Backup, 0) - for file := range test.expectedBackupFiles { - backup := &api.Backup{ObjectMeta: metav1.ObjectMeta{Name: file}} - expectedBackups = append(expectedBackups, backup) - } - assert.Equal(t, expectedBackups, backupService.backupsByBucket["bucket-1"]) - - // remaining object storage backup metadata files - expectedBackups = make([]*api.Backup, 0) - for file := range test.expectedMetadataFiles { - backup := &api.Backup{ObjectMeta: metav1.ObjectMeta{Name: file}} - expectedBackups = append(expectedBackups, backup) - } - assert.Equal(t, expectedBackups, backupService.backupMetadataByBucket["bucket-1"]) - expectedActions := make([]core.Action, 0) // Restore client deletes for _, restore := range test.expectedRestoreDeletes { @@ -446,44 +285,32 @@ func TestGarbageCollectBackup(t *testing.T) { } assert.Equal(t, expectedActions, client.Actions()) + + backupService.AssertExpectations(t) }) } } func TestGarbageCollectPicksUpBackupUponExpiration(t *testing.T) { var ( - backupService = &fakeBackupService{} + backupService = &BackupService{} snapshotService = &FakeSnapshotService{} fakeClock = clock.NewFakeClock(time.Now()) assert = assert.New(t) ) scenario := gcTest{ - name: "basic-expired", - bucket: "bucket-1", - backups: map[string][]*api.Backup{ - "bucket-1": []*api.Backup{ - NewTestBackup().WithName("backup-1"). - WithExpiration(fakeClock.Now().Add(1*time.Second)). - WithSnapshot("pv-1", "snapshot-1"). - WithSnapshot("pv-2", "snapshot-2"). - Backup, - }, + name: "basic-expired", + backups: []*api.Backup{ + NewTestBackup().WithName("backup-1"). + WithExpiration(fakeClock.Now().Add(1*time.Second)). + WithSnapshot("pv-1", "snapshot-1"). + WithSnapshot("pv-2", "snapshot-2"). + Backup, }, snapshots: sets.NewString("snapshot-1", "snapshot-2"), } - backupService.backupsByBucket = make(map[string][]*api.Backup) - - for bucket, backups := range scenario.backups { - data := make([]*api.Backup, 0, len(backups)) - for _, backup := range backups { - data = append(data, backup) - } - - backupService.backupsByBucket[bucket] = data - } - snapshotService.SnapshotsTaken = scenario.snapshots var ( @@ -494,7 +321,7 @@ func TestGarbageCollectPicksUpBackupUponExpiration(t *testing.T) { controller := NewGCController( backupService, snapshotService, - scenario.bucket, + "bucket", 1*time.Millisecond, sharedInformers.Ark().V1().Backups(), client.ArkV1(), @@ -503,109 +330,20 @@ func TestGarbageCollectPicksUpBackupUponExpiration(t *testing.T) { ).(*gcController) controller.clock = fakeClock + backupService.On("GetAllBackups", "bucket").Return(scenario.backups, nil) + // PASS 1 controller.processBackups() - assert.Equal(scenario.backups, backupService.backupsByBucket, "backups should not be garbage-collected yet.") + backupService.AssertExpectations(t) assert.Equal(scenario.snapshots, snapshotService.SnapshotsTaken, "snapshots should not be garbage-collected yet.") // PASS 2 fakeClock.Step(1 * time.Minute) + backupService.On("DeleteBackupDir", "bucket", "backup-1").Return(nil) controller.processBackups() - assert.Equal(0, len(backupService.backupsByBucket[scenario.bucket]), "backups should have been garbage-collected.") assert.Equal(0, len(snapshotService.SnapshotsTaken), "snapshots should have been garbage-collected.") -} - -// TODO remove this and use util/test mock instead -type fakeBackupService struct { - backupMetadataByBucket map[string][]*api.Backup - backupsByBucket map[string][]*api.Backup - mock.Mock -} - -func (s *fakeBackupService) GetAllBackups(bucket string) ([]*api.Backup, error) { - backups, found := s.backupsByBucket[bucket] - if !found { - return nil, errors.New("bucket not found") - } - return backups, nil -} - -func (s *fakeBackupService) GetBackup(bucket, name string) (*api.Backup, error) { - backups, err := s.GetAllBackups(bucket) - if err != nil { - return nil, err - } - - for _, itm := range backups { - if itm.Name == name { - return itm, nil - } - } - - return nil, errors.New("backup not found") -} - -func (bs *fakeBackupService) UploadBackup(bucket, name string, metadata, backup, log io.ReadSeeker) error { - args := bs.Called(bucket, name, metadata, backup, log) - return args.Error(0) -} - -func (s *fakeBackupService) DownloadBackup(bucket, name string) (io.ReadCloser, error) { - return ioutil.NopCloser(bytes.NewReader([]byte("hello world"))), nil -} - -func (s *fakeBackupService) DownloadBackupLogs(bucket, name string) (io.ReadCloser, error) { - return ioutil.NopCloser(bytes.NewReader([]byte("hello world in a log"))), nil -} - -func (s *fakeBackupService) DeleteBackupMetadataFile(bucket, backupName string) error { - backups, found := s.backupMetadataByBucket[bucket] - if !found { - return errors.New("bucket not found") - } - - deleteIdx := -1 - for i, backup := range backups { - if backup.Name == backupName { - deleteIdx = i - break - } - } - - if deleteIdx == -1 { - return errors.New("backup not found") - } - - s.backupMetadataByBucket[bucket] = append(s.backupMetadataByBucket[bucket][0:deleteIdx], s.backupMetadataByBucket[bucket][deleteIdx+1:]...) - - return nil -} - -func (s *fakeBackupService) DeleteBackupFile(bucket, backupName string) error { - backups, err := s.GetAllBackups(bucket) - if err != nil { - return err - } - - deleteIdx := -1 - for i, backup := range backups { - if backup.Name == backupName { - deleteIdx = i - break - } - } - - if deleteIdx == -1 { - return errors.New("backup not found") - } - - s.backupsByBucket[bucket] = append(s.backupsByBucket[bucket][0:deleteIdx], s.backupsByBucket[bucket][deleteIdx+1:]...) - - return nil -} - -func (s *fakeBackupService) CreateBackupLogSignedURL(bucket, backupName string, ttl time.Duration) (string, error) { - return fmt.Sprintf("http://some.server/%s/%s/%d", bucket, backupName, ttl), nil + + backupService.AssertExpectations(t) } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 0d4053c55..da567a5e0 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -17,8 +17,11 @@ limitations under the License. package controller import ( + "bytes" "errors" + "fmt" "io" + "io/ioutil" "testing" "github.com/stretchr/testify/assert" @@ -35,14 +38,15 @@ import ( . "github.com/heptio/ark/pkg/util/test" ) -func TestFetchRestore(t *testing.T) { +func TestFetchBackup(t *testing.T) { tests := []struct { - name string - backupName string - informerBackups []*api.Backup - backupSvcBackups map[string][]*api.Backup - expectedRes *api.Backup - expectedErr bool + name string + backupName string + informerBackups []*api.Backup + backupServiceBackup *api.Backup + backupServiceError error + expectedRes *api.Backup + expectedErr bool }{ { name: "lister has backup", @@ -51,17 +55,16 @@ func TestFetchRestore(t *testing.T) { expectedRes: NewTestBackup().WithName("backup-1").Backup, }, { - name: "backupSvc has backup", - backupName: "backup-1", - backupSvcBackups: map[string][]*api.Backup{ - "bucket": []*api.Backup{NewTestBackup().WithName("backup-1").Backup}, - }, - expectedRes: NewTestBackup().WithName("backup-1").Backup, + name: "backupSvc has backup", + backupName: "backup-1", + backupServiceBackup: NewTestBackup().WithName("backup-1").Backup, + expectedRes: NewTestBackup().WithName("backup-1").Backup, }, { - name: "no backup", - backupName: "backup-1", - expectedErr: true, + name: "no backup", + backupName: "backup-1", + backupServiceError: errors.New("no backup here"), + expectedErr: true, }, } @@ -71,7 +74,7 @@ func TestFetchRestore(t *testing.T) { client = fake.NewSimpleClientset() restorer = &fakeRestorer{} sharedInformers = informers.NewSharedInformerFactory(client, 0) - backupSvc = &fakeBackupService{} + backupSvc = &BackupService{} ) c := NewRestoreController( @@ -89,28 +92,34 @@ func TestFetchRestore(t *testing.T) { sharedInformers.Ark().V1().Backups().Informer().GetStore().Add(itm) } - backupSvc.backupsByBucket = test.backupSvcBackups + if test.backupServiceBackup != nil || test.backupServiceError != nil { + backupSvc.On("GetBackup", "bucket", test.backupName).Return(test.backupServiceBackup, test.backupServiceError) + } backup, err := c.fetchBackup("bucket", test.backupName) if assert.Equal(t, test.expectedErr, err != nil) { assert.Equal(t, test.expectedRes, backup) } + + backupSvc.AssertExpectations(t) }) } } func TestProcessRestore(t *testing.T) { tests := []struct { - name string - restoreKey string - restore *api.Restore - backup *api.Backup - restorerError error - allowRestoreSnapshots bool - expectedErr bool - expectedRestoreUpdates []*api.Restore - expectedRestorerCall *api.Restore + name string + restoreKey string + restore *api.Restore + backup *api.Backup + restorerError error + allowRestoreSnapshots bool + expectedErr bool + expectedRestoreUpdates []*api.Restore + expectedRestorerCall *api.Restore + backupServiceGetBackupError error + expectRestore bool }{ { name: "invalid key returns error", @@ -161,18 +170,17 @@ func TestProcessRestore(t *testing.T) { }, }, { - name: "restore with non-existent backup name fails", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, - expectedErr: false, + name: "restore with non-existent backup name fails", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, + expectedErr: false, + backupServiceGetBackupError: errors.New("no backup here"), expectedRestoreUpdates: []*api.Restore{ NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, NewTestRestore("foo", "bar", api.RestorePhaseCompleted). WithBackup("backup-1"). WithIncludedNamespace("ns-1"). WithErrors(api.RestoreResult{ - // TODO this is the error msg returned by the fakeBackupService. When we switch to a mock obj, - // this will likely need to change. - Cluster: []string{"bucket not found"}, + Cluster: []string{"no backup here"}, }). Restore, }, @@ -181,6 +189,7 @@ func TestProcessRestore(t *testing.T) { name: "restorer throwing an error causes the restore to fail", restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, backup: NewTestBackup().WithName("backup-1").Backup, + expectRestore: true, restorerError: errors.New("blarg"), expectedErr: false, expectedRestoreUpdates: []*api.Restore{ @@ -197,10 +206,11 @@ func TestProcessRestore(t *testing.T) { expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, }, { - name: "valid restore gets executed", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, - backup: NewTestBackup().WithName("backup-1").Backup, - expectedErr: false, + name: "valid restore gets executed", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, + backup: NewTestBackup().WithName("backup-1").Backup, + expectRestore: true, + expectedErr: false, expectedRestoreUpdates: []*api.Restore{ NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, @@ -208,10 +218,11 @@ func TestProcessRestore(t *testing.T) { expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, }, { - name: "restore with no restorable namespaces gets defaulted to *", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").Restore, - backup: NewTestBackup().WithName("backup-1").Backup, - expectedErr: false, + name: "restore with no restorable namespaces gets defaulted to *", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").Restore, + backup: NewTestBackup().WithName("backup-1").Backup, + expectRestore: true, + expectedErr: false, expectedRestoreUpdates: []*api.Restore{ NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("*").Restore, NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithIncludedNamespace("*").Restore, @@ -222,6 +233,7 @@ func TestProcessRestore(t *testing.T) { name: "valid restore with RestorePVs=true gets executed when allowRestoreSnapshots=true", restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, backup: NewTestBackup().WithName("backup-1").Backup, + expectRestore: true, allowRestoreSnapshots: true, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ @@ -242,17 +254,14 @@ func TestProcessRestore(t *testing.T) { }, } - // flag.Set("logtostderr", "true") - // flag.Set("v", "4") - for _, test := range tests { t.Run(test.name, func(t *testing.T) { - + fmt.Println(test.name) var ( client = fake.NewSimpleClientset() restorer = &fakeRestorer{} sharedInformers = informers.NewSharedInformerFactory(client, 0) - backupSvc = &fakeBackupService{} + backupSvc = &BackupService{} ) c := NewRestoreController( @@ -290,7 +299,11 @@ func TestProcessRestore(t *testing.T) { if test.restorerError != nil { errors.Namespaces = map[string][]string{"ns-1": {test.restorerError.Error()}} } - restorer.On("Restore", mock.Anything, mock.Anything, mock.Anything).Return(warnings, errors) + if test.expectRestore { + downloadedBackup := ioutil.NopCloser(bytes.NewReader([]byte("hello world"))) + backupSvc.On("DownloadBackup", mock.Anything, mock.Anything).Return(downloadedBackup, nil) + restorer.On("Restore", mock.Anything, mock.Anything, mock.Anything).Return(warnings, errors) + } var ( key = test.restoreKey @@ -303,7 +316,13 @@ func TestProcessRestore(t *testing.T) { } } + if test.backupServiceGetBackupError != nil { + backupSvc.On("GetBackup", "bucket", test.restore.Spec.BackupName).Return(nil, test.backupServiceGetBackupError) + } + err = c.processRestore(key) + backupSvc.AssertExpectations(t) + restorer.AssertExpectations(t) assert.Equal(t, test.expectedErr, err != nil, "got error %v", err) diff --git a/pkg/util/test/backup_service.go b/pkg/util/test/backup_service.go index be85eb747..85352d369 100644 --- a/pkg/util/test/backup_service.go +++ b/pkg/util/test/backup_service.go @@ -19,6 +19,7 @@ package test import io "io" import mock "github.com/stretchr/testify/mock" +import time "time" import v1 "github.com/heptio/ark/pkg/apis/ark/v1" // BackupService is an autogenerated mock type for the BackupService type @@ -26,8 +27,29 @@ type BackupService struct { mock.Mock } -// DeleteBackup provides a mock function with given fields: bucket, backupName -func (_m *BackupService) DeleteBackup(bucket string, backupName string) error { +// CreateBackupLogSignedURL provides a mock function with given fields: bucket, backupName, ttl +func (_m *BackupService) CreateBackupLogSignedURL(bucket string, backupName string, ttl time.Duration) (string, error) { + ret := _m.Called(bucket, backupName, ttl) + + var r0 string + if rf, ok := ret.Get(0).(func(string, string, time.Duration) string); ok { + r0 = rf(bucket, backupName, ttl) + } else { + r0 = ret.Get(0).(string) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string, time.Duration) error); ok { + r1 = rf(bucket, backupName, ttl) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// DeleteBackupDir provides a mock function with given fields: bucket, backupName +func (_m *BackupService) DeleteBackupDir(bucket string, backupName string) error { ret := _m.Called(bucket, backupName) var r0 error @@ -86,6 +108,29 @@ func (_m *BackupService) GetAllBackups(bucket string) ([]*v1.Backup, error) { return r0, r1 } +// GetBackup provides a mock function with given fields: bucket, name +func (_m *BackupService) GetBackup(bucket string, name string) (*v1.Backup, error) { + ret := _m.Called(bucket, name) + + var r0 *v1.Backup + if rf, ok := ret.Get(0).(func(string, string) *v1.Backup); ok { + r0 = rf(bucket, name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v1.Backup) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(bucket, name) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // UploadBackup provides a mock function with given fields: bucket, name, metadata, backup, log func (_m *BackupService) UploadBackup(bucket string, name string, metadata io.ReadSeeker, backup io.ReadSeeker, log io.ReadSeeker) error { ret := _m.Called(bucket, name, metadata, backup, log)