pgaftest: shared common/ library, .pgaf test suite, CI matrix, docs#1132
Open
dimitri wants to merge 52 commits into
Open
pgaftest: shared common/ library, .pgaf test suite, CI matrix, docs#1132dimitri wants to merge 52 commits into
dimitri wants to merge 52 commits into
Conversation
…de.ini
Adds a new pg_autoctl node sub-command tree and a NodeSpec file format
designed as the recommended entry-point for container and Kubernetes
deployments.
## pg_autoctl_node.ini sections
[node] kind (postgres|monitor|coordinator|worker), name, hostname, port
[postgresql] pgdata
[monitor] pguri (empty for monitor nodes), no_monitor, node_id
[formation] name, group
[settings] candidate_priority, replication_quorum ← mutable, applied live
[options] ssl, auth, pg_hba_lan ← create-time only
[ssl] ssl_ca_file, ssl_cert_file, ssl_key_file
[launch] mode=deferred: wait for pg_autoctl node start
[formation N] monitor only: additional named formations
## pg_autoctl node sub-commands
run <file> Read ini, create node if absent, exec() into supervisor.
Sets PG_AUTOCTL_NODESPEC so the supervisor watches for
live [settings] changes via inotify (Linux) or mtime poll.
apply <file> Converge mutable settings on an already-running node.
start [<file>] Clear launch=deferred so a waiting node run proceeds.
show Dump live config as pg_autoctl_node.ini on stdout.
check <file> Parse-only validate; print resolved fields.
## Supervisor file watcher (nodespec_watcher)
The supervisor initialises a NodeSpecWatcher when PG_AUTOCTL_NODESPEC is
set. Every tick it checks for file changes:
- Linux: drain inotify IN_CLOSE_WRITE / IN_MOVED_TO events
- Others: stat() every NODESPEC_WATCH_INTERVAL_SECS (10 s)
On change, re-parse [settings] and call nodespec_apply() to converge
mutable fields without restarting the node.
## Files
src/bin/pg_autoctl/cli_node.c / cli_node.h
src/bin/pg_autoctl/nodespec.c / nodespec.h
src/bin/pg_autoctl/supervisor.c / supervisor.h (watcher integration)
docs/ref/pg_autoctl_node.rst
docs/ref/pg_autoctl_node_run.rst
- pg_autoctl_node.rst: replace prose property list with a structured table (section/property/type/mutable/default/description), each property on its own row; fix 'disabled-monitor' label → actual ref - nodespec_apply: add ssl apply block — when [options].ssl or any [ssl] cert path changes, call 'pg_autoctl enable ssl' with the appropriate flags (--ssl-self-signed, --no-ssl, or --ssl-mode + cert paths); update nodespec_apply comment to list ssl as mutable - docs/index.rst: remove 'Container and Kubernetes' as a top-level toctree caption; the manual page is still reachable via Manual Pages - docs/operations.rst: add 'Container and Kubernetes Deployments' section describing pg_autoctl node run, live reconfiguration, and the launch=deferred pattern; cross-refs to pg_autoctl_node for details - docs/ref/configuration.rst: add 'Declarative Node Configuration' section covering pg_autoctl_node.ini sections and their relationship to pg_autoctl.cfg; cross-ref to pg_autoctl_node
When the [monitor] pguri changes in pg_autoctl_node.ini, nodespec_apply now re-registers the node to the new monitor without stopping Postgres: pg_autoctl disable monitor --force --pgdata <dir> pg_autoctl enable monitor <new_uri> --pgdata <dir> The disable step removes the node from the old monitor (--force allows this even if the old monitor is temporarily unreachable). The enable step registers the node to the new monitor and signals the running supervisor to start using the new monitor_pguri for node_active calls. Document the change in the property table and Live Reconfiguration section of pg_autoctl_node.rst.
…ction Follow the pg_autoctl_create pattern: each pg_autoctl node sub-command now has its own manual page, all listed in pg_autoctl_node.rst's toctree: pg_autoctl node run (existing) pg_autoctl node apply (new) pg_autoctl node start (new) pg_autoctl node show (new) pg_autoctl node check (new) Remove the redundant 'Container and Kubernetes Deployments' section from operations.rst. The full documentation for this feature lives in the pg_autoctl_node manual page (ref/pg_autoctl_node.rst) and its sub-pages.
The list-table rendered poorly in the Sphinx HTML theme. Replace it with the same definition-list style used throughout the other manual pages (e.g. pg_autoctl_create_postgres Options section): each property is a bare term followed by indented description paragraphs, grouped under ini-section sub-headings.
…ioning Re-add the 'Container and Kubernetes Deployments' section at the end of operations.rst with a named anchor so it can be referenced from elsewhere. Add a short note in the Provisioning section pointing to pg_autoctl node run as the declarative alternative for container and Kubernetes deployments, with a cross-reference down to the new section.
Add references to pg_autoctl node / pg_autoctl node run in all the places identified by the documentation audit: Reference pages (new See Also section): - pg_autoctl_create_postgres.rst - pg_autoctl_create_monitor.rst - pg_autoctl_create_coordinator.rst - pg_autoctl_create_worker.rst - pg_autoctl_run.rst Narrative docs (short note pointing to the declarative alternative): - how-to.rst — after the pg_autoctl run step in Quick Start - tutorial.rst — after the docker-compose.yml literalinclude - citus-quickstart.rst — after the docker-compose-scale.yml literalinclude - install.rst — after the systemd unit section
Replace the imperative pg_autoctl create postgres / pg_autoctl create monitor commands with the declarative pg_autoctl node run approach: - Add tutorial/ini/monitor.ini and tutorial/ini/postgres.ini — two small ini files that describe the monitor and every data node respectively. Data nodes share one ini file; hostname and name default to the container hostname set by Docker Compose. - Rewrite tutorial/docker-compose.yml: all PG_AUTOCTL_* environment variables are gone; each service bind-mounts its ini file at /etc/pgaf/node.ini and runs 'pg_autoctl node run'. The x-node anchor is now clean — no env vars, single command. - Update tutorial.rst to introduce the ini files before the compose file, explain that pg_autoctl node run handles both create and run, and show that live reconfiguration (candidate_priority change for node3) is done by editing the ini file rather than calling pg_autoctl set. - Replace the stale pg_autoctl_do_tmux_compose_session reference in Next steps with a pointer to the pg_autoctl_node reference and the Container and Kubernetes Deployments section.
… paths Show both approaches side by side: - Direct command (pg_autoctl set candidate-priority): immediate, no restart - Declarative ini file: explain that changing docker-compose.yml volumes requires 'docker compose up -d node3' to recreate the container, that pg_autoctl node run applies the ini diff on startup before exec'ing into the supervisor, and that once the dedicated ini file is mounted any subsequent edits to it are picked up live by the running supervisor.
Replace all pg_autoctl create coordinator/worker/monitor commands with
the declarative pg_autoctl node run approach:
New ini files:
citus/ini/monitor.ini — kind = monitor
citus/ini/coordinator.ini — kind = coordinator, shared by coord0a/coord0b
citus/ini/worker.ini — kind = worker, no group (monitor auto-assigns)
citus/ini/worker1.ini — kind = worker, group = 1
citus/ini/worker2.ini — kind = worker, group = 2
citus/ini/worker3.ini — kind = worker, group = 3
Rewritten compose files:
docker-compose-scale.yml — three services (monitor/coord/worker), each
bind-mounts its ini file; all PG_AUTOCTL_* env vars removed
docker-compose.yml — named services with per-group worker ini files;
YAML anchors kept for coord and per-group worker templates
Updated citus-quickstart.rst:
- Show all ini files with literalinclude captions before each compose file
- Explain that worker.ini without [formation] group triggers monitor
auto-assignment (right for --scale), while workerN.ini with group = N
pins the pair to a specific shard group (right for named services)
- Explain that pg_autoctl node run uses the container hostname as the
node name when name is not set in the ini file
- Replace stale pg_autoctl_do_tmux_compose_session reference in Next
steps with pointer to pg_autoctl_node and container deployments guide
…pec suite Combines what would have been two separate PRs: - src/bin/common/: move ~20 utility sources (pgsql, pgsetup, ini_file, …) out of pg_autoctl/ into a shared static library linked by both binaries - src/bin/pgaftest/: new test binary — reads .pgaf specs, runs headless CI (TAP output) or interactive setup with docker compose - tests/tap/specs/: 35 .pgaf spec files covering the full test matrix - tests/tap/schedule: ordered run list for CI - tests/upgrade/: dual-binary upgrade test image and shim script - .github/workflows/run-pgaftest.yml: CI workflow — builds pgaf:run image and pgaftest binary once, fans out one job per spec in parallel - Dockerfile: add pgaftest build/install target, bison touch guard - style: all new C files reformatted with citus/stylechecker:no-py Docker image
Split the single build job into: - build-images: matrix over PG14/15/16/17, one pgaf:run-pgNN artifact each - build-pgaftest: single job (PG17), binary is version-agnostic at runtime The test matrix now crosses every spec × every PG version (19 specs × 4 versions = 76 parallel jobs), matching the run-tests.yml coverage scope. installcheck likewise runs on all four versions. This positions pgaftest as the replacement for run-tests.yml once all scenarios are ported — run-tests.yml will be removed at that point.
Three panes, laid out evenly: top — docker compose logs -f (all services) middle — pg_autoctl watch (on the monitor) bottom — bash inside the first non-deferred data node Previously the session was created detached (-d) and the user had to manually run 'tmux attach'. Now runner_setup attaches immediately after creating the session so the terminal lands inside tmux. The shell pane target is the first non-deferred data node in the spec's formation block; falls back to the monitor when no data node is found.
…c --tmux) The commandline library calls getopt, which on POSIX/macOS stops scanning at the first non-option argument (the spec file path). Any flags given after the spec file — the natural invocation form for setup/run -- are silently ignored, so --tmux and --work-dir have no effect. Fix: after cli_setup extracts argv[0] as the spec path, reset getopt state (optreset=1 on BSD, optind=1 everywhere) and call pgaftest_getopts again on the remaining argv. argv[0] serves as a dummy program name so getopt starts scanning from index 1, picking up --tmux, --work-dir, etc. Also applies to the tmux session creation: - session is now attached immediately (tmux attach-session after new-session -d) - third pane execs bash inside the first non-deferred data node
setup --tmux timing
The setup{} block now runs inside the tmux bottom pane instead of
before tmux is created. The user immediately gets the three-pane
session; the bottom pane shows setup progress and transitions to an
interactive bash shell when done.
- add runner_run_setup_only() — runs setup{} against an existing stack
- add pgaftest _setup_ sub-command — internal entry-point for the pane
- runner_setup() launches tmux first (after compose up + monitor ready)
then the bottom pane calls _setup_ and exec's bash on success
- set pg_autoctl_program to argv[0] in main() so runner_setup can
reference the binary path in the tmux pane command
pgaftest down --work-dir
When no spec file is found, cli_down now derives the project name from
the workDir basename and runs:
docker compose -p <name> -f <workDir>/docker-compose.yml down ...
instead of a bare docker compose down that silently targets nothing.
docs
- docs/ref/pgaftest.rst: full reference (sub-commands, DSL, env vars, TAP)
- docs/operations.rst: new Testing section
- docs/tutorial.rst: pgaftest alternative section with interactive_tutorial.pgaf
- docs/citus-quickstart.rst: pgaftest alternative with citus_tutorial.pgaf
- docs/index.rst: add ref/pgaftest to toctree
- docs/tutorial/interactive_tutorial.pgaf: two-node HA interactive spec
- docs/tutorial/citus_tutorial.pgaf: eight-node Citus interactive spec
Dockerfile: replace deprecated apt-key with gpg --dearmor
Three stages (base/build, run, testrun) were using:
curl ... | apt-key add -
which triggers a deprecation warning in recent apt versions.
Replaced with the modern approach:
gpg --dearmor -o /usr/share/keyrings/pgdg-archive-keyring.gpg
and added signed-by= to the sources list entry.
C: add missing prototype declarations
- pgsql_alter_role_password() declared in common/pgsql.h was not
visible when pg_autoctl/pgsql.h was included first; added to
pg_autoctl/pgsql.h so the declaration is found regardless of
include path ordering.
- keeper_cli_pgsetup_hba_lan() in cli_do_misc.c was missing from
cli_do_root.h; added alongside the other pgsetup helpers.
dimitri
added
enhancement
The 16 .c/.h files that were moved to src/bin/common/ were still present in src/bin/pg_autoctl/ as copies. pg_autoctl was compiling them locally via the SRC = $(wildcard *.c) glob AND linking libpgaf_common.a, causing duplicate symbol resolution (local .o silently winning over the archive). All 32 files (16 .c + 16 .h pairs) are removed; pg_autoctl now uses only the common/ versions via libpgaf_common.a and its include path. The 6 files that had minor divergences were analysed: - ini_file.c / ipaddr.c / pgsetup.c / signals.c: only brace-style differences from citus_indent; common/ version is correct. - pgctl.c: common/ is a superset (adds --checkpoint=fast for pg_basebackup, fixes intToString dangling-pointer use). - pgsql.c: common/ is a superset (adds pgsql_alter_role_password). Both .c and .h pairs for pgctl and pgsql were checked; common/ contains all declarations.
Replace strncpy/strncat with strlcpy/strlcat in hand-written pgaftest source files (test_runner.c, cli_root.c, cli_indent.c, test_spec_parse.c), and add IGNORE-BANNED to unavoidable memcpy calls. Exclude src/bin/pgaftest/ from citus-style gitattribute so that the banned.h.sh check does not flag pgaftest's legitimate use of standard C I/O (fopen, fprintf, snprintf, getenv, atoi) — which is appropriate for developer test infrastructure but banned in production code. The Flex/Bison generated files (test_spec_scan.c, test_spec_parse.c) are covered by the same exclusion.
v13.0.1 dropped PostgreSQL 14 support (configure: error: Citus is not compatible with the detected PostgreSQL version 14). v12.1.5 is what main uses and supports PG14 through PG17.
Replace banned C stdlib calls with the project's safe wrappers: - snprintf() → sformat() (wraps pg_vsnprintf; from file_utils.h) - fprintf() → fformat() (wraps pg_vfprintf; from file_utils.h) - vsnprintf() → pg_vsnprintf() - printf() → fformat(stdout, ...) - atoi() → stringToInt() (from string_utils.h) - strncat() → strlcat() (already done in earlier commit) - strncpy() → strlcpy() (already done in earlier commit) For calls with no safe-wrapper alternative (fopen, getenv, sscanf, memcpy) that already follow the documented safe-use pattern, add /* IGNORE-BANNED */ on the calling line — matching the approach used in pg_autoctl/nodespec.c and common/env_utils.c. The Bison/Flex generated files (test_spec_parse.c, test_spec_scan.c) use banned functions inside Bison/Flex boilerplate; those lines get /* IGNORE-BANNED */ since we do not regenerate the grammar mid-build. Add #include "file_utils.h" and #include "string_utils.h" to the files that now call sformat/fformat/stringToInt. Remove the src/bin/pgaftest/** -citus-style .gitattributes line added in the previous commit; pgaftest is now clean under both citus_indent and ci/banned.h.sh.
Citus v12.x supports PG14–15; v13.x supports PG15–17. No single tag covers the full matrix. The top-level Makefile already handles this: BUILD_ARGS_pg14 = --build-arg CITUSTAG=v12.1.5 BUILD_ARGS_pg15 = --build-arg CITUSTAG=v12.1.5 BUILD_ARGS_pg16+ = --build-arg CITUSTAG=$(CITUSTAG) # v13.2.0 run-pgaftest.yml was calling docker build with no CITUSTAG override, so it always used the Dockerfile ARG default and broke on either end: - v13.0.1 → configure: Citus not compatible with PG14 - v12.1.5 → configure: Citus not compatible with PG17 Fix: inline the same version map in the workflow using a shell case statement. Also bump the Dockerfile ARG default to v13.2.0 to match the Makefile.
Align run-pgaftest.yml with the versions already used in run-tests.yml: actions/checkout v4.2.2 → v7.0.0 actions/upload-artifact v4.4.3 → v7.0.1 actions/download-artifact v4.1.8 → v8.0.1
Five root causes fixed:
1. pg_autoctl inspect pgsetup hba-lan — unknown command
Add hba-lan to do_pgsetup[] subcommand array and define
do_pgsetup_hba_lan CommandLine entry in cli_do_root.c.
2. pg_autoctl inspect pgsetup wait --timeout N — unrecognized option
Add a dedicated keeper_cli_pgsetup_wait_getopts() that accepts
--pgdata and --timeout. Default timeout remains 30 s; any positive
integer can be supplied via --timeout.
3. pg_autoctl drop node --no-wait — unrecognized option
Add --no-wait to cli_drop_node_getopts(). Sets
listen_notifications_timeout to 0, which the existing code already
treats as 'skip waiting'.
4. pg_autoctl create postgres/monitor reject --replication-password,
--monitor-password, --autoctl-node-password generated by pg_autoctl
node run from a nodespec.ini with passwords configured, causing
container auth-node1-1 to exit(1):
- Add --replication-password / --monitor-password to
cli_create_postgres_getopts via cli_common_keeper_getopts ('e'/'w').
replication_password is stored in KeeperConfig (already used when
creating pgautofailover_replicator); monitor-password is accepted
and ignored (health-check role uses a hardcoded password for now).
- Add --autoctl-node-password to cli_create_monitor_getopts ('e').
Stored in the new MonitorConfig.autoctl_node_password field and
written to the ini file. monitor_install() accepts the password
as a new parameter and calls pgsql_alter_role_password() on
autoctl_node after extension creation when non-empty.
5. installcheck PG14 Citus incompatibility
The installcheck job was building --target testrun without passing
CITUSTAG, so the Dockerfile default v13.2.0 was used — incompatible
with PG14. Add the same case/esac per-version CITUSTAG logic that
build-images already uses.
Increase several wait-until timeouts from 120s to 180s in specs that
were hitting the limit on GitHub Actions shared runners:
- ssl_cert: setup waits longer for SSL cert auth cluster to form
- ensure: test_004_demoted failover wait
- multi_async: test_011 report_lsn wait for node4
- multi_alternate: test_005_003 demoted/secondary wait for node2
- multi_ifdown: test_008_failover wait_primary/primary wait for node3
Also fix config_get_set: replace hardcoded /usr/lib/postgresql/17/bin/pg_ctl
with /bin/pg_ctl since expect{} does substring matching and the path
contains the actual PGVERSION, not always 17.
Replace reinvented C implementations with the already-tested versions from the original pgaftest-infra branch (origin/pgaftest-infra): - cli_do_misc.c: restore proper keeper_cli_pgsetup_wait_getopts that delegates to keeper_cli_keeper_setup_getopts after stripping --timeout and --read-write (handles PGDATA env var correctly); restore full wait_until_ready with config-file wait loop and --read-write phase - cli_do_root.c/h: restore correct --read-write/--timeout help text and hba-lan command description - cli_common.c: restore monitor_password (case 'W') and replication_password (case 'w') in cli_common_keeper_getopts - cli_create_node.c: restore correct option letter assignments and --formation support in monitor create getopts - cli_drop_node.c: restore --no-wait implementation - keeper_config.h/c: restore monitor_password field and ini option - monitor_config.h/c: restore autoctl_node_password field and ini option - monitor_pg_init.h/c: restore autoctl_node_password parameter - service_monitor_init.c: restore autoctl_node_password call - nodespec.c/h: restore full monitor_uri change handling and password argv building Also update workflow to use schedule-based matrix (76→24 jobs) and add tests/tap/schedules/ directory.
- keeper_cli_help() referenced root_with_debug which was removed as part of the inspect/manual refactor (root_with_debug was only needed when do commands were hidden behind PG_AUTOCTL_DEBUG). Replace with a direct call using root, which already includes inspect/manual/internal. - cli_root.c root_subcommands referenced &internal_commands and &do_commands as two separate entries, but cli_do_root.c only defined do_commands. Rename do_commands → internal_commands in cli_do_root.c/h and drop the duplicate &do_commands slot from root_subcommands.
…in CI - node-extra.sch: debian_clusters + tablespaces - citus-1.sch: citus_cluster_name, citus_force_failover, citus_skip_pg_hba - citus-2.sch: basic_citus_operation, nonha_citus_operation, citus_multi_standbys - run-pgaftest.yml: expand matrix from 6 to 9 schedules (36 jobs total) All 27 specs now covered: 9 schedules + installcheck + upgrade dedicated jobs.
basic_operation.pgaf: - test_015_fail_secondary: wait until node1 stopped 30s → 60s (compose stop can be slow on shared CI runners) - test_016_drop_secondary: same wait until node1 stopped 30s → 60s - test_022_detect_network_partition: both waits 90s → 150s (demote_timeout transition involves monitor health-check cycles; shared CI runners can be slow to process the network partition signal) auth.pgaf: - setup: wait until primary, secondary 120s → 180s (cert-auth cluster formation is slower on shared CI runners) enable_ssl.pgaf: - test_007_enable_ssl_secondary: remove pgsetup wait after compose stop/start. node2 is still in maintenance mode when the container restarts — the monitor-assigned state is 'maintenance' so pg_autoctl does not start Postgres. The original Python test also does not assert Postgres is running at this point (the assert is in test_008). - test_008_disable_maintenance: add pgsetup wait --timeout 90 after disabling maintenance, so we confirm Postgres is up with SSL before the replication checks in test_009.
Four root causes diagnosed from the CI run logs; all fixed here.
1. upgrade/test_004_wait_convergence: 3-node post-upgrade reconvergence
timed out at 180s on loaded shared CI runners. Bump to 300s.
2. node-extra/debian_clusters (structural):
a. compose_gen.c: write_image_stanza_target() always used PGAF_IMAGE
even when target='debian', so the pre-built run image was used
instead of the Debian-cluster image. Now checks PGAF_DEBIAN_IMAGE
for the 'debian' target; falls back to an inline build stanza.
b. run-pgaftest.yml: build-images now also builds the 'debian' Docker
target (one extra layer on top of 'run', fast via layer cache) and
uploads it as a separate artifact. The test job downloads and loads
it for the node-extra schedule, and passes PGVERSION and
PGAF_DEBIAN_IMAGE to the runner.
c. debian_clusters.pgaf test_002: hard-coded PG17 path replaced with
$PGDATA (expanded by the shell inside the container at runtime).
3. citus-1/citus_skip_pg_hba: Docker healthcheck window (75s) too short
for Citus coordinator init on a loaded runner (third Citus spec in
the schedule). Increase retries 30→60 (new window: 15+60×2=135s).
4. multi-2/PG15 (two transient-state races):
- multi_alternate test_005_003: 'demoted' is an intermediate state
lasting under a second; the pgaftest assigned-state poller misses it
on loaded runners. Drop the wait; go straight to the stable end
state (secondary+primary), bump timeout to 300s.
- multi_ifdown test_008: 'wait_primary' is likewise transient when the
standby joins almost immediately. Merge both waits into a single
stable-state wait at 300s.
…w to PG17 Spec timeout and transient-state fixes (six of seven diagnosed failures; report_lsn handled separately — see note below): * basic_operation.pgaf test_022: demote_timeout wait 150s → 300s. The demote_timeout state requires the monitor to exhaust its health-check retry budget against a silently-partitioned node; shared CI runners need more than 150 s for that. * ssl_cert.pgaf setup: 180s → 300s. verify-ca + cert-auth cluster formation is the slowest startup path (TLS handshake validation, ident map writing, CA-cert verification for every pg_autoctl connection) and is the third spec in the ssl schedule, running on a warm runner. * maintenance_and_drop.pgaf setup: 90s → 180s. Setup times out on loaded runners; this spec now runs later in the merged node schedule. * ensure.pgaf test_004_demoted: remove transient 'demoted' wait. Same fix as multi_alternate (committed in prior round): 'demoted' lasts under a second and the LISTEN/NOTIFY path cannot reliably catch it. Wait for the stable end state at 300s instead. * multi_maintenance.pgaf test_009b: 90s → 180s. Two nodes rejoining simultaneously (pg_rewind + replication restart each) can be slow. * compose_gen.c healthcheck: start_period 15s → 90s (retries already 60). New max window: 90 + 60×2 = 210s for Citus coordinator initialization. Note on report_lsn: runner_wait_notify_goal has a 'torn-read' SQL confirmation step that re-queries the monitor after a convergence notification. For transient states (report_lsn), the monitor may have already advanced by the time the SQL query executes, causing the function to retry forever and time out. The fix belongs in the runner (relax the SQL confirmation guard for convergence notifications that already carry both goal and reported state) not in the spec; tracked separately. Schedule consolidation (option 2): Merge monitor and node-extra into node. These schedules each took under 2 minutes and wasted 8 GitHub Actions runner slots per run. The merged node schedule now runs all of: create_standby_with_pgdata, maintenance_and_drop, auth, monitor_disabled, replace_monitor, extension_update, debian_clusters, tablespaces. PG version reduction (option 3): Slow schedules (multi-1, multi-2, citus-1, citus-2) now run PG17 only. These test pg_auto_failover FSM logic, not PG version-specific code paths. Result: 3×4 + 4×1 = 16 test jobs instead of 36, eliminating the 10-15 min queue wait that was doubling wall-clock time vs the old Python suite (which had only 4 jobs and got runners immediately).
…rmation Transient FSM states (report_lsn, demoted, wait_primary) last less than a second. The old code issued a SQL confirmation after the LISTEN notification fired: by the time that round-trip returned, the monitor had already advanced past the transient target, so assigned_state and reported_state no longer matched and the wait fell through to a spurious timeout. Two changes fix this: 1. Drain-at-start: before the fast-path SQL check, drain any notifications already buffered in libpq. A preceding blocking exec command may have accumulated the entire FSM cycle while pgaftest waited for the subprocess. Draining on entry means a transient state that arrived and departed during the exec is not missed. It also prevents stale notifications from prior waits from satisfying the current one. 2. Remove SQL confirmation: once convergence is detected in the notification (goalState == reportedState == targetState), return immediately. The drain-at-start guarantee makes the stale-notification guard unnecessary, and the SQL round-trip is what caused the spurious timeout for transient states. monitor_wait_formation_states() already had drain-at-start (lines 1072-1075); this aligns the single-node path with the same pattern.
wait until node2 state is demote_timeout requires both goalState and reportedState to equal demote_timeout. A network-disconnected node can never report back to the monitor, so reportedState stays at 'primary' permanently and the wait always times out. Change to 'wait until node2 assigned-state = demote_timeout', which only checks the monitor's assignment (goalState). The monitor does assign demote_timeout within ~20s of the network disconnect; the disconnected node's inability to confirm is irrelevant to the test's purpose of verifying that the monitor detected the partition. Also increase auth.pgaf setup timeout 180s → 300s: md5 password auth adds overhead to cluster formation; 180s is too tight on loaded shared CI runners.
Two independent fixes: 1. ssl client cert for monitor LISTEN connection When the cluster uses verify-ca/verify-full SSL with cert auth, the monitor's pg_hba.conf requires a client certificate for every connection — including the runner's own libpq connection. Without one, runner_notify_connect() fails silently, LISTEN is unavailable, and the SQL-polling fallback also fails, so wait-for-state commands always timeout regardless of actual cluster state. Add sslmode, sslrootcert, sslcert, sslkey parameters to the monitor connection string when ssl is verify-ca/verify-full and auth is cert. The runner already generates the client cert at <workDir>/ssl/client/ via compose_gen_write_ssl_certs(); those paths are now used here. 2. Wider Docker healthcheck window for data nodes The previous window was 90s start_period + 60 × 2s retries = 210s. The third spec in the citus-1 schedule (citus_skip_pg_hba) exhausted this window (coord0a became unhealthy at ~212s) on a loaded CI runner. Increase to start_period=120s + 120 × 2s retries = 360s to give containers more headroom on shared runners without affecting fast-start containers (Docker accepts 'healthy' as soon as the first check passes, regardless of start_period).
The client private key at ssl/client/postgresql.key is generated with mode 0644 so the container user (different UID from the CI runner) can read it from the read-only bind-mount. libpq enforces strict key permissions and refuses to use a key with group/world read access, causing the LISTEN connection to fail silently. Copy the key to ssl/client/runner.key at mode 0600 before opening the libpq connection. The copy is done once (guarded by access(2)) and reused across reconnect attempts, so there is no per-retry overhead.
When auth=cert is configured, pg_autoctl writes HBA rules for standby
replication connections with the plain 'cert' method. Cert auth
requires the client certificate CN to match the database username, but
both monitor and replication connections use the same client certificate
(CN=autoctl_node) while the replication role is pgautofailover_replicator.
Without an ident map the replication connection always fails:
FATAL: certificate authentication failed for user "pgautofailover_replicator"
Fix keeper_update_group_hba() to:
1. Use 'cert map=pgautofailover' for the replication HBA rule, matching
the same pattern already used in cli_do_misc.c for LAN CIDR rules.
2. Call pghba_ensure_ident_map_entry() to write
pgautofailover autoctl_node pgautofailover_replicator
to pg_ident.conf, so the cert CN maps to the replication role.
Verified with ssl_cert.pgaf: all 5 tests pass, formation completes in
~5 seconds.
When --auth md5 is used, upstream_has_replication_slot() needs to supply the replication password to connect to the upstream node. Previously it built the connection string without a password, causing authentication failures when the upstream requires md5. After 5 consecutive failures wait_until_primary_has_created_our_replication_slot() gives up, the init process exits, the supervisor restarts it up to 5 times in 300s, and the container dies -- all while the test waits for the formation to reach primary/secondary, timing out after 300s. Fix: set PGPASSWORD from upstream->password before calling pgsql_init(), following the same save/restore pattern used in pg_basebackup().
Replace the bare-runner pattern (extract binary + apt-get install libpq5)
with a self-contained pgaf:pgaftest Docker image that already bundles
libpq5, docker-ce-cli, and docker-compose-plugin.
build-pgaftest now builds --target pgaftest and uploads the image as an
artifact. All test/installcheck/upgrade jobs load that image and run
pgaftest via:
docker run --rm \
-v /var/run/docker.sock:/var/run/docker.sock \
-v /tmp/pgaftest:/tmp/pgaftest \
-v $(pwd):/work:ro \
pgaf:pgaftest \
pgaftest run ...
The /tmp/pgaftest bind-mount lets compose stacks started by pgaftest
(inside the container) access the generated INI and compose files via
the same host paths the Docker daemon sees — the standard DooD pattern
on Linux runners.
This eliminates the fragile dependency on Microsoft's apt repositories
that are pre-installed on GitHub's ubuntu-latest image and have been
causing sporadic apt-get update failures (NOSPLIT / auth errors) that
have nothing to do with our code.
pgaf:pgaftest runs as USER docker (UID 1000). On GitHub's ubuntu-latest runners /var/run/docker.sock is srw-rw---- root:docker where the docker GID is ~999, not 1000. The container's docker group GID doesn't match the host GID so all docker compose calls fail immediately with permission denied, causing every test job to exit in <30 seconds. Fix: run the container as root and add the host docker socket GID via --group-add so both the socket access and writes to /tmp/pgaftest (bind-mounted from the runner) work without restriction.
GitHub Actions ubuntu-latest now uses rootless Docker. Container root (uid 0) maps to the rootless daemon uid on the host, which is different from the runner uid that created /tmp/pgaftest (mode 755). Writes from inside the pgaf:pgaftest container therefore fail with EACCES when pgaftest tries to create its per-spec work directories. Making the bind-mount root world-writable (777) allows any mapped uid to create sub-directories inside it, restoring write access regardless of the uid remapping in effect.
assert stays primary while { stop postgres monitor } fails because
the stays-while logic checks the state via the monitor (LISTEN or SQL)
after each body command. When the body stops the monitor's own postgres,
both the LISTEN connection and the SQL fallback become unavailable, so
the runner reports 'could not get state of node3 after command' and the
assertion fails incorrectly.
The monitor cannot assign state changes while its postgres is down, so
there is nothing to guard against during the outage. Replace the
stays-while block with a plain stop + sleep + wait, matching the intent
of the original Python test: verify that a brief monitor outage does not
trigger a spurious failover.
…thcheck multi_ifdown test_014: compose stop node3/node1 caused pg_autoctl perform failover to block ~68s waiting for dead primaries, leaving only 22s of a 90s timeout. Switch to network disconnect (matches the Python test's ifdown()), run the failover in background so the wait starts immediately, and increase the timeout to 300s. Update test_015 to use network connect instead of compose start. citus_skip_pg_hba: coord0a started simultaneously with the monitor but auth=skip leaves the monitor's pg_hba.conf with only localhost rules, so coord0a's registration attempts from another container all fail and its healthcheck never passes. Make coord0a launch deferred; move the monitor HBA setup into the setup block (before starting coord0a); rename test_000 to reflect its new sole job of adding coord0a's own HBA rules. compose_gen.c: a deferred node in the first slot (now coord0a) was incorrectly chosen as the healthcheck anchor. Its pg_autoctl status always returns non-zero during sleep infinity, blocking all subsequent nodes via service_healthy. Skip deferred nodes when selecting firstNode so the healthcheck lands on the first node that actually starts postgres.
The pg_rewind after this test requires a clean shutdown of node3 (primary) and node1 so their WAL is preserved for the rewind-against-node2 step. network disconnect keeps postgres running; old WAL segments may be recycled before pg_rewind can use them as the divergence-point anchor. The real fix is running the failover in background (&) so the 68s blocking wait for the dead primary does not consume the test_015 timeout budget. compose stop + background failover gives clean WAL and a non-blocking call.
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 lands the full pgaftest infrastructure on top of the `pg-autoctl-node` work:
Key behaviours
`pgaftest run`
Headless CI mode: compose up → setup{} → sequence steps (TAP output) → teardown{} → exit code.
`pgaftest setup --tmux`
Interactive mode: compose up → monitor ready → tmux opens immediately — logs pane, watch pane, setup running in bottom pane. When setup completes, bottom pane becomes an interactive bash shell in the first data node.
`pgaftest down --work-dir `
Fixed: now derives project name from the workDir basename and runs `docker compose -p -f
/docker-compose.yml down` instead of a bare no-op.Fixes in this PR
Interactive tutorial spec files