pgaftest: root user DooD, direct libpq, perform failover DSL, UX polish#1150
Open
dimitri wants to merge 53 commits into
Open
pgaftest: root user DooD, direct libpq, perform failover DSL, UX polish#1150dimitri wants to merge 53 commits into
dimitri wants to merge 53 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
compose_gen.c:
- Default to pre-built images (pg_auto_failover:pg<N>, pgaf:pgaftest);
PGAF_BUILD=1 env var forces inline build: stanzas
- Setup service: working_dir /root, spec at /root/spec.pgaf
- Emit [formation default] num_sync_standbys in monitor.ini
test_runner.c:
- monitor_get_node_state(), exec_sql_on_service(), runner_promote_one(),
runner_perform_failover(): all use direct libpq via the existing
LISTEN connection instead of docker exec on the monitor container
- New runner_perform_failover(): calls pgautofailover.perform_failover()
- Remove runner_apply_formation_settings() (replaced by monitor.ini)
- Setup/teardown: --profile setup in compose down, named shell container
%s-sh for reliable cleanup; pre-clean docker rm -f %s-sh
- runner_show_state(): add fputc('\n') after output (fixes missing newline)
- In-container fast paths: pg_autoctl show state for log_formation_state
- Periodic SQL poll inside LISTEN loop (every 5s) to catch already-achieved states
test_spec.h / test_spec_parse.y / test_spec_scan.l:
- Add CMD_FAILOVER to TestCmdKind
- Add 'perform failover [in formation F] [group G]' grammar variants
nodespec.c/.h:
- Read/write/apply num_sync_standbys from [formation ...] ini sections
cli_root.c:
- In-container quick-start note in pgaftest help output (PGAFTEST_IN_CONTAINER)
'promote nodeX' calls pgautofailover.perform_promotion(formation, node_name) which targets nodeX for promotion TO primary. This is the correct DSL command when we want a specific node to become primary. 'exec nodeX pg_autoctl perform switchover' hands off FROM nodeX (makes it hand over its primary role). These are OPPOSITE operations — using switchover where promote was intended caused test_003 to fail after setup because node1 (primary) would hand off, then sql node1 would write to a secondary. Changes across 20 .pgaf spec files: - All 'exec nodeX pg_autoctl perform switchover' → 'promote nodeX' - 'exec monitor pg_autoctl perform failover' → 'perform failover' (uses new direct libpq DSL command)
- DooD section: document root user setup, no entrypoint, /root home - State file: /root/pgaftest.state (not /var/lib/postgres) - Failover DSL: document 'perform failover [in formation F] [group G]' - Env vars: PGAF_IMAGE, PGAFTEST_IMAGE, PGAF_BUILD - PGAFTEST_HOST_WORK_DIR description corrected
Add /* IGNORE-BANNED */ to all getenv(), fprintf(), fopen(), printf(), memcpy(), and related calls in the pgaftest sources and nodespec.c that were added in the previous commits but missing the required annotation for ci/banned.h.sh. Also re-run citus_indent to normalise whitespace introduced by the automated comment insertion.
docs/ref/pgaftest.rst: - Add ASCII architecture diagram (host / compose stack / tmux panes) - Fix interactive session example: basic_operation.pgaf (not basic_failover) - Move DooD architecture and step state file detail to src/bin/pgaftest/README.md; replace with a brief pointer - Fix TAP output example: no 'TAP version 13' header is emitted - Fix Failover DSL section: 'promote <node>' targets that node FOR promotion (to primary); 'exec <node> pg_autoctl perform switchover' asks it to HAND OFF its primary role — document both correctly docs/operations.rst: - Remove the 50-line Testing section; replace with a 3-line stub that cross-references the new top-level pages docs/testing.rst (new): - New top-level Operations page: tutorial for discovering network fault tolerance interactively with 'pgaftest tmux basic_operation.pgaf' - Prerequisites, CI run, step-by-step interactive walkthrough docs/reporting-bugs.rst (new): - New top-level Operations page: how to report a bug with a .pgaf spec - Uses issue #997 (replication stall in 3-DC topology) as a concrete example - Shows diagnostic commands, what to include, how to run the spec headlessly or interactively docs/index.rst: - Add testing and reporting-bugs to the Operations toctree - Add ref/pgaftest to the Manual Pages toctree (was unreachable before) docs/fault-tolerance.rst: - Add 'See also' section pointing to the new testing and reporting-bugs pages src/bin/pgaftest/README.md (new): - Internal implementation notes: DooD architecture, state file format, direct libpq fast-paths, build targets, source file layout
…n, not pg_autoctl version)
…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.
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.