tests: add pgaftest .pgaf spec suite and CI workflow - #1138
Merged
Conversation
dimitri
added
enhancement
Introduces the full pgaftest test suite under tests/tap/ and the GitHub Actions workflow that runs it. Depends on the pgaftest binary (#1137). ## tests/tap/specs/ 32 .pgaf specs covering the complete test matrix previously expressed as Python/nosetests files. Each spec encodes topology (cluster{}), setup/teardown, and an ordered step sequence. Single-node / basic: basic_operation, basic_operation_listen_flag, create_run (via ensure), create_standby_with_pgdata, config_get_set, skip_pg_hba, auth, enable_ssl, ssl_cert, ssl_self_signed, ensure, maintenance_and_drop Monitor: monitor_disabled, replace_monitor, extension_update, installcheck Multi-standby: multi_standbys, multi_alternate, multi_async, multi_maintenance, multi_ifdown Citus: basic_citus_operation, nonha_citus_operation, citus_multi_standbys, citus_cluster_name, citus_force_failover, citus_skip_pg_hba Platform / upgrade: debian_clusters, tablespaces, upgrade ## tests/tap/schedule Lists all specs in the run order used by the CI matrix. ## tests/tap/README.md Documents the .pgaf DSL, how to run specs locally with pgaftest, and how to add new specs. ## .github/workflows/run-pgaftest.yml Builds the pgaftest Docker image once (FROM Dockerfile pgaftest target), then fans out one job per entry in tests/tap/schedule in parallel. Each job mounts the spec file, runs pgaftest run, and reports TAP.
dimitri
force-pushed
the
pgaftest/test-suite
branch
from
July 9, 2026 16:54
63a7a51 to
d17e706
Compare
… targets Two fixes found by analyzing CI failures on PR #1138: 1. pg_autoctl inspect pgsetup wait: picks up PGDATA from environment The previous keeper_cli_pgsetup_wait_getopts had its own minimal option parser that never called cli_common_get_set_pgdata_or_exit, so keeperOptions.pgSetup.pgdata was empty when --pgdata was not given on the command line. cli_common_pgsetup_init then failed immediately with "BUG: keeper_config_set_pathnames_from_pgdata: empty pgdata". Fix: delegate to keeper_cli_keeper_setup_getopts (after stripping --timeout and --read-write) so that PGDATA is picked up from the environment. Also port the richer implementation from pgaftest-infra: config-file appearance wait, shared deadline, and --read-write phase. 2. tests/upgrade/Makefile: add pgaf-next / pgaf-current targets The CI workflow runs make -C tests/upgrade pgaf-next / pgaf-current but the old Makefile had no such targets (only build / up / down etc). Replace with the new Makefile that auto-detects PREV_TAG from git tags and defaults.h, and add the companion files it needs: Dockerfile.current, pg_autoctl_shim.sh, install-extension.sh.
…n-agnostic paths
pg_autoctl drop node --no-wait (exit 35072):
- Add early exit in cli_drop_local_node() after cli_drop_node_from_monitor()
when config->listen_notifications_timeout == 0 (i.e. --no-wait). Without
this the code falls through into the supervisor-wait loop where the process
gets SIGKILLed (exit 137 = raw waitpid 35072) before the supervisor exits.
pg_autoctl create monitor --formation <name>:
- Restore MonitorConfig.formationCount / formationNames / formationKinds
fields and MONITOR_MAX_FORMATIONS removed in a previous refactor.
- Restore --formation and --autoctl-node-password options in
cli_create_monitor_getopts() so nonha_citus_operation and similar specs
that declare named formations actually create them.
- Restore monitor_install() autoctl_node_password parameter and the
ALTER ROLE ... PASSWORD call after CREATE EXTENSION.
- Restore the post-init formation-creation loop in service_monitor_init_start()
that calls monitor_create_formation() for each --formation argument.
Spec fixes (version-agnostic paths, transient-state races):
- debian_clusters.pgaf: use ${PGDATA}/postgresql.conf instead of hardcoded
/var/lib/postgresql/17/main/postgresql.conf so PG14-16 CI jobs pass.
- config_get_set.pgaf: expect /bin/pg_ctl (substring) instead of full
/usr/lib/postgresql/17/bin/pg_ctl path; also sync ssl.sslmode test from
reference branch.
- ensure.pgaf, multi_alternate.pgaf: skip waiting for the transient 'demoted'
state (sub-second, races on loaded CI runners); wait for the stable
secondary+primary end state with a 300s timeout instead.
- multi_ifdown.pgaf: replace the unreliable test_014_secondary_reports_lsn
(compose stop + network connect + blocking perform failover) with the
reference design: stop node3+node1 cleanly, reconnect node2, non-blocking
failover, wait for node2 at report_lsn. Add test_015_start_node3_node1
step so node2 can pg_rewind missing WAL from the survivors.
Full sync of pgaftest/test-suite to match pgaftest-infra-combined exactly.
Restores all spec, C-source, and build-file differences that had drifted
since the branch was split:
Spec fixes (transient-state races, version-agnostic paths, test redesigns):
- basic_operation.pgaf: wait for stable end state around demote_timeout;
use 'assert stays primary while { stop postgres monitor }' pattern;
longer CI-safe timeouts
- auth.pgaf, enable_ssl.pgaf, ssl_cert.pgaf, upgrade.pgaf: minor alignment
- citus_skip_pg_hba.pgaf: coord0a no longer deferred; use
'pg_autoctl override pgsetup hba-lan'; rewritten test sequence
- monitor_disabled.pgaf: inline JSON via shell printf instead of
/etc/pgaf/specs/*.json bind-mount; remove companion JSON files
- multi_async.pgaf: reorder node4 disconnect around LSN election
- multi_maintenance.pgaf, multi_standbys.pgaf: remove redundant intermediate
waits; adjust timeouts to match reference
C source (functional):
- cli_common.c: --monitor-password mapped to no-op 'w'; --replication-password
to 'e'; matches option letters expected by pg_autoctl node run
- cli_create_node.c: remove --monitor-password / --replication-password from
cli_create_postgres_getopts (handled via node.ini, not CLI)
- cli_do_misc.c, cli_do_root.c/.h: restore reference implementation of
'pg_autoctl inspect/override pgsetup' sub-commands
- cli_drop_node.c: restore dropNoWait bool (reference style)
- cli_root.c, config.c, coordinator.c, demoapp.c, fsm.c, keeper.c,
keeper_config.c/.h, keeper_pg_init.c, monitor.c, monitor_config.c,
nodespec.c/.h, primary_standby.c, state.c, watch.c: variable renames,
comment updates, and minor refactors aligned to reference
- ipaddr.c: minor fix from reference
- formation_metadata.c, node_metadata.c: minor monitor-side tweaks
Build / misc:
- Makefile, Dockerfile: realigned to reference versions
- Makefile.azure: removed (deleted in reference)
- .gitignore: restored reference entries
- docs/: restore citus-quickstart.rst, operations.rst, tutorial.rst
- tests/network.py: minor update
- tests/upgrade/install-extension.sh, pg_autoctl_shim.sh: file-mode sync
…ucceeds v2.1's Makefile includes Makefile.azure (line 37). When the upgrade test extracts the v2.1 git archive into a tmpdir and substitutes the current Dockerfile, the build stage only COPYd Makefile and Makefile.citus — so make failed with 'Makefile.azure: No such file or directory'. Add a glob COPY so Makefile.azure is picked up when present in the build context (v2.1 archive) and silently skipped when absent (current tree).
dimitri
force-pushed
the
pgaftest/test-suite
branch
from
July 10, 2026 00:57
c5c069a to
e91612b
Compare
GCC cannot prove the fill loop runs at least once, so it warns that ms may be uninitialized when passed to runner_drain_notify. Zero-init the array to make the intent explicit and suppress the warning cleanly.
On Docker Desktop for Mac, connections through published ports appear to the container as 192.168.65.1 (the Docker Desktop VM gateway), which is outside the Docker bridge CIDR that pg_autoctl adds to pg_hba when --pg-hba-lan is set. This caused the direct libpq LISTEN connection to fail immediately, falling through to subprocess polling with the message 'LISTEN not available' — meaning no real-time state notifications and no '* [notify]' convergence markers in the output. When the subprocess readiness check confirms the monitor is up but LISTEN has not connected yet, append 'host all all 0.0.0.0/0 trust' to pg_hba and reload. This is safe for local test containers that already run with --auth trust. Retry the LISTEN connection once after the reload; the 'LISTEN not available' fallback remains for environments where even that fails.
Two related fixes: 1. Inter-command drain must not run before wait commands. runner_exec_step drained the libpq notify buffer before EVERY command using NULL mark arrays. When a wait command (CMD_WAIT_STATE, CMD_WAIT_STATES, CMD_WAIT_MULTI) followed an exec that triggered state transitions, the convergence notifications were consumed without '*' marks, then the wait's own initial drain found nothing left — the '*' prefix was silently dropped. Skip the inter-command drain when the upcoming command is a wait. Each wait command already opens with its own drain that passes the correct mark arrays, so the '*' prefix is applied where it belongs. 2. Suppress pg_autoctl's own log output from stop/start postgres. CMD_STOP_POSTGRES and CMD_START_POSTGRES called 'docker compose exec' without -T and without output redirection, so pg_autoctl's internal log lines (with their own timestamp/PID) appeared on stdout interleaved with pgaftest's output. Add -T and redirect to /dev/null.
Replace the /dev/null redirect with run_cmd_capture_both(), which captures stdout and stderr in memory and shows the combined output only when the command fails. Normal successful runs stay quiet; error output is available exactly where it's needed. run_cmd_capture_both() appends '2>&1' to the shell command before calling popen(cmd, "r"). The shell redirects fd 2 onto fd 1 before exec'ing the child, so both streams arrive on the single pipe end that popen returns. We read until EOF, trim trailing whitespace, and drain any overflow past the buffer so pclose() doesn't leave an unread pipe (which would send SIGPIPE to the child and make docker report exit 137).
Two-phase drain strategy for CMD_WAIT_MULTI:
1. Drain with marks immediately after each subprocess check, so
notifications that accumulated during the ~200ms docker exec
are consumed and marked before we test allMet.
2. When allMet (or allListenMet) turns true, loop wait_socket(200ms)
until the socket goes quiet, draining with marks on each pass.
This catches notifications that were sent by the monitor after the
database commit (which the subprocess reads) but haven't yet
traversed the Docker Desktop VM network layer to our libpq socket.
The race that caused missing '*' markers:
- Monitor commits state change → sends NOTIFY → responds to node-active
- Subprocess queries the database and sees the committed state
- NOTIFY is still in transit through the VM network (~5-50ms on Mac)
- Without the post-success drain loop, NOTIFY arrived after return
and was consumed unmarked by the next command's inter-drain
The 200ms wait_socket loop terminates as soon as the socket is idle,
so in the common case (CI Linux where NOTIFY arrives before subprocess
returns) there is no extra latency; on Docker Desktop for Mac it waits
just long enough for the in-flight NOTIFY to arrive.
Introduce notify_flush_until_satisfied() for the post-convergence
drain in CMD_WAIT_MULTI instead of a fixed-window 200ms wait loop.
The previous code looped while wait_socket(200ms) returned true,
so it exited on the first 200ms window of silence. The race:
- drain (after subprocess): consumes wait_primary→primary (no *)
- post-success loop: wait_socket(200ms) returns false (socket quiet)
- return true
- primary→primary arrives 50ms later → consumed by [03-03]
inter-drain without marks
The new helper loops until either ALL listenSatisfied[i] flags are
set (every expected convergence NOTIFY has arrived and been marked)
or 1 second has elapsed. This means the flush continues as long as
there are still outstanding NOTIFYs to collect, and only exits early
when the socket is quiet AND all conditions are already marked.
The 1-second cap is a generous safety valve; in practice the helper
returns as soon as the last in-flight NOTIFY lands (sub-second on
local Docker networks).
Two changes to test_004_demoted: 1. Wait for node1=primary before stopping it. After step 003 stops and restarts postgres on node2, the cluster may go through a second oscillation (node2→catchingup, node1→ wait_primary) before settling again. If compose stop node1 fires while node1 is in wait_primary the monitor does not trigger a failover, and the step hangs until timeout. 2. Wait for node2=wait_primary (not primary) before restarting node1. node2 reaches 'wait_primary' (point of no return) once the monitor has committed to promoting it, but it cannot reach 'primary' while node1 is stopped — it needs a synchronous standby to acknowledge WAL. When node1 restarts it connects as a sync standby and node2 then transitions wait_primary → primary. Waiting for 'primary' here would time out because node2 can never reach that state without node1.
…on test
Both tests now cover the same scenarios as their Python predecessor
(tests/test_ensure.py):
test_004_demoted:
- Stop postgres first while pg_autoctl is still running so it can
report the outage to the monitor before being killed (matches Python's
node1.stop_postgres() followed by node1.stop_pg_autoctl())
- Add explicit wait for the 'demoted' state after node1 comes back,
matching Python's assert node1.wait_until_state('demoted')
test_005_inject_error_in_node2:
- After injecting the bad config and stopping postgres (pgctl off),
re-enable auto-start with 'pgctl on' (ignoring its exit code, since
postgres cannot come up with the broken config) so pg_autoctl keeps
retrying — this matches Python's node2.restart_postgres() intent
where postgres repeatedly fails to start, and the monitor eventually
triggers a failover to node1
The 'demoted' state is sub-second transient: node1's pg_autoctl may execute the demoted transition and report 'catchingup' before the convergence NOTIFY (demoted ➜ demoted) is ever emitted on the LISTEN channel, causing 'wait until node1 state is demoted' to time out on loaded CI runners. The Python test handles this differently: it starts node1 without waiting for postgres to come up and polls the database directly. The pgaf LISTEN-based approach cannot reliably catch the window. Keep only the stable end state check (node2=primary, node1=secondary), which is what the test ultimately cares about.
This was referenced Jul 10, 2026
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
Introduces the full pgaftest test suite under
tests/tap/and the CI workflow that runs it. Depends on thepgaftestbinary (#1137, already merged).tests/tap/specs/
32
.pgafspecs covering the complete test matrix previously expressed as Python/nosetests files. Each spec encodes topology (cluster{}), setup/teardown, and an ordered step sequence.Single-node / basic
basic_operation,basic_operation_listen_flag,create_standby_with_pgdata,config_get_set,skip_pg_hba,auth,enable_ssl,ssl_cert,ssl_self_signed,ensure,maintenance_and_dropMonitor
monitor_disabled,replace_monitor,extension_update,installcheckMulti-standby
multi_standbys,multi_alternate,multi_async,multi_maintenance,multi_ifdownCitus
basic_citus_operation,nonha_citus_operation,citus_multi_standbys,citus_cluster_name,citus_force_failover,citus_skip_pg_hbaPlatform / upgrade
debian_clusters,tablespaces,upgradetests/tap/schedule
Lists all specs in the run order used by the CI matrix.
tests/tap/README.md
Documents the
.pgafDSL, how to run specs locally withpgaftest, and how to add new specs..github/workflows/run-pgaftest.yml
Builds the
pgaftestDocker image once (from thepgaftestDockerfile target), then fans out one job per entry intests/tap/schedulein parallel. Each job runspgaftest runand reports TAP output.