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) }) }