From 56820e95e059e1b7ed4c5241838878e866843ab9 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Tue, 15 Aug 2023 11:43:21 -0700 Subject: [PATCH] Don't fail deletion on missing backups (#4038) Allows ignoring missing backups during deletion by passing in an addition bool param. Since this changes the API, we can remove the final commit to make ignoring missing backups the default behavior. This will change CLI behavior though --- #### 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) * #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 | 22 ++++- src/pkg/repository/repository_test.go | 2 +- .../repository/repository_unexported_test.go | 85 ++++++++++++++----- 5 files changed, 83 insertions(+), 30 deletions(-) diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index e53faf9f1..c8df39902 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.DeleteBackups(ctx, bID); err != nil { + if err := r.DeleteBackups(ctx, true, 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 a5545617e..b3d6f865d 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.DeleteBackups(ctx, backup.ID.String()); err != nil { + if err := r.DeleteBackups(ctx, true, 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 d439fbf2e..fe607a4c5 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) - DeleteBackups(ctx context.Context, ids ...string) error + DeleteBackups(ctx context.Context, failOnMissing bool, ids ...string) error BackupGetter // ConnectToM365 establishes graph api connections // and initializes api client configurations. @@ -633,16 +633,26 @@ func getBackupErrors( // DeleteBackups removes the backups from both the model store and the backup // storage. // +// If failOnMissing is true then returns an error if a backup model can't be +// found. Otherwise ignores missing backup models. +// +// Missing models or snapshots during the actual deletion do not cause errors. +// // 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...) +func (r repository) DeleteBackups( + ctx context.Context, + failOnMissing bool, + ids ...string, +) error { + return deleteBackups(ctx, store.NewWrapper(r.modelStore), failOnMissing, ids...) } -// deleteBackup handles the processing for Backup. +// deleteBackup handles the processing for backup deletion. func deleteBackups( ctx context.Context, sw store.BackupGetterModelDeleter, + failOnMissing bool, ids ...string, ) error { // Although we haven't explicitly stated it, snapshots are technically @@ -655,6 +665,10 @@ func deleteBackups( for _, id := range ids { b, err := sw.GetBackup(ctx, model.StableID(id)) if err != nil { + if !failOnMissing && errors.Is(err, data.ErrNotFound) { + continue + } + return clues.Stack(errWrapper(err)). WithClues(ctx). With("delete_backup_id", id) diff --git a/src/pkg/repository/repository_test.go b/src/pkg/repository/repository_test.go index a15addd5b..b051581f0 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.DeleteBackups(ctx, backupID) + err = r.DeleteBackups(ctx, true, 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 4e03dd23c..ea15899e8 100644 --- a/src/pkg/repository/repository_unexported_test.go +++ b/src/pkg/repository/repository_unexported_test.go @@ -373,46 +373,47 @@ func (m *mockBackupGetterModelDeleter) DeleteWithModelStoreIDs( func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() { bup := &backup.Backup{ BaseModel: model.BaseModel{ - ID: model.StableID(uuid.NewString()), - ModelStoreID: manifest.ID(uuid.NewString()), + ID: model.StableID("current-bup-id"), + ModelStoreID: manifest.ID("current-bup-msid"), }, - SnapshotID: uuid.NewString(), - StreamStoreID: uuid.NewString(), + SnapshotID: "current-bup-dsid", + StreamStoreID: "current-bup-ssid", } bupLegacy := &backup.Backup{ BaseModel: model.BaseModel{ - ID: model.StableID(uuid.NewString()), - ModelStoreID: manifest.ID(uuid.NewString()), + ID: model.StableID("legacy-bup-id"), + ModelStoreID: manifest.ID("legacy-bup-msid"), }, - SnapshotID: uuid.NewString(), - DetailsID: uuid.NewString(), + SnapshotID: "legacy-bup-dsid", + DetailsID: "legacy-bup-did", } bupNoSnapshot := &backup.Backup{ BaseModel: model.BaseModel{ - ID: model.StableID(uuid.NewString()), - ModelStoreID: manifest.ID(uuid.NewString()), + ID: model.StableID("ns-bup-id"), + ModelStoreID: manifest.ID("ns-bup-id-msid"), }, - StreamStoreID: uuid.NewString(), + StreamStoreID: "ns-bup-ssid", } bupNoDetails := &backup.Backup{ BaseModel: model.BaseModel{ - ID: model.StableID(uuid.NewString()), - ModelStoreID: manifest.ID(uuid.NewString()), + ID: model.StableID("nssid-bup-id"), + ModelStoreID: manifest.ID("nssid-bup-msid"), }, - SnapshotID: uuid.NewString(), + SnapshotID: "nssid-bup-dsid", } table := []struct { - name string - inputIDs []model.StableID - gets []getRes - expectGets []model.StableID - dels []error - expectDels [][]string - expectErr func(t *testing.T, result error) + name string + inputIDs []model.StableID + gets []getRes + expectGets []model.StableID + dels []error + expectDels [][]string + failOnMissing bool + expectErr func(t *testing.T, result error) }{ { name: "SingleBackup NoError", @@ -450,6 +451,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() { expectGets: []model.StableID{ bup.ID, }, + failOnMissing: true, expectErr: func(t *testing.T, result error) { assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result)) assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result)) @@ -585,7 +587,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() { }, }, { - name: "MultipleBackups GetError", + name: "MultipleBackups GetError FailOnMissing", inputIDs: []model.StableID{ bup.ID, bupLegacy.ID, @@ -604,11 +606,48 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() { bupNoSnapshot.ID, bupNoDetails.ID, }, + failOnMissing: true, expectErr: func(t *testing.T, result error) { assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result)) assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result)) }, }, + { + name: "MultipleBackups GetError NoFailOnMissing", + inputIDs: []model.StableID{ + bup.ID, + bupLegacy.ID, + bupNoSnapshot.ID, + bupNoDetails.ID, + }, + gets: []getRes{ + {bup: bup}, + {err: data.ErrNotFound}, + {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(bupNoSnapshot.ModelStoreID), + bupNoSnapshot.StreamStoreID, + string(bupNoDetails.ModelStoreID), + bupNoDetails.SnapshotID, + }, + }, + expectErr: func(t *testing.T, result error) { + assert.NoError(t, result, clues.ToCore(result)) + }, + }, } for _, test := range table { suite.Run(test.name, func() { @@ -631,7 +670,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() { strIDs = append(strIDs, string(id)) } - err := deleteBackups(ctx, m, strIDs...) + err := deleteBackups(ctx, m, test.failOnMissing, strIDs...) test.expectErr(t, err) }) }