From dcffc61bf5a76f69bb27aa6cb8fffc7d3554a5ac Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 22 Sep 2023 10:39:16 -0600 Subject: [PATCH] do not mark not-found items as skipped (#4342) This was an accidental mis-use of the skipped item pattern. Items deleted during in flight during backup due to race conditions do not count as permanent skips in the way that other skipped items do. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :broom: Tech Debt/Cleanup #### Issue(s) * closes #4044 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- .../m365/collection/drive/collection.go | 4 +--- src/internal/stats/stats.go | 1 - src/pkg/backup/backup.go | 12 ++---------- src/pkg/backup/backup_test.go | 19 +++---------------- src/pkg/fault/item.go | 4 ---- src/pkg/services/m365/api/mail.go | 4 ++-- 6 files changed, 8 insertions(+), 36 deletions(-) diff --git a/src/internal/m365/collection/drive/collection.go b/src/internal/m365/collection/drive/collection.go index ecf254f80..a69e771a6 100644 --- a/src/internal/m365/collection/drive/collection.go +++ b/src/internal/m365/collection/drive/collection.go @@ -282,9 +282,7 @@ func (oc *Collection) getDriveItemContent( } if clues.HasLabel(err, graph.LabelStatus(http.StatusNotFound)) || graph.IsErrDeletedInFlight(err) { - logger.CtxErr(ctx, err).With("skipped_reason", fault.SkipNotFound).Info("item not found") - errs.AddSkip(ctx, fault.FileSkip(fault.SkipNotFound, driveID, itemID, itemName, graph.ItemInfo(item))) - + logger.CtxErr(ctx, err).Info("item not found, probably deleted in flight") return nil, clues.Wrap(err, "deleted item").Label(graph.LabelsSkippable) } diff --git a/src/internal/stats/stats.go b/src/internal/stats/stats.go index 88f54b992..69f71d65a 100644 --- a/src/internal/stats/stats.go +++ b/src/internal/stats/stats.go @@ -34,6 +34,5 @@ func (bc *ByteCounter) Count(i int64) { type SkippedCounts struct { TotalSkippedItems int `json:"totalSkippedItems"` SkippedMalware int `json:"skippedMalware"` - SkippedNotFound int `json:"skippedNotFound"` SkippedInvalidOneNoteFile int `json:"skippedInvalidOneNoteFile"` } diff --git a/src/pkg/backup/backup.go b/src/pkg/backup/backup.go index f9bee844b..fe741d798 100644 --- a/src/pkg/backup/backup.go +++ b/src/pkg/backup/backup.go @@ -90,8 +90,7 @@ func New( skipCount = len(fe.Skipped) failMsg string - malware, notFound, - invalidONFile, otherSkips int + malware, invalidONFile, otherSkips int ) if fe.Failure != nil { @@ -103,8 +102,6 @@ func New( switch true { case s.HasCause(fault.SkipMalware): malware++ - case s.HasCause(fault.SkipNotFound): - notFound++ case s.HasCause(fault.SkipBigOneNote): invalidONFile++ default: @@ -139,7 +136,6 @@ func New( SkippedCounts: stats.SkippedCounts{ TotalSkippedItems: skipCount, SkippedMalware: malware, - SkippedNotFound: notFound, SkippedInvalidOneNoteFile: invalidONFile, }, } @@ -234,7 +230,7 @@ func (b Backup) Values() []string { if b.TotalSkippedItems > 0 { status += fmt.Sprintf("%d skipped", b.TotalSkippedItems) - if b.SkippedMalware+b.SkippedNotFound+b.SkippedInvalidOneNoteFile > 0 { + if b.SkippedMalware+b.SkippedInvalidOneNoteFile > 0 { status += ": " } } @@ -245,10 +241,6 @@ func (b Backup) Values() []string { skipped = append(skipped, fmt.Sprintf("%d malware", b.SkippedMalware)) } - if b.SkippedNotFound > 0 { - skipped = append(skipped, fmt.Sprintf("%d not found", b.SkippedNotFound)) - } - if b.SkippedInvalidOneNoteFile > 0 { skipped = append(skipped, fmt.Sprintf("%d invalid OneNote file", b.SkippedInvalidOneNoteFile)) } diff --git a/src/pkg/backup/backup_test.go b/src/pkg/backup/backup_test.go index 6cb009b3a..c1ce6f1cf 100644 --- a/src/pkg/backup/backup_test.go +++ b/src/pkg/backup/backup_test.go @@ -166,17 +166,6 @@ func (suite *BackupUnitSuite) TestBackup_Values_statusVariations() { }, expect: "test (2 skipped: 1 malware)", }, - { - name: "not found", - bup: backup.Backup{ - Status: "test", - SkippedCounts: stats.SkippedCounts{ - TotalSkippedItems: 2, - SkippedNotFound: 1, - }, - }, - expect: "test (2 skipped: 1 not found)", - }, { name: "errors and malware", bup: backup.Backup{ @@ -190,16 +179,15 @@ func (suite *BackupUnitSuite) TestBackup_Values_statusVariations() { expect: "test (42 errors, 1 skipped: 1 malware)", }, { - name: "errors and not found", + name: "errors and skipped", bup: backup.Backup{ Status: "test", ErrorCount: 42, SkippedCounts: stats.SkippedCounts{ TotalSkippedItems: 1, - SkippedNotFound: 1, }, }, - expect: "test (42 errors, 1 skipped: 1 not found)", + expect: "test (42 errors, 1 skipped)", }, { name: "errors and invalid OneNote", @@ -221,11 +209,10 @@ func (suite *BackupUnitSuite) TestBackup_Values_statusVariations() { SkippedCounts: stats.SkippedCounts{ TotalSkippedItems: 1, SkippedMalware: 1, - SkippedNotFound: 1, SkippedInvalidOneNoteFile: 1, }, }, - expect: "test (42 errors, 1 skipped: 1 malware, 1 not found, 1 invalid OneNote file)", + expect: "test (42 errors, 1 skipped: 1 malware, 1 invalid OneNote file)", }, } for _, test := range table { diff --git a/src/pkg/fault/item.go b/src/pkg/fault/item.go index 8b5f8c929..166a914a7 100644 --- a/src/pkg/fault/item.go +++ b/src/pkg/fault/item.go @@ -167,10 +167,6 @@ const ( // permanently fail any attempts to backup or restore. SkipMalware skipCause = "malware_detected" - // SkipNotFound identifies that a file was skipped because we could - // not find it when trying to download contents - SkipNotFound skipCause = "file_not_found" - // SkipBigOneNote identifies that a file was skipped because it // was big OneNote file and we can only download OneNote files which // are less that 2GB in size. diff --git a/src/pkg/services/m365/api/mail.go b/src/pkg/services/m365/api/mail.go index 0064da90b..63c3684dd 100644 --- a/src/pkg/services/m365/api/mail.go +++ b/src/pkg/services/m365/api/mail.go @@ -344,9 +344,9 @@ func (c Mail) GetItem( if graph.IsErrCannotOpenFileAttachment(err) { logger.CtxErr(ctx, err). With( - "skipped_reason", fault.SkipNotFound, "attachment_id", ptr.Val(a.GetId()), - "attachment_size", ptr.Val(a.GetSize())).Info("attachment not found") + "attachment_size", ptr.Val(a.GetSize())). + Info("attachment not found") // TODO This should use a `AddSkip` once we have // figured out the semantics for skipping // subcomponents of an item