From 4a6ef5bfca3eb2a9954259c6695f5458e0e5e6ee Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 14:59:00 +0200 Subject: [PATCH 1/7] Route graceful SIGTERM shutdown through maintenance instead of a signal 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). --- docs/ref/pgaftest.rst | 22 +- src/bin/pg_autoctl/defaults.h | 18 +- src/bin/pg_autoctl/service_keeper.c | 334 +++++++++++++++++++++++++-- src/bin/pg_autoctl/state.c | 2 + src/bin/pg_autoctl/state.h | 18 ++ src/bin/pg_autoctl/supervisor.c | 165 ++++++++++++- src/bin/pg_autoctl/supervisor.h | 10 + src/bin/pgaftest/README.md | 12 +- tests/network.py | 20 +- tests/pgautofailover_utils.py | 168 ++++++++++++-- tests/tap/specs/basic_operation.pgaf | 32 ++- tests/tap/specs/multi_async.pgaf | 5 +- tests/tap/specs/multi_standbys.pgaf | 30 ++- tests/test_multi_ifdown.py | 7 +- 14 files changed, 761 insertions(+), 82 deletions(-) diff --git a/docs/ref/pgaftest.rst b/docs/ref/pgaftest.rst index a47795e3b..d771152e6 100644 --- a/docs/ref/pgaftest.rst +++ b/docs/ref/pgaftest.rst @@ -517,19 +517,23 @@ is actually testing, not whichever one happens to make the test pass. ``compose stop `` ``docker compose stop`` — SIGTERM to the container's PID 1 - (``pg_autoctl``), which propagates an orderly shutdown to its child - services (a grace period applies before Docker escalates to SIGKILL). - Exercises orderly shutdown: ``pg_autoctl``'s supervisor - (``supervisor_stop_subprocesses()``) runs its normal shutdown sequence, - so the monitor sees a clean disconnect, then the same failover trigger - as any other lost connection. + (``pg_autoctl``), which forwards a plain SIGTERM to the node-active + (keeper) service only (a grace period applies before Docker escalates to + SIGKILL). Exercises graceful shutdown: for a primary, the keeper calls + ``start_maintenance()`` on its own behalf and hands off to a standby + through the ordinary maintenance FSM (``prepare_maintenance`` -> + ``maintenance``), the same transitions an operator-run ``pg_autoctl + enable maintenance --allow-failover`` would drive. For anything else + (a secondary, or a primary maintenance can't be started for), Postgres + is simply stopped and the process exits. ``compose kill `` ``docker compose kill`` — immediate SIGKILL, no grace period at all. Exercises hard-crash recovery: the process gets zero chance to shut - down cleanly. Use only when a test specifically needs to rule out a - race where the dying node reports a state transition on its way out - (see the ``multi_alternate.pgaf`` header comment). + down cleanly, not even a signal handler. Use only when a test + specifically needs to rule out a race where the dying node reports a + state transition on its way out (see the ``multi_alternate.pgaf`` + header comment). ``stop postgres `` / ``start postgres `` ``pg_autoctl manual service pgctl off`` — writes a persistent diff --git a/src/bin/pg_autoctl/defaults.h b/src/bin/pg_autoctl/defaults.h index 7873f0d98..bf931db87 100644 --- a/src/bin/pg_autoctl/defaults.h +++ b/src/bin/pg_autoctl/defaults.h @@ -13,7 +13,7 @@ #include "git-version.h" /* to be written in the state file */ -#define PG_AUTOCTL_STATE_VERSION 1 +#define PG_AUTOCTL_STATE_VERSION 2 /* additional version information for printing version on CLI */ #define PG_AUTOCTL_VERSION GIT_VERSION @@ -94,6 +94,22 @@ #define PG_AUTOCTL_MONITOR_SLEEP_TIME 10 /* seconds */ #define PG_AUTOCTL_MONITOR_RETRY_TIME 1 /* seconds */ +/* + * A primary's graceful SIGTERM shutdown (see keeper_graceful_shutdown() in + * service_keeper.c) may spend up to KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS + * attempting a maintenance handoff, and if that does not complete in time, + * up to another KEEPER_SHUTDOWN_LOOP_MAX_SECS in the fallback reporting + * loop. The supervisor (supervisor.c) must not treat the keeper as "stuck" + * and escalate a plain SIGTERM to the rest of the services before that full + * combined window has elapsed. + */ +#define KEEPER_SHUTDOWN_LOOP_MAX_SECS 30 +#define KEEPER_SHUTDOWN_LOOP_STOPPED_REPORT_INTERVAL_SECS 5 +#define KEEPER_SHUTDOWN_STOPPED_REPORT_MAX_ATTEMPTS 2 +#define KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS 30 +#define KEEPER_GRACEFUL_SHUTDOWN_MAX_SECS \ + (KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS + KEEPER_SHUTDOWN_LOOP_MAX_SECS) + #define PG_AUTOCTL_LISTEN_NOTIFICATIONS_TIMEOUT 60 #define COORDINATOR_IS_READY_TIMEOUT 300 diff --git a/src/bin/pg_autoctl/service_keeper.c b/src/bin/pg_autoctl/service_keeper.c index 713df0be6..667484ca0 100644 --- a/src/bin/pg_autoctl/service_keeper.c +++ b/src/bin/pg_autoctl/service_keeper.c @@ -25,6 +25,7 @@ #include "monitor.h" #include "pgctl.h" #include "pidfile.h" +#include "primary_standby.h" #include "service_keeper.h" #include "service_postgres_ctl.h" #include "signals.h" @@ -61,6 +62,9 @@ static void check_for_network_partitions(Keeper *keeper); static bool is_network_healthy(Keeper *keeper); static bool in_network_partition(KeeperStateData *keeperState, uint64_t now, int networkPartitionTimeout); +static void keeper_graceful_shutdown(Keeper *keeper); +static bool keeper_shutdown_via_maintenance(Keeper *keeper); +static void keeper_auto_recover_shutdown_maintenance(Keeper *keeper); /* @@ -240,17 +244,77 @@ service_keeper_node_active_init(Keeper *keeper) exit(EXIT_CODE_PGCTL); } + (void) keeper_auto_recover_shutdown_maintenance(keeper); + return true; } +/* + * keeper_auto_recover_shutdown_maintenance runs once at node-active service + * startup, right after the keeper state has been loaded. If the previous + * run's graceful shutdown got far enough to persist + * maintenanceEnteredOnShutdown (see keeper_shutdown_via_maintenance()), + * this node's own SIGTERM handling entered maintenance on its own behalf -- + * as opposed to an operator running `pg_autoctl enable maintenance` -- so it + * should self-clear here rather than waiting for an explicit + * `pg_autoctl disable maintenance`. + * + * monitor_stop_maintenance() has its own precondition check on the monitor + * side (the node must currently be in MAINTENANCE_STATE or + * PREPARE_MAINTENANCE_STATE): calling it when the node has already left + * maintenance by some other means (an operator already disabled it, or the + * state was never actually committed on the monitor side because the + * process was killed between persisting this flag and the + * start_maintenance() call landing) is expected to be a harmless no-op or + * error, logged but not treated as fatal here. Either way the flag is + * cleared and persisted immediately, so this only ever attempts the + * auto-recovery once per shutdown. + */ +static void +keeper_auto_recover_shutdown_maintenance(Keeper *keeper) +{ + KeeperStateData *keeperState = &(keeper->state); + + if (!keeperState->maintenanceEnteredOnShutdown || + keeper->config.monitorDisabled) + { + return; + } + + log_info("This node entered maintenance as part of its own graceful " + "shutdown; automatically disabling maintenance now"); + + bool mayRetry = false; + + if (!monitor_stop_maintenance(&(keeper->monitor), + keeperState->current_node_id, + &mayRetry)) + { + log_warn("Failed to automatically disable maintenance for this " + "node; if it is still in maintenance, run " + "`pg_autoctl disable maintenance` manually"); + } + + keeperState->maintenanceEnteredOnShutdown = false; + (void) keeper_store_state(keeper); +} + + /* * keeper_node_active_shutdown_loop sends node_active reports to the monitor - * every second for up to KEEPER_SHUTDOWN_LOOP_MAX_SECS while PostgreSQL stops. + * every second for up to KEEPER_SHUTDOWN_LOOP_MAX_SECS, confirming Postgres + * has stopped. * - * This runs after the main node-active loop exits on SIGTERM, concurrently - * with the postgres-controller service (a sibling process) calling - * "pg_ctl stop -m fast". + * This is the fallback path for a plain SIGTERM shutdown, used when + * keeper_shutdown_via_maintenance() could not be used (not currently a + * primary, monitor disabled, or start_maintenance() itself failed, e.g. no + * candidate is available to take over). The caller, keeper_graceful_ + * shutdown(), calls ensure_postgres_service_is_stopped() before this loop + * runs: unlike before, the Postgres-controller sibling process is not + * directly signalled by the supervisor on a plain SIGTERM (see + * supervisor_stop_subprocesses()), so nothing else would stop Postgres on + * its own if we didn't. * * When SIGTERM reaches the postmaster, process_pm_shutdown_request() * (src/backend/postmaster/postmaster.c) sets Shutdown = FastShutdown and @@ -263,20 +327,41 @@ service_keeper_node_active_init(Keeper *keeper) * By continuing to call node_active with the current pgIsRunning value here, * we ensure the monitor learns the primary is going away within one second of * the shutdown starting, and can begin failover right away rather than waiting - * for a health-check timeout. + * for a health-check timeout. In the ordinary case Postgres is already down + * by the time this loop starts, so its first iteration reports that and + * returns immediately; the loop only runs longer if the stop is still in + * flight or slow to confirm, or if reporting itself fails. + * + * The 1-second cadence only applies while Postgres is still going down: once + * pgIsRunning is observed false, there is no more urgency, so the loop backs + * off to a KEEPER_SHUTDOWN_LOOP_STOPPED_REPORT_INTERVAL_SECS-second retry + * cadence for the rest of its time budget. That slow phase only runs at all + * when the report carrying pgIsRunning=false itself failed to reach the + * monitor (a transient connection issue, say) -- it exists as insurance for + * that case, not as a normal path. + * + * That insurance is bounded to KEEPER_SHUTDOWN_STOPPED_REPORT_MAX_ATTEMPTS + * attempts once Postgres is confirmed stopped, rather than running for the + * full KEEPER_SHUTDOWN_LOOP_MAX_SECS budget: a genuinely unreachable monitor + * (as opposed to a momentary blip) means every attempt pays a full + * connection-timeout's worth of time (which can itself be several seconds), + * and Postgres being down locally is already the main thing this function + * exists to ensure -- there is no reason to hold up the rest of the shutdown + * for many multiples of that timeout just to keep trying to tell a monitor + * that is not there to listen. */ -#define KEEPER_SHUTDOWN_LOOP_MAX_SECS 30 - static void keeper_node_active_shutdown_loop(Keeper *keeper) { LocalPostgresServer *postgres = &(keeper->postgres); + time_t start = time(NULL); + int stoppedReportAttempts = 0; log_info("Graceful shutdown: reporting node state to monitor " "while PostgreSQL stops (up to %d seconds)", KEEPER_SHUTDOWN_LOOP_MAX_SECS); - for (int i = 0; i < KEEPER_SHUTDOWN_LOOP_MAX_SECS; i++) + while (time(NULL) - start < KEEPER_SHUTDOWN_LOOP_MAX_SECS) { /* escalated signal: exit without further reporting */ if (asked_to_quit || asked_to_stop_fast) @@ -285,17 +370,235 @@ keeper_node_active_shutdown_loop(Keeper *keeper) } (void) keeper_update_pg_state(keeper, LOG_DEBUG); - (void) service_keeper_node_active(keeper, false); + bool reported = service_keeper_node_active(keeper, false); if (!postgres->pgIsRunning) { - log_info("PostgreSQL has stopped; " - "final node_active report sent to monitor"); + if (reported) + { + log_info("PostgreSQL has stopped; " + "final node_active report sent to monitor"); + break; + } + + if (++stoppedReportAttempts >= KEEPER_SHUTDOWN_STOPPED_REPORT_MAX_ATTEMPTS) + { + log_warn("PostgreSQL has stopped, but the final node_active " + "report could not be delivered after %d attempts; " + "exiting anyway", + stoppedReportAttempts); + break; + } + } + + int sleepSecs = postgres->pgIsRunning + ? 1 + : KEEPER_SHUTDOWN_LOOP_STOPPED_REPORT_INTERVAL_SECS; + + pg_usleep(sleepSecs * 1000000L); + } +} + + +/* + * keeper_shutdown_via_maintenance implements a graceful shutdown by calling + * start_maintenance() on the node's own behalf -- the same monitor call + * `pg_autoctl enable maintenance [--allow-failover]` already uses -- then + * driving the ordinary FSM towards maintenance with the same + * keeper_fsm_step() primitive used by `pg_autoctl do fsm step`. + * + * This covers both roles start_maintenance() accepts: + * + * - a primary (PRIMARY_STATE -> prepare_maintenance -> maintenance), + * promoting a standby in the process; + * + * - a secondary or catching-up standby (SECONDARY_STATE/CATCHINGUP_STATE -> + * [wait_maintenance ->] maintenance), which prompts the primary to drop + * it from the replication quorum immediately instead of only noticing + * once the monitor's health check times out -- the same rationale as the + * primary case, just for the standby side of a graceful stop. + * + * start_maintenance() itself decides which of these applies (and whether an + * intermediate wait_maintenance/prepare_maintenance step happens at all) + * based on the node's currently reported state; this function only drives + * whichever FSM path results, via keeper_fsm_step(). + * + * Routing the shutdown through maintenance means Postgres gets stopped by + * the ordinary FSM action functions (fsm_stop_postgres_for_primary_ + * maintenance for a primary, fsm_start_maintenance_on_standby for a + * secondary), via the existing keeper/Postgres-controller state-file + * protocol (ensure_postgres_service_is_stopped()). No new coordination with + * the sibling Postgres-controller process is needed. For a primary, it is + * also excluded from candidate selection from the moment the monitor + * assigns PREPARE_MAINTENANCE_STATE -- structurally, not by timing luck, + * unlike the old draining/demote_timeout path (see BuildCandidateList()'s + * "skip old/new primary unless draining/demoted" exemption, which does not + * extend to the maintenance states). + * + * Returns false only when start_maintenance() itself could not be called at + * all (e.g. no candidate available for a primary, or for a secondary, the + * primary isn't currently stable enough to accept the quorum change), in + * which case the caller falls back to today's shutdown-reporting behaviour. + * A timeout or an escalated signal arriving mid-transition also returns + * false, as a safety net: if maintenanceEnteredOnShutdown was already + * persisted by that point, the flag still drives the auto + * stop_maintenance() call on next startup regardless of whether this + * function itself reported success. + * + * Once current_role locally reaches MAINTENANCE_STATE, one more + * keeper_fsm_step() call is made before returning: monitor_node_active() + * reports the state as of the *start* of each step, before that step's own + * transition runs, so the step that transitions into maintenance still only + * reports the prior state (wait_maintenance or prepare_maintenance) to the + * monitor. Skipping this extra call would exit with the monitor's + * reportedState never having caught up to "maintenance" -- and + * stop_maintenance() rejects a node whose reportedState isn't "maintenance" + * outright, so keeper_auto_recover_shutdown_maintenance() on next startup + * would fail every time, leaving the node stuck in maintenance until a + * manual `pg_autoctl disable maintenance`. + */ +static bool +keeper_shutdown_via_maintenance(Keeper *keeper) +{ + Monitor *monitor = &(keeper->monitor); + KeeperStateData *keeperState = &(keeper->state); + bool mayRetry = false; + + if (!monitor_start_maintenance(monitor, keeperState->current_node_id, + &mayRetry)) + { + log_warn("Failed to enter maintenance for a graceful shutdown, " + "likely because there is no candidate currently available " + "to take over, or the primary is not currently stable"); + return false; + } + + log_info("Graceful shutdown: requested maintenance (up to %d seconds)", + KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS); + + for (int i = 0; i < KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS; i++) + { + /* escalated signal: stop driving the FSM, let the caller take over */ + if (asked_to_quit || asked_to_stop_fast) + { break; } + if (!keeper_fsm_step(keeper)) + { + /* errors have already been logged, try again next second */ + pg_usleep(1000000L); + continue; + } + + /* + * Flag as soon as we're committed to maintenance, not only once + * fully there: a primary always passes through prepare_maintenance + * first, and a secondary sometimes passes through wait_maintenance + * first, but a secondary that isn't the last quorum member goes + * SECONDARY/CATCHINGUP -> MAINTENANCE directly in one step, with no + * intermediate state to catch here -- hence also checking + * MAINTENANCE_STATE itself, not just the two "in progress" states. + */ + if (!keeperState->maintenanceEnteredOnShutdown && + (keeperState->current_role == PREPARE_MAINTENANCE_STATE || + keeperState->current_role == WAIT_MAINTENANCE_STATE || + keeperState->current_role == MAINTENANCE_STATE)) + { + keeperState->maintenanceEnteredOnShutdown = true; + (void) keeper_store_state(keeper); + } + + if (keeperState->current_role == MAINTENANCE_STATE) + { + log_info("Reached maintenance; PostgreSQL has stopped"); + + /* + * Report the new current_role to the monitor before exiting + * (see the doc comment above): this step's own transition, if + * any, is a no-op since assigned_role already equals + * current_role at this point. Best-effort: even if this fails, + * maintenanceEnteredOnShutdown is already persisted, and the + * auto-recovery attempt on next startup logs its own warning + * rather than looping here. + */ + (void) keeper_fsm_step(keeper); + + return true; + } + pg_usleep(1000000L); /* 1 second */ } + + log_warn("Did not reach maintenance within %d seconds, " + "falling back to reporting the shutdown state", + KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS); + + return false; +} + + +/* + * keeper_graceful_shutdown implements pg_autoctl's response to a plain + * SIGTERM, once the main node-active loop has exited because asked_to_stop + * was set (and neither asked_to_stop_fast nor asked_to_quit). + * + * Since supervisor_stop_subprocesses() (supervisor.c) now forwards a plain + * SIGTERM to the node-active service only, the Postgres-controller sibling + * process is not directly signalled and won't stop Postgres on its own + * initiative as it used to: it only reacts to the keeper/controller + * state-file protocol. This function is therefore responsible for making + * sure Postgres actually stops as part of a graceful shutdown, one way or + * another, before the node-active process exits. + */ +static void +keeper_graceful_shutdown(Keeper *keeper) +{ + KeeperStateData *keeperState = &(keeper->state); + + /* + * PRIMARY_STATE, SECONDARY_STATE, and CATCHINGUP_STATE are exactly the + * roles start_maintenance() accepts (see node_active_protocol.c's + * start_maintenance(): IsCurrentState(primary) or reportedState in + * {secondary, catchingup}) -- every other current_role would just make + * that monitor call fail outright, so there is no point attempting it. + */ + bool canAttemptMaintenance = + (keeperState->current_role == PRIMARY_STATE || + keeperState->current_role == SECONDARY_STATE || + keeperState->current_role == CATCHINGUP_STATE) && + !keeper->config.monitorDisabled; + + if (canAttemptMaintenance) + { + if (keeper_shutdown_via_maintenance(keeper)) + { + /* + * The FSM's own maintenance action function (fsm_stop_postgres_ + * for_primary_maintenance or fsm_start_maintenance_on_standby) + * already stopped Postgres as part of reaching maintenance. + */ + return; + } + } + + /* + * Whether we're not a primary, the monitor is disabled, or maintenance + * could not be used: make sure Postgres actually stops now, using the + * same state-file protocol any other FSM transition relies on. This is + * a no-op if Postgres is already stopped (e.g. maintenance got far + * enough to stop it before timing out above). + * + * This must happen before keeper_node_active_shutdown_loop() below, not + * after: that loop only reports state and waits for pgIsRunning to go + * false, and nothing else is going to make that happen on its own + * anymore. Stopping Postgres first means the loop's first iteration + * typically already observes it down and returns immediately, instead + * of always running for its full duration. + */ + (void) ensure_postgres_service_is_stopped(&(keeper->postgres)); + + (void) keeper_node_active_shutdown_loop(keeper); } @@ -715,13 +1018,14 @@ keeper_node_active_loop(Keeper *keeper, pid_t start_pid) } /* - * Graceful SIGTERM shutdown: keep reporting state to the monitor while - * PostgreSQL finishes its checkpoint and stops. Skip on SIGINT/SIGQUIT - * which request immediate exit. + * Graceful SIGTERM shutdown: route a primary through maintenance so a + * standby can take over immediately, or otherwise make sure Postgres + * stops as part of our own exit. Skip on SIGINT/SIGQUIT which request + * immediate exit. */ if (asked_to_stop && !asked_to_stop_fast && !asked_to_quit) { - (void) keeper_node_active_shutdown_loop(keeper); + (void) keeper_graceful_shutdown(keeper); } /* One last check that we do not have any connections open */ diff --git a/src/bin/pg_autoctl/state.c b/src/bin/pg_autoctl/state.c index 04cdb1d38..c4df76266 100644 --- a/src/bin/pg_autoctl/state.c +++ b/src/bin/pg_autoctl/state.c @@ -255,6 +255,8 @@ log_keeper_state(KeeperStateData *keeperState) log_trace("state.keeper_is_paused: %d", keeperState->keeper_is_paused); log_trace("state.pg_version: %d", keeperState->pg_version); + log_trace("state.maintenanceEnteredOnShutdown: %d", + keeperState->maintenanceEnteredOnShutdown); } diff --git a/src/bin/pg_autoctl/state.h b/src/bin/pg_autoctl/state.h index 391e7185b..92243347d 100644 --- a/src/bin/pg_autoctl/state.h +++ b/src/bin/pg_autoctl/state.h @@ -120,6 +120,24 @@ typedef struct uint64_t last_secondary_contact; int64_t xlog_lag; int keeper_is_paused; + + /* + * Set when a graceful SIGTERM shutdown of a primary calls + * start_maintenance() on its own behalf (see keeper_shutdown_via_ + * maintenance() in service_keeper.c), as soon as the monitor has + * assigned PREPARE_MAINTENANCE_STATE -- not only once MAINTENANCE_STATE + * is fully reached, so that a restart at any point after that assignment + * (including one that interrupts the checkpoint-and-stop transition + * itself) is still recognised as self-triggered maintenance. + * + * On the next startup, this flag distinguishes "maintenance because this + * node's own shutdown sequence entered it" (auto-clear it by calling + * stop_maintenance() on the node's behalf) from "maintenance because an + * operator explicitly ran `pg_autoctl enable maintenance`" (leave it + * alone; only an explicit `pg_autoctl disable maintenance` should end + * that one). + */ + bool maintenanceEnteredOnShutdown; } KeeperStateData; _Static_assert(sizeof(KeeperStateData) < PG_AUTOCTL_KEEPER_STATE_FILE_SIZE, diff --git a/src/bin/pg_autoctl/supervisor.c b/src/bin/pg_autoctl/supervisor.c index fc98d7eaa..a6f02bf23 100644 --- a/src/bin/pg_autoctl/supervisor.c +++ b/src/bin/pg_autoctl/supervisor.c @@ -43,6 +43,10 @@ static bool supervisor_find_service(Supervisor *supervisor, pid_t pid, static void supervisor_stop_subprocesses(Supervisor *supervisor); +static bool supervisor_have_keeper_service(Supervisor *supervisor); + +static int supervisor_stuck_threshold_loops(Supervisor *supervisor); + static void supervisor_stop_other_services(Supervisor *supervisor, pid_t pid); static bool supervisor_signal_process_group(int signal); @@ -404,18 +408,69 @@ supervisor_reload_services(Supervisor *supervisor) /* * supervisor_stop_subprocesses calls the stopFunction for all the registered * services to initiate the shutdown sequence. + * + * A plain SIGTERM is forwarded to the node-active (keeper) service only, + * when one is present in this supervisor's service list. This lets the + * keeper drive a graceful shutdown through the ordinary FSM transitions + * (routing a primary through maintenance, see fsm_stop_postgres_for_ + * primary_maintenance) using the existing keeper/postgres-controller + * state-file protocol to decide when Postgres actually stops, rather than + * the Postgres controller reacting to its own independently-delivered + * SIGTERM and racing the keeper's FSM-driven shutdown. + * + * SIGINT/SIGQUIT -- whether received directly (e.g. Docker/Kubernetes/ + * systemd escalating to a harder signal after a grace period) or via our + * own internal escalation in supervisor_shutdown_sequence -- mean "stop + * now, no graceful handoff", and are forwarded to every service exactly as + * before. + * + * Supervisors with no service named SERVICE_NAME_KEEPER (the monitor's + * services, or the keeper before `--run` has handed control to the + * node-active service) have no FSM handoff to drive either way, so this + * signal-restriction does not apply to them: every service is signalled, + * same as for SIGINT/SIGQUIT. + * + * Once the keeper service has actually exited (supervisor->keeperExited), + * there is no FSM handoff left to protect: this function is called again + * from supervisor_restart_service() at that point specifically to cascade + * the signal to the remaining services right away, rather than waiting on + * supervisor_shutdown_sequence()'s stuck-process timer. + * + * That cascade call happens outside of direct signal reception, so it can't + * rely on get_current_signal(): the asked_to_stop/asked_to_stop_fast/ + * asked_to_quit flags it reads are reset right after being processed (see + * supervisor_handle_signals()), so by the time the cascade runs they no + * longer reflect the signal that actually started this shutdown -- it would + * silently fall back to the SIGTERM default and downgrade, say, a SIGQUIT- + * driven shutdown. supervisor->shutdownSignal is the sticky, escalation- + * aware value that survives that reset, so prefer it whenever a shutdown is + * genuinely already in flight (it stays 0, its zero-initialized value, for + * the one synthetic caller that isn't -- the pidfile-write-failure path in + * supervisor_start() -- which is exactly when falling back to + * get_current_signal()'s SIGTERM default is still correct). */ static void supervisor_stop_subprocesses(Supervisor *supervisor) { - int signal = get_current_signal(SIGTERM); + int signal = supervisor->shutdownSignal != 0 + ? supervisor->shutdownSignal + : get_current_signal(SIGTERM); int serviceCount = supervisor->serviceCount; int serviceIndex = 0; + bool keeperOnly = signal == SIGTERM && + !supervisor->keeperExited && + supervisor_have_keeper_service(supervisor); + for (serviceIndex = 0; serviceIndex < serviceCount; serviceIndex++) { Service *service = &(supervisor->services[serviceIndex]); + if (keeperOnly && strcmp(service->name, SERVICE_NAME_KEEPER) != 0) + { + continue; + } + if (kill(service->pid, signal) != 0) { log_error("Failed to send signal %s to service %s with pid %d", @@ -425,6 +480,58 @@ supervisor_stop_subprocesses(Supervisor *supervisor) } +/* + * supervisor_have_keeper_service returns true when this supervisor has a + * service named SERVICE_NAME_KEEPER registered, regardless of whether it is + * still running. + */ +static bool +supervisor_have_keeper_service(Supervisor *supervisor) +{ + int serviceIndex = 0; + + for (serviceIndex = 0; serviceIndex < supervisor->serviceCount; serviceIndex++) + { + if (strcmp(supervisor->services[serviceIndex].name, + SERVICE_NAME_KEEPER) == 0) + { + return true; + } + } + + return false; +} + + +/* + * supervisor_stuck_threshold_loops computes the stoppingLoopCounter value at + * which supervisor_shutdown_sequence() should stop waiting and escalate to + * the whole process group. + * + * When a plain SIGTERM shutdown is relying on the keeper alone to drive a + * graceful maintenance handoff (see supervisor_stop_subprocesses()), that + * handoff has its own grace period of up to KEEPER_GRACEFUL_SHUTDOWN_MAX_SECS + * (see defaults.h): escalating sooner would signal the Postgres controller + * directly and race the very FSM-driven handoff this is all for. A short + * margin is added on top for the reporting/logging done in between. In every + * other case (no keeper service, or the signal has already been escalated to + * SIGINT/SIGQUIT), fall back to the original, much shorter threshold. + */ +static int +supervisor_stuck_threshold_loops(Supervisor *supervisor) +{ + bool keeperGraceActive = supervisor->shutdownSignal == SIGTERM && + supervisor_have_keeper_service(supervisor); + + if (keeperGraceActive) + { + return ((KEEPER_GRACEFUL_SHUTDOWN_MAX_SECS + 5) * 1000) / 100; + } + + return 50; +} + + /* * supervisor_stop_other_subprocesses sends the QUIT signal to other known * sub-processes when on of does is reported dead. @@ -588,6 +695,16 @@ supervisor_handle_signals(Supervisor *supervisor) { supervisor->exitMode = SUPERVISOR_EXIT_CLEAN; supervisor->shutdownSequenceInProgress = true; + + /* + * Freeze the stuck-process threshold now, based on the shutdown + * signal we're starting from. It must not be recomputed on every + * loop: shutdownSignal itself gets escalated later on in + * supervisor_shutdown_sequence() as part of applying that very + * threshold, which would otherwise make it a moving target. + */ + supervisor->stuckThresholdLoops = + supervisor_stuck_threshold_loops(supervisor); } /* forward the signal to all our service to terminate them */ @@ -626,14 +743,17 @@ supervisor_handle_signals(Supervisor *supervisor) * it's 1 we have been waiting once without any child process reported absent * by waitpid(), tell the user we are waiting. * - * At 50 loops (typically we add a 100ms wait per loop), send either SIGTERM or - * SIGINT. + * At stuckThresholdLoops loops (typically we add a 100ms wait per loop, so + * that's 50 loops / 5s in the ordinary case, see supervisor_stuck_threshold_ + * loops()), send either SIGTERM or SIGINT. * - * At every 100 loops, send SIGINT. + * At every 100 loops after that, send SIGINT. */ static void supervisor_shutdown_sequence(Supervisor *supervisor) { + int stuckThresholdLoops = supervisor->stuckThresholdLoops; + if (supervisor->stoppingLoopCounter == 1) { log_info("Waiting for subprocesses to terminate."); @@ -644,7 +764,7 @@ supervisor_shutdown_sequence(Supervisor *supervisor) * Let's signal again all our process group ourselves and see what happens * next. */ - if (supervisor->stoppingLoopCounter == 50) + if (supervisor->stoppingLoopCounter == stuckThresholdLoops) { log_info("pg_autoctl services are still running, " "signaling them with %s.", @@ -659,8 +779,8 @@ supervisor_shutdown_sequence(Supervisor *supervisor) /* * Wow it's been a very long time now... */ - if (supervisor->stoppingLoopCounter > 0 && - supervisor->stoppingLoopCounter % 100 == 0) + if (supervisor->stoppingLoopCounter > stuckThresholdLoops && + (supervisor->stoppingLoopCounter - stuckThresholdLoops) % 100 == 0) { log_info("pg_autoctl services are still running, " "signaling them with SIGINT."); @@ -699,6 +819,37 @@ supervisor_restart_service(Supervisor *supervisor, Service *service, int status) if (supervisor->shutdownSequenceInProgress) { log_trace("supervisor_restart_service: shutdownSequenceInProgress"); + + /* + * A plain SIGTERM only ever signalled the keeper service directly + * (see supervisor_stop_subprocesses()), so that it could drive a + * graceful maintenance handoff without racing the Postgres + * controller. Now that the keeper itself has exited -- one way or + * another, its own graceful shutdown is done -- there is no handoff + * left to protect, so cascade the signal to whichever other + * services are still running right away, rather than waiting on + * the stuck-process timer in supervisor_shutdown_sequence(). + * + * Only do that cascade when the shutdown actually started as a + * keeper-only SIGTERM: for SIGINT/SIGQUIT, supervisor_stop_ + * subprocesses() already signalled every service directly up front + * (keeperOnly never applied), so the Postgres controller already + * has its signal -- re-sending it here would deliver a second, + * redundant signal while it may still be mid-shutdown (e.g. + * stopping Postgres gracefully in reaction to the first one), + * which is exactly the kind of double-delivery this code must not + * cause. + */ + if (strcmp(service->name, SERVICE_NAME_KEEPER) == 0) + { + supervisor->keeperExited = true; + + if (supervisor->shutdownSignal == SIGTERM) + { + (void) supervisor_stop_subprocesses(supervisor); + } + } + return false; } diff --git a/src/bin/pg_autoctl/supervisor.h b/src/bin/pg_autoctl/supervisor.h index a043f1a11..0cddf99d7 100644 --- a/src/bin/pg_autoctl/supervisor.h +++ b/src/bin/pg_autoctl/supervisor.h @@ -125,6 +125,16 @@ typedef struct Supervisor int shutdownSignal; int stoppingLoopCounter; + /* + * Tracking for the keeper-only SIGTERM graceful shutdown (see + * supervisor_stop_subprocesses() in supervisor.c): whether the + * node-active (keeper) service has exited yet, and the stoppingLoopCounter + * value at which the stuck-process escalation should kick in, computed + * once when the shutdown sequence begins. + */ + bool keeperExited; + int stuckThresholdLoops; + /* * Optional node spec watcher. When pg_autoctl is started via * `pg_autoctl node run `, the supervisor watches the ini file for diff --git a/src/bin/pgaftest/README.md b/src/bin/pgaftest/README.md index bb197b87c..a9e0affe8 100644 --- a/src/bin/pgaftest/README.md +++ b/src/bin/pgaftest/README.md @@ -104,21 +104,21 @@ and where the `.pgaf` ports currently stand relative to that mechanism. | Method | Mechanism | Notes | |---|---|---| -| `PGNode.stop_pg_autoctl()` / `PGAutoCtl.stop()` | `os.kill(pid, SIGTERM)` on the `pg_autoctl run` process | Despite the docstring ("Kills the keeper..."), this is graceful: `pg_autoctl`'s supervisor runs its normal shutdown sequence on SIGTERM. | +| `PGNode.stop_pg_autoctl()` / `PGAutoCtl.stop()` | `os.kill(pid, SIGTERM)` on the `pg_autoctl run` process | Graceful: `pg_autoctl`'s supervisor forwards a plain SIGTERM to the node-active (keeper) service only, which drives a graceful shutdown — for a primary, that means calling `start_maintenance()` on its own behalf and handing off to a standby via the normal maintenance FSM, rather than an abrupt stop. | | `PGNode.stop_postgres()` | `pg_ctl -D --wait --mode fast stop` (SIGINT to postmaster), retried up to 60× | Bypasses `pg_autoctl` entirely — the keeper stays up and will try to restart Postgres, which is why the retry loop exists (races against that restart). | -| `PGNode.fail()` | `stop_pg_autoctl()` then, if Postgres is still up, `stop_postgres()` | The suite's standard "simulate a node failure" call. Composite of the two graceful primitives above — **not** a hard crash, no SIGKILL involved anywhere. | +| `PGNode.fail()` | `os.kill(pid, SIGQUIT)` on the `pg_autoctl run` process by default | The suite's standard "simulate a node failure" call. Deliberately the hardest signal pg_autoctl handles without going through the OS's default disposition: for the node-active service, SIGQUIT is wired to an immediate `exit()` from inside the signal handler, no cleanup, no FSM-driven handoff — the closest thing to a real crash `.fail()` can produce without an actual SIGKILL. Pass `sig=signal.SIGINT` explicitly for tests that need the softer, one-more-iteration-then-exit variant instead. | | `PGNode.ifdown()` / `.ifup()` | `pyroute2` NDB: veth interface administratively down/up | Genuine network partition — processes keep running, only reachability is cut. | No Python test helper ever sends SIGKILL to a node's `pg_autoctl` or -`postgres` process — the hardest failure the old suite could inflict was -SIGTERM + a fast `pg_ctl stop`. +`postgres` process — the hardest failure the suite can inflict via `.fail()` +is SIGQUIT's immediate, no-cleanup exit. ### `.pgaf` DSL equivalents | `.pgaf` command | Mechanism | Closest Python equivalent | |---|---|---| -| `compose stop ` | `docker compose stop` → SIGTERM to container PID 1 (`pg_autoctl`) | `node.fail()` / `stop_pg_autoctl()` | -| `compose kill ` | `docker compose kill` → immediate SIGKILL | none — stricter than anything in the Python suite; use only with a documented reason (see `multi_alternate.pgaf`'s header comment) | +| `compose stop ` | `docker compose stop` → SIGTERM to container PID 1 (`pg_autoctl`) | `stop_pg_autoctl()` — a graceful shutdown, routed through a maintenance handoff for a primary. No longer `.fail()`: since `.fail()` now defaults to SIGQUIT, a `.pgaf` spec step that wants to match `.fail()`'s hard-crash intent should use `compose kill` instead (or `compose stop` if the scenario specifically wants to exercise the graceful/maintenance path). | +| `compose kill ` | `docker compose kill` → immediate SIGKILL | closest to `.fail()`'s current (SIGQUIT) intent, though still one step harder — SIGKILL bypasses pg_autoctl's signal handling entirely, where SIGQUIT at least runs `pg_autoctl`'s own signal handler before exiting | | `stop postgres ` / `start postgres ` | `pg_autoctl manual service pgctl off/on` inside the container | Close in intent to `stop_postgres()`, but goes through `pg_autoctl` rather than calling `pg_ctl` directly, so it does not reproduce the restart race the Python retry loop was written around | | `network disconnect ` / `network connect ` | Docker network disconnect/connect (+ static `--ip` on reconnect, see below) | `ifdown()` / `ifup()` | diff --git a/tests/network.py b/tests/network.py index 02e2d91f8..93e96759c 100644 --- a/tests/network.py +++ b/tests/network.py @@ -167,14 +167,28 @@ def run(self, command, user=os.getenv("USER")): start_new_session=True, ) - def run_unmanaged(self, command, user=os.getenv("USER")): + def run_unmanaged(self, command, user=os.getenv("USER"), stdout=None, stderr=None): """ Executes a command under the given user from this virtual node. Returns an NSPopen object to control the process. NSOpen has the same API as subprocess.Popen. This NSPopen object needs to be manually release. In general you should prefer using run, where this is done automatically by the context manager. + + stdout/stderr default to subprocess.PIPE, matching Popen's own + default -- pass real file objects instead for a long-running + background process (one that isn't communicate()'d with soon after + starting): nothing drains a PIPE while the process keeps running, so + once its cumulative output exceeds the OS pipe buffer the child + blocks on write() and, since nothing reads it until much later, + deadlocks. A real file never blocks regardless of volume. """ + if stdout is None: + stdout = subprocess.PIPE + + if stderr is None: + stderr = subprocess.PIPE + sudo_command = [ "sudo", "-E", @@ -189,8 +203,8 @@ def run_unmanaged(self, command, user=os.getenv("USER")): self.namespace, sudo_command, stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + stdout=stdout, + stderr=stderr, encoding="utf-8", errors="replace", start_new_session=True, diff --git a/tests/pgautofailover_utils.py b/tests/pgautofailover_utils.py index e826a8bca..501696569 100644 --- a/tests/pgautofailover_utils.py +++ b/tests/pgautofailover_utils.py @@ -517,7 +517,9 @@ def set_user_password(self, username, password): def stop_pg_autoctl(self, sig=signal.SIGTERM): """ - Kills the keeper by sending "sig" to keeper's process group. + Kills the keeper by sending "sig" to keeper's process group. Default + SIGTERM is a graceful stop, used by tests that want a plain + restart (not a simulated failure -- see DataNode.fail() for that). See PGAutoCtl.stop() for what signal choice controls. """ if self.pg_autoctl: @@ -628,20 +630,26 @@ def wait_until_pg_is_running(self, timeout=STATE_CHANGE_TIMEOUT): return ret == 0 - def fail(self, sig=signal.SIGTERM): + def fail(self, sig=signal.SIGQUIT): """ Simulates a data node failure by terminating the keeper and stopping postgres. - Default SIGTERM triggers pg_autoctl's graceful shutdown reporting - (see PGAutoCtl.stop()) -- fine for most tests, but it means the - node can keep participating in FSM transitions for a few seconds - after fail() returns, with timing that depends on how long - PostgreSQL's own shutdown checkpoint takes. Tests asserting on a - node being permanently excluded from candidate selection right - after a failure (e.g. a quorum stall with no other node to satisfy - it) should pass sig=signal.SIGINT/SIGQUIT for a true hard-crash - simulation instead. + Default SIGQUIT exits immediately, without running pg_autoctl's own + signal handler cleanup (see PGAutoCtl.stop()) -- the closest + simulation of a real crash. SIGTERM is no longer a hard-crash + simulation: it now triggers a graceful shutdown that, for a primary, + requests maintenance so a standby can take over immediately (see + keeper_shutdown_via_maintenance() in service_keeper.c); most tests + asserting on a node being permanently excluded from candidate + selection right after a failure need the default here, not SIGTERM. + + If a specific test turns out to be flaky with SIGQUIT (too little + control over exactly how abruptly the process exits), pass + sig=signal.SIGINT instead: it is still an immediate exit request that + bypasses the graceful/maintenance path, but runs one more loop + iteration and a clean libpq disconnect before exiting, which can be + easier to reason about under test-timing pressure. """ self.stop_pg_autoctl(sig) @@ -1183,6 +1191,76 @@ def __init__( self.formation = formation self.monitorDisabled = None + def run_and_wait_with_retry( + self, + target_state, + timeout=STATE_CHANGE_TIMEOUT, + retries=3, + name=None, + host=None, + port=None, + ): + """ + Runs pg_autoctl and waits for this node to reach target_state, + restarting (not just re-waiting) up to `retries` times if it + doesn't get there. + + This exists for restarting a node after a hard-crash simulation + (node.fail()): Postgres occasionally fails to (re)bind its port + with "no socket created for listening" even though nothing else + holds it -- suspected to be a pyroute2/network-namespace- + attachment race in this test harness's per-node namespace + simulation, not a pg_autoctl bug, since the port is confirmed + free at the OS level when this happens. pg_autoctl's own keeper + does not retry that internally, so plain node.run() can get + stuck. Use this instead of a plain node.run() wherever a test + restarts a node that was previously killed hard. + """ + per_attempt_timeout = max(timeout // retries, 30) + + for attempt in range(1, retries + 1): + self.run(name=name, host=host, port=port) + + if self.wait_until_state( + target_state=target_state, timeout=per_attempt_timeout + ): + return True + + print( + "%s did not reach '%s' within %ds (attempt %d/%d), " + "restarting and retrying" + % ( + self.logger_name(), + target_state, + per_attempt_timeout, + attempt, + retries, + ) + ) + + # the stuck attempt's pg_autoctl may not be responsive to a + # plain stop if it's wedged on the failed bind; fail() (SIGQUIT, + # then a direct Postgres stop if needed) is more likely to + # actually free the pgdata lock before the next attempt. + # + # fail() itself can raise subprocess.TimeoutExpired here: its + # underlying Cluster.communicate() waits up to its own timeout + # in 1-second steps, then makes one last attempt with whatever + # is left of the budget, which by then can be ~0 (or a tiny + # negative float from rounding) and raise even though the + # SIGQUIT'd process is, in practice, already gone -- that's a + # pre-existing quirk of that helper, unrelated to the bind race + # this method exists for. Don't let it abort the retry loop. + try: + self.fail() + except subprocess.TimeoutExpired as e: + print( + "%s: cleanup before retry raised %s (continuing anyway)" + % (self.logger_name(), e) + ) + + return False + def create( self, run=False, @@ -2131,6 +2209,14 @@ def __init__(self, pgnode, argv=None): self.err = "" self.cmd = "" + # Only set by run(): stdout/stderr log files for the long-running + # background process it starts, in place of pipes (see its + # docstring). None for anything started via execute() instead. + self.stdout_path = None + self.stderr_path = None + self.stdout_file = None + self.stderr_file = None + if argv: self.command = [self.program] + argv @@ -2164,7 +2250,29 @@ def run(self, level="-vv", name=None, host=None, port=None): if self.run_proc: self.run_proc.release() - self.run_proc = self.vnode.run_unmanaged(self.command) + # In case a previous run() was never followed by a communicate() + # call (e.g. release()'d directly), don't leak its file handles. + if self.stdout_file is not None: + self.stdout_file.close() + + if self.stderr_file is not None: + self.stderr_file.close() + + # `pg_autoctl run` is a long-running background process: this method + # returns immediately, and nothing calls communicate() on it again + # until much later (stop()/fail()), if ever. Redirecting its stdout/ + # stderr to files rather than pipes means its own (possibly `-vv`) + # logging can never fill an undrained pipe buffer and deadlock it -- + # see run_unmanaged()'s docstring in network.py. + self.stdout_path = self.datadir.rstrip("/") + ".stdout.log" + self.stderr_path = self.datadir.rstrip("/") + ".stderr.log" + + self.stdout_file = open(self.stdout_path, "w") + self.stderr_file = open(self.stderr_path, "w") + + self.run_proc = self.vnode.run_unmanaged( + self.command, stdout=self.stdout_file, stderr=self.stderr_file + ) def execute(self, name, *args, timeout=COMMAND_TIMEOUT): """ @@ -2194,12 +2302,15 @@ def stop(self, sig=signal.SIGTERM): """ Kills the keeper by sending "sig" to keeper's process group. - SIGTERM triggers pg_autoctl's graceful shutdown path: the - node-active service keeps reporting to the monitor for up to 30s - while PostgreSQL stops (see keeper_node_active_shutdown_loop() in - service_keeper.c). SIGINT/SIGQUIT skip that loop and exit - immediately -- pass one of those to simulate a true hard crash - instead of an operator-initiated stop. + SIGTERM triggers pg_autoctl's graceful shutdown path (see + keeper_graceful_shutdown() in service_keeper.c): for a primary, this + now requests maintenance so a standby can take over immediately, + driving the FSM through prepare_maintenance -> maintenance; for + anything else (or if maintenance isn't possible, e.g. no candidate + is available), it falls back to reporting node state to the + monitor for up to 30s while PostgreSQL stops. SIGINT/SIGQUIT skip + all of that and exit immediately -- pass one of those to simulate a + true hard crash instead of a graceful, operator-initiated stop. """ if self.run_proc and self.run_proc.pid: try: @@ -2219,7 +2330,9 @@ def stop(self, sig=signal.SIGTERM): def communicate(self, timeout=COMMAND_TIMEOUT): """ - Read all data from the Unix PIPE + Read all data from the Unix PIPE, or from the stdout/stderr log + files when run() redirected them there instead (see run()'s + docstring for why). This call is idempotent. If it is called a second time after an earlier successful call, then it returns the results from when the process @@ -2230,6 +2343,23 @@ def communicate(self, timeout=COMMAND_TIMEOUT): self.out, self.err = self.run_proc.communicate(timeout=timeout) + # run() redirects to files rather than pipes for a process it + # doesn't communicate() with right away (see its docstring): in that + # case the line above returns (None, None), since Popen only ever + # captures output itself when it owns the pipes. Read it back from + # the files instead, now that the process has exited. + if self.stdout_file is not None: + self.stdout_file.close() + self.stdout_file = None + with open(self.stdout_path, "r") as f: + self.out = f.read() + + if self.stderr_file is not None: + self.stderr_file.close() + self.stderr_file = None + with open(self.stderr_path, "r") as f: + self.err = f.read() + # The process exited, so let's clean this process up. Calling # communicate again would otherwise cause an "Invalid file object" # error. diff --git a/tests/tap/specs/basic_operation.pgaf b/tests/tap/specs/basic_operation.pgaf index 5734d4da1..846e6d504 100644 --- a/tests/tap/specs/basic_operation.pgaf +++ b/tests/tap/specs/basic_operation.pgaf @@ -43,12 +43,15 @@ teardown { # sequence), so the keeper's local "ensure" restart never races against a # synchronous secondary. This spec's setup{} brings up all three nodes # upfront, so node1 already has a synchronous secondary (node2) by the time -# any step runs; stopping node1's Postgres there reliably triggers a real -# failover (node1 primary -> draining -> demoted, node2 -> primary) rather -# than a quiet local self-heal — that scenario is already covered by -# test_010/test_012 (compose stop, the SIGTERM path). ensure.pgaf's -# test_003_init_secondary covers the true "external stop, keeper -# self-heals" case on a standby, where it doesn't trigger a failover. +# any step runs; stopping node1's Postgres there (bypassing pg_autoctl, +# e.g. a raw pg_ctl stop) reliably triggers a real failover (node1 primary +# -> draining -> demoted, node2 -> primary) rather than a quiet local +# self-heal — that scenario is already covered, via a different mechanism +# (a graceful SIGTERM-driven maintenance handoff rather than an unexpected +# Postgres death), by test_010/test_012 (compose stop, see the comment +# there). ensure.pgaf's test_003_init_secondary covers the true "external +# stop, keeper self-heals" case on a standby, where it doesn't trigger a +# failover. # # @@ -182,12 +185,17 @@ step test_009_failback { } # -# test_010: stop node1 (primary) via compose stop — Postgres shuts down -# cleanly. Matches the Python predecessor's node1.fail(), which -# sends SIGTERM to pg_autoctl then a "pg_ctl fast stop" to -# Postgres — a graceful process-level stop, not a network -# partition. (Partition detection has its own dedicated test: -# see test_021-023 below, ported from ifdown()/ifup().) +# test_010: stop node1 (primary) via compose stop — a plain SIGTERM to +# pg_autoctl, which now drives a graceful maintenance handoff: the +# keeper calls start_maintenance() on its own behalf, transitions +# primary -> prepare_maintenance -> maintenance (stopping Postgres +# as part of that), and node2 is promoted the same way it would be +# for an operator-run `pg_autoctl enable maintenance +# --allow-failover`. This is a graceful process-level stop, not a +# network partition (partition detection has its own dedicated +# test: see test_021-023 below, ported from ifdown()/ifup()), and +# it no longer matches the Python predecessor's node1.fail(), +# which now defaults to SIGQUIT to simulate a hard crash instead. # step test_010_fail_primary { diff --git a/tests/tap/specs/multi_async.pgaf b/tests/tap/specs/multi_async.pgaf index 6280d0a42..4c3feeb9a 100644 --- a/tests/tap/specs/multi_async.pgaf +++ b/tests/tap/specs/multi_async.pgaf @@ -148,8 +148,11 @@ step test_013_drop_node4 { } step test_014_001_fail_node1 { + # With 3+ nodes in the group, the candidate election goes through + # ProceedGroupStateForMSFailover()'s report_lsn-based path (not + # stop_replication, which is only used by the direct 2-node promotion) -- + # assert only the stable end state, not an intermediate one. compose stop node1 - wait until node2 state is stop_replication timeout 120s wait until node2 state is wait_primary and node3 state is secondary timeout 120s diff --git a/tests/tap/specs/multi_standbys.pgaf b/tests/tap/specs/multi_standbys.pgaf index 16281fba0..0effef1e7 100644 --- a/tests/tap/specs/multi_standbys.pgaf +++ b/tests/tap/specs/multi_standbys.pgaf @@ -166,17 +166,26 @@ step test_011_write_into_new_primary { } # -# test_012: fail primary node2 via compose stop — matches the Python -# predecessor's node2.fail() (graceful SIGTERM to pg_autoctl, not -# a network partition). See docs/ref/pgaftest.rst -# "Failure-simulation semantics" for why this matters. +# test_012: fail primary node2 via compose stop — a graceful SIGTERM to +# pg_autoctl, not a network partition, which now drives a +# maintenance handoff to node1 rather than an abrupt stop (see the +# comment on basic_operation.pgaf's test_010). This no longer +# matches the Python predecessor's node2.fail(), which now +# defaults to SIGQUIT to simulate a hard crash instead. See +# docs/ref/pgaftest.rst "Failure-simulation semantics" for why +# this matters. +# +# With 3+ nodes in the group, the candidate election goes through +# ProceedGroupStateForMSFailover()'s report_lsn-based path (not +# stop_replication, which is only used by the direct 2-node +# promotion) -- assert only the stable end state, not an +# intermediate one, same as basic_operation.pgaf's 2-node case. # step test_012_fail_primary { compose stop node2 # 60s: compose stop can be slow on shared CI runners wait until node2 stopped timeout 60s - wait until node1 state is stop_replication timeout 90s wait until node1 state is primary and node3 state is secondary timeout 90s @@ -206,10 +215,17 @@ step test_014_001_fail_set_properties { } step test_014_002_fail_two_standby_nodes { + # compose stop is a graceful SIGTERM: both secondaries now go through + # start_maintenance() on their own behalf (see keeper_shutdown_via_ + # maintenance() in service_keeper.c) rather than just vanishing, so they + # land in "maintenance", not "catchingup" -- a node in maintenance can + # only leave it via an explicit stop_maintenance() call, so "catchingup" + # can no longer happen here at all. compose stop node2 compose stop node3 - wait until node2 assigned-state = catchingup timeout 180s - wait until node3 assigned-state = catchingup timeout 180s + wait until node2 state is maintenance + and node3 state is maintenance + timeout 180s # node1 remains primary (blocking writes at postgres level) until # number-sync-standbys is explicitly set to 0 in the next step. wait until node1 state is primary timeout 180s diff --git a/tests/test_multi_ifdown.py b/tests/test_multi_ifdown.py index 64f8e7aea..f2f35eb81 100644 --- a/tests/test_multi_ifdown.py +++ b/tests/test_multi_ifdown.py @@ -169,8 +169,11 @@ def test_009_read_from_new_primary(): def test_010_start_node1_again(): - node1.run() - assert node1.wait_until_state(target_state="secondary") + # node1 was hard-killed in test_008 (node1.fail()); restarting it right + # after occasionally hits a Postgres bind race in this test harness (see + # run_and_wait_with_retry()'s docstring), so retry the restart itself + # rather than a plain node1.run(). + assert node1.run_and_wait_with_retry(target_state="secondary") assert node2.wait_until_state(target_state="secondary") assert node3.wait_until_state(target_state="primary") From 5d1f0ee8320d378c0cb127695c38f8224f6229a1 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 15:08:32 +0200 Subject: [PATCH 2/7] Fix citus_indent formatting in service_keeper.c 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. --- src/bin/pg_autoctl/service_keeper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/pg_autoctl/service_keeper.c b/src/bin/pg_autoctl/service_keeper.c index 667484ca0..9150447f4 100644 --- a/src/bin/pg_autoctl/service_keeper.c +++ b/src/bin/pg_autoctl/service_keeper.c @@ -465,7 +465,7 @@ keeper_shutdown_via_maintenance(Keeper *keeper) bool mayRetry = false; if (!monitor_start_maintenance(monitor, keeperState->current_node_id, - &mayRetry)) + &mayRetry)) { log_warn("Failed to enter maintenance for a graceful shutdown, " "likely because there is no candidate currently available " From 35293f452d0def3cd794a8c0beec6e9b9a182c4b Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 15:41:02 +0200 Subject: [PATCH 3/7] Fix run_and_wait_with_retry() to avoid wait_until_state()'s mass-stop 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). --- tests/pgautofailover_utils.py | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/tests/pgautofailover_utils.py b/tests/pgautofailover_utils.py index 501696569..ef9513437 100644 --- a/tests/pgautofailover_utils.py +++ b/tests/pgautofailover_utils.py @@ -1215,15 +1215,42 @@ def run_and_wait_with_retry( does not retry that internally, so plain node.run() can get stuck. Use this instead of a plain node.run() wherever a test restarts a node that was previously killed hard. + + Deliberately polls directly (like wait_until_state() does + internally) rather than calling wait_until_state() itself: despite + its docstring ("returns False" on timeout), it actually *raises* on + timeout, and before doing so it calls print_debug_logs(), which + iterates every node in the cluster and stop_pg_autoctl()'s any that + are still running -- a much bigger side effect than "log something + and let the caller retry". Since this method only restarts *this* + node on the next attempt, that would silently leave every other + node in the cluster stopped and never restarted, turning one node's + transient bind race into the whole cluster wedging. Polling + directly avoids that side effect entirely. """ per_attempt_timeout = max(timeout // retries, 30) for attempt in range(1, retries + 1): self.run(name=name, host=host, port=port) - if self.wait_until_state( - target_state=target_state, timeout=per_attempt_timeout - ): + deadline = dt.datetime.now() + dt.timedelta( + seconds=per_attempt_timeout + ) + reached = False + + while dt.datetime.now() < deadline: + self.sleep(POLLING_INTERVAL) + + try: + current_state, _ = self.get_state() + except Exception: + continue + + if current_state == target_state: + reached = True + break + + if reached: return True print( From 8de7a9c9beaa6fd6524af9a5ec16afd57ff4b7c9 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 15:41:08 +0200 Subject: [PATCH 4/7] Fix pg_ctl_postgres() to allow a socket in PG_REGRESS_SOCK_DIR="" test 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. --- src/bin/common/pgctl.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/bin/common/pgctl.c b/src/bin/common/pgctl.c index c0c5662b0..7a1193f55 100644 --- a/src/bin/common/pgctl.c +++ b/src/bin/common/pgctl.c @@ -1647,6 +1647,23 @@ pg_ctl_postgres(const char *pg_ctl, const char *pgdata, int pgport, args[argsIndex++] = "-h"; args[argsIndex++] = (char *) listen_addresses; } + else if (env_found_empty("PG_REGRESS_SOCK_DIR")) + { + /* + * PG_REGRESS_SOCK_DIR="" means unix sockets are unavailable in this + * environment (see pg_setup_get_local_connection_string, which then + * forces client connections to use "host=localhost" instead). If we + * also pass an empty listen_addresses here, postgres has no way to + * create any socket at all -- TCP disabled by "-h ''", unix socket + * disabled by the "-k" added below -- and fails outright with + * "FATAL: no socket created for listening", even though nothing + * external is meant to connect to it in this "do not open the + * service just yet" mode. Fall back to the loopback interface only, + * matching the same PG_REGRESS_SOCK_DIR convention used elsewhere. + */ + args[argsIndex++] = "-h"; + args[argsIndex++] = "localhost"; + } else { args[argsIndex++] = "-h"; From ef6633e74c47d6a56cb1cdd885ed892984cca962 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 16:04:51 +0200 Subject: [PATCH 5/7] Fix PGAutoCtl.run()'s log-directory creation: FileNotFoundError, then 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. --- tests/pgautofailover_utils.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/pgautofailover_utils.py b/tests/pgautofailover_utils.py index ef9513437..9e235279e 100644 --- a/tests/pgautofailover_utils.py +++ b/tests/pgautofailover_utils.py @@ -2294,6 +2294,20 @@ def run(self, level="-vv", name=None, host=None, port=None): self.stdout_path = self.datadir.rstrip("/") + ".stdout.log" self.stderr_path = self.datadir.rstrip("/") + ".stderr.log" + # The parent directory only exists already by accident, when some + # earlier step in the same test (e.g. creating the monitor) happened + # to create it first: nothing guarantees it in monitor-disabled + # tests, where this can be the very first path created under + # datadir's parent. Chmod it wide open: pytest itself runs as root + # (via plain `sudo`), so a bare os.makedirs() here leaves the + # directory root-owned and mode 0755 -- but pg_autoctl itself runs as + # the unprivileged "docker" user (via `sudo -u docker`), and needs to + # create its own subdirectories under the same parent (e.g. its + # backup directory), which then fails with "Permission denied". + parent_dir = os.path.dirname(self.stdout_path) + os.makedirs(parent_dir, exist_ok=True) + os.chmod(parent_dir, 0o777) + self.stdout_file = open(self.stdout_path, "w") self.stderr_file = open(self.stderr_path, "w") From 8c801ae6fe9ad84d8f70feea05fec033f10e8229 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 16:17:36 +0200 Subject: [PATCH 6/7] Fix two tests broken by SIGTERM now driving a graceful maintenance handoff 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). --- tests/test_basic_citus_operation.py | 11 ++++++++++- tests/test_ensure.py | 9 ++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_basic_citus_operation.py b/tests/test_basic_citus_operation.py index de4935c38..04fb4db4a 100644 --- a/tests/test_basic_citus_operation.py +++ b/tests/test_basic_citus_operation.py @@ -2,6 +2,7 @@ from nose.tools import eq_ import os.path +import signal import time import subprocess import pprint @@ -225,7 +226,15 @@ def test_012_perform_failover_worker2(): def test_013_perform_failover_worker2b_draining(): print() - worker2a.stop_pg_autoctl() + # SIGQUIT here, not the default SIGTERM: this test wants worker2a (the + # secondary) simply down while a failover is forced on the monitor, to + # reproduce the historical get_primary() race described above. SIGTERM + # would instead drive worker2a into maintenance, which -- since it's the + # only secondary with number_sync_standbys=0 -- also forces worker2b's + # goal state to wait_primary as a side effect, so the very next + # perform_failover() call fails outright with "couldn't find the primary + # node" instead of reproducing the race this test is about. + worker2a.stop_pg_autoctl(sig=signal.SIGQUIT) print("Calling pgautofailover.failover(group => 2) on the monitor") cluster.monitor.failover(group=2) diff --git a/tests/test_ensure.py b/tests/test_ensure.py index d89abb672..64e362e65 100644 --- a/tests/test_ensure.py +++ b/tests/test_ensure.py @@ -1,6 +1,7 @@ import tests.pgautofailover_utils as pgautofailover from nose.tools import * +import signal import time import os.path @@ -57,7 +58,13 @@ def test_003_init_secondary(): def test_004_demoted(): print() node1.stop_postgres() - node1.stop_pg_autoctl() + # SIGQUIT here, not the default SIGTERM: this test wants to simulate + # node1's Postgres having already crashed out from under a keeper that + # then also goes away abruptly, so the *next* pg_autoctl run() reports + # "demoted" -- SIGTERM would instead drive a graceful maintenance + # handoff (node1 is still primary at this point), which succeeds and + # never produces "demoted" at all. + node1.stop_pg_autoctl(sig=signal.SIGQUIT) # we need the pg_autoctl process to run to reach the state demoted, # otherwise the monitor assigns that state to node1 but we never reach # it From 7bf73ac5d08d937ff8b428df498e2012a716228e Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 24 Jul 2026 17:28:43 +0200 Subject: [PATCH 7/7] docs: document SIGTERM-to-maintenance for primary, secondary, and catching-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. --- docs/failover-state-machine.rst | 36 +++++++++++++--------- docs/install.rst | 3 +- docs/operations.rst | 10 ++++++ docs/ref/pg_autoctl_enable_maintenance.rst | 8 +++++ docs/ref/pg_autoctl_node_run.rst | 3 +- docs/ref/pg_autoctl_stop.rst | 26 +++++++++++++++- docs/ref/pgaftest.rst | 17 +++++----- 7 files changed, 79 insertions(+), 24 deletions(-) diff --git a/docs/failover-state-machine.rst b/docs/failover-state-machine.rst index 35027adef..4d25dac99 100644 --- a/docs/failover-state-machine.rst +++ b/docs/failover-state-machine.rst @@ -161,29 +161,37 @@ Maintenance ^^^^^^^^^^^ The cluster administrator can manually move a secondary into the -maintenance state to gracefully take it offline. The primary will then -transition from state primary to wait_primary, during which time the -secondary will be online to accept writes. When the old primary reaches -the wait_primary state then the secondary is safe to take offline with -minimal consequences. +maintenance state to gracefully take it offline, and a secondary or +catching-up node reaches this state on its own too when it receives a +plain ``SIGTERM`` (see :ref:`pg_autoctl_stop`) — ``pg_autoctl`` requests +maintenance on its own behalf as part of a graceful shutdown. The primary +will then transition from state primary to wait_primary, during which time +the secondary will be online to accept writes. When the old primary +reaches the wait_primary state then the secondary is safe to take offline +with minimal consequences. Prepare_maintenance ^^^^^^^^^^^^^^^^^^^ The cluster administrator can manually move a primary node into the -maintenance state to gracefully take it offline. The primary then -transitions to the prepare_maintenance state to make sure the secondary is -not missing any writes. In the prepare_maintenance state, the primary shuts -down. +maintenance state to gracefully take it offline, and a primary reaches this +state on its own too when it receives a plain ``SIGTERM`` (see +:ref:`pg_autoctl_stop`) — ``pg_autoctl`` requests maintenance on its own +behalf as part of a graceful shutdown. The primary then transitions to the +prepare_maintenance state to make sure the secondary is not missing any +writes. In the prepare_maintenance state, the primary shuts down. Wait_maintenance ^^^^^^^^^^^^^^^^ -The custer administrator can manually move a secondary into the maintenance -state to gracefully take it offline. Before reaching the maintenance state -though, we want to switch the primary node to asynchronous replication, in -order to avoid writes being blocked. In the state wait_maintenance the -standby waits until the primary has reached wait_primary. +The cluster administrator can manually move a secondary into the maintenance +state to gracefully take it offline, and a secondary reaches this state on +its own too when it receives a plain ``SIGTERM`` (see :ref:`pg_autoctl_stop`) +— ``pg_autoctl`` requests maintenance on its own behalf as part of a +graceful shutdown. Before reaching the maintenance state though, we want to +switch the primary node to asynchronous replication, in order to avoid +writes being blocked. In the state wait_maintenance the standby waits until +the primary has reached wait_primary. Draining ^^^^^^^^ diff --git a/docs/install.rst b/docs/install.rst index 0a69ac790..5670efec4 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -111,7 +111,8 @@ For container and Kubernetes deployments, systemd is not used. Instead, then exec's into the supervisor. The standard Unix signal contract (``SIGTERM`` to stop, ``SIGHUP`` to reload) is preserved because the supervisor becomes the direct child process. See :ref:`pg_autoctl_node` -for the full reference. +for the full reference, and :ref:`pg_autoctl_stop` for what a graceful +``SIGTERM`` actually does before the node stops. Building pg_auto_failover from sources diff --git a/docs/operations.rst b/docs/operations.rst index 518d89802..f619e63db 100644 --- a/docs/operations.rst +++ b/docs/operations.rst @@ -204,6 +204,16 @@ following examples we're directly connecting as the ``autoctl`` role. The main operations with pg_auto_failover are node maintenance and manual failover, also known as a controlled switchover. +.. note:: + + A plain ``pg_autoctl stop`` (a graceful ``SIGTERM``, see + :ref:`pg_autoctl_stop`) requests maintenance the same way as the manual + ``pg_autoctl enable maintenance`` commands shown below, automatically, + on the node's own behalf before stopping. This section covers the + manual commands for when you want a node to stay registered in + maintenance mode independently of a restart, e.g. for a longer OS + maintenance window. + Maintenance of a secondary node ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/ref/pg_autoctl_enable_maintenance.rst b/docs/ref/pg_autoctl_enable_maintenance.rst index 5539282bb..97b5be5cc 100644 --- a/docs/ref/pg_autoctl_enable_maintenance.rst +++ b/docs/ref/pg_autoctl_enable_maintenance.rst @@ -16,6 +16,14 @@ for promotion. Typical use of the maintenance state include Operating System or Postgres reboot, e.g. when applying security upgrades. +A plain ``pg_autoctl stop`` (a graceful ``SIGTERM``, see +:ref:`pg_autoctl_stop`) requests maintenance the same way on its own, +automatically, before stopping — there is no need to run this command +first just to get a clean handoff before stopping a node. A node that +entered maintenance that way also leaves it automatically on its next +start. Use this command directly when you want the node to stay +registered in maintenance mode for a while, independently of any restart. + :: usage: pg_autoctl enable maintenance [ --pgdata --allow-failover ] diff --git a/docs/ref/pg_autoctl_node_run.rst b/docs/ref/pg_autoctl_node_run.rst index 51140694c..c57ab76d9 100644 --- a/docs/ref/pg_autoctl_node_run.rst +++ b/docs/ref/pg_autoctl_node_run.rst @@ -40,7 +40,8 @@ deployments. Given a ``pg_autoctl_node.ini`` file it: Because the command uses ``execv()``, the pg_autoctl supervisor becomes the direct child process (PID 1 in a container), preserving the standard Unix signal contract — ``SIGTERM`` stops the supervisor cleanly, -``SIGHUP`` reloads configuration. +``SIGHUP`` reloads configuration. See :ref:`pg_autoctl_stop` for what a +graceful ``SIGTERM`` actually does before the node stops. The ``launch = deferred`` pattern ---------------------------------- diff --git a/docs/ref/pg_autoctl_stop.rst b/docs/ref/pg_autoctl_stop.rst index 41a650299..11f6d5f39 100644 --- a/docs/ref/pg_autoctl_stop.rst +++ b/docs/ref/pg_autoctl_stop.rst @@ -27,12 +27,36 @@ The ``pg_autoctl stop`` commands finds the PID of the running service for the given ``--pgdata``, and if the process is still running, sends a ``SIGTERM`` signal to the process. -When ``pg_autoclt`` receives a shutdown signal a shutdown sequence is +When ``pg_autoctl`` receives a shutdown signal a shutdown sequence is triggered. Depending on the signal received, an operation that has been started (such as a state transition) is either run to completion, stopped as the next opportunity, or stopped immediately even when in the middle of the transition. +A plain ``SIGTERM`` (the default, no ``--fast`` or ``--immediate`` flag) is +a **graceful** shutdown: for a primary, secondary, or catching-up node, +running with the monitor enabled, ``pg_autoctl`` requests :ref:`maintenance +` on its own behalf before stopping, the same +mechanism used by ``pg_autoctl enable maintenance``. A healthy standby can +then take over immediately, rather than waiting for the monitor's own +health-check timeout to notice the node is gone. This can take up to 30 +seconds. If maintenance cannot be requested — the monitor is disabled, the +monitor is unreachable, or there is no candidate currently available to +take over — ``pg_autoctl`` falls back to stopping Postgres directly and +reporting the shutdown to the monitor for up to another 30 seconds, so a +failover can still be driven by the usual health-check mechanism. + +A node that entered maintenance this way (as opposed to an operator running +``pg_autoctl enable maintenance`` directly) automatically leaves maintenance +the next time it is started, with no need to run ``pg_autoctl disable +maintenance`` manually. An operator-initiated maintenance session is left +untouched by a restart, and still needs an explicit ``pg_autoctl disable +maintenance`` to end. + +The ``--fast`` and ``--immediate`` options skip all of that: they stop the +node right away without attempting a graceful handoff, which is the closest +equivalent to simulating a hard crash. + Options ------- diff --git a/docs/ref/pgaftest.rst b/docs/ref/pgaftest.rst index d771152e6..b655e8a10 100644 --- a/docs/ref/pgaftest.rst +++ b/docs/ref/pgaftest.rst @@ -519,13 +519,16 @@ is actually testing, not whichever one happens to make the test pass. ``docker compose stop`` — SIGTERM to the container's PID 1 (``pg_autoctl``), which forwards a plain SIGTERM to the node-active (keeper) service only (a grace period applies before Docker escalates to - SIGKILL). Exercises graceful shutdown: for a primary, the keeper calls - ``start_maintenance()`` on its own behalf and hands off to a standby - through the ordinary maintenance FSM (``prepare_maintenance`` -> - ``maintenance``), the same transitions an operator-run ``pg_autoctl - enable maintenance --allow-failover`` would drive. For anything else - (a secondary, or a primary maintenance can't be started for), Postgres - is simply stopped and the process exits. + SIGKILL). Exercises graceful shutdown: for a primary, secondary, or + catching-up node, the keeper calls ``start_maintenance()`` on its own + behalf and drives the ordinary maintenance FSM (``prepare_maintenance`` + -> ``maintenance`` for a primary, straight to ``maintenance`` or via + ``wait_maintenance`` for a secondary/catching-up node), the same + transitions an operator-run ``pg_autoctl enable maintenance + --allow-failover`` would drive. If maintenance can't be started (the + monitor is unreachable, or no candidate is available), Postgres is + simply stopped and the process exits, falling back to the monitor's own + health-check-driven failover. ``compose kill `` ``docker compose kill`` — immediate SIGKILL, no grace period at all.