Skip to content

fix(monitor): refresh cluster status on shutdown, run housekeeping off the main loop - #1503

Merged
wmousa merged 2 commits into
R26.3from
fix/r263-monitor-status-latency
Oct 19, 2026
Merged

wmousa merged 2 commits into
R26.3from
fix/r263-monitor-status-latency

Conversation

@wmousa

@wmousa wmousa commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Problem

On a customer cluster (2026-10-19, mgmt just upgraded R26.2-PRE → 26.3.2), two storage nodes on one host were shut down with sbctl sn shutdown. The node went OFFLINE at 15:42:46, but the cluster only changed from ACTIVE to DEGRADED at 15:49:47. For seven minutes it reported full redundancy while one host was down.

Two causes:

  1. The shutdown never refreshes the cluster status. shutdown_storage_node sets the node to OFFLINE itself. The monitor then finds the node already OFFLINE, so set_node_offline never runs, and that is the only per-node path that calls update_cluster_status. The status only changes on the monitor's next main-loop tick.

  2. The main-loop tick was blocked by retention housekeeping. _run_periodic_housekeeping ran in the main loop between update_cluster_status calls. Each sweep:

    • read the whole events table with db.get_events() (every cluster, unfiltered);
    • cleared one key per expired event.

    R26.2-PRE never pruned events, so the first sweeps after the upgrade deleted months of them, and the cluster status went unrefreshed for that whole time.

Change

  • shutdown_storage_node: once the node is OFFLINE, calls the new apply_node_loss_status(cluster_id) (monitor module). It applies only the downgrades a node loss can cause: ACTIVE → DEGRADED/SUSPENDED and DEGRADED → SUSPENDED. It uses the same set_cluster_status the monitor makes for those transitions. It deliberately does not run update_cluster_status, the monitor's full pass, because on a SUSPENDED cluster that pass calls cluster_activate, and a CLI shutdown must not start an activation or wait on one. Recovery transitions stay with the monitor. This is best effort: a failure is logged with a traceback and the shutdown still succeeds.
  • Housekeeping thread: housekeeping now runs on its own daemon thread (_housekeeping_loop, every HOUSEKEEPING_INTERVAL_SEC, all clusters). The main loop only refreshes the status.
  • _delete_old_logs(cluster_id): event keys are <cluster_uuid>/<date ms>/<uuid>, so one cluster's expired events form a single contiguous key range. They are now removed with one clear_range(<cluster>/, <cluster>/<cutoff ms>), with no table read and a single mutation however large the backlog is.

Integration CI failure (fixed)

test_cluster_status_is_refreshed_once_offline failed on R26.3 (py3.9) with 0 refreshes recorded.

  • Cause: test_secondary_promotion.py::test_escalates_unreachable_primary assigned a MagicMock to sys.modules["simplyblock_core.services.storage_node_monitor"] and never restored it.
  • Why 3.9 only: on Python 3.9, mock.patch resolves its target through package attributes, so the test patched the real module while the lazy import in shutdown_storage_node got the leaked mock. On 3.11+, patch resolves through sys.modules, which is why main passed.
  • Fix: the leaking test now uses patch.dict(sys.modules, ...), which restores the real module.
  • Reproduced locally: with a 3.9-style patch resolver in the same test order, the test fails without the fix and passes with it.

Tests

  • tests/unit/test_ucs_debounce_bound.py:
    • apply_node_loss_status applies node-loss downgrades only: never a recovery step, and no verdict at all for SUSPENDED / IN_ACTIVATION / READONLY / IN_EXPANSION / UNREADY;
    • the main loop does no housekeeping and starts the housekeeping thread;
    • the housekeeping loop sweeps every cluster;
    • _delete_old_logs clears only this cluster's expired events, keeps recent events and other clusters' events, does not read the events table, and cl-1 does not reach into cl-10.
  • tests/integration/test_shutdown_no_suspension.py:
    • the status is refreshed exactly once, after OFFLINE, on both the graceful and the forced path;
    • a failing refresh does not fail the shutdown.
  • The 5 new behavioural tests fail without the production change.
  • ruff check and mypy are clean.
  • Unit tier: 3188 passed (baseline 3184), with no failures before or after.
  • Not run locally: the integration tier needs Docker + FDB, which this machine doesn't have. I ran test_shutdown_no_suspension.py locally with two extra FDB reads stubbed (get_active_lvol_migration, cancel_pending_node_restart_tasks): 17/17 pass. Without those stubs, 6 tests fail on the unchanged baseline too, for lack of FDB. CI covers this tier.

Notes

  • Seconds-dated events: event dates are written in milliseconds (round(time.time()*1000)). A legacy event dated in seconds would sort by its digits and may now be kept where the old code deleted it.
  • Not addressed here (see the review discussion):
    • the main loop's except Exception: logger.error("Error while updating cluster status") still drops the traceback;
    • other paths that set a node non-ONLINE outside the monitor may have the same missing refresh.

🤖 Generated with Claude Code

wmousa and others added 2 commits October 19, 2026 18:03
…f the main loop

On 2026-10-19 two nodes were shut down at 15:42:46 and the cluster read
ACTIVE until 15:49:47, seven minutes with reduced redundancy.

shutdown_storage_node flips the node to OFFLINE itself, so the monitor's
set_node_offline (the path that refreshes the cluster status) never runs
for it. The only remaining refresh is the monitor main loop, and that loop
also ran the retention housekeeping, which read the whole events table and
cleared one key per expired event. The cluster had just been upgraded from
R26.2-PRE, which never pruned events, so the first sweeps deleted months of
them while the status went unrefreshed.

- shutdown_storage_node refreshes the cluster status once the node is
  OFFLINE (best effort; a failure there does not fail the shutdown).
- Housekeeping runs on its own thread; the main loop only refreshes the
  status.
- _delete_old_logs clears the cluster's expired events with one range
  clear over <cluster_uuid>/ .. <cluster_uuid>/<cutoff ms>, instead of a
  full-table read and per-event clears.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… leaking a mock monitor

Review of the shutdown refresh. update_cluster_status is the monitor's full
pass: requeue and re-admit sweeps, rebalancing flags, shared-placement and
write-protection switches, and on a SUSPENDED cluster cluster_activate. A CLI
`sn shutdown` must not start an activation or wait on one. The shutdown now
calls apply_node_loss_status, which applies only the downgrades a node loss
can cause (ACTIVE -> DEGRADED/SUSPENDED, DEGRADED -> SUSPENDED) with the same
set_cluster_status the monitor makes for them.

CI failure: test_secondary_promotion assigned a MagicMock to
sys.modules["simplyblock_core.services.storage_node_monitor"] and never
restored it. On Python 3.9 mock.patch resolves its target through package
attributes, so the shutdown test patched the real module while the lazy
import in shutdown_storage_node got the leaked mock, and the call went
nowhere. 3.11+ resolves through sys.modules, which is why main passed. The
test now uses patch.dict(sys.modules, ...), which restores the module.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wmousa
wmousa force-pushed the fix/r263-monitor-status-latency branch from e877e71 to 4df7674 Compare October 19, 2026 16:08
@wmousa
wmousa merged commit 4cf2bdd into R26.3 Oct 19, 2026
8 checks passed
@wmousa
wmousa deleted the fix/r263-monitor-status-latency branch October 19, 2026 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

target/R26.3 Targets release branch R26.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant