pgaftest: root user DooD, direct libpq, perform failover DSL, UX polish#1150
Open
dimitri wants to merge 68 commits into
Open
pgaftest: root user DooD, direct libpq, perform failover DSL, UX polish#1150dimitri wants to merge 68 commits into
dimitri wants to merge 68 commits into
Conversation
dimitri
force-pushed
the
pgaftest-improvements
branch
2 times, most recently
from
July 16, 2026 16:19
e7e89d4 to
d9e27ac
Compare
- Dockerfile: trailing backslash from debug edit made the next FROM line get appended to the RUN command, breaking make build entirely. Remove the stray continuation backslash. - expected/upgrade_1.out: PG18 added a 'Default version' column to \dx output; upgrade_1.out is the PG18+ variant. Still showed '2.2' as the default version; update all four rows to '2.3'. - expected/pg19/expected/monitor.out: mirror of monitor.out for PG19 (different pg_lsn display format). Did not include the ORDER BY nodeid clause added to monitor.sql; add it to keep the echoed SQL in the expected output in sync with the query file.
- test_runner.c (runner_compose_generate): after writing docker-compose.yml and node ini files, copy the spec file to <workdir>/spec.pgaf. This lets 'pgaftest step' reload the spec without the user supplying the original path again. - test_runner.c (runner_init): derive the Docker Compose project name from the work-directory basename (e.g. replication_stall_3dc) rather than the spec filename basename. When the spec is loaded from the in-workdir copy spec.pgaf the old code yielded 'spec', which did not match the running stack and caused 'no such container: spec-node2-1' errors. COMPOSE_PROJECT_NAME env var still takes precedence (used inside the compose network). - replication_stall_3dc.pgaf: add 'ssl off' to the cluster block. Without it pgaftest defaults to self-signed SSL, which requires certificate generation before Postgres starts; node1 failed with 'could not load server certificate file server.crt'.
parseCurrentNodeState already parsed 17 columns (0-16), including noderegion at column 14 added with the --region / issue-997 work. The guard check above it still said 16, causing pg_autoctl watch to log 'Query returned 17 columns, expected 16' in a tight loop and refuse to display any node state.
…ries The old Makefile had a phony 'common' target that ran make -C common, then both pg_autoctl and pgaftest depended on it. However, because 'common' was .PHONY, make always considered it out of date and rebuilt it on every invocation — and a parallel make -j could still race the two sub-makes against each other's common/*.o output files since both include Makefile.common and carry the same compile rules. Fix: make a real file target. Both pg_autoctl and pgaftest depend on it, so make builds the archive once serially first, then the two binaries in parallel without contention.
In --tmux mode the bottom pane previously exec'd into the first data node container, which had no pgaftest binary, no docker CLI, and no knowledge of the spec file. New approach: the pgaftest service (already emitted by compose_gen for CI) is reused as the interactive shell target. That container has: - the pgaftest binary - docker + docker compose (DooD via /var/run/docker.sock) - /spec.pgaf bind-mounted from the host workDir - COMPOSE_PROJECT_NAME, PGAFTEST_HOST_WORK_DIR, PG_AUTOCTL_MONITOR set compose_gen_write gains an 'interactive' parameter: when true the pgaftest service uses 'sleep infinity' instead of 'pgaftest run /spec.pgaf', keeping it alive for a shell. The interactive flag is set when runner_setup is called with withTmux=true. The bottom pane command changes from: docker compose exec -it <node1> bash to: docker compose exec -it pgaftest sh -c 'pgaftest _setup_ ... && exec bash' Four new sub-commands mirror the DSL keywords, callable directly from the interactive shell (context resolved from PGAFTEST_SPEC / PGAFTEST_HOST_WORK_DIR env vars injected by the compose stack): pgaftest wait until <node> state = <state> [timeout <N>s] pgaftest sql <node> "<query>" pgaftest network connect|disconnect <node> pgaftest assert <node> state = <state> After setup the bottom pane prints a hint listing available steps and example commands so the user knows immediately what they can type.
Two bugs in the previous commit:
1. hintCmd[256] was smaller than the literal format string (292 bytes)
before even inserting the step list, triggering sformat's BUG assert.
2. The bottom pane command used sh -c "..." with single-quote delimiters
inside; step names or other text containing apostrophes would break
the shell syntax, causing 'unexpected EOF' on startup.
Fix: drop hintCmd and the sh -c wrapper entirely. The bottom pane now
runs pgaftest _setup_ directly via docker compose exec (no sh -c), and
cli_run_setup_only prints the hint to stdout (the pane tty) after the
setup block completes, then execlp("bash") to hand the pane over to an
interactive shell. No string embedding, no quoting concerns.
…TEST_SPEC - pgaftest help (and bare pgaftest) now prints usage instead of "unknown command" - pgaftest show is now a sub-command set: show compose|spec|steps|services - pgaftest step accepts optional <spec.pgaf> second arg; when omitted it falls back to PGAFTEST_SPEC env var, /spec.pgaf in CWD, or <workDir>/spec.pgaf - compose_gen emits PGAFTEST_SPEC=/spec.pgaf into the pgaftest service env so all interactive sub-commands (step, wait, assert, network, sql) can discover the spec without explicit path arguments inside the container - resolve_interactive_context() helper centralises the env fallback logic
- pgaftest show step: lists sequence steps with:
* next step to run
! last failed step (will retry)
(space) completed steps
- pgaftest step (no args): auto-advances to the next step using a state
file at <workDir>/pgaftest.state. On failure current stays at the
failed step so the next no-arg invocation retries it.
- pgaftest step <name>: still works; also updates the state file and
advances current when the named step matches the cursor position.
- TestRunnerState struct + runner_state_read / runner_state_write /
runner_step_next added to test_runner.c/h; state is JSON for easy
inspection with a text editor.
- show_resolve_spec() now derives workDir when it is not set, so
pgaftest show step <spec.pgaf> picks up the state file automatically.
getopt permutes argv to move flags before positionals when optind is reset to 0 (full re-initialisation). Without this, the library's first getopt pass at the root level stopped at the first non-option (the subcommand name), leaving any flags that appeared before the spec file — e.g. "setup --tmux spec.pgaf" — unprocessed by the sub-command's getopt, which then saw an empty positional list and printed the usage error. Setting optind=0 (as every pg_autoctl getopts function does) tells getopt to reorder the argument vector so all options are seen before non-option positionals, regardless of the order the user typed them. Also simplify cli_setup back to its original form now that ordering is handled at the getopt level instead of by hand.
Replaces commandline_help() (one-level flat listing) with commandline_print_command_tree() so that pgaftest help renders the full command tree with sub-command groups expanded, matching the output style of pg_autoctl help.
The workDir bind-mount was :ro, preventing runner_state_write from creating pgaftest.state inside the container. The state file belongs in workDir alongside the generated compose files, so drop the :ro flag.
The pgaftest image already creates a 'docker' user with HOME=/var/lib/postgres. Switch the pgaftest compose service from root to that user: user: docker working_dir: /var/lib/postgres spec mounted at /var/lib/postgres/spec.pgaf This lands the interactive shell in the docker user's HOME with the spec file right there, and aligns the SSL cert paths with ~/.postgresql/.
…file Adds: - Host vs. container command table with descriptions - Typical interactive session walkthrough - DooD architecture section explaining the pgaftest service container - Step state file section - Updated synopsis and sub-command reference to match current CLI (show compose|spec|step|services, step auto-advance, etc.)
runner_state_path() picks the state file location based on whether PGAFTEST_COMPOSE_SERVICE is set: - inside container → $HOME/pgaftest.state (always writable by docker user) - on host → <workDir>/pgaftest.state (alongside compose files) The bind-mounted workDir may not be writable by the container's docker user on Linux hosts where UIDs don't align. $HOME (/var/lib/postgres) is owned by the docker user in the image, so it is unconditionally writable. Update docs/ref/pgaftest.rst to reflect the new state file locations.
…ate/step Command table changes: - setup / prepare / down moved under new 'cluster' subcommand group (pgaftest cluster setup|prepare|down) - pgaftest tmux <spec.pgaf> replaces pgaftest setup --tmux; --tmux option removed from pgaftest_getopts entirely - pgaftest wait and pgaftest assert removed (not useful interactively) show subcommand changes: - show steps (plural, restored) — sequence list with */ ! progress markers - show step (singular, new) — prints the DSL commands of the next step to run - show state (new) — 'Step X/N: name' header then pg_autoctl show state output - show services removed New functions in test_runner.c: - test_cmd_print(): prints a TestCmd in DSL form to a FILE* - runner_show_state(): prints step progress header then pg_autoctl show state
The old name implied the variable held a service name. It is actually a boolean flag (set to "1") that tells the binary it is running inside the pgaftest container — which determines where to write the step state file (container $HOME vs host workDir). Also complete the docs/ref/pgaftest.rst update: Synopsis and Sub-commands sections now reflect the restructured CLI (cluster setup/prepare/down, tmux, show steps/step/state, removal of wait/assert/show services).
…eady When the monitor container is still starting up, 'pg_autoctl watch' fails on its first connection attempt and exits, closing the tmux pane. Add --wait to pg_autoctl watch: it calls pgsql_set_monitor_interactive_retry_policy() before entering the main loop, which makes the internal pgsql_open_connection() retry for up to 15 minutes with exponential back-off (same policy used by the demo app and enable/disable maintenance commands). pgaftest now passes --wait in the tmux middle pane so the watch pane stays alive while the monitor container initialises.
…r startup Makefile.common: replace individual -Werror=implicit-* flags with a single -Werror so all -Wall warnings are fatal on both macOS and Linux. The generated bison/flex files already carry -Wno-error in the pgaftest Makefile so those remain unaffected. test_runner.c: the tmux bottom pane (docker compose exec -it pgaftest) exited immediately when the container wasn't yet running, closing the pane before the user could see it. Fix: probe with a no-TTY 'docker compose exec -T pgaftest true' loop until the container accepts exec, then hand off to the interactive command. Same timing issue as the watch pane (fixed earlier with --wait). Also log the manual exec command on startup so the user can reattach to the pgaftest shell from the host tmux pane if needed: docker compose -p <project> -f <workdir>/docker-compose.yml exec -it pgaftest bash
Convenience command for opening a new interactive shell in the running pgaftest container from a host tmux pane: pgaftest cluster sh [--work-dir <dir>] Equivalent to: docker compose -p <project> -f <workdir>/docker-compose.yml exec -it pgaftest bash Resolves the project name and workdir the same way as the other cluster subcommands (--work-dir flag, PGAFTEST_HOST_WORK_DIR env, or derived from the spec file path).
The 'sleep infinity + exec' model caused a timing race: tmux opens the
pane before the pgaftest container is in 'running' state, so exec fails
and the pane closes immediately. The probe-loop workaround was a band-
aid; the real fix is to not require a background service at all.
docker compose run starts a fresh container on demand from the service
definition (same image, bind-mounts, env, network) without needing the
service to already be running. No timing race, no probe loop.
Changes:
- runner_compose_up: pass --scale pgaftest=0 in interactive mode so the
image is built but no background container is started
- tmux session now has 4 panes when the spec has a setup{} block:
pane 0 docker compose logs -f
pane 1 pg_autoctl watch --wait
pane 2 pgaftest _setup_ via docker compose run --rm (closes when done)
pane 3 interactive bash via docker compose run --rm -it
Without setup{}: 3 panes (pane 2 is the interactive shell directly)
- cluster sh: updated to use docker compose run --rm -it as well
…est/.last) pgaftest cluster setup / tmux write the active work directory to $TMPDIR/pgaftest/.last on startup. resolve_interactive_context() reads it as the final fallback when neither --work-dir nor PGAFTEST_HOST_WORK_DIR nor a spec file is available. Effect: after 'pgaftest tmux tests/.../foo.pgaf', all subsequent host commands work without arguments: pgaftest cluster sh pgaftest cluster down pgaftest show steps pgaftest show state
runner_down() now kills the tmux session (named after the project) after compose down, so a subsequent 'pgaftest tmux' for the same spec does not hit 'duplicate session: <name>'. The kill is best-effort (2>/dev/null || true) so it is harmless when no tmux session exists.
The pgaftest container runs as the 'docker' user but /var/run/docker.sock is owned by a host group (GID 1 on macOS Docker Desktop, typically 999 on Linux) that does not match any group inside the container. This caused: permission denied while trying to connect to the docker API at unix:///var/run/docker.sock Fix: stat /var/run/docker.sock at compose-generation time and emit 'group_add: [<gid>]' in the pgaftest service so the container user gains the socket's group as a supplementary group, regardless of what numeric GID the host assigns to it.
- runner_setup: check 'tmux -V' before doing anything when withTmux is
true; log the version at NOTICE level, error out cleanly if tmux is
not on PATH. Uses run_cmd_capture, the same helper used everywhere
else in the runner.
- Setup container renamed from <project>-pgaftest-run-<hash> to
<project>-setup so compose logs show a short, readable name.
Interactive shell container named <project>-sh for the same reason.
Both still carry --rm so they are removed when they exit.
docker socket permission history
---------------------------------
Previously the pgaftest service ran as 'user: root'. Root bypasses the
socket's group-ownership check, so DooD worked without any group
membership.
We changed to 'user: docker' (an unprivileged UID) to avoid running
test commands as root inside the container. /var/run/docker.sock is
typically owned root:docker with mode 0660 (or root:<N> where N is the
host docker GID, which varies: 1 on macOS Docker Desktop, ~999 on most
Linux distros). The container's 'docker' user is not in that group, so
every docker CLI call got EACCES.
The previous commit added 'group_add: ["<gid>"]' by statting the
socket at compose-generation time. 'group_add' is honoured by both
'docker compose up' and 'docker compose run', so it applies to the
setup and shell containers started via 'run --rm' as well. The compose
file must be regenerated ('pgaftest cluster down && pgaftest tmux') for
the fix to take effect on an existing cluster.
…ude from up
Root cause of monitor not running
-----------------------------------
In interactive mode, runner_compose_up passed --scale pgaftest=0 to
prevent the sleep-infinity container from starting. Docker Compose v2
still rebuilds ALL service images when --build is present, even for
scaled-to-zero services. On a re-run (stack already up from a previous
pgaftest tmux invocation), 'up --build -d --scale pgaftest=0' rebuilds
and recreates the monitor/node containers, causing a brief downtime
window during which runner_wait_for_monitor could time out, tear the
stack down, and leave the cluster gone — hence 'monitor not running'.
Fix
---
In interactive mode the service is now named 'setup' and carries
'profiles: [setup]'. Services with a profile are completely invisible
to 'docker compose up' (no build, no start, no scale interaction).
'docker compose run setup' still works and starts the container on
demand. This means:
- 'docker compose up --build -d' only builds and starts cluster
services (monitor, nodes) — never touches the setup image.
- Subsequent pgaftest tmux runs on an already-running stack are
non-destructive: up is a no-op for running healthy containers.
- --scale pgaftest=0 removed; no longer needed.
Service naming
--------------
Interactive mode: service named 'setup' — appears as '<project>-setup'
in logs and docker ps. The --name flag on docker compose run gives
containers the stable names '<project>-setup' (setup pane) and
'<project>-sh' (interactive shell pane).
CI mode: service still named 'pgaftest', no profile, command set to
'pgaftest run <spec.pgaf>' so 'docker compose up --exit-code-from
pgaftest' continues to work unchanged.
…y ensures readiness) In tmux mode the host process does not use the monitor LISTEN connection at all — it just starts the stack and hands off to the tmux session. docker compose up -d already waits for the full depends_on: service_healthy chain before returning, so the monitor is provably ready when up exits. runner_wait_for_monitor from the host is both unnecessary and the source of the intermittent 'monitor not running' failure: on Docker Desktop for Mac, published-port connections appear as 192.168.65.1, which is outside the monitor's pg_hba trust CIDR. The fallback pg_hba patch and the 120s timeout loop both ran on the critical path before tmux launched, creating a wide window for races. runner_apply_formation_settings only needs docker compose exec (internal network) — no host->monitor libpq — so it works fine without the wait. runner_wait_for_monitor is still called in CI mode (runner_run) where the host process genuinely drives steps via LISTEN/NOTIFY.
…ke build The setup container runs Docker-out-of-Docker (DooD) by bind-mounting /var/run/docker.sock. Running as root eliminates GID mismatch between macOS Docker Desktop and Linux CI, where the docker socket group ID varies by host. Changes: - Dockerfile: remove docker user, sudo, entrypoint.sh; add WORKDIR /root - Makefile.docker: add build-pgaftest and force-build-pgaftest targets, include build-pgaftest in the default 'make build' target
…en CI timeouts The direct libpq path for promote and perform failover unconditionally required r->notifyConnected, so any CI environment where the runner cannot reach the monitor's published host port (firewall, different network namespace) caused immediate failure for every spec that calls 'promote' or 'perform failover'. Replace the PQexecParams calls with exec_sql_on_service(), which already has dual-path logic: uses the LISTEN connection when available, falls back to docker compose exec psql otherwise. The fallback path was already exercised by monitor_get_node_state() and all wait-until polling, so this makes promote/perform-failover consistent with the rest of the runner. Also tighten CI timeouts: job 40→25 min, step 35→20 min. Schedules complete in under 10 minutes when healthy; a 20-minute cap fails fast without burning an entire runner-hour on a hung job.
Failure 1 (tablespaces): perform failover DSL is async (fires SQL, returns
immediately). The spec expected sync behaviour like the old 'exec monitor
pg_autoctl perform failover' CLI, which blocks on LISTEN until a node
reaches primary. Add an explicit 'wait until node2 state is primary' before
the first write to the new primary.
Failure 2 (citus_force_failover setup): 'exec coordinator1a pg_autoctl
perform switchover' is synchronous but the monitor stalls the worker group
FSM until the coordinator group is fully settled. Running worker switchovers
immediately after coordinator switchover races against that settling. Add
explicit 'wait until coordinator1b state is primary and coordinator1a state
is secondary' before each worker switchover.
Failure 3 (upgrade): v2.2 supervisor spawns service subprocesses via
'pg_autoctl do service {postgres,listener,node-active}'. Commit c32eb03
renamed the 'do' prefix to 'internal'. After the symlink flip in the
upgrade test, the v2.2 supervisor re-execs the shim which now delegates to
the v2.3 binary. v2.3 does not recognise 'do service ...', prints the root
help, and exits 1. The supervisor retries five times in rapid succession and
hits its crash-loop guard, killing the monitor container.
Fix: add 'do service → internal service' translation in pg_autoctl_shim.sh.
The shim comment that said v2.2 uses 'internal service' was wrong; corrected.
The file is more accurately described as a version-compatibility wrapper
than a shim — it translates old command forms (v2.2 'do service') to the
current form ('internal service') in addition to delegating to the current
binary. Update Dockerfile.current COPY reference accordingly.
v2.2 supervisor spawns service subprocesses as 'pg_autoctl do service X'. v2.3 renamed that to 'pg_autoctl internal service X' (commit c32eb03). During an in-place upgrade the v2.2 supervisor (PID 1) re-execs after the symlink flip, so v2.3 must still accept the old 'do service X' form. Fix: register 'do' as a hidden root command in v2.3, pointing to the same do_subcommands[] as 'internal'. The routing table matches 'do service postgres', 'do service listener', and 'do service node-active' to the same callbacks. No version detection or translation is needed. With 'do service X' working natively in v2.3, the bash compat wrapper (pg_autoctl_compat.sh) is entirely redundant: - 'pg_autoctl node run <ini>' is handled natively by both v2.2 and v2.3. - 'do service X' now works in v2.3 without translation. Replace the compat wrapper with a plain symlink: /usr/local/bin/pg_autoctl -> pgaf/current/pg_autoctl The symlink resolves through pgaf/current (initially -> pgaf/2.2, after flip -> pgaf/2.3). PG_AUTOCTL_DEBUG_BIN_PATH keeps pointing to the symlink path so pg_autoctl_argv0 stays consistent across the flip and the version-check re-exec works correctly. Also drop the bash apt install from Dockerfile.current (no longer needed).
…ropped keeper_ensure_node_has_been_dropped() queries the monitor via libpq. It expects keeper->monitor.pgsql.connectionType == PGSQL_CONN_MONITOR, set by monitor_init(). In the state-file-exists code path the monitor struct was zero-initialised (PGSQL_CONN_LOCAL = 0), so the query went to local Postgres instead, which has no pgautofailover schema. The v2.2 -> v2.3 upgrade exposed this: after the monitor extension reaches 2.3, v2.2 node-active children exit(EXIT_CODE_MONITOR) and the v2.2 supervisor re-execs them from the v2.3 binary. The re-exec runs pg_autoctl create postgres --run, which lands in keeper_pg_init. With a pre-existing state file the old code path called keeper_ensure_node_has_been_dropped before any monitor_init, producing "schema pgautofailover does not exist" with a "Postgres" error prefix (the local-connection tell-tale). Five rapid FATAL exits tripped the v2.2 supervisor's restart-rate limit and killed the container. Fix: call monitor_init() immediately after local_postgres_init() so the monitor pgsql handle carries PGSQL_CONN_MONITOR before the first query.
…onitor
Adds the 'legacy-startup' cluster keyword to the pgaftest DSL. When set,
compose_gen emits 'pg_autoctl create <kind> --run' as the container command
instead of the v2.3 'pg_autoctl node run <ini>' form. This matches exactly
what production operators running v2.2 had, so the upgrade test starts the
cluster the same way real deployments do.
The upgrade spec also switches from 'pg_autoctl manual service restart
postgres' to 'compose stop monitor' + 'compose start monitor'. The in-
process restart triggered a race: the v2.2 listener reconnected to the
just-restarted Postgres at the same moment data-node keepers did, producing
transient 'schema pgautofailover does not exist' errors that exhausted the
v2.2 supervisor restart limit and killed all three data-node containers.
A full container restart gives data nodes a clean 'monitor is down, keep
retrying' signal; when the monitor comes back with the v2.3 binary (symlink
already flipped) there is no listener race.
The 'legacy-startup' keyword drives:
- test_spec.h: legacyStartup bool on TestCluster
- test_spec_scan.l / test_spec_parse.y: T_LEGACY_STARTUP token + rule
- compose_gen.c: write_legacy_monitor_command / write_legacy_node_command
emit the v2.2-style create … --run command with ssl flags spelled out
on the command line (the ini-based path is not available in v2.2)
Dockerfile.current comment updated to explain the compose stop/start
rationale and document the full upgrade mechanism.
Dockerfile.current:
- Rename pgaf/2.1 → pgaf/prev, pgaf/2.2 → pgaf/next (version-agnostic)
- Replace symlink-based shim with a plain symlink:
/usr/local/bin/pg_autoctl → pgaf/current/pg_autoctl
- Use pgautofailover* wildcard COPY for extension files
install-extension.sh:
- Rewrite as version-agnostic using pg_config at runtime
- Copies any pgautofailover*.control / *.sql from pgaf/next/ into
the system extension directory; copies pgautofailover.so into pkglibdir
Together these make the upgrade test independent of hardcoded version
strings in filenames: the same Dockerfile and install script work for
any prev→next pair without modification.
dimitri
force-pushed
the
pgaftest-improvements
branch
from
July 16, 2026 16:39
d9e27ac to
1a7e577
Compare
…rade debug queries
The SQL query in monitor_get_current_state explicitly selects 16 columns (formation_kind … nodecluster, plus healthlag and reportlag from the JOIN). The check in parseCurrentNodeStateArray was left at 17 from an aborted noderegion feature that added the column to the pgautofailover--2.2.sql function definition but never updated the C SELECT to request it, and never updated pgautofailover.sql (the canonical source that make rebuilds --2.2.sql from). Result: make rebuilds --2.2.sql with 16 columns, binary checks for 17, every pg_autoctl show state call fails. Fix: correct the check to 16, and remove the orphaned noderegion OUT parameter from pgautofailover--2.2.sql so it stays consistent with pgautofailover.sql after a make install.
… PID 1 When the supervisor runs as PID 1 inside a container, orphaned grandchildren (e.g. the postgres process after the postgres service controller exits) are reparented to it by the kernel and show up in waitpid(). This is expected behaviour, not a bug. Log at INFO with a clear message instead of ERROR so it does not look like an internal error in CI logs. Non-PID-1 deployments keep the log_error to surface genuine unexpected child reaping.
Two bugs in keeper_pg_init_and_register hit during upgrades from v2.1: 1. The state-file path (when pgdata already exists) called keeper_ensure_node_has_been_dropped without first calling monitor_init, causing the pgsql struct to use PGSQL_CONN_LOCAL (zero-init default) instead of PGSQL_CONN_MONITOR. The dropped-node query then went to local Postgres, which has no pgautofailover schema. (First fixed in commit 7697949, now protected by the test.) 2. The same path used config->groupId straight from CLI defaults (-1), so get_nodes('default', -1) returned no rows and the keeper wrongly concluded it had been dropped and re-registered from scratch. Fix: call keeper_config_read_file before the dropped-node check so the persisted groupId, formation, and monitor_pguri are loaded from disk. The v2.1 supervisor hits both bugs before dying (5 restarts, then exit). The upgrade test is updated to accommodate this: after the monitor extension is upgraded the test waits 20 s for the v2.1 containers to exit, then restarts them with 'compose start'. The restarted containers run the v2.2 binary (symlink already flipped) which handles the state-file path correctly. pgaftest fix: runner_promote_one now resolves the docker service name (nodehost) to the pgautofailover-registered nodename before calling perform_promotion(). v2.1 assigns sequential names (node_1, node_2, ...) that differ from the container hostnames (node1, node2, ...) so calling perform_promotion('default', 'node2') would fail with 'node not registered'; the lookup by nodehost finds the correct name first.
Replace the two-step lookup (resolve nodename then call perform_promotion) with a single subquery that does both at once. UNIQUE (nodehost, nodeport) in the schema guarantees nodehost is unique across the cluster, so no LIMIT is needed.
test_004_verify_pgdata_kept used 'exec node2' which requires a running container. After pg_autoctl drop node --no-wait, the keeper detects the monitor-side removal on its next heartbeat and calls pg_ctl stop before exiting, taking the container with it. By the time test_004 runs the container is already stopped so docker compose exec returns exit 256. Fix: add 'wait until node2 stopped' first (confirms keeper self-exited), then use 'run node2' to access the named volume directly via a fresh one-shot container.
monitor_wait_until_primary_applied_settings() waits for a primary/apply_settings notification that never fires when all nodes are in report_lsn. This happens during the test_016 scenario: all candidate-priorities set to 0, failover triggered, all nodes enter report_lsn, then one priority is raised. The function timed out after 60 s (PG_AUTOCTL_LISTEN_NOTIFICATIONS_TIMEOUT) and returned false, causing the CLI to exit with EXIT_CODE_MONITOR. Fix: before calling the wait function, scan the node list we already fetched. If no node is in PRIMARY_STATE or WAIT_PRIMARY_STATE, skip the wait entirely. In the no-primary case the candidate-priority change triggers the election FSM directly (not through apply_settings), so there is nothing to wait for — the caller can poll node states independently.
…mary When pg_autoctl enable maintenance --allow-failover is called on the primary, two sequential phases must complete within the wait window: 1. Promote a secondary to primary (up to ~60 s) 2. Assign and reach MAINTENANCE_STATE on the old primary (up to ~60 s) monitor_wait_until_node_reported_state() used PG_AUTOCTL_LISTEN_NOTIFICATIONS_ TIMEOUT (60 s) as a single hard ceiling, shared across both phases. In CI the failover leg alone consumed ~34 s, leaving only ~26 s for the maintenance assignment — not always enough. The timer expired, the CLI exited with EXIT_CODE_MONITOR, and pgaftest marked the step failed. Fix: add a timeoutSecs parameter to monitor_wait_until_node_reported_state so callers can control the window. The enable-maintenance path passes 3 * PG_AUTOCTL_LISTEN_NOTIFICATIONS_TIMEOUT (180 s) when --allow-failover is set on a primary. All other callers pass the original 60 s constant.
…ner DNS Docker's embedded DNS (127.0.0.11) relies on per-container iptables DNAT rules that can be lost under heavy container churn on GHA, causing sporadic 'Name or service not known' failures when nodes try to reach each other by hostname. This change makes the generated docker-compose.yml: - Assign a deterministic /24 subnet per project name (djb2 hash of the project name into the 172.20-172.255 range) - Give every service a fixed static IPv4 address within that subnet - Add a lightweight _dns service (alpine + dnsmasq) at .2 that serves /etc/pgaf-hosts (written next to docker-compose.yml by pgaftest) - Point every node's 'dns:' to the dnsmasq container IP instead of Docker's embedded resolver IP layout within each /24: .2 _dns (dnsmasq) .3 monitor .4 second_monitor (when present) .5+ data nodes The pgaf-hosts file is skipped when compose_gen_write() is called with /dev/stdout (the 'pgaftest show compose' dry-run path).
…ECONDARY
The postmaster writes PM_STATUS_READY / PM_STATUS_STANDBY to postmaster.pid
a few milliseconds before the TCP listener is actually bound.
pg_setup_wait_until_is_ready() (called via ensure_postgres_service_is_running)
returns as soon as the PID-file flag is set, leaving a narrow window where
pgIsRunning is reported as true to the monitor but connections are still
refused.
This causes two distinct CI failures:
1. Connection refused: test queries a standby immediately after it reaches
SECONDARY state, hitting the window before the TCP socket is open.
2. start_maintenance '0 candidate nodes available': CountHealthyCandidates()
called IsHealthy() on the secondary just as the health-check worker ran
and marked it BAD (because TCP was not yet open).
Fix: in the two FSM transitions that promote a standby to SECONDARY
(CATCHINGUP->SECONDARY via fsm_prepare_for_secondary, and
JOIN_SECONDARY->SECONDARY via fsm_follow_new_primary), switch the local sql
client to the init retry policy (up to 15 minutes, exponential back-off from
5 ms to 2 s) and call pgsql_get_postgres_metadata() before declaring success.
pgsql_get_postgres_metadata calls pgsql_open_connection, which falls through
to pgsql_retry_open_connection (PQping loop) when the initial PQconnectdb
fails, so no new mechanism is needed. The function also captures the current
LSN, sync state and control-file data as a useful side-effect.
tests/test_multi_alternate_primary_failures.py: test_005_002 was checking for
the transient assigned state 'report_lsn' which lasts only milliseconds; the
monitor advances through it to 'prepare_promotion' before the poll can see it.
Wait for the stable end state 'primary' instead (300s timeout).
tests/tap/specs/multi_alternate.pgaf: same fix for the pgaftest path.
…est spec
The 'wait until node3 assigned-state = report_lsn' used
runner_wait_assigned_goal which does NOT pre-drain the libpq notification
buffer on entry. Under load on GHA the state can arrive and be consumed
during the preceding compose-kill exec, making it invisible to the
assigned-state poll loop.
Switch to:
wait until node3 state is primary passing through report_lsn timeout 300s
This routes through runner_wait_notify_goal which:
1. Drains the entire buffered notification queue on entry via
runner_drain_notify(), catching states that arrived while the
previous exec was blocked.
2. Tracks report_lsn as a required pass-through goalState — fails if
the monitor skipped it.
3. Waits for the stable end-state (primary reported-state) rather than
a transient millisecond assignment.
The pytest counterpart (test_multi_alternate_primary_failures.py) was
already fixed in the prior commit to poll wait_until_state('primary')
since SQL polling cannot catch transient states at all.
write_legacy_node_command() generated 'pg_autoctl create postgres' without --name, causing the monitor to auto-assign node_N names from the sequence counter. Those names are derived from the insertion order, not from the service names in the compose file, so they diverge when nodes restart in a different order — for example in test_008_failover_post_upgrade where the upgrade process brings nodes up sequentially. The registered name mismatch showed up as nodes not being found by service name in subsequent steps. Fix: pass the service name (n->name) as --name so the monitor always registers the node under the same identifier as the compose service.
Replace the inline 'alpine:3 + apk add dnsmasq' command in the generated
docker-compose.yml with a pre-built pgaf:dnsmasq image.
Problems with the previous approach:
- apk add runs at container start, fetching from Alpine CDN on every test
run: slow (~2-5s) and fragile if the network is slow on GHA runners.
- No healthcheck: other containers could start before dnsmasq was ready,
causing initial hostname resolution failures.
- Extra network failure mode independent of the test under execution.
Changes:
Dockerfile — new 'dnsmasq' target: FROM alpine:3, apk add dnsmasq,
HEALTHCHECK with nslookup, ENTRYPOINT that reads
/etc/pgaf-hosts via --addn-hosts and forwards to
configurable upstream DNS servers (DNS1/DNS2 env vars).
compose_gen.c — _dns service uses PGAF_DNS_IMAGE env var (default
pgaf:dnsmasq) instead of alpine:3 with inline install.
Healthcheck is emitted in the compose YAML so depends_on
can use service_healthy.
monitor, second monitor, and all data nodes gain
depends_on: _dns: condition: service_healthy
so no node starts before DNS is available.
Build:
docker build --target dnsmasq -t pgaf:dnsmasq .
Override image:
PGAF_DNS_IMAGE=my-registry/dnsmasq:latest pgaftest run spec.pgaf
Add a build_dnsmasq job that builds the new Dockerfile 'dnsmasq' target
once per workflow run and passes the image to every pgaftest consumer:
build_dnsmasq:
- FROM alpine:3 target, no PGVERSION dependency
- GHA layer cache (scope=pgaf-dnsmasq) — fast cache hits on re-runs
- Saves pgaf:dnsmasq to a tarball artifact (retention-days: 1)
Wire-up:
test_pgaftest — needs: [build_run_images, build_pgaftest, build_dnsmasq]
downloads + loads dnsmasq-image artifact
passes -e PGAF_DNS_IMAGE=pgaf:dnsmasq to docker run
upgrade — same: needs build_dnsmasq, downloads + loads, passes env var
The generated docker-compose.yml already uses PGAF_DNS_IMAGE (default
pgaf:dnsmasq) for the _dns service, so once the image is loaded on the
runner the variable just confirms the name. Previously the compose file
used 'alpine:3' with 'apk add dnsmasq' at startup, which fetched from
Alpine CDN on every test run and had no healthcheck, causing a race where
data nodes could start before DNS was ready.
…node
master_update_node() on the Citus coordinator takes a distributed lock
that conflicts with active queries touching the old worker's shards.
Without a timeout the call blocks indefinitely. This was observed as a
20-minute CI hang: the citus_cluster_name spec performs three switchovers
in sequence; the coordinator switchover completed in ~6s but the
immediately-following worker1a switchover blocked in coordinator_update_node_prepare
because coordinator1b (the newly-promoted coordinator) had in-flight
distributed transactions from before its own failover.
Fix: after pgsql_begin() and before the master_update_node() call, issue:
SET LOCAL lock_timeout = '<cooldown>ms';
SET LOCAL statement_timeout = '<cooldown*2>ms';
lock_timeout fires if the distributed lock isn't acquired within the
cooldown window (default 10 000 ms). statement_timeout is a backstop for
the whole call in case something else inside master_update_node stalls
after the lock is acquired. Both are SET LOCAL so they are cleared
automatically at transaction end and do not affect other uses of the
connection.
On timeout the transaction is aborted with an error; the FSM logs the
failure and retries the transition on the next keeper tick instead of
hanging forever. The no-force path (older Citus without the force
argument) was previously completely unbounded; it now also benefits from
this guard.
When pg_autoctl set node candidate-priority raises a node's priority above the current primary, the monitor may trigger a full failover instead of the usual apply_settings cycle. In that case the old primary is assigned report_lsn (not apply_settings) as its first FSM step, so the apply_settings wait loop never saw the new primary reach primary/primary and timed out after 60 s. Fix: add failoverInProgress to ApplySettingsNotificationContext. When the known primary node is assigned report_lsn we set this flag and switch to waiting for ANY node in the formation to reach primary/primary instead of continuing to track the apply_settings cycle for the original primary node.
The grammar comment and test spec files use 'passing through' as the natural English phrasing: wait until node3 state is primary passing through report_lsn but the scanner only recognised the single keyword 'through'. Add a two-word flex rule that matches 'passing' followed by whitespace and 'through', returning a single T_THROUGH token — so both forms work with no grammar changes. Update test_spec_scan.c accordingly.
compose_gen.c: compose_network_name() was returning '<project>_default' but the generated compose YAML defines the network as 'pgafnet', so 'docker network disconnect/connect' were failing with 'network not found'. Fix: return '<project>_pgafnet' to match the compose YAML. test_runner.c: 'docker network connect' without --ip lets Docker assign a fresh DHCP address. The dnsmasq pgaf-hosts file maps each node to its original static IP; PostgreSQL resolves HBA hostnames through that file. When the container gets a different IP after reconnect, the HBA rule no longer matches and streaming replication fails with 'no pg_hba.conf entry'. Fix: read the static IP from workDir/pgaf-hosts and pass --ip to 'docker network connect' so the container re-uses its original address. cli_enable_disable.c: align ternary operator indentation to pass citus_indent. multi_alternate.pgaf: test_005_002 starts node2 before killing node1 so that node2 participates in the LSN quorum. node2 was SIGKILLed in test_005_001 and never reported 'demoted'; it remained registered as a sync standby with replication_quorum=true. With number_sync_standbys=1 the monitor needs 2 LSN reporters; without node2 back online only node3 would report, stalling the failover indefinitely. test_005_003 no longer needs compose start node2. multi_standbys.pgaf: increase test_013_restart_node2 timeout from 90s to 180s. After network disconnect/reconnect, node2 must go through: contact monitor -> monitor notifies node1 -> node1 updates HBA -> pg_rewind/basebackup from node1 -> streaming replication -> secondary. 90s was insufficient.
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
This PR bundles three related improvements to the
pgaftestintegration test runner that eliminate Docker socket permission issues, remove a class of docker-exec round-trips, and add a new failover DSL command.Changes
1. Root user / DooD fix (Dockerfile + Makefile)
The setup service bind-mounts
/var/run/docker.sockto run Docker-out-of-Docker. Previously it ran as adockeruser with a GID that had to match the host socket group — fragile on macOS Docker Desktop and Linux CI.dockeruser,sudo, andentrypoint.shfrom the pgaftest Dockerfile stageFROM debian:bookworm-slimdefaults to root; addWORKDIR /rootso the state file lands in/root/pgaftest.statebuild-pgaftestandforce-build-pgaftestMakefile targets; includebuild-pgaftestinmake buildpg_auto_failover:pg\<N\>,pgaf:pgaftest); setPGAF_BUILD=1to force inlinebuild:stanzas2. Direct libpq paths (test_runner.c, compose_gen.c, nodespec.c/.h)
Several operations previously shelled out to
docker exec monitor pg_autoctl .... These now go directly via the existing LISTEN connection (r->notifyConn):monitor_get_node_state(): direct libpq fast-path before docker-exec fallbackrunner_promote_one(): callspgautofailover.perform_promotion(formation, node_name)runner_perform_failover(): callspgautofailover.perform_failover(formation, group)exec_sql_on_service(): direct libpq for monitor SQLrunner_apply_formation_settings()removed —num_sync_standbysis now written intomonitor.iniat compose-gen time and applied bynodespec_apply()3.
perform failoverDSL command (test_spec grammar)New DSL command for untargeted failover (monitor picks best secondary):
Distinct from
promote nodeX(targeted promotion of a specific node to primary).4. Spec corrections (tests/tap/specs/*.pgaf)
Fixed a semantic inversion across 20 spec files:
exec nodeX pg_autoctl perform switchover→promote nodeX(switchover hands off from nodeX; promote targets nodeX to primary — opposite semantics)
exec monitor pg_autoctl perform failover→perform failover5. UX polish (cli_root.c, test_runner.c)
pgaftest helpappends an interactive quick-start block whenPGAFTEST_IN_CONTAINER=1pgaftest show stateoutput now ends with a newline (was missing)--profile setupincompose down, named shell container%s-shfor reliabledocker rm -fon exitTesting
basic_operation.pgaf27/27 PASSmulti_standbys.pgaf27/27 PASSFollow-up
PR #1149 (issue #997 monitor stall fix) will be rebased on top of this once merged.