diff --git a/src/internal/kopia/wrapper.go b/src/internal/kopia/wrapper.go index d73490c08..7e23b1553 100644 --- a/src/internal/kopia/wrapper.go +++ b/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) } diff --git a/src/internal/kopia/wrapper_test.go b/src/internal/kopia/wrapper_test.go index 4acf0370e..a21b954a9 100644 --- a/src/internal/kopia/wrapper_test.go +++ b/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)) - }) - } -} diff --git a/src/internal/streamstore/streamstore.go b/src/internal/streamstore/streamstore.go index 6a350d7c3..58f98f2a8 100644 --- a/src/internal/streamstore/streamstore.go +++ b/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 { diff --git a/src/pkg/repository/repository.go b/src/pkg/repository/repository.go index 01147511f..310526f69 100644 --- a/src/pkg/repository/repository.go +++ b/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.BackupGetterDeleter, + sw store.BackupGetterModelDeleter, ) 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 { - return err - } + toDelete = append(toDelete, manifest.ID(b.SnapshotID)) } ssid := b.StreamStoreID @@ -662,12 +663,10 @@ func deleteBackup( } if len(ssid) > 0 { - if err := kw.DeleteSnapshot(ctx, ssid); err != nil { - return err - } + toDelete = append(toDelete, manifest.ID(ssid)) } - return sw.DeleteBackup(ctx, model.StableID(id)) + return sw.DeleteWithModelStoreIDs(ctx, toDelete...) } func (r repository) ConnectToM365( diff --git a/src/pkg/repository/repository_test.go b/src/pkg/repository/repository_test.go index 2ee90b5da..f6d758f79 100644 --- a/src/pkg/repository/repository_test.go +++ b/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() diff --git a/src/pkg/repository/repository_unexported_test.go b/src/pkg/repository/repository_unexported_test.go index d5369b6e6..820451559 100644 --- a/src/pkg/repository/repository_unexported_test.go +++ b/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) }) } diff --git a/src/pkg/store/backup.go b/src/pkg/store/backup.go index 86d439f2e..f5df2dbb7 100644 --- a/src/pkg/store/backup.go +++ b/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 { diff --git a/src/pkg/store/mock/wrapper.go b/src/pkg/store/mock/wrapper.go index 1802e1053..d01a7b8c4 100644 --- a/src/pkg/store/mock/wrapper.go +++ b/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() +}