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 <andy.goldstein@gmail.com>

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)
 

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)
 		})
 	}
 }

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))

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)
 }

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)