SDK support for multi-backup deletion (#4026)

Allow SDK users to delete multiple backups
at the same time. This will result in fewer
kopia manifest blobs being made in S3 which
makes things simpler overall

This does change the name of the delete
function, which will break SDK consumers.
That change can be removed from the PR if
that's a better path forward

This does not add the ability to delete
multiple backups at once to the CLI. That
change should be fairly simple to do in
another PR if desired

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* closes #4019

#### Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/cli/backup/backup.go

@@ -283,7 +283,7 @@ func genericDeleteCommand(
 
 	defer utils.CloseRepo(ctx, r)
 
-	if err := r.DeleteBackup(ctx, bID); err != nil {
+	if err := r.DeleteBackups(ctx, bID); err != nil {
 		return Only(ctx, clues.Wrap(err, "Deleting backup "+bID))
 	}
 

src/cmd/longevity_test/longevity.go

@@ -48,7 +48,7 @@ func deleteBackups(
 
 	for _, backup := range backups {
 		if backup.StartAndEndTime.CompletedAt.Before(cutoff) {
-			if err := r.DeleteBackup(ctx, backup.ID.String()); err != nil {
+			if err := r.DeleteBackups(ctx, backup.ID.String()); err != nil {
 				return nil, clues.Wrap(
 					err,
 					"deleting backup").

src/pkg/repository/repository.go

@@ -89,7 +89,7 @@ type Repository interface {
 		ctx context.Context,
 		rcOpts ctrlRepo.Retention,
 	) (operations.RetentionConfigOperation, error)
-	DeleteBackup(ctx context.Context, id string) error
+	DeleteBackups(ctx context.Context, ids ...string) error
 	BackupGetter
 	// ConnectToM365 establishes graph api connections
 	// and initializes api client configurations.
@@ -630,40 +630,50 @@ func getBackupErrors(
 	return &fe, b, nil
 }
 
-// DeleteBackup removes the backup from both the model store and the backup storage.
+// DeleteBackups removes the backups from both the model store and the backup
-func (r repository) DeleteBackup(ctx context.Context, id string) error {
+// storage.
-	return deleteBackup(ctx, id, store.NewWrapper(r.modelStore))
+//
+// All backups are delete as an atomic unit so any failures will result in no
+// deletions.
+func (r repository) DeleteBackups(ctx context.Context, ids ...string) error {
+	return deleteBackups(ctx, store.NewWrapper(r.modelStore), ids...)
 }
 
 // deleteBackup handles the processing for Backup.
-func deleteBackup(
+func deleteBackups(
 	ctx context.Context,
-	id string,
 	sw store.BackupGetterModelDeleter,
+	ids ...string,
 ) error {
-	b, err := sw.GetBackup(ctx, model.StableID(id))
-	if err != nil {
-		return clues.Stack(errWrapper(err)).WithClues(ctx)
-	}
-
 	// Although we haven't explicitly stated it, snapshots are technically
 	// manifests in kopia. This means we can use the same delete API to remove
 	// them and backup models. Deleting all of them together gives us both
 	// atomicity guarantees (around when data will be flushed) and helps reduce
 	// the number of manifest blobs that kopia will create.
-	toDelete := []manifest.ID{manifest.ID(b.ModelStoreID)}
+	var toDelete []manifest.ID
 
-	if len(b.SnapshotID) > 0 {
+	for _, id := range ids {
-		toDelete = append(toDelete, manifest.ID(b.SnapshotID))
+		b, err := sw.GetBackup(ctx, model.StableID(id))
-	}
+		if err != nil {
+			return clues.Stack(errWrapper(err)).
+				WithClues(ctx).
+				With("delete_backup_id", id)
+		}
 
-	ssid := b.StreamStoreID
+		toDelete = append(toDelete, b.ModelStoreID)
-	if len(ssid) == 0 {
-		ssid = b.DetailsID
-	}
 
-	if len(ssid) > 0 {
+		if len(b.SnapshotID) > 0 {
-		toDelete = append(toDelete, manifest.ID(ssid))
+			toDelete = append(toDelete, manifest.ID(b.SnapshotID))
+		}
+
+		ssid := b.StreamStoreID
+		if len(ssid) == 0 {
+			ssid = b.DetailsID
+		}
+
+		if len(ssid) > 0 {
+			toDelete = append(toDelete, manifest.ID(ssid))
+		}
 	}
 
 	return sw.DeleteWithModelStoreIDs(ctx, toDelete...)

src/pkg/repository/repository_test.go

@@ -322,7 +322,7 @@ func (suite *RepositoryIntegrationSuite) TestNewBackupAndDelete() {
 
 	backupID := string(bo.Results.BackupID)
 
-	err = r.DeleteBackup(ctx, backupID)
+	err = r.DeleteBackups(ctx, backupID)
 	require.NoError(t, err, "deleting backup: %v", clues.ToCore(err))
 
 	// This operation should fail since the backup doesn't exist anymore.

src/pkg/repository/repository_unexported_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/alcionai/clues"
 	"github.com/google/uuid"
+	"github.com/kopia/kopia/repo/manifest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -320,81 +321,317 @@ func (suite *RepositoryBackupsUnitSuite) TestBackupsByTag() {
 	}
 }
 
-func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
+type getRes struct {
+	bup *backup.Backup
+	err error
+}
+
+type mockBackupGetterModelDeleter struct {
+	t *testing.T
+
+	gets       []getRes
+	deleteErrs []error
+
+	expectGets []model.StableID
+	expectDels [][]string
+
+	getCount int
+	delCount int
+}
+
+func (m *mockBackupGetterModelDeleter) GetBackup(
+	_ context.Context,
+	id model.StableID,
+) (*backup.Backup, error) {
+	defer func() {
+		m.getCount++
+	}()
+
+	assert.Equal(m.t, m.expectGets[m.getCount], id)
+
+	return m.gets[m.getCount].bup, clues.Stack(m.gets[m.getCount].err).OrNil()
+}
+
+func (m *mockBackupGetterModelDeleter) DeleteWithModelStoreIDs(
+	_ context.Context,
+	ids ...manifest.ID,
+) error {
+	defer func() {
+		m.delCount++
+	}()
+
+	converted := make([]string, 0, len(ids))
+	for _, id := range ids {
+		converted = append(converted, string(id))
+	}
+
+	assert.ElementsMatch(m.t, m.expectDels[m.delCount], converted)
+
+	return clues.Stack(m.deleteErrs[m.delCount]).OrNil()
+}
+
+func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 	bup := &backup.Backup{
 		BaseModel: model.BaseModel{
-			ID: model.StableID(uuid.NewString()),
+			ID:           model.StableID(uuid.NewString()),
+			ModelStoreID: manifest.ID(uuid.NewString()),
 		},
+		SnapshotID:    uuid.NewString(),
+		StreamStoreID: uuid.NewString(),
+	}
+
+	bupLegacy := &backup.Backup{
+		BaseModel: model.BaseModel{
+			ID:           model.StableID(uuid.NewString()),
+			ModelStoreID: manifest.ID(uuid.NewString()),
+		},
+		SnapshotID: uuid.NewString(),
+		DetailsID:  uuid.NewString(),
 	}
 
 	bupNoSnapshot := &backup.Backup{
-		BaseModel: model.BaseModel{},
+		BaseModel: model.BaseModel{
+			ID:           model.StableID(uuid.NewString()),
+			ModelStoreID: manifest.ID(uuid.NewString()),
+		},
+		StreamStoreID: uuid.NewString(),
+	}
+
+	bupNoDetails := &backup.Backup{
+		BaseModel: model.BaseModel{
+			ID:           model.StableID(uuid.NewString()),
+			ModelStoreID: manifest.ID(uuid.NewString()),
+		},
+		SnapshotID: uuid.NewString(),
 	}
 
 	table := []struct {
-		name      string
+		name       string
-		sw        mock.BackupWrapper
+		inputIDs   []model.StableID
-		expectErr func(t *testing.T, result error)
+		gets       []getRes
-		expectID  model.StableID
+		expectGets []model.StableID
+		dels       []error
+		expectDels [][]string
+		expectErr  func(t *testing.T, result error)
 	}{
 		{
-			name: "no error",
+			name: "SingleBackup NoError",
-			sw: mock.BackupWrapper{
+			inputIDs: []model.StableID{
-				Backup:    bup,
+				bup.ID,
-				GetErr:    nil,
+			},
-				DeleteErr: nil,
+			gets: []getRes{
+				{bup: bup},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
+			},
+			dels: []error{
+				nil,
+			},
+			expectDels: [][]string{
+				{
+					string(bup.ModelStoreID),
+					bup.SnapshotID,
+					bup.StreamStoreID,
+				},
 			},
 			expectErr: func(t *testing.T, result error) {
 				assert.NoError(t, result, clues.ToCore(result))
 			},
-			expectID: bup.ID,
 		},
 		{
-			name: "get error",
+			name: "SingleBackup GetError",
-			sw: mock.BackupWrapper{
+			inputIDs: []model.StableID{
-				Backup:    bup,
+				bup.ID,
-				GetErr:    data.ErrNotFound,
+			},
-				DeleteErr: nil,
+			gets: []getRes{
+				{err: data.ErrNotFound},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
 			},
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result))
 				assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result))
 			},
-			expectID: bup.ID,
 		},
 		{
-			name: "delete error",
+			name: "SingleBackup DeleteError",
-			sw: mock.BackupWrapper{
+			inputIDs: []model.StableID{
-				Backup:    bup,
+				bup.ID,
-				GetErr:    nil,
+			},
-				DeleteErr: assert.AnError,
+			gets: []getRes{
+				{bup: bup},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
+			},
+			dels: []error{assert.AnError},
+			expectDels: [][]string{
+				{
+					string(bup.ModelStoreID),
+					bup.SnapshotID,
+					bup.StreamStoreID,
+				},
 			},
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, assert.AnError, clues.ToCore(result))
 			},
-			expectID: bup.ID,
 		},
 		{
-			name: "no snapshot present",
+			name: "SingleBackup NoSnapshot",
-			sw: mock.BackupWrapper{
+			inputIDs: []model.StableID{
-				Backup:    bupNoSnapshot,
+				bupNoSnapshot.ID,
-				GetErr:    nil,
+			},
-				DeleteErr: nil,
+			gets: []getRes{
+				{bup: bupNoSnapshot},
+			},
+			expectGets: []model.StableID{
+				bupNoSnapshot.ID,
+			},
+			dels: []error{nil},
+			expectDels: [][]string{
+				{
+					string(bupNoSnapshot.ModelStoreID),
+					bupNoSnapshot.StreamStoreID,
+				},
 			},
 			expectErr: func(t *testing.T, result error) {
 				assert.NoError(t, result, clues.ToCore(result))
 			},
-			expectID: bupNoSnapshot.ID,
+		},
+		{
+			name: "SingleBackup NoDetails",
+			inputIDs: []model.StableID{
+				bupNoDetails.ID,
+			},
+			gets: []getRes{
+				{bup: bupNoDetails},
+			},
+			expectGets: []model.StableID{
+				bupNoDetails.ID,
+			},
+			dels: []error{nil},
+			expectDels: [][]string{
+				{
+					string(bupNoDetails.ModelStoreID),
+					bupNoDetails.SnapshotID,
+				},
+			},
+			expectErr: func(t *testing.T, result error) {
+				assert.NoError(t, result, clues.ToCore(result))
+			},
+		},
+		{
+			name: "SingleBackup OldDetailsID",
+			inputIDs: []model.StableID{
+				bupLegacy.ID,
+			},
+			gets: []getRes{
+				{bup: bupLegacy},
+			},
+			expectGets: []model.StableID{
+				bupLegacy.ID,
+			},
+			dels: []error{nil},
+			expectDels: [][]string{
+				{
+					string(bupLegacy.ModelStoreID),
+					bupLegacy.SnapshotID,
+					bupLegacy.DetailsID,
+				},
+			},
+			expectErr: func(t *testing.T, result error) {
+				assert.NoError(t, result, clues.ToCore(result))
+			},
+		},
+		{
+			name: "MultipleBackups NoError",
+			inputIDs: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			gets: []getRes{
+				{bup: bup},
+				{bup: bupLegacy},
+				{bup: bupNoSnapshot},
+				{bup: bupNoDetails},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			dels: []error{nil},
+			expectDels: [][]string{
+				{
+					string(bup.ModelStoreID),
+					bup.SnapshotID,
+					bup.StreamStoreID,
+					string(bupLegacy.ModelStoreID),
+					bupLegacy.SnapshotID,
+					bupLegacy.DetailsID,
+					string(bupNoSnapshot.ModelStoreID),
+					bupNoSnapshot.StreamStoreID,
+					string(bupNoDetails.ModelStoreID),
+					bupNoDetails.SnapshotID,
+				},
+			},
+			expectErr: func(t *testing.T, result error) {
+				assert.NoError(t, result, clues.ToCore(result))
+			},
+		},
+		{
+			name: "MultipleBackups GetError",
+			inputIDs: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			gets: []getRes{
+				{bup: bup},
+				{bup: bupLegacy},
+				{bup: bupNoSnapshot},
+				{err: data.ErrNotFound},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			expectErr: func(t *testing.T, result error) {
+				assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result))
+				assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result))
+			},
 		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {
 			t := suite.T()
+			m := &mockBackupGetterModelDeleter{
+				t: t,
+
+				gets:       test.gets,
+				deleteErrs: test.dels,
+
+				expectGets: test.expectGets,
+				expectDels: test.expectDels,
+			}
 
 			ctx, flush := tester.NewContext(t)
 			defer flush()
 
-			err := deleteBackup(ctx, string(test.sw.Backup.ID), test.sw)
+			strIDs := make([]string, 0, len(test.inputIDs))
+			for _, id := range test.inputIDs {
+				strIDs = append(strIDs, string(id))
+			}
+
+			err := deleteBackups(ctx, m, strIDs...)
 			test.expectErr(t, err)
 		})
 	}