From e88553d073d9073ad211078c1c9901f9982f068a Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Tue, 10 Jan 2023 12:32:17 -0800 Subject: [PATCH] Refactor the Reason struct and how tags for lookups are generated (#2087) ## Description Solidify the interface between BackupOp and KopiaWrapper by making Reason the de facto way to pass/generate tags for things. The Reason struct now includes a function to generate tags for that instance. KopiaWrapper also now hides the fact that it prefixes tags from other components ## 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: Test - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup ## Issue(s) * #1916 ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/kopia/snapshot_manager.go | 38 ++++++++---- src/internal/kopia/snapshot_manager_test.go | 15 ++--- src/internal/kopia/wrapper.go | 2 +- src/internal/kopia/wrapper_test.go | 58 ++++++++++++++----- src/internal/operations/backup.go | 21 +++---- .../operations/backup_integration_test.go | 8 ++- src/internal/operations/backup_test.go | 10 +++- 7 files changed, 101 insertions(+), 51 deletions(-) diff --git a/src/internal/kopia/snapshot_manager.go b/src/internal/kopia/snapshot_manager.go index fa2cb78b1..55cf7b8a6 100644 --- a/src/internal/kopia/snapshot_manager.go +++ b/src/internal/kopia/snapshot_manager.go @@ -32,6 +32,13 @@ type Reason struct { Category path.CategoryType } +func (r Reason) TagKeys() []string { + return []string{ + r.ResourceOwner, + serviceCatString(r.Service, r.Category), + } +} + type ManifestEntry struct { *snapshot.Manifest // Reason contains the ResourceOwners and Service/Categories that caused this @@ -44,6 +51,13 @@ type ManifestEntry struct { Reasons []Reason } +func (me ManifestEntry) GetTag(key string) (string, bool) { + k, _ := makeTagKV(key) + v, ok := me.Tags[k] + + return v, ok +} + type snapshotManager interface { FindManifests( ctx context.Context, @@ -68,6 +82,10 @@ func MakeServiceCat(s path.ServiceType, c path.CategoryType) (string, ServiceCat return serviceCatString(s, c), ServiceCat{s, c} } +// TODO(ashmrtn): Remove in a future PR. +// +//nolint:unused +//lint:ignore U1000 will be removed in future PR. func serviceCatTag(p path.Path) string { return serviceCatString(p.Service(), p.Category()) } @@ -82,7 +100,7 @@ func serviceCatString(s path.ServiceType, c path.CategoryType) string { // Returns the normalized Key plus a default value. If you're embedding a // key-only tag, the returned default value msut be used instead of an // empty string. -func MakeTagKV(k string) (string, string) { +func makeTagKV(k string) (string, string) { return userTagPrefix + k, defaultTagValue } @@ -101,12 +119,12 @@ func tagsFromStrings(oc *OwnersCats) map[string]string { res := make(map[string]string, len(oc.ServiceCats)+len(oc.ResourceOwners)) for k := range oc.ServiceCats { - tk, tv := MakeTagKV(k) + tk, tv := makeTagKV(k) res[tk] = tv } for k := range oc.ResourceOwners { - tk, tv := MakeTagKV(k) + tk, tv := makeTagKV(k) res[tk] = tv } @@ -195,14 +213,14 @@ func fetchPrevManifests( reason Reason, tags map[string]string, ) ([]*ManifestEntry, error) { - tags = normalizeTagKVs(tags) - serviceCatKey, _ := MakeServiceCat(reason.Service, reason.Category) - allTags := normalizeTagKVs(map[string]string{ - serviceCatKey: "", - reason.ResourceOwner: "", - }) + allTags := map[string]string{} + + for _, k := range reason.TagKeys() { + allTags[k] = "" + } maps.Copy(allTags, tags) + allTags = normalizeTagKVs(allTags) metas, err := sm.FindManifests(ctx, allTags) if err != nil { @@ -335,7 +353,7 @@ func normalizeTagKVs(tags map[string]string) map[string]string { t2 := make(map[string]string, len(tags)) for k, v := range tags { - mk, mv := MakeTagKV(k) + mk, mv := makeTagKV(k) if len(v) == 0 { v = mv diff --git a/src/internal/kopia/snapshot_manager_test.go b/src/internal/kopia/snapshot_manager_test.go index 324f471ad..31c06ba33 100644 --- a/src/internal/kopia/snapshot_manager_test.go +++ b/src/internal/kopia/snapshot_manager_test.go @@ -29,16 +29,9 @@ var ( testID2 = manifest.ID("snap2") testID3 = manifest.ID("snap3") - testMail = path.ExchangeService.String() + path.EmailCategory.String() - testMailServiceCat = ServiceCat{ - Service: path.ExchangeService, - Category: path.EmailCategory, - } - testEvents = path.ExchangeService.String() + path.EventsCategory.String() - testEventsServiceCat = ServiceCat{ - Service: path.ExchangeService, - Category: path.EventsCategory, - } + testMail = path.ExchangeService.String() + path.EmailCategory.String() + testEvents = path.ExchangeService.String() + path.EventsCategory.String() + testUser1 = "user1" testUser2 = "user2" testUser3 = "user3" @@ -115,7 +108,7 @@ func newManifestInfo( structTags := make(map[string]struct{}, len(tags)) for _, t := range tags { - tk, _ := MakeTagKV(t) + tk, _ := makeTagKV(t) structTags[tk] = struct{}{} } diff --git a/src/internal/kopia/wrapper.go b/src/internal/kopia/wrapper.go index ab0331ff8..3e8f5b338 100644 --- a/src/internal/kopia/wrapper.go +++ b/src/internal/kopia/wrapper.go @@ -230,7 +230,7 @@ func (w Wrapper) makeSnapshotWithRoot( man.Tags = map[string]string{} for k, v := range addlTags { - mk, mv := MakeTagKV(k) + mk, mv := makeTagKV(k) if len(v) == 0 { v = mv diff --git a/src/internal/kopia/wrapper_test.go b/src/internal/kopia/wrapper_test.go index 947b4439e..07fa78567 100644 --- a/src/internal/kopia/wrapper_test.go +++ b/src/internal/kopia/wrapper_test.go @@ -212,11 +212,25 @@ func (suite *KopiaIntegrationSuite) TestBackupCollections() { tags := map[string]string{ "fnords": "smarf", "brunhilda": "", + } - suite.testPath1.ResourceOwner(): "", - suite.testPath2.ResourceOwner(): "", - serviceCatTag(suite.testPath1): "", - serviceCatTag(suite.testPath2): "", + reasons := []Reason{ + { + ResourceOwner: suite.testPath1.ResourceOwner(), + Service: suite.testPath1.Service(), + Category: suite.testPath1.Category(), + }, + { + ResourceOwner: suite.testPath2.ResourceOwner(), + Service: suite.testPath2.Service(), + Category: suite.testPath2.Category(), + }, + } + + for _, r := range reasons { + for _, k := range r.TagKeys() { + tags[k] = "" + } } expectedTags := map[string]string{} @@ -305,9 +319,15 @@ func (suite *KopiaIntegrationSuite) TestRestoreAfterCompressionChange() { w := &Wrapper{k} - tags := map[string]string{ - testUser: "", - serviceCatString(path.ExchangeService, path.EmailCategory): "", + tags := map[string]string{} + reason := Reason{ + ResourceOwner: testUser, + Service: path.ExchangeService, + Category: path.EmailCategory, + } + + for _, k := range reason.TagKeys() { + tags[k] = "" } dc1 := mockconnector.NewMockExchangeCollection(suite.testPath1, 1) @@ -353,9 +373,15 @@ func (suite *KopiaIntegrationSuite) TestRestoreAfterCompressionChange() { func (suite *KopiaIntegrationSuite) TestBackupCollections_ReaderError() { t := suite.T() - tags := map[string]string{ - testUser: "", - serviceCatString(path.ExchangeService, path.EmailCategory): "", + tags := map[string]string{} + reason := Reason{ + ResourceOwner: testUser, + Service: path.ExchangeService, + Category: path.EmailCategory, + } + + for _, k := range reason.TagKeys() { + tags[k] = "" } collections := []data.Collection{ @@ -586,9 +612,15 @@ func (suite *KopiaSimpleRepoIntegrationSuite) SetupTest() { collections = append(collections, collection) } - tags := map[string]string{ - testUser: "", - serviceCatString(path.ExchangeService, path.EmailCategory): "", + tags := map[string]string{} + reason := Reason{ + ResourceOwner: testUser, + Service: path.ExchangeService, + Category: path.EmailCategory, + } + + for _, k := range reason.TagKeys() { + tags[k] = "" } stats, deets, _, err := suite.w.BackupCollections( diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index 1c6f790e3..856335a85 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -344,10 +344,8 @@ func produceManifestsAndMetadata( continue } - tk, _ := kopia.MakeTagKV(kopia.TagBackupID) - - bID := man.Tags[tk] - if len(bID) == 0 { + bID, ok := man.GetTag(kopia.TagBackupID) + if !ok { return nil, nil, false, errors.New("snapshot manifest missing backup ID") } @@ -502,12 +500,9 @@ func consumeBackupDataCollections( } for _, reason := range reasons { - tags[reason.ResourceOwner] = "" - - // TODO(ashmrtn): Create a separate helper function to go from service/cat - // to a tag. - serviceCat, _ := kopia.MakeServiceCat(reason.Service, reason.Category) - tags[serviceCat] = "" + for _, k := range reason.TagKeys() { + tags[k] = "" + } } bases := make([]kopia.IncrementalBase, 0, len(mans)) @@ -568,8 +563,10 @@ func mergeDetails( continue } - k, _ := kopia.MakeTagKV(kopia.TagBackupID) - bID := man.Tags[k] + bID, ok := man.GetTag(kopia.TagBackupID) + if !ok { + return errors.Errorf("no backup ID in snapshot manifest with ID %s", man.ID) + } _, baseDeets, err := getBackupAndDetailsFromID( ctx, diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index 6df6da829..5876cd331 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -191,8 +191,12 @@ func checkBackupIsInManifests( require.NoError(t, err) for _, man := range mans { - tk, _ := kopia.MakeTagKV(kopia.TagBackupID) - if man.Tags[tk] == string(bo.Results.BackupID) { + bID, ok := man.GetTag(kopia.TagBackupID) + if !assert.Truef(t, ok, "snapshot manifest %s missing backup ID tag", man.ID) { + continue + } + + if bID == string(bo.Results.BackupID) { found = true break } diff --git a/src/internal/operations/backup_test.go b/src/internal/operations/backup_test.go index e858a319e..90c5aa50e 100644 --- a/src/internal/operations/backup_test.go +++ b/src/internal/operations/backup_test.go @@ -276,14 +276,20 @@ func makeDetailsEntry( return res } +// TODO(ashmrtn): This should belong to some code that lives in the kopia +// package that is only compiled when running tests. +func makeKopiaTagKey(k string) string { + return "tag:" + k +} + func makeManifest(t *testing.T, backupID model.StableID, incompleteReason string) *snapshot.Manifest { t.Helper() - backupIDTagKey, _ := kopia.MakeTagKV(kopia.TagBackupID) + tagKey := makeKopiaTagKey(kopia.TagBackupID) return &snapshot.Manifest{ Tags: map[string]string{ - backupIDTagKey: string(backupID), + tagKey: string(backupID), }, IncompleteReason: incompleteReason, }