From 1bacad72aa71ea3b1c39cedb35f40850b626047b Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Mon, 18 Sep 2023 13:14:03 -0700 Subject: [PATCH] Minor test expansion for failed backup cleanup (#4244) Add or update tests to ensure garbage collection works (or at least behaves reasonably and in a known fashion) if: * the youngest assist base has a merge base older than it * there's a legacy backup model merge base younger than the assist base * there's legacy and current backup model merge bases younger than the assist base --- #### 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 - [x] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * #3217 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/kopia/cleanup_backups.go | 8 ++ src/internal/kopia/cleanup_backups_test.go | 105 ++++++++++++++++++--- 2 files changed, 102 insertions(+), 11 deletions(-) diff --git a/src/internal/kopia/cleanup_backups.go b/src/internal/kopia/cleanup_backups.go index 2e3cd7da9..03a642024 100644 --- a/src/internal/kopia/cleanup_backups.go +++ b/src/internal/kopia/cleanup_backups.go @@ -287,6 +287,14 @@ var skipKeys = []string{ TagBackupCategory, } +// transferTags copies (ideally) service and category tags from a snapshot to a +// backup tag set to make it easier for other code to determine what a backup +// contains. +// +// This function will improperly transfer the protected resource if it's an +// older backup that used an alternative to the ProtectedResouceID field or the +// protected resource ID field in the backup doesn't match the protected +// resource tag on the snapshot. func transferTags(snap *manifest.EntryMetadata, bup *backup.Backup) error { tenant, err := decodeElement(snap.Labels[kopiaPathLabel]) if err != nil { diff --git a/src/internal/kopia/cleanup_backups_test.go b/src/internal/kopia/cleanup_backups_test.go index d82f21338..6407485a5 100644 --- a/src/internal/kopia/cleanup_backups_test.go +++ b/src/internal/kopia/cleanup_backups_test.go @@ -355,14 +355,24 @@ func (suite *BackupCleanupUnitSuite) TestCleanupOrphanedData() { res := *b res.ProtectedResourceID = protectedResource + t := model.MergeBackup if isAssist { - if res.Tags == nil { - res.Tags = map[string]string{} - } - - res.Tags[model.BackupTypeTag] = model.AssistBackup + t = model.AssistBackup } + if res.Tags == nil { + res.Tags = map[string]string{} + } + + res.Tags[model.BackupTypeTag] = t + + return &res + } + + backupWithLegacyResource := func(protectedResource string, b *backup.Backup) *backup.Backup { + res := *b + res.ResourceOwnerID = protectedResource + return &res } @@ -657,8 +667,10 @@ func (suite *BackupCleanupUnitSuite) TestCleanupOrphanedData() { }, { // Test that an assist base that has the same Reasons as a newer assist - // base is garbage collected when it's outside the buffer period. - name: "AssistBases NotYoungest CausesCleanup", + // base is garbage collected when it's outside the buffer period. Also + // check the most recent assist base isn't garbage collected if it's + // younger than the most recent merge base. + name: "AssistAndMergeBases MixedAges CausesCleanup", snapshots: []*manifest.EntryMetadata{ manifestWithReasons( manifestWithTime(baseTime, snapCurrent()), @@ -680,21 +692,92 @@ func (suite *BackupCleanupUnitSuite) TestCleanupOrphanedData() { }, backups: []backupRes{ {bup: backupWithResource("ro", true, backupWithTime(baseTime, bupCurrent()))}, - {bup: backupWithResource("ro", true, backupWithTime(baseTime.Add(time.Second), bupCurrent2()))}, + {bup: backupWithResource("ro", false, backupWithTime(baseTime.Add(time.Second), bupCurrent2()))}, {bup: backupWithResource("ro", true, backupWithTime(baseTime.Add(time.Minute), bupCurrent3()))}, }, expectDeleteIDs: []manifest.ID{ snapCurrent().ID, deetsCurrent().ID, manifest.ID(bupCurrent().ModelStoreID), - snapCurrent2().ID, - deetsCurrent2().ID, - manifest.ID(bupCurrent2().ModelStoreID), }, time: baseTime.Add(48 * time.Hour), buffer: 24 * time.Hour, expectErr: assert.NoError, }, + { + // Test that an assist base that has the same Reasons as a newer merge + // base but the merge base is from an older version of corso for some + // reason doesn't cause the assist base to be garbage collected. This is + // not ideal, but some older versions of corso didn't even populate the + // resource owner ID. + // + // Worst case, the assist base will be cleaned up when the user upgrades + // corso and generates either a new assist base or merge base with the + // same reason. + name: "AssistAndLegacyMergeBases NotYoungest Noops", + snapshots: []*manifest.EntryMetadata{ + manifestWithReasons( + manifestWithTime(baseTime, snapCurrent()), + "tenant1", + NewReason("", "ro", path.ExchangeService, path.EmailCategory)), + manifestWithTime(baseTime, deetsCurrent()), + + manifestWithReasons( + manifestWithTime(baseTime.Add(time.Second), snapCurrent2()), + "tenant1", + NewReason("", "ro", path.ExchangeService, path.EmailCategory)), + manifestWithTime(baseTime.Add(time.Second), deetsCurrent2()), + }, + backups: []backupRes{ + {bup: backupWithResource("ro", true, backupWithTime(baseTime, bupCurrent()))}, + {bup: backupWithLegacyResource("ro", backupWithTime(baseTime.Add(time.Second), bupCurrent2()))}, + }, + time: baseTime.Add(48 * time.Hour), + buffer: 24 * time.Hour, + expectErr: assert.NoError, + }, + { + // Test that an assist base that has the same Reasons as a newer merge + // base but the merge base is from an older version of corso for some + // reason and an even newer merge base from a current version of corso + // causes the assist base to be garbage collected. + // + // This also tests that bases without a merge or assist tag are not + // garbage collected as an assist base. + name: "AssistAndLegacyAndCurrentMergeBases NotYoungest CausesCleanup", + snapshots: []*manifest.EntryMetadata{ + manifestWithReasons( + manifestWithTime(baseTime, snapCurrent()), + "tenant1", + NewReason("", "ro", path.ExchangeService, path.EmailCategory)), + manifestWithTime(baseTime, deetsCurrent()), + + manifestWithReasons( + manifestWithTime(baseTime.Add(time.Second), snapCurrent2()), + "tenant1", + NewReason("", "ro", path.ExchangeService, path.EmailCategory)), + manifestWithTime(baseTime.Add(time.Second), deetsCurrent2()), + + manifestWithReasons( + manifestWithTime(baseTime.Add(time.Minute), snapCurrent3()), + "tenant1", + NewReason("", "ro", path.ExchangeService, path.EmailCategory)), + manifestWithTime(baseTime.Add(time.Minute), deetsCurrent3()), + }, + backups: []backupRes{ + {bup: backupWithResource("ro", true, backupWithTime(baseTime, bupCurrent()))}, + {bup: backupWithLegacyResource("ro", backupWithTime(baseTime.Add(time.Second), bupCurrent2()))}, + {bup: backupWithResource("ro", false, backupWithTime(baseTime.Add(time.Minute), bupCurrent3()))}, + }, + time: baseTime.Add(48 * time.Hour), + buffer: 24 * time.Hour, + expectDeleteIDs: []manifest.ID{ + snapCurrent().ID, + deetsCurrent().ID, + manifest.ID(bupCurrent().ModelStoreID), + }, + expectErr: assert.NoError, + }, { // Test that the most recent assist base is garbage collected if there's a // newer merge base that has the same Reasons as the assist base.