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