Route graceful SIGTERM shutdown through maintenance instead of a signal race#1155
Merged
Conversation
…al race
Previously, a plain SIGTERM to pg_autoctl was forwarded by the supervisor to
every child service (the node-active/keeper process and the Postgres
controller) independently. Each reacted on its own: the Postgres controller
would stop Postgres directly while the keeper separately reported node state
for up to 30s, racing the controller rather than coordinating with it via the
FSM.
This commit changes a plain SIGTERM to only signal the keeper (node-active)
service. The keeper then drives a graceful handoff through the monitor's
existing maintenance machinery:
- PRIMARY_STATE: calls start_maintenance() on its own behalf (the same
monitor call `pg_autoctl enable maintenance --allow-failover` uses),
then drives prepare_maintenance -> maintenance via keeper_fsm_step(),
letting a standby take over immediately instead of waiting out the old
30s reporting loop / health-check timeout.
- SECONDARY_STATE / CATCHINGUP_STATE: same mechanism, so a secondary's
graceful shutdown lets the primary drop it from the replication quorum
right away instead of waiting for the monitor's own health-check
timeout to notice.
- Anything else (monitor disabled, or start_maintenance() itself fails,
e.g. no candidate available): falls back to the previous
reporting-loop-then-stop behaviour, now stopping Postgres first so the
common case is fast rather than always taking the full 30s.
SIGINT/SIGQUIT are unaffected: they still signal every service immediately,
whether received directly (Docker/Kubernetes/systemd escalating after a
grace period) or via the supervisor's own internal escalation.
A new maintenanceEnteredOnShutdown state-file flag (state version 1->2)
records that this node's own shutdown is what triggered maintenance, so the
next startup automatically calls stop_maintenance() on its own behalf rather
than requiring a manual `pg_autoctl disable maintenance` -- distinguishing
this from an operator-initiated maintenance session, which is left alone.
Test harness: DataNode.fail() now defaults to SIGQUIT to simulate a hard
crash (SIGTERM no longer means "crash" now that it triggers a graceful
maintenance handoff); stop_pg_autoctl()/PGAutoCtl.stop() keep their SIGTERM
default since several tests rely on it for a plain graceful restart.
Bugs found and fixed while implementing and verifying this feature:
- supervisor_stop_subprocesses()'s keeper-only SIGTERM restriction was
fine for the initial signal, but the supervisor's pre-existing
stuck-process escalation (a fixed ~5s timer that signals the whole
process group) could still fire before the keeper's own up-to-60s
graceful window elapsed, racing the very handoff this change is for.
Extended that threshold to cover the keeper's full grace period, and
added a cascade that signals the remaining services immediately once
the keeper actually exits (rather than waiting on the timer at all in
the common case).
- That cascade computed its signal via get_current_signal(), which
resets right after the initiating signal is processed -- so by the
time the cascade ran (in reaction to the keeper's exit, not a fresh
signal), it always saw the default and downgraded an escalated
SIGQUIT/SIGINT shutdown to plain SIGTERM. Fixed to use the sticky
supervisor->shutdownSignal instead.
- For SIGQUIT/SIGINT specifically, that cascade was also unconditional,
re-signalling every service a second time even though the initial
broadcast already reached everyone (keeper-only routing never applied
to those signals in the first place). Now only cascades when the
shutdown actually started as a keeper-only SIGTERM.
- The fallback path's reporting loop ran *before* stopping Postgres, so
once the sibling Postgres controller stopped receiving its own direct
SIGTERM, nothing else stopped Postgres during that loop -- every
non-primary (or maintenance-unavailable) shutdown took the full 30s
instead of being near-instant. Fixed by stopping Postgres first.
- keeper_shutdown_via_maintenance() returned success as soon as the local
FSM transition reached MAINTENANCE_STATE, but monitor_node_active()
only reports the state as of the *start* of each keeper_fsm_step() call
-- so the monitor's reportedState never caught up to "maintenance"
before the process exited. On next startup, the auto stop_maintenance()
call was rejected outright ("current state is not maintenance"), and
the node was stuck in maintenance until a manual `pg_autoctl disable
maintenance`. Fixed by doing one more keeper_fsm_step() to report the
final state before returning.
- The fallback reporting loop's exit condition required a *successful*
final report, added so a transient failure would be retried -- but
combined with a genuinely unreachable monitor, every connection
attempt pays a multi-second timeout and the loop ran for the full 30s
budget even though Postgres was already confirmed stopped locally.
Bounded to a small number of attempts once Postgres is stopped.
Also reworked the reporting loop's cadence per review: 1s while Postgres is
still stopping (unchanged), backing off to 5s once it's confirmed stopped,
and now actually checking that the report was delivered rather than
assuming success.
pgaf spec fixes (tests/tap/specs/): two specs asserted an intermediate FSM
state (stop_replication) that only applies to the direct 2-node promotion
path; 3+-node topologies go through ProceedGroupStateForMSFailover's
report_lsn-based election instead, so those waits timed out even though the
cluster reached the correct end state in a few seconds. One spec asserted
`catchingup` for two stopped secondaries, which can no longer happen now
that they gracefully enter `maintenance` instead of just vanishing. Updated
all three to assert the actual (and now faster) states reached.
Test harness fix (tests/network.py, tests/pgautofailover_utils.py):
`PGAutoCtl.run()` is a long-running background process communicated with
via a pipe that nothing drains while it keeps running; once its own (often
-vv) logging exceeds the OS pipe buffer, the child blocks on write() and
deadlocks against callers that only read much later. Redirects to files
instead, which never block regardless of volume, and added a bounded
restart-retry helper (DataNode.run_and_wait_with_retry()) for restarting a
node after a hard-crash simulation, since Postgres occasionally fails to
rebind its port on restart in this test harness's per-node network-namespace
simulation (suspected pyroute2/namespace-attachment race, not a pg_autoctl
bug: the port is confirmed free at the OS level when this happens).
Known issue, not blocking: test_multi_ifdown.py::test_010_start_node1_again
can still fail/hang under the old Python test harness's namespace
simulation specifically -- the equivalent real-Docker-networking scenario in
tests/tap/specs/multi_ifdown.pgaf passes cleanly and quickly, so this looks
like an environment-specific limitation of the pyroute2-based harness rather
than a product bug. Investigation continues in parallel.
Verification: `make force-build` clean across all 6 supported PG versions
(14-19), each with 12/12 monitor regress + 6/6 isolation tests passing.
citus_indent style check and banned-API check clean. Full pgaftest suite
green across all schedules (quick, node, ssl, multi-misc, multi-async,
citus-1, citus-2). pytest groups single/multi/monitor/citus/ssl green aside
from the known ifdown issue above and a pre-existing, timing-sensitive
health-check race in test_basic_operation.py (test_007_004, confirmed
equally present on origin/main, unrelated to this change).
CI's style_checker job caught a misaligned continuation line in keeper_shutdown_via_maintenance() that my earlier local check missed: I had been running `docker run --rm -v "$(pwd):/data" citus/stylechecker:no-py` with no command, which mounts to the wrong path and never actually invokes `citus_indent --check` -- it was a silent no-op reporting false success all along. The correct invocation is `make docker-check` (or `make docker-indent` to auto-fix), which mounts to /workdir and runs citus_indent explicitly, per the Makefile's own CITUS_INDENT_DOCKER target. Re-verified with the correct invocation: style check, banned-API check, and a local compile are all clean.
… side effect wait_until_state() raises on timeout (despite its docstring claiming it returns False), and before raising it calls print_debug_logs(), which iterates every node in the cluster and stop_pg_autoctl()'s any that are still running. Since run_and_wait_with_retry() only restarts the one node it's called on, a single timeout during its polling would silently stop every other node in the cluster too, leaving them stopped and never restarted -- almost certainly the actual cause of the multi-hour hangs observed in test_multi_ifdown.py::test_010_start_node1_again, far beyond what a simple "node1 occasionally fails to rebind" issue would cause. Rewritten to poll self.get_state() directly, matching wait_until_state()'s own internal polling loop but without calling it (and therefore without triggering its side effect).
…t environments pg_ctl_postgres()'s listen=false branch (used only for the crash-recovery step before pg_rewind: "do not open the service just yet") unconditionally passed -h '' to postgres. When PG_REGRESS_SOCK_DIR is also set to the empty string -- which the pytest test harness does unconditionally for every test -- this function also appends -k '' a few lines later. With both TCP and the Unix socket disabled, postgres has no way to create any socket at all and dies instantly with FATAL: no socket created for listening. This was previously assumed to be a flaky pyroute2/network-namespace race specific to test_multi_ifdown.py::test_010_start_node1_again (restarting node1 after a hard-crash simulation), and mitigated with a restart-retry loop in the test harness. It's actually fully deterministic: any node that needs pg_rewind after a crash hits this exact code path, and it always fails whenever PG_REGRESS_SOCK_DIR="" is set, regardless of retries. Fixed by falling back to "-h localhost" instead of "-h ''" specifically when PG_REGRESS_SOCK_DIR is set-and-empty, giving postgres a loopback TCP listener to satisfy its own startup requirement -- matching the same PG_REGRESS_SOCK_DIR="" -> "use localhost" convention already used in pg_setup_get_local_connection_string() (pgsetup.c). Nothing external is meant to connect to postgres in this mode either way, so restricting it to loopback rather than the node's normal listen_addresses is still correct. Verified: tests/test_multi_ifdown.py now passes in full (16/16, ~112s), including test_010_start_node1_again, which previously hung for hours or failed after exhausting restart retries.
… Permission denied CI (PR #1155) showed FileNotFoundError: /tmp/no-monitor/node1.stdout.log in PGAutoCtl.run(), which redirects a long-running pg_autoctl process's stdout/ stderr to files instead of pipes (to avoid a pipe-deadlock, see run_unmanaged() in network.py). That redirect assumed datadir's parent directory always already exists -- true by accident in monitor-based tests (created as a side effect of the monitor's own setup), but not in monitor-disabled tests, where no earlier step creates it first. First fix attempt (os.makedirs(..., exist_ok=True) before opening the log files) traded one bug for another: pytest itself runs as root (via a plain `sudo`), so the bare os.makedirs() left the directory root-owned at mode 0755. pg_autoctl itself runs as the unprivileged "docker" user (via `sudo -u docker`), and needs to create its own subdirectories under that same parent (e.g. its backup directory) -- which then failed with "Permission denied", turning a fast, obvious FileNotFoundError into a 30-second "Failed to reach goal state" timeout in test_monitor_disabled.py:: test_002_init_to_single, confirmed reproducible (2/2 local trials) and absent on origin/main (1/1 trial, plus this exact test passing before hitting an unrelated pre-existing flake later in the same run). Fixed by chmod'ing the directory to 0o777 right after creating it, so both users can write into it regardless of the creating process's umask. Verified: test_monitor_disabled.py now passes in full (13/13) locally.
…ndoff Both tests used the default SIGTERM (via stop_pg_autoctl()) to simulate a node going away, relying on the old semantics where SIGTERM just killed the process without touching FSM state on the monitor. Now that SIGTERM drives pg_autoctl's own graceful maintenance handoff (for a primary) or maintenance request (for a secondary/catching-up node), these tests observed different, new behavior: - test_ensure.py::test_004_demoted: stops node1's Postgres, then sends it SIGTERM while still primary. SIGTERM now succeeds at a full graceful handoff to node2 (confirmed via monitor logs: node1 primary -> prepare_maintenance -> maintenance, node2 secondary -> prepare_promotion -> primary), so node1 never passes through "demoted" as the test's core assertion expects. The test is specifically about the demoted-state fallback path, i.e. a hard-crash scenario, so switched to SIGQUIT to bypass the new maintenance routing and reproduce that path directly. - test_basic_citus_operation.py::test_013_perform_failover_worker2b_draining: stops worker2a (the secondary) then immediately forces a manual failover. SIGTERM now requests maintenance for worker2a; since it's the only secondary with number_sync_standbys=0, start_maintenance() also forces worker2b's (the primary's) goal state to wait_primary as a documented, pre-existing side effect (node_active_protocol.c). That means IsCurrentState(worker2b, primary) is no longer true by the time the test's own perform_failover() call runs, which fails outright with "couldn't find the primary node in formation ..., group 2" instead of reproducing the historical get_primary() race the test is about. Switched to SIGQUIT so worker2a is simply down, matching the test's actual intent. Both verified locally: test_ensure.py passes in full (6/6), and test_basic_citus_operation.py's test_013 passes (confirmed via a full local run through test_015).
…ching-up The docs never caught up with the SIGTERM-to-maintenance feature: pg_autoctl stop's own reference page still described the pre-feature behavior (any signal either runs a transition to completion, stops at the next opportunity, or stops immediately -- no mention of maintenance at all), and the maintenance state descriptions in the failover state machine reference only covered the manual `pg_autoctl enable maintenance` path, not the equivalent automatic one now taken by a plain SIGTERM. - docs/ref/pg_autoctl_stop.rst: describe the graceful SIGTERM path in full (maintenance handoff attempt, up to 30s, falling back to the old report-and-stop behavior for another 30s), the auto-recovery of self-triggered maintenance on next start vs. an operator-initiated session staying until explicitly disabled, and that --fast/--immediate bypass all of it. Also fixes a pre-existing typo (pg_autoclt). - docs/failover-state-machine.rst: the Maintenance, Prepare_maintenance, and Wait_maintenance sections now mention both the manual and the automatic (SIGTERM-triggered) path into each state. Also fixes a pre-existing typo (custer). - docs/ref/pg_autoctl_enable_maintenance.rst, docs/operations.rst: note that a plain `pg_autoctl stop` already does this automatically, and that the manual command is for keeping a node registered in maintenance independently of a restart. - docs/install.rst, docs/ref/pg_autoctl_node_run.rst: cross-reference pg_autoctl_stop from the existing "SIGTERM stops cleanly" mentions in the container/systemd signal-contract sections. - docs/ref/pgaftest.rst: `compose stop`'s description still said only a primary attempts start_maintenance() on shutdown; updated to cover secondary/catching-up nodes too, matching the extension made earlier in this same effort. Verified: `make -C docs html` builds clean (also checked with `sphinx-build -E -n` for a full rebuild with nitpicky mode), no new warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A plain
SIGTERMtopg_autoctlnow drives a graceful handoff through the monitor's existing maintenance machinery, instead of racing an independent signal to each child service.Before: the supervisor forwarded
SIGTERMto every child service (keeper/node-active and the Postgres controller) independently. Each reacted on its own — the controller stopped Postgres directly while the keeper separately reported node state for up to 30s — racing rather than coordinating via the FSM.After: a plain
SIGTERMonly signals the keeper. The keeper then:start_maintenance()on its own behalf (same callpg_autoctl enable maintenance --allow-failoveruses), drivesprepare_maintenance -> maintenance, letting a standby take over immediately instead of waiting on the old reporting loop / health-check timeout.start_maintenance()fails, e.g. no candidate): falls back to the previous behavior, now stopping Postgres first so the common case is fast.SIGINT/SIGQUITare unaffected — still signal every service immediately.A new
maintenanceEnteredOnShutdownstate-file flag (state version 1→2) lets a restarting node automatically callstop_maintenance()on its own behalf if its own shutdown is what triggered maintenance — distinguishing that from an operator-initiated maintenance session, which is left alone.Test harness:
DataNode.fail()now defaults toSIGQUIT(hard-crash simulation), sinceSIGTERMno longer means "crash."stop_pg_autoctl()/PGAutoCtl.stop()keep theirSIGTERMdefault (several tests rely on it for a plain graceful restart).Bugs found and fixed along the way
get_current_signal(), which resets right after the initiating signal is processed — silently downgrading an escalatedSIGQUIT/SIGINTshutdown to plainSIGTERM. Fixed to use the stickyshutdownSignal.SIGQUIT/SIGINT: the cascade re-signalled every service even though the initial broadcast already reached everyone for those signals. Now only cascades for a keeper-onlySIGTERMshutdown.monitor_node_active()only reports state as of the start of eachkeeper_fsm_step()call, so the transition intomaintenancewas never actually told to the monitor before the process exited. On restart, the autostop_maintenance()call was rejected outright, leaving the node stuck in maintenance until a manualpg_autoctl disable maintenance. Fixed with one extrakeeper_fsm_step()before returning.pgaf spec corrections
Two specs asserted an intermediate FSM state (
stop_replication) that only applies to the direct 2-node promotion path — 3+-node topologies go throughProceedGroupStateForMSFailover'sreport_lsn-based election instead, so those waits timed out even though the cluster reached the correct end state in a few seconds. One spec assertedcatchingupfor two stopped secondaries, which can no longer happen now that they gracefully entermaintenanceinstead of vanishing. All three updated to assert the actual, faster states now reached.Test harness fix (unrelated to product code, but needed for reliable coverage)
PGAutoCtl.run()'s stdout/stderr were piped but nothing drains that pipe while the process keeps running as a long-lived background service — once its own (often-vv) logging exceeded the OS pipe buffer, the child would block onwrite()and deadlock against callers that only read much later. Redirected to files instead. Also added a bounded restart-retry helper for restarting a node after a hard-crash simulation, working around an occasional Postgres bind failure on restart in this test harness's per-node network-namespace simulation.Known issue — not blocking, investigating in parallel
test_multi_ifdown.py::test_010_start_node1_againcan still fail/hang under the old Python test harness'spyroute2namespace simulation specifically. The equivalent scenario using real Docker networking (tests/tap/specs/multi_ifdown.pgaf) passes cleanly and quickly (~4s), so this looks like an environment-specific limitation of the namespace-simulation harness rather than a product bug. Opening this PR now per plan, to get CI's read on branch stability while this is investigated further locally.Verification
make force-build: clean across all 6 supported PG versions (14–19), each with 12/12 monitor regress + 6/6 isolation tests passing.citus_indentstyle check and banned-API check: clean.pgaftestsuite: green across all schedules (quick, node, ssl, multi-misc, multi-async, citus-1, citus-2).pytestgroups single/multi/monitor/citus/ssl: green aside from the known ifdown issue above, and a pre-existing, timing-sensitive health-check race intest_basic_operation.py::test_007_004(confirmed equally present onorigin/main, unrelated to this change).