diff --git a/docs/content/about/release-notes.md b/docs/content/about/release-notes.md index 10fca47..4367805 100644 --- a/docs/content/about/release-notes.md +++ b/docs/content/about/release-notes.md @@ -65,6 +65,7 @@ Default sink HTTP clients now have a 10-second whole-request timeout plus bounde * **Behavior change:** The `repl.lag` heartbeat reader now uses `repl-check`; previously it ignored it at every level. With the Blip heartbeat writer and `repl-check` set, an instance that isn't a replica now drops `repl.lag.current`, or reports -1 with `report-not-a-replica=yes`, instead of reporting heartbeat lag. * **Behavior change:** Plans that set the `repl.lag` `writer` to `pfs` at one level and `blip` at another now fail to prepare, and the monitor keeps retrying the plan. Previously each level used its own writer. Set the same `writer` at every level, or set it at one level only. * Changed `heartbeat.BlipReader.Stop` to wait for the reader to return, which takes up to the heartbeat read timeout (2s) if a heartbeat query is running. Previously the reader could report status after its monitor was stopped and removed, so the removed monitor reappeared in [`GET /status/monitors`]({{< ref "/api/status#get-statusmonitors" >}}). +* **Behavior change:** Fixed `monitor-loader.stop-loss` to count the net drop in the number of monitors. Previously it compared the number of removed monitors with the number loaded, so a small removal could trigger stop-loss and removing every monitor could not. A percent stop-loss (for example, `50%`) never triggered; it now triggers when the drop is greater than that percent of the monitors loaded before the reload. ### v2.1.0 (10 Aug 2026) diff --git a/docs/content/config/config-file.md b/docs/content/config/config-file.md index c96376b..3acf17e 100644 --- a/docs/content/config/config-file.md +++ b/docs/content/config/config-file.md @@ -178,7 +178,9 @@ The `local` subsection has only two variables: |**Valid values**|• "N%" (percentage) where N is an integer between 0 and 100 (exclusive)
• "N" where N is an integer greater than 0| |**Default value**|| -The `stop-loss` variable enables the [stop-lost feature]({{< ref "/monitors/loading#stop-loss" >}}). +The `stop-loss` variable enables the [stop-loss feature]({{< ref "/monitors/loading#stop-loss" >}}). +A reload fails if the net drop in the number of monitors is greater than `N` monitors, or greater than `N` percent of the monitors loaded before the reload. +For example, with 10 monitors loaded and `stop-loss: 30%`, a reload can drop 3 monitors but not 4. ##### `stop-timeout` diff --git a/docs/content/monitors/loading.md b/docs/content/monitors/loading.md index 5a72945..e0001aa 100644 --- a/docs/content/monitors/loading.md +++ b/docs/content/monitors/loading.md @@ -54,4 +54,10 @@ Stop-loss prevents reloading from dropping too many MySQL instances due to unrel For example, maybe the `LoadMonitors` plugin has a bug that causes it to return zero monitors when there are really 10 monitors. Stop-loss would prevent Blip from unloading all monitors, which could have adverse side effects for visibility and alerting. +Stop-loss measures the net drop in the number of monitors: the number loaded before the reload minus the number that would be loaded after it. +New monitors offset removed monitors, and changed monitors don't count. +For example, with 10 monitors loaded, a reload that removes 4 monitors and adds 1 drops 3 monitors. +If the drop is greater than the stop-loss limit, the reload fails and the loaded monitors are not changed. +A drop equal to the limit is allowed. + Set [`config.monitor-loader.stop-loss`]({{< ref "/config/config-file#stop-loss" >}}) to enable. diff --git a/monitor/loader.go b/monitor/loader.go index c4f911b..e21be00 100644 --- a/monitor/loader.go +++ b/monitor/loader.go @@ -311,20 +311,30 @@ func (ml *Loader) loadAndUnload(ctx context.Context) ([]*stoppingMonitor, error) // Check config.monitor-loader.stop-loss: don't change monitors if there's // a big drop in the number because it might be a false-positive that will // set off alarms when a bunch of metrics fail to report. - nBefore := float64(len(ml.repo)) - nNow := float64(len(diff.removed)) + // + // The loss is the net drop in the number of monitors: nBefore is the number + // currently loaded, and nNow is the number that would be loaded after this + // change. New monitors offset removed ones; changed monitors are in both + // diff.added and diff.changed, so they don't change the number. Stop-loss + // is a number or a percent, not both (see blip.StopLoss), and the loss must + // be greater than the limit to trigger it. + nBefore := len(ml.repo) + nNow := nBefore - len(diff.removed) + len(diff.added) - len(diff.changed) if nNow < nBefore { + lost := nBefore - nNow var errMsg string - if ml.stopLossPercent > 0 { - lost := (nBefore - nNow) / nBefore - if lost > ml.stopLossPercent { - errMsg = fmt.Sprintf("before: %d; now: %d; lost %f%% > limit %f%%", int(nBefore), int(nNow), lost, ml.stopLossPercent) + switch { + case ml.stopLossNumber > 0: + if uint(lost) > ml.stopLossNumber { + errMsg = fmt.Sprintf("before: %d; now: %d; lost %d > limit %d", nBefore, nNow, lost, ml.stopLossNumber) } - } - if ml.stopLossNumber > 0 { - lost := uint(nBefore - nNow) - if lost > ml.stopLossNumber { - errMsg = fmt.Sprintf("before: %d; now: %d; lost %d > limit %d", int(nBefore), int(nNow), lost, ml.stopLossNumber) + case ml.stopLossPercent > 0: + // stopLossPercent is a whole number (50 for 50%). Compare + // lost*100 > limit*before, not lost/before*100 > limit, so a + // loss exactly at the limit (3 of 10 for 30%) doesn't round + // over it. + if float64(lost*100) > ml.stopLossPercent*float64(nBefore) { + errMsg = fmt.Sprintf("before: %d; now: %d; lost %.1f%% > limit %.0f%%", nBefore, nNow, float64(lost*100)/float64(nBefore), ml.stopLossPercent) } } if errMsg != "" { diff --git a/monitor/loader_counters_test.go b/monitor/loader_counters_test.go index 1c9e546..7045878 100644 --- a/monitor/loader_counters_test.go +++ b/monitor/loader_counters_test.go @@ -154,10 +154,10 @@ func TestLoaderUnloadedMonitorsHaveNoCounters(t *testing.T) { t.Errorf("counters reported for loaded but not started monitors: %v", got) } - // Remove 2 of 4 monitors (stop-loss 1) and add a new one: Load makes the - // new monitor, then returns ErrStopLoss without adding it. + // Remove 3 of 4 monitors and add a new one, a net loss of 2 (stop-loss 1): + // Load makes the new monitor, then returns ErrStopLoss without adding it. mux.Lock() - configs = []blip.ConfigMonitor{configs[0], configs[1], chronosphereMonitor(newId, "1")} + configs = []blip.ConfigMonitor{configs[0], chronosphereMonitor(newId, "1")} mux.Unlock() if err := ml.Load(context.Background()); err != ErrStopLoss { t.Fatalf("Load returned %v, expected ErrStopLoss", err) diff --git a/monitor/loader_stoploss_test.go b/monitor/loader_stoploss_test.go new file mode 100644 index 0000000..2d52871 --- /dev/null +++ b/monitor/loader_stoploss_test.go @@ -0,0 +1,161 @@ +// Copyright 2026 Block, Inc. + +package monitor_test + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/aws" + "github.com/cashapp/blip/v2/dbconn" + "github.com/cashapp/blip/v2/event" + "github.com/cashapp/blip/v2/monitor" + "github.com/cashapp/blip/v2/plan" + "github.com/cashapp/blip/v2/test/mock" +) + +func TestLoaderStopLoss(t *testing.T) { + // Every case loads 10 monitors, then reloads after removing, adding, and + // changing some. Stop-loss is the net drop in the number of monitors: + // removed minus added. Changed monitors don't count. The drop must be + // greater than the limit to trigger stop-loss. + const nBefore = 10 + tests := []struct { + name string + stopLoss string + removed int + added int + changed int + stopped bool // expect ErrStopLoss + msg string // expected monitors-stoploss event message + }{ + {name: "number below", stopLoss: "3", removed: 2}, + {name: "number at", stopLoss: "3", removed: 3}, + {name: "number above", stopLoss: "3", removed: 4, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, + {name: "number all", stopLoss: "3", removed: 10, stopped: true, msg: "before: 10; now: 0; lost 10 > limit 3"}, + {name: "number below with new", stopLoss: "3", removed: 3, added: 1}, + {name: "number at with new", stopLoss: "3", removed: 4, added: 1}, + {name: "number above with new", stopLoss: "3", removed: 5, added: 1, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, + {name: "number replaced", stopLoss: "3", removed: 5, added: 5}, + {name: "number changed", stopLoss: "3", changed: 5}, + {name: "number at with changed", stopLoss: "3", removed: 3, changed: 2}, + {name: "number above with changed", stopLoss: "3", removed: 4, changed: 2, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, + {name: "percent below", stopLoss: "30%", removed: 2}, + {name: "percent at", stopLoss: "30%", removed: 3}, + {name: "percent above", stopLoss: "30%", removed: 4, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, + {name: "percent all", stopLoss: "30%", removed: 10, stopped: true, msg: "before: 10; now: 0; lost 100.0% > limit 30%"}, + {name: "percent below with new", stopLoss: "30%", removed: 3, added: 1}, + {name: "percent at with new", stopLoss: "30%", removed: 4, added: 1}, + {name: "percent above with new", stopLoss: "30%", removed: 5, added: 1, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, + {name: "percent replaced", stopLoss: "30%", removed: 5, added: 5}, + {name: "percent changed", stopLoss: "30%", changed: 5}, + {name: "percent at with changed", stopLoss: "30%", removed: 3, changed: 2}, + {name: "percent above with changed", stopLoss: "30%", removed: 4, changed: 2, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, + {name: "disabled", stopLoss: "", removed: 10}, + } + var evmux sync.Mutex + var stoplossEvents []event.Event + event.Subscribe(mock.EventReceiver{ + RecvFunc: func(e event.Event) { + if e.Event == event.MONITORS_STOPLOSS { + evmux.Lock() + stoplossEvents = append(stoplossEvents, e) + evmux.Unlock() + } + }, + }) + t.Cleanup(event.RemoveSubscribers) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evmux.Lock() + stoplossEvents = nil + evmux.Unlock() + + var mux sync.Mutex + configs := make([]blip.ConfigMonitor, nBefore) + for i := range configs { + configs[i] = stopLossMonitor(fmt.Sprintf("stoploss-%d", i)) + } + ml := stopLossLoader(tt.stopLoss, func() []blip.ConfigMonitor { + mux.Lock() + defer mux.Unlock() + return configs + }) + require.NoError(t, ml.Load(context.Background())) + require.Equal(t, uint(nBefore), ml.Count()) + defer func() { + for _, mon := range ml.Monitors() { + ml.Unload(mon.MonitorId(), true) + } + }() + loaded := map[string]*monitor.Monitor{} + for _, mon := range ml.Monitors() { + loaded[mon.MonitorId()] = mon + } + + // Remove the first monitors, change the next ones, and add new ones. + mux.Lock() + next := append([]blip.ConfigMonitor{}, configs[tt.removed:]...) + for i := 0; i < tt.changed; i++ { + next[i].Password = "changed" + } + for i := 0; i < tt.added; i++ { + next = append(next, stopLossMonitor(fmt.Sprintf("stoploss-new-%d", i))) + } + configs = next + mux.Unlock() + + err := ml.Load(context.Background()) + evmux.Lock() + events := stoplossEvents + evmux.Unlock() + if tt.stopped { + assert.ErrorIs(t, err, monitor.ErrStopLoss) + // No monitor is removed, replaced, or added. + assert.Equal(t, uint(nBefore), ml.Count(), "monitors changed after ErrStopLoss") + for id, mon := range loaded { + assert.Same(t, mon, ml.Monitor(id), "monitor %s replaced after ErrStopLoss", id) + } + if assert.Len(t, events, 1) { + assert.True(t, events[0].Error) + assert.Equal(t, tt.msg, events[0].Message) + } + } else { + assert.NoError(t, err) + assert.Equal(t, uint(nBefore-tt.removed+tt.added), ml.Count()) + assert.Empty(t, events) + } + }) + } +} + +// stopLossLoader returns a Loader with the given stop-loss that loads the +// monitor configs returned by the configs func. The monitors aren't started. +func stopLossLoader(stopLoss string, configs func() []blip.ConfigMonitor) *monitor.Loader { + cfg := blip.DefaultConfig() + cfg.MonitorLoader.StopLoss = stopLoss + return monitor.NewLoader(monitor.LoaderArgs{ + Config: cfg, + Factories: blip.Factories{DbConn: dbconn.NewConnFactory(nil, nil)}, + PlanLoader: plan.NewLoader(nil), + RDSLoader: aws.RDSLoader{ClientFactory: mock.RDSClientFactory{}}, + Plugins: blip.Plugins{ + LoadMonitors: func(blip.Config) ([]blip.ConfigMonitor, error) { return configs(), nil }, + }, + }) +} + +func stopLossMonitor(monitorId string) blip.ConfigMonitor { + return blip.ConfigMonitor{ + MonitorId: monitorId, + Hostname: "127.0.0.1:1", + Sinks: map[string]map[string]string{"noop": {}}, + } +}