Batch deletion of individual backup components (#4023)

Reduce the number of kopia manifest blobs
created during backup deletion by batching
the deletion of snapshots and the backup
model into a single kopia operation

Also reorangizes/updates test code for
backup deletion

---

#### 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
- [x] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* #4019

#### Test Plan

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

src/internal/kopia/wrapper.go

@@ -559,37 +559,6 @@ func (w Wrapper) ProduceRestoreCollections(
 	return res, el.Failure()
 }
 
-// DeleteSnapshot removes the provided manifest from kopia.
-func (w Wrapper) DeleteSnapshot(
-	ctx context.Context,
-	snapshotID string,
-) error {
-	mid := manifest.ID(snapshotID)
-	if len(mid) == 0 {
-		return clues.New("snapshot ID required for deletion").WithClues(ctx)
-	}
-
-	err := repo.WriteSession(
-		ctx,
-		w.c,
-		repo.WriteSessionOptions{Purpose: "KopiaWrapperBackupDeletion"},
-		func(innerCtx context.Context, rw repo.RepositoryWriter) error {
-			if err := rw.DeleteManifest(ctx, mid); err != nil {
-				return clues.Wrap(err, "deleting snapshot").WithClues(ctx)
-			}
-
-			return nil
-		},
-	)
-	// Telling kopia to always flush may hide other errors if it fails while
-	// flushing the write session (hence logging above).
-	if err != nil {
-		return clues.Wrap(err, "deleting backup manifest").WithClues(ctx)
-	}
-
-	return nil
-}
-
 func (w Wrapper) NewBaseFinder(bg store.BackupGetter) (*baseFinder, error) {
 	return newBaseFinder(w.c, bg)
 }

src/internal/kopia/wrapper_test.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/alcionai/clues"
-	"github.com/google/uuid"
 	"github.com/kopia/kopia/repo"
 	"github.com/kopia/kopia/repo/blob"
 	"github.com/kopia/kopia/repo/format"
@@ -2157,51 +2156,3 @@ func (suite *KopiaSimpleRepoIntegrationSuite) TestProduceRestoreCollections_Erro
 		})
 	}
 }
-
-func (suite *KopiaSimpleRepoIntegrationSuite) TestDeleteSnapshot() {
-	t := suite.T()
-
-	err := suite.w.DeleteSnapshot(suite.ctx, string(suite.snapshotID))
-	assert.NoError(t, err, clues.ToCore(err))
-
-	// assert the deletion worked
-	itemPath := suite.files[suite.testPath1.String()][0].itemPath
-	ic := i64counter{}
-
-	c, err := suite.w.ProduceRestoreCollections(
-		suite.ctx,
-		string(suite.snapshotID),
-		toRestorePaths(t, itemPath),
-		&ic,
-		fault.New(true))
-	assert.Error(t, err, "snapshot should be deleted", clues.ToCore(err))
-	assert.Empty(t, c)
-	assert.Zero(t, ic.i)
-}
-
-func (suite *KopiaSimpleRepoIntegrationSuite) TestDeleteSnapshot_BadIDs() {
-	table := []struct {
-		name       string
-		snapshotID string
-		expect     assert.ErrorAssertionFunc
-	}{
-		{
-			name:       "no id",
-			snapshotID: "",
-			expect:     assert.Error,
-		},
-		{
-			name:       "unknown id",
-			snapshotID: uuid.NewString(),
-			expect:     assert.NoError,
-		},
-	}
-	for _, test := range table {
-		suite.Run(test.name, func() {
-			t := suite.T()
-
-			err := suite.w.DeleteSnapshot(suite.ctx, test.snapshotID)
-			test.expect(t, err, clues.ToCore(err))
-		})
-	}
-}

src/internal/streamstore/streamstore.go

@@ -79,16 +79,6 @@ func (ss *storeStreamer) Read(ctx context.Context, snapshotID string, col Collec
 	return nil
 }
 
-// Delete deletes a `details.Details` object from the kopia repository
-func (ss *storeStreamer) Delete(ctx context.Context, detailsID string) error {
-	err := ss.kw.DeleteSnapshot(ctx, detailsID)
-	if err != nil {
-		return clues.Wrap(err, "deleting snapshot in stream store")
-	}
-
-	return nil
-}
-
 // ---------------------------------------------------------------------------
 // interfaces
 // ---------------------------------------------------------------------------
@@ -99,7 +89,6 @@ type Streamer interface {
 	Collector
 	Writer
 	Reader
-	Delete(context.Context, string) error
 }
 
 type CollectorWriter interface {

src/pkg/repository/repository.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/alcionai/clues"
 	"github.com/google/uuid"
+	"github.com/kopia/kopia/repo/manifest"
 	"github.com/pkg/errors"
 
 	"github.com/alcionai/corso/src/internal/common/crash"
@@ -629,31 +630,31 @@ func getBackupErrors(
 	return &fe, b, nil
 }
 
-type snapshotDeleter interface {
-	DeleteSnapshot(ctx context.Context, snapshotID string) error
-}
-
 // DeleteBackup removes the backup from both the model store and the backup storage.
 func (r repository) DeleteBackup(ctx context.Context, id string) error {
-	return deleteBackup(ctx, id, r.dataLayer, store.NewWrapper(r.modelStore))
+	return deleteBackup(ctx, id, store.NewWrapper(r.modelStore))
 }
 
 // deleteBackup handles the processing for Backup.
 func deleteBackup(
 	ctx context.Context,
 	id string,
-	kw snapshotDeleter,
+	sw store.BackupGetterModelDeleter,
-	sw store.BackupGetterDeleter,
 ) error {
 	b, err := sw.GetBackup(ctx, model.StableID(id))
 	if err != nil {
-		return errWrapper(err)
+		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)}
+
 	if len(b.SnapshotID) > 0 {
-		if err := kw.DeleteSnapshot(ctx, b.SnapshotID); err != nil {
+		toDelete = append(toDelete, manifest.ID(b.SnapshotID))
-			return err
-		}
 	}
 
 	ssid := b.StreamStoreID
@@ -662,12 +663,10 @@ func deleteBackup(
 	}
 
 	if len(ssid) > 0 {
-		if err := kw.DeleteSnapshot(ctx, ssid); err != nil {
+		toDelete = append(toDelete, manifest.ID(ssid))
-			return err
-		}
 	}
 
-	return sw.DeleteBackup(ctx, model.StableID(id))
+	return sw.DeleteWithModelStoreIDs(ctx, toDelete...)
 }
 
 func (r repository) ConnectToM365(

src/pkg/repository/repository_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/alcionai/corso/src/pkg/control/testdata"
 	"github.com/alcionai/corso/src/pkg/extensions"
 	"github.com/alcionai/corso/src/pkg/selectors"
+	"github.com/alcionai/corso/src/pkg/services/m365/api"
 	"github.com/alcionai/corso/src/pkg/storage"
 	storeTD "github.com/alcionai/corso/src/pkg/storage/testdata"
 )
@@ -288,6 +289,57 @@ func (suite *RepositoryIntegrationSuite) TestNewRestore() {
 	require.NotNil(t, ro)
 }
 
+func (suite *RepositoryIntegrationSuite) TestNewBackupAndDelete() {
+	t := suite.T()
+
+	ctx, flush := tester.NewContext(t)
+	defer flush()
+
+	acct := tconfig.NewM365Account(t)
+
+	// need to initialize the repository before we can test connecting to it.
+	st := storeTD.NewPrefixedS3Storage(t)
+
+	r, err := Initialize(
+		ctx,
+		acct,
+		st,
+		control.DefaultOptions(),
+		ctrlRepo.Retention{})
+	require.NoError(t, err, clues.ToCore(err))
+
+	userID := tconfig.M365UserID(t)
+	sel := selectors.NewExchangeBackup([]string{userID})
+	sel.Include(sel.MailFolders([]string{api.MailInbox}, selectors.PrefixMatch()))
+	sel.DiscreteOwner = userID
+
+	bo, err := r.NewBackup(ctx, sel.Selector)
+	require.NoError(t, err, clues.ToCore(err))
+	require.NotNil(t, bo)
+
+	err = bo.Run(ctx)
+	require.NoError(t, err, "running backup operation: %v", clues.ToCore(err))
+
+	backupID := string(bo.Results.BackupID)
+
+	err = r.DeleteBackup(ctx, backupID)
+	require.NoError(t, err, "deleting backup: %v", clues.ToCore(err))
+
+	// This operation should fail since the backup doesn't exist anymore.
+	restoreCfg := testdata.DefaultRestoreConfig("")
+
+	ro, err := r.NewRestore(
+		ctx,
+		backupID,
+		selectors.Selector{DiscreteOwner: userID},
+		restoreCfg)
+	require.NoError(t, err, clues.ToCore(err))
+	require.NotNil(t, ro)
+
+	_, err = ro.Run(ctx)
+	assert.Error(t, err, "running restore operation")
+}
+
 func (suite *RepositoryIntegrationSuite) TestNewMaintenance() {
 	t := suite.T()
 

src/pkg/repository/repository_unexported_test.go

@@ -320,14 +320,6 @@ func (suite *RepositoryBackupsUnitSuite) TestBackupsByTag() {
 	}
 }
 
-type mockSSDeleter struct {
-	err error
-}
-
-func (sd mockSSDeleter) DeleteSnapshot(_ context.Context, _ string) error {
-	return sd.err
-}
-
 func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 	bup := &backup.Backup{
 		BaseModel: model.BaseModel{
@@ -342,7 +334,6 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 	table := []struct {
 		name      string
 		sw        mock.BackupWrapper
-		kw        mockSSDeleter
 		expectErr func(t *testing.T, result error)
 		expectID  model.StableID
 	}{
@@ -353,7 +344,6 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 				GetErr:    nil,
 				DeleteErr: nil,
 			},
-			kw: mockSSDeleter{},
 			expectErr: func(t *testing.T, result error) {
 				assert.NoError(t, result, clues.ToCore(result))
 			},
@@ -366,7 +356,6 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 				GetErr:    data.ErrNotFound,
 				DeleteErr: nil,
 			},
-			kw: mockSSDeleter{},
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result))
 				assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result))
@@ -380,20 +369,6 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 				GetErr:    nil,
 				DeleteErr: assert.AnError,
 			},
-			kw: mockSSDeleter{},
-			expectErr: func(t *testing.T, result error) {
-				assert.ErrorIs(t, result, assert.AnError, clues.ToCore(result))
-			},
-			expectID: bup.ID,
-		},
-		{
-			name: "snapshot delete error",
-			sw: mock.BackupWrapper{
-				Backup:    bup,
-				GetErr:    nil,
-				DeleteErr: nil,
-			},
-			kw: mockSSDeleter{assert.AnError},
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, assert.AnError, clues.ToCore(result))
 			},
@@ -406,7 +381,6 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 				GetErr:    nil,
 				DeleteErr: nil,
 			},
-			kw: mockSSDeleter{assert.AnError},
 			expectErr: func(t *testing.T, result error) {
 				assert.NoError(t, result, clues.ToCore(result))
 			},
@@ -420,7 +394,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackup() {
 			ctx, flush := tester.NewContext(t)
 			defer flush()
 
-			err := deleteBackup(ctx, string(test.sw.Backup.ID), test.kw, test.sw)
+			err := deleteBackup(ctx, string(test.sw.Backup.ID), test.sw)
 			test.expectErr(t, err)
 		})
 	}

src/pkg/store/backup.go

@@ -59,14 +59,23 @@ type (
 		DeleteBackup(ctx context.Context, backupID model.StableID) error
 	}
 
+	ModelDeleter interface {
+		DeleteWithModelStoreIDs(ctx context.Context, ids ...manifest.ID) error
+	}
+
+	BackupGetterModelDeleter interface {
+		BackupGetter
+		ModelDeleter
+	}
+
 	Storer interface {
 		Delete(ctx context.Context, s model.Schema, id model.StableID) error
-		DeleteWithModelStoreIDs(ctx context.Context, ids ...manifest.ID) error
 		Get(ctx context.Context, s model.Schema, id model.StableID, data model.Model) error
 		GetIDsForType(ctx context.Context, s model.Schema, tags map[string]string) ([]*model.BaseModel, error)
 		GetWithModelStoreID(ctx context.Context, s model.Schema, id manifest.ID, data model.Model) error
 		Put(ctx context.Context, s model.Schema, m model.Model) error
 		Update(ctx context.Context, s model.Schema, m model.Model) error
+		ModelDeleter
 	}
 
 	BackupStorer interface {

src/pkg/store/mock/wrapper.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/alcionai/clues"
+	"github.com/kopia/kopia/repo/manifest"
 
 	"github.com/alcionai/corso/src/internal/model"
 	"github.com/alcionai/corso/src/pkg/backup"
@@ -22,14 +23,14 @@ func (bw BackupWrapper) GetBackup(
 ) (*backup.Backup, error) {
 	bw.Backup.SnapshotID = bw.Backup.ID.String()
 
-	return bw.Backup, bw.GetErr
+	return bw.Backup, clues.Stack(bw.GetErr).OrNil()
 }
 
 func (bw BackupWrapper) DeleteBackup(
 	ctx context.Context,
 	backupID model.StableID,
 ) error {
-	return bw.DeleteErr
+	return clues.Stack(bw.DeleteErr).OrNil()
 }
 
 func (bw BackupWrapper) GetBackups(
@@ -38,3 +39,10 @@ func (bw BackupWrapper) GetBackups(
 ) ([]*backup.Backup, error) {
 	return nil, clues.New("GetBackups mock not implemented yet")
 }
+
+func (bw BackupWrapper) DeleteWithModelStoreIDs(
+	ctx context.Context,
+	ids ...manifest.ID,
+) error {
+	return clues.Stack(bw.DeleteErr).OrNil()
+}