From f433876eb409d410604fd1e5576d3cdeb78b299b Mon Sep 17 00:00:00 2001 From: Jussi Maki Date: Wed, 8 May 2024 16:20:56 +0200 Subject: [PATCH] reconciler: Drop out-of-sync metric and changed parameter from Update The "changed *bool" passed to Update() was confusing and it was often not easy or cheap to check whether the target had the expected value or not. Let's err on the side of simplicity and drop the "out-of-sync" metric for the time being. Signed-off-by: Jussi Maki --- reconciler/benchmark/main.go | 5 +---- reconciler/example/ops.go | 18 ++---------------- reconciler/full.go | 12 +----------- reconciler/incremental.go | 2 +- reconciler/metrics.go | 15 ++++----------- reconciler/reconciler_test.go | 8 ++------ reconciler/types.go | 9 +-------- 7 files changed, 12 insertions(+), 57 deletions(-) diff --git a/reconciler/benchmark/main.go b/reconciler/benchmark/main.go index 02a8978..af21ad6 100644 --- a/reconciler/benchmark/main.go +++ b/reconciler/benchmark/main.go @@ -68,11 +68,8 @@ func (mt *mockOps) Prune(ctx context.Context, txn statedb.ReadTxn, iter statedb. } // Update implements reconciler.Operations. -func (mt *mockOps) Update(ctx context.Context, txn statedb.ReadTxn, obj *testObject, changed *bool) error { +func (mt *mockOps) Update(ctx context.Context, txn statedb.ReadTxn, obj *testObject) error { mt.numUpdates.Add(1) - if changed != nil { - *changed = true - } return nil } diff --git a/reconciler/example/ops.go b/reconciler/example/ops.go index 9f77dd2..5eb5902 100644 --- a/reconciler/example/ops.go +++ b/reconciler/example/ops.go @@ -4,7 +4,6 @@ package main import ( - "bytes" "context" "errors" "log/slog" @@ -75,22 +74,9 @@ func (ops *MemoOps) Prune(ctx context.Context, txn statedb.ReadTxn, iter statedb } // Update a memo. -func (ops *MemoOps) Update(ctx context.Context, txn statedb.ReadTxn, memo *Memo, changed *bool) error { +func (ops *MemoOps) Update(ctx context.Context, txn statedb.ReadTxn, memo *Memo) error { filename := path.Join(ops.directory, memo.Name) - - // Read the old file to figure out if it had changed. - // The 'changed' boolean is used by full reconciliation to keep track of when the target - // has gone out-of-sync (e.g. there has been some outside influence to it). - old, err := os.ReadFile(filename) - if err == nil && bytes.Equal(old, []byte(memo.Content)) { - - // Nothing to do. - return nil - } - if changed != nil { - *changed = true - } - err = os.WriteFile(filename, []byte(memo.Content), 0644) + err := os.WriteFile(filename, []byte(memo.Content), 0644) ops.log.Info("Update", "filename", filename, "error", err) return err } diff --git a/reconciler/full.go b/reconciler/full.go index fa37a88..a0bf2a4 100644 --- a/reconciler/full.go +++ b/reconciler/full.go @@ -15,14 +15,12 @@ import ( // Update() is called for each object. Full reconciliation is used to recover from unexpected outside modifications. func (r *reconciler[Obj]) full(ctx context.Context, txn statedb.ReadTxn) []error { var errs []error - outOfSync := false ops := r.Config.Operations // First perform pruning to make room in the target. iter, _ := r.Config.Table.All(txn) start := time.Now() if err := ops.Prune(ctx, txn, iter); err != nil { - outOfSync = true errs = append(errs, fmt.Errorf("pruning failed: %w", err)) } r.metrics.FullReconciliationDuration(r.ModuleID, OpPrune, time.Since(start)) @@ -32,12 +30,10 @@ func (r *reconciler[Obj]) full(ctx context.Context, txn statedb.ReadTxn) []error iter, _ = r.Config.Table.All(txn) // Grab a new iterator as Prune() may have consumed it. for obj, rev, ok := iter.Next(); ok; obj, rev, ok = iter.Next() { start := time.Now() - var changed bool obj = r.Config.CloneObject(obj) - err := ops.Update(ctx, txn, obj, &changed) + err := ops.Update(ctx, txn, obj) r.metrics.FullReconciliationDuration(r.ModuleID, OpUpdate, time.Since(start)) - outOfSync = outOfSync || changed if err == nil { updateResults[obj] = opResult{rev: rev, status: StatusDone()} r.retries.Clear(obj) @@ -47,12 +43,6 @@ func (r *reconciler[Obj]) full(ctx context.Context, txn statedb.ReadTxn) []error } } - // Increment the out-of-sync counter if full reconciliation catched any out-of-sync - // objects. - if outOfSync { - r.metrics.FullReconciliationOutOfSync(r.ModuleID) - } - // Commit the new desired object status. This is performed separately in order // to not lock the table when performing long-running target operations. // If the desired object has been updated in the meanwhile the status update is dropped. diff --git a/reconciler/incremental.go b/reconciler/incremental.go index 7d6c5e1..7e021ba 100644 --- a/reconciler/incremental.go +++ b/reconciler/incremental.go @@ -220,7 +220,7 @@ func (round *incrementalRound[Obj]) processSingle(obj Obj, rev statedb.Revision, // Clone the object so it can be mutated by Update() obj = round.config.CloneObject(obj) op = OpUpdate - err = round.config.Operations.Update(round.ctx, round.txn, obj, nil /* changed */) + err = round.config.Operations.Update(round.ctx, round.txn, obj) if err == nil { round.results[obj] = opResult{rev: rev, status: StatusDone()} } else { diff --git a/reconciler/metrics.go b/reconciler/metrics.go index 8987d6c..c806344 100644 --- a/reconciler/metrics.go +++ b/reconciler/metrics.go @@ -14,7 +14,6 @@ type Metrics interface { IncrementalReconciliationDuration(moduleID cell.FullModuleID, operation string, duration time.Duration) IncrementalReconciliationErrors(moduleID cell.FullModuleID, errs []error) - FullReconciliationOutOfSync(moduleID cell.FullModuleID) FullReconciliationErrors(moduleID cell.FullModuleID, errs []error) FullReconciliationDuration(moduleID cell.FullModuleID, operation string, duration time.Duration) } @@ -31,21 +30,16 @@ type ExpVarMetrics struct { IncrementalReconciliationTotalErrorsVar *expvar.Map IncrementalReconciliationCurrentErrorsVar *expvar.Map - FullReconciliationCountVar *expvar.Map - FullReconciliationDurationVar *expvar.Map - FullReconciliationTotalErrorsVar *expvar.Map - FullReconciliationCurrentErrorsVar *expvar.Map - FullReconciliationOutOfSyncCountVar *expvar.Map + FullReconciliationCountVar *expvar.Map + FullReconciliationDurationVar *expvar.Map + FullReconciliationTotalErrorsVar *expvar.Map + FullReconciliationCurrentErrorsVar *expvar.Map } func (m *ExpVarMetrics) FullReconciliationDuration(moduleID cell.FullModuleID, operation string, duration time.Duration) { m.FullReconciliationDurationVar.AddFloat(moduleID.String()+"/"+operation, duration.Seconds()) } -func (m *ExpVarMetrics) FullReconciliationOutOfSync(moduleID cell.FullModuleID) { - m.FullReconciliationOutOfSyncCountVar.Add(moduleID.String(), 1) -} - func (m *ExpVarMetrics) FullReconciliationErrors(moduleID cell.FullModuleID, errs []error) { m.FullReconciliationCountVar.Add(moduleID.String(), 1) m.FullReconciliationTotalErrorsVar.Add(moduleID.String(), int64(len(errs))) @@ -94,6 +88,5 @@ func newExpVarMetrics(publish bool) *ExpVarMetrics { FullReconciliationDurationVar: newMap("full_reconciliation_duration"), FullReconciliationTotalErrorsVar: newMap("full_reconciliation_total_errors"), FullReconciliationCurrentErrorsVar: newMap("full_reconciliation_current_errors"), - FullReconciliationOutOfSyncCountVar: newMap("full_reconciliation_out_of_sync_count"), } } diff --git a/reconciler/reconciler_test.go b/reconciler/reconciler_test.go index d93f441..629feaf 100644 --- a/reconciler/reconciler_test.go +++ b/reconciler/reconciler_test.go @@ -290,7 +290,6 @@ func testReconciler(t *testing.T, batchOps bool) { } assert.Greater(t, getInt(expVarMetrics.FullReconciliationCountVar.Get("test")), int64(0), "FullReconciliationCount") - assert.Greater(t, getInt(expVarMetrics.FullReconciliationOutOfSyncCountVar.Get("test")), int64(0), "FullReconciliationOutOfSyncCount") assert.Greater(t, getFloat(expVarMetrics.FullReconciliationDurationVar.Get("test/prune")), float64(0), "FullReconciliationDuration/prune") assert.Greater(t, getFloat(expVarMetrics.FullReconciliationDurationVar.Get("test/update")), float64(0), "FullReconciliationDuration/update") assert.Equal(t, getInt(expVarMetrics.FullReconciliationCurrentErrorsVar.Get("test")), int64(0), "FullReconciliationCurrentErrors") @@ -421,7 +420,7 @@ func (mt *mockOps) DeleteBatch(ctx context.Context, txn statedb.ReadTxn, batch [ // UpdateBatch implements reconciler.BatchOperations. func (mt *mockOps) UpdateBatch(ctx context.Context, txn statedb.ReadTxn, batch []reconciler.BatchEntry[*testObject]) { for i := range batch { - batch[i].Result = mt.Update(ctx, txn, batch[i].Object, nil) + batch[i].Result = mt.Update(ctx, txn, batch[i].Object) } } @@ -444,10 +443,7 @@ func (mt *mockOps) Prune(ctx context.Context, txn statedb.ReadTxn, iter statedb. } // Update implements reconciler.Operations. -func (mt *mockOps) Update(ctx context.Context, txn statedb.ReadTxn, obj *testObject, changed *bool) error { - if changed != nil { - *changed = true - } +func (mt *mockOps) Update(ctx context.Context, txn statedb.ReadTxn, obj *testObject) error { mt.updates.incr(obj.id) if mt.faulty.Load() || obj.faulty { mt.history.add(opFail(opUpdate(obj.id))) diff --git a/reconciler/types.go b/reconciler/types.go index 0cb7e81..a897cb0 100644 --- a/reconciler/types.go +++ b/reconciler/types.go @@ -135,16 +135,9 @@ type Operations[Obj any] interface { // reconciliation is performed when the desired state is updated. A full // reconciliation is done periodically by calling 'Update' on all objects. // - // If 'changed' is non-nil then the Update must compare the realized state - // with the desired state and set it to true if they differ, e.g. whether - // the operation resulted in a change to the realized state. This is used - // during full reconciliation to catch cases where the realized state has - // gone out of sync due to outside influence. This is tracked in the - // "full_out_of_sync_total" metric. - // // The object handed to Update is a clone produced by Config.CloneObject // and thus Update can mutate the object. - Update(ctx context.Context, txn statedb.ReadTxn, obj Obj, changed *bool) error + Update(ctx context.Context, txn statedb.ReadTxn, obj Obj) error // Delete the object in the target. Same semantics as with Update. // Deleting a non-existing object is not an error and returns nil.