From 132ef2e010affc702178bfec404152ca8abfb707 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Mon, 14 Aug 2023 13:13:36 -0700 Subject: [PATCH] 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? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * closes #4019 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/cli/backup/backup.go | 2 +- src/cmd/longevity_test/longevity.go | 2 +- src/pkg/repository/repository.go | 52 +-- src/pkg/repository/repository_test.go | 2 +- .../repository/repository_unexported_test.go | 301 ++++++++++++++++-- 5 files changed, 303 insertions(+), 56 deletions(-) diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index 56be6a92b..e53faf9f1 100644 --- a/src/cli/backup/backup.go +++ b/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)) } diff --git a/src/cmd/longevity_test/longevity.go b/src/cmd/longevity_test/longevity.go index c106a6a84..b2ca512a2 100644 --- a/src/cmd/longevity_test/longevity.go +++ b/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"). diff --git a/src/pkg/repository/repository.go b/src/pkg/repository/repository.go index 310526f69..d439fbf2e 100644 --- a/src/pkg/repository/repository.go +++ b/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. -func (r repository) DeleteBackup(ctx context.Context, id string) error { - return deleteBackup(ctx, id, store.NewWrapper(r.modelStore)) +// DeleteBackups removes the backups from both the model store and the backup +// storage. +// +// 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 { - toDelete = append(toDelete, manifest.ID(b.SnapshotID)) - } + for _, id := range ids { + 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 - if len(ssid) == 0 { - ssid = b.DetailsID - } + toDelete = append(toDelete, b.ModelStoreID) - if len(ssid) > 0 { - toDelete = append(toDelete, manifest.ID(ssid)) + if len(b.SnapshotID) > 0 { + 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...) diff --git a/src/pkg/repository/repository_test.go b/src/pkg/repository/repository_test.go index f6d758f79..a15addd5b 100644 --- a/src/pkg/repository/repository_test.go +++ b/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. diff --git a/src/pkg/repository/repository_unexported_test.go b/src/pkg/repository/repository_unexported_test.go index 820451559..4e03dd23c 100644 --- a/src/pkg/repository/repository_unexported_test.go +++ b/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 - sw mock.BackupWrapper - expectErr func(t *testing.T, result error) - expectID model.StableID + name string + inputIDs []model.StableID + gets []getRes + expectGets []model.StableID + dels []error + expectDels [][]string + expectErr func(t *testing.T, result error) }{ { - name: "no error", - sw: mock.BackupWrapper{ - Backup: bup, - GetErr: nil, - DeleteErr: nil, + name: "SingleBackup NoError", + inputIDs: []model.StableID{ + bup.ID, + }, + 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", - sw: mock.BackupWrapper{ - Backup: bup, - GetErr: data.ErrNotFound, - DeleteErr: nil, + name: "SingleBackup GetError", + inputIDs: []model.StableID{ + bup.ID, + }, + 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", - sw: mock.BackupWrapper{ - Backup: bup, - GetErr: nil, - DeleteErr: assert.AnError, + name: "SingleBackup DeleteError", + inputIDs: []model.StableID{ + bup.ID, + }, + 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", - sw: mock.BackupWrapper{ - Backup: bupNoSnapshot, - GetErr: nil, - DeleteErr: nil, + name: "SingleBackup NoSnapshot", + inputIDs: []model.StableID{ + bupNoSnapshot.ID, + }, + 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) }) }