separate fault bus in fetch manifests (#3090)

The global aggregation of recoverable errors
causes us to mark operations as failed even in
conditions where they should have succeeded,
because higher level callers don't have sufficient
control over ignorable error conditions.  This
is a short-term fix that allows us to log and dump
fault error accumulation in a specific, scoped instance.

---

#### Does this PR need a docs update or release note?

- [x]  No

#### Type of change

- [x] 🐛 Bugfix

#### Test Plan

- [x]  Unit test
- [x] 💚 E2E
This commit is contained in:
Keepers 2023-04-11 19:16:40 -06:00 committed by GitHub
parent 30c86797d3
commit 1f8c7598b4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 19 additions and 9 deletions

View File

@ -245,8 +245,7 @@ func (op *BackupOperation) do(
op.store,
reasons, fallbackReasons,
op.account.ID(),
op.incremental,
op.Errors)
op.incremental)
if err != nil {
return nil, clues.Wrap(err, "producing manifests and metadata")
}

View File

@ -46,7 +46,6 @@ func produceManifestsAndMetadata(
reasons, fallbackReasons []kopia.Reason,
tenantID string,
getMetadata bool,
errs *fault.Bus,
) ([]*kopia.ManifestEntry, []data.RestoreCollection, bool, error) {
var (
tags = map[string]string{kopia.TagBackupCategory: ""}
@ -151,7 +150,17 @@ func produceManifestsAndMetadata(
return ms, nil, false, nil
}
colls, err := collectMetadata(mctx, mr, man, metadataFiles, tenantID, errs)
// a local fault.Bus intance is used to collect metadata files here.
// we avoid the global fault.Bus because all failures here are ignorable,
// and cascading errors up to the operation can cause a conflict that forces
// the operation into a failure state unnecessarily.
// TODO(keepers): this is not a pattern we want to
// spread around. Need to find more idiomatic handling.
fb := fault.New(true)
colls, err := collectMetadata(mctx, mr, man, metadataFiles, tenantID, fb)
LogFaultErrors(ctx, fb.Errors(), "collecting metadata")
if err != nil && !errors.Is(err, data.ErrNotFound) {
// prior metadata isn't guaranteed to exist.
// if it doesn't, we'll just have to do a
@ -162,7 +171,11 @@ func produceManifestsAndMetadata(
collections = append(collections, colls...)
}
return ms, collections, true, err
if err != nil {
return nil, nil, false, err
}
return ms, collections, true, nil
}
// unionManifests reduces the two manifest slices into a single slice.

View File

@ -678,8 +678,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
&test.gb,
test.reasons, nil,
tid,
test.getMeta,
fault.New(true))
test.getMeta)
test.assertErr(t, err, clues.ToCore(err))
test.assertB(t, b)
@ -844,8 +843,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
[]kopia.Reason{manReason},
[]kopia.Reason{fbReason},
"tid",
false,
fault.New(true))
false)
require.NoError(t, err, clues.ToCore(err))
assert.False(t, b, "no-metadata is forced for this test")