From 5f036a0cc11d5b87fe79c42e3624a455dffe4a3e Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Fri, 26 Jan 2024 20:04:12 -0800 Subject: [PATCH] Use fault bus and alerts instead of error for config verify (#5122) Switch to using alerts and the fault bus instead of errors. Hopefully this will make it easier to ensure this verify code doesn't fail maintenance overall and that callers can consume the info in a standardized manner --- #### 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 - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) merge after: * #5117 * #5118 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/kopia/conn.go | 102 ++++++++++++++++++++++---------- src/internal/kopia/conn_test.go | 63 ++++++++++++++------ 2 files changed, 118 insertions(+), 47 deletions(-) diff --git a/src/internal/kopia/conn.go b/src/internal/kopia/conn.go index 7e3a06659..f36b263c6 100644 --- a/src/internal/kopia/conn.go +++ b/src/internal/kopia/conn.go @@ -25,11 +25,14 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/kopia/retention" "github.com/alcionai/corso/src/pkg/control/repository" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/storage" ) const ( + corsoWrapperAlertNamespace = "corso-kopia-wrapper" + defaultKopiaConfigDir = "/tmp/" kopiaConfigFileTemplate = "repository-%s.config" defaultCompressor = "zstd-better-compression" @@ -738,59 +741,97 @@ func (w *conn) updatePersistentConfig( OrNil() } -func (w *conn) verifyDefaultPolicyConfigOptions(ctx context.Context) error { - var errs *clues.Err +func (w *conn) verifyDefaultPolicyConfigOptions( + ctx context.Context, + errs *fault.Bus, +) { + const alertName = "kopia-global-policy" globalPol, err := w.getGlobalPolicyOrEmpty(ctx) if err != nil { - return clues.Stack(err) + errs.AddAlert(ctx, fault.NewAlert( + err.Error(), + corsoWrapperAlertNamespace, + "fetch-policy", + alertName, + nil)) + + return } ctx = clues.Add(ctx, "current_global_policy", globalPol.String()) if globalPol.CompressionPolicy.CompressorName != defaultCompressor { - errs = clues.Stack(errs, clues.NewWC( - ctx, - "current compressor doesn't match default"). - With("expected_compression_policy", defaultCompressor)) + errs.AddAlert(ctx, fault.NewAlert( + "unexpected compressor", + corsoWrapperAlertNamespace, + "compressor", + alertName, + nil)) } // Need to use deep equals because the values are pointers to optional types. // That makes regular equality checks fail even if the data contained in each // policy is the same. if !reflect.DeepEqual(globalPol.RetentionPolicy, defaultRetention) { - // Unfortunately the policy has pointers to things and doesn't serialize - // well. This makes it difficult to add the expected retention policy. - errs = clues.Stack(errs, clues.NewWC( - ctx, - "current snapshot retention policy doesn't match default")) + errs.AddAlert(ctx, fault.NewAlert( + "unexpected retention policy", + corsoWrapperAlertNamespace, + "retention-policy", + alertName, + nil)) } if globalPol.SchedulingPolicy.Interval() != defaultSchedulingInterval { - errs = clues.Stack(errs, clues.NewWC( - ctx, - "current scheduling policy doesn't match default"). - With( - "expected_scheduling_policy", defaultSchedulingInterval)) + errs.AddAlert(ctx, fault.NewAlert( + "unexpected scheduling interval", + corsoWrapperAlertNamespace, + "scheduling-interval", + alertName, + nil)) } - - return errs.OrNil() } -func (w *conn) verifyRetentionConfig(ctx context.Context) error { +func (w *conn) verifyRetentionConfig( + ctx context.Context, + errs *fault.Bus, +) { + const alertName = "kopia-object-locking" + directRepo, ok := w.Repository.(repo.DirectRepository) if !ok { - return clues.NewWC(ctx, "getting repo handle") + errs.AddAlert(ctx, fault.NewAlert( + "", + corsoWrapperAlertNamespace, + "fetch-direct-repo", + alertName, + nil)) + + return } blobConfig, maintenanceParams, err := getRetentionConfigs(ctx, directRepo) if err != nil { - return clues.Stack(err) + errs.AddAlert(ctx, fault.NewAlert( + err.Error(), + corsoWrapperAlertNamespace, + "fetch-config", + alertName, + nil)) + + return } - return clues.Stack(retention.OptsFromConfigs( - *blobConfig, - *maintenanceParams).Verify(ctx)).OrNil() + err = retention.OptsFromConfigs(*blobConfig, *maintenanceParams). + Verify(ctx) + if err != nil { + errs.AddAlert(ctx, fault.NewAlert( + err.Error(), + corsoWrapperAlertNamespace, + "config-values", + alertName, + nil)) + } } // verifyDefaultConfigOptions checks the following configurations: @@ -802,9 +843,10 @@ func (w *conn) verifyRetentionConfig(ctx context.Context) error { // object locking: // - maintenance and blob config blob parameters are consistent (i.e. all // enabled or all disabled) -func (w *conn) verifyDefaultConfigOptions(ctx context.Context) error { - errs := clues.Stack(w.verifyDefaultPolicyConfigOptions(ctx)) - errs = clues.Stack(errs, w.verifyRetentionConfig(ctx)) - - return errs.OrNil() +func (w *conn) verifyDefaultConfigOptions( + ctx context.Context, + errs *fault.Bus, +) { + w.verifyDefaultPolicyConfigOptions(ctx, errs) + w.verifyRetentionConfig(ctx, errs) } diff --git a/src/internal/kopia/conn_test.go b/src/internal/kopia/conn_test.go index 152a707d2..4513160a0 100644 --- a/src/internal/kopia/conn_test.go +++ b/src/internal/kopia/conn_test.go @@ -20,6 +20,7 @@ import ( strTD "github.com/alcionai/corso/src/internal/common/str/testdata" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/control/repository" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/storage" storeTD "github.com/alcionai/corso/src/pkg/storage/testdata" ) @@ -788,14 +789,13 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { nonzeroOpt := policy.OptionalInt(42) table := []struct { - name string - setupRepo func(context.Context, *testing.T, *conn) - expectErr assert.ErrorAssertionFunc + name string + setupRepo func(context.Context, *testing.T, *conn) + expectAlerts int }{ { name: "ValidConfigs NoRetention", setupRepo: func(context.Context, *testing.T, *conn) {}, - expectErr: assert.NoError, }, { name: "ValidConfigs Retention", @@ -809,7 +809,6 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { }) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.NoError, }, { name: "ValidRetentionButNotExtending", @@ -823,7 +822,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { }) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "ExtendingRetentionButNotConfigured", @@ -835,7 +834,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { }) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroScheduleInterval", @@ -848,7 +847,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonDefaultCompression", @@ -862,7 +861,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionLatest", @@ -883,7 +882,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionHourly", @@ -904,7 +903,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionWeekly", @@ -925,7 +924,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionDaily", @@ -946,7 +945,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionMonthly", @@ -967,7 +966,7 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, }, { name: "NonZeroSnapshotRetentionAnnual", @@ -988,7 +987,32 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { err = con.writeGlobalPolicy(ctx, "test", pol) require.NoError(t, err, clues.ToCore(err)) }, - expectErr: assert.Error, + expectAlerts: 1, + }, + { + name: "MultipleAlerts", + setupRepo: func(ctx context.Context, t *testing.T, con *conn) { + err := con.setRetentionParameters( + ctx, + repository.Retention{ + Mode: ptr.To(repository.GovernanceRetention), + Duration: ptr.To(48 * time.Hour), + Extend: ptr.To(false), + }) + require.NoError(t, err, clues.ToCore(err)) + + pol, err := con.getGlobalPolicyOrEmpty(ctx) + require.NoError(t, err, clues.ToCore(err)) + + updateSchedulingOnPolicy(time.Hour, pol) + + _, err = updateCompressionOnPolicy("pgzip-best-speed", pol) + require.NoError(t, err, clues.ToCore(err)) + + err = con.writeGlobalPolicy(ctx, "test", pol) + require.NoError(t, err, clues.ToCore(err)) + }, + expectAlerts: 3, }, } @@ -1010,8 +1034,13 @@ func (suite *ConnRetentionIntegrationSuite) TestVerifyDefaultConfigOptions() { test.setupRepo(ctx, t, con) - err = con.verifyDefaultConfigOptions(ctx) - test.expectErr(t, err, clues.ToCore(err)) + errs := fault.New(true) + con.verifyDefaultConfigOptions(ctx, errs) + + // There shouldn't be any reported failures because this is just to check + // if things are alright. + assert.NoError(t, errs.Failure(), clues.ToCore(errs.Failure())) + assert.Len(t, errs.Alerts(), test.expectAlerts) }) } }