pgaftest: new test infrastructure, GitHub Actions CI, upgrade test#1125
Closed
dimitri wants to merge 48 commits into
Closed
pgaftest: new test infrastructure, GitHub Actions CI, upgrade test#1125dimitri wants to merge 48 commits into
dimitri wants to merge 48 commits into
Conversation
Add pgaftest binary and .pgaf test spec infrastructure:
- src/bin/common/Makefile.common: shared build variables included
by both pg_autoctl and pgaftest Makefiles
- src/bin/pgaftest/: new binary for CI testing and interactive
cluster prep (not shipped on production nodes)
- Flex/Bison DSL parser (test_spec_scan.l, test_spec_parse.y)
- AST structs (test_spec.h)
- Docker Compose generator (compose_gen.c)
- TAP test runner (test_runner.c): CI mode (pgaftest run) and
interactive mode (pgaftest setup)
- CLI dispatch (cli_root.c): run/setup/step/show/down/demo
- pg_autoctl inspect: always-visible read-only diagnostics grouping
do_show, do_pgsetup, do_monitor, do_service commands
- pg_autoctl override: always-visible manual FSM operations grouping
do_fsm, do_primary, do_standby, do_coordinator, do_service commands
(intended for manual recovery, not normal automation)
- PG_AUTOCTL_DEBUG no longer gates command visibility; inspect and
override are always shown in --help output
- Azure integration removed from cli_do_root.c and cli_do_root.h
(unmaintained; no replacement in pgaftest)
- tests/tap/specs/: 27 .pgaf spec files porting all Python tests
- tests/tap/schedule: ordered test schedule for pgaftest run
Four issues fixed: 1. Remove azure source files (azure.c, azure_config.c, cli_do_azure.c, cli_do_tmux_azure.c, azure.h, azure_config.h): the wildcard SRC= in pg_autoctl/Makefile picked them up, causing compile errors because the azure integration was already stripped from cli_do_root.c. 2. Makefile.common: detect macOS keg-only gettext and add its include path so postgres_fe.h → c.h → libintl.h resolves without error. 3. pgaftest grammar (test_spec_parse.y): remove dead %type declarations for step_block and command/command_list that had no grammar rules, which caused bison to fail. 4. pgaftest lexer (test_spec_scan.l): remove %option noinput since the brace-block collector rule calls input() directly. 5. pgaftest/Makefile: expand SHARED_SRCS to all non-CLI pg_autoctl sources (except watch.c which needs ncurses); remove watch.c to avoid -lncurses dependency in the test binary. 6. pgaftest/main.c: add globals required by shared pg_autoctl sources that pg_autoctl's main.c normally provides: pgconnect_timeout, ps_buffer, last_status_len, log_semaphore, pg_autoctl_program, pg_autoctl_argv0, monitorOptions, root, root_with_debug. 7. pgaftest/test_runner.c: include postgres_fe.h before pqexpbuffer.h to ensure pg_attribute_printf and bool are defined first. 8. pgaftest/cli_root.c: include defaults.h for EXIT_CODE_QUIT.
16 C source modules and 15 headers moved out of pg_autoctl/ into the shared common/ directory, built once as libpgaf_common.a and linked by both pg_autoctl and pgaftest: debian env_utils file_utils ini_file ini_implementation ipaddr lock_utils parsing pgctl pgsetup pgsql pgtuning pidfile signals string_utils system_utils These are generic PostgreSQL/system infrastructure utilities with no pg_auto_failover FSM/keeper/monitor business logic. pg_autoctl-specific files (keeper*, monitor*, fsm*, coordinator, service_*, state, supervisor, pghba, systemd_config, watch, cli_*) remain in pg_autoctl/. Makefile.common: add COMMON_SRC/COMMON_OBJ/COMMON_LIB variables and rules to build src/bin/common/*.c → libpgaf_common.a. Both pg_autoctl and pgaftest Makefiles add $(COMMON_LIB) to OBJS and depend on it. pgaftest/Makefile: remove the 16 moved modules from SHARED_SRCS; they are now covered by libpgaf_common.a. Include path already has -I../common via Makefile.common so all #include "filename.h" still resolve.
Default goal (link failure when invoking make directly in a sub-directory): Makefile.common defines as its first explicit target. When included before the 'all:' rule it becomes the default goal, so 'make -C src/bin/pgaftest' rebuilds libpgaf_common.a and exits without ever linking the pgaftest binary. Fix: add .DEFAULT_GOAL := all before the include in both pg_autoctl/Makefile and pgaftest/Makefile. Dangling-pointer warnings in common/: intToString() returns an IntString struct by value. Storing .strValue (a pointer into that temporary) in an array slot or local pointer variable leaves a dangling pointer once the full-expression ends. Fix: capture the IntString in a named local variable so its lifetime covers all subsequent uses of the pointer. - pgctl.c:1635 args[argsIndex] = intToString(pgport).strValue - pgsql.c:1583 nodeIdString = intToString(node->nodeId).strValue
- compose_gen: add monitor healthcheck so data nodes wait until monitor
PostgreSQL is accepting connections before starting; make node2 depend
on node1 so registration order is deterministic (node1 gets nodeid=1)
- cli_root (pg_autoctl): re-add do_commands to the root command table;
the 'do service postgres/node-active/monitor' sub-commands are spawned
internally by the supervisor and must remain accessible
- pgctl: add --checkpoint=fast to pg_basebackup to avoid waiting for a
checkpoint (default spread mode could block 5+ minutes); fix args[16]
→ args[18] to prevent buffer overflow when slot name is provided
- basic_operation.pgaf: complete rewrite matching test_basic_operation.py
· remove candidate-priority=0 (was blocking maintenance with failover)
· fix multi-line expect {} blocks (closing } must be on its own line)
· add verify_initial_setup step: check SSN and replication slots
· add SSN checks after disable_maintenance_{rejoin,secondary}
· add SSN checks after restart_node1
· add multiple_failovers step with replication slot verification
· fix perform promotion to use --name (not --nodename)
· fix drop node to use --name nodeN (--pgdata is ignored with monitor)
· fix drop_secondary replication-slot empty check after node drop
· all 16 steps now pass
The Docker network disconnect relies on TCP keepalive timeout detection. With wal_sender_timeout=30s, the full demote cycle (health-check failure → draining → demote_timeout → demoted → standby catchingup → wait_primary) can take 60–90s. 90s was sometimes not enough; 120s provides a safe margin.
…y points
Command-line split:
pg_autoctl inspect — read-only diagnostics, safe on live nodes
show / pgsetup unchanged
monitor get + parse-notification ONLY (not register/active/version)
service getpid ONLY (not restart — that mutates)
pg_autoctl manual — operator-driven FSM operations (replaces 'override')
fsm assign / step / nodes set
service getpid + restart + pgctl on|off
monitor register + active + version (ALTER EXTENSION)
primary / standby / coordinator unchanged
pg_autoctl do — hidden from --help; internal subprocess entry points only
service postgres|listener|node-active|pgcontroller spawned by supervisor
still routable so 'pg_autoctl do service node-active --pgdata …' works
commandline framework:
- Add 'bool hidden' field to CommandLine struct
- Add make_hidden_command_set() macro
- commandline_pretty_print_subcommands() skips hidden commands
Why 'manual'? It is the antonym of 'automated' — the mental model central
to pg_auto_failover. 'override' described a consequence; 'manual' describes
who is acting (a human, by hand, instead of the FSM).
Why fork+exec for the internal entry points? The supervisor restarts child
processes via fork()+execve() so that when the pg_autoctl binary is updated
in place (e.g. by a package manager), a restarted child picks up the new
version without restarting the supervisor — which may be PID 1 in a
Docker/Kubernetes container. The version-mismatch exit in
keeper_check_monitor_extension_version() exploits this path intentionally.
Comments added to service_keeper/monitor/postgres_ctl_runprogram().
inspect now contains: show / pgsetup — unchanged fsm — state / list / gv (read-only FSM introspection) monitor — get / parse-notification (read-only monitor queries) getpid — postgres / listener / node-active manual now contains: fsm — init / assign / step / nodes (mutating FSM operations) service — restart / pgctl on|off (getpid removed — it was duplicated) monitor — register / active / version primary / standby / coordinator — unchanged Fixes: - getpid was present in both 'inspect getpid' and 'manual service getpid' (duplicate); removed from manual service - manual fsm contained state/list/gv which are read-only; moved to inspect - do_coordinator_commands description was 'Query a Citus coordinator' (wrong); changed to 'Manage Citus coordinator node metadata' de-static'd fsm_state/list/gv/init/assign/step in cli_do_fsm.c so cli_inspect.c and cli_manual.c can reference them without going through the do_fsm_commands aggregate.
…s only
The old 'pg_autoctl do' tree was a shadow of everything in inspect+manual
plus the actual subprocess entry points. Now:
pg_autoctl internal (hidden)
service
pgcontroller — debug: runs just the postgres-controller supervisor
postgres — spawned by service_postgres_ctl_start()
listener — spawned by service_monitor_start()
node-active — spawned by service_keeper_start()
tmux — dev/QA tooling (not yet moved to pgaftest)
demo — demo app (not yet moved to pgaftest)
All commands that were previously duplicated under 'do' (monitor, coordinator,
fsm, primary, standby, show, pgsetup, pgctl, service getpid/restart) are now
only reachable through 'inspect' or 'manual'.
Spawn calls updated in service_keeper/monitor/postgres_ctl_runprogram():
pg_autoctl do service … → pg_autoctl internal service …
The 'internal' name in ps(1) output now immediately communicates that
this is an internal subprocess, not a command meant for operator use.
Introduces a new pg_autoctl_node.ini file format and a matching
command tree (pg_autoctl node run/apply/show/check) that lets operators
describe a node fully in one file and hand it to pg_autoctl rather than
assembling long create-flag command lines.
## pg_autoctl_node.ini sections
[node] kind, hostname, port
[postgresql] pgdata
[monitor] pguri (empty for monitor nodes)
[formation] name, group
[settings] candidate_priority, replication_quorum ← mutable
[create] ssl, auth, pg_hba_lan ← create-time only
## pg_autoctl node subcommands
run <file> Create (if needed) then execv() into run/create --run.
Sets PG_AUTOCTL_NODESPEC env var so the supervisor picks
up the file path for watching.
apply <file> Converge mutable fields on an already-running node
(calls pg_autoctl set node ... for changed values).
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 100 ms tick:
- Linux: drain inotify IN_CLOSE_WRITE / IN_MOVED_TO events
- Others: stat() the file every NODESPEC_WATCH_INTERVAL_SECS (10 s)
On change, re-parse and call nodespec_apply() to converge mutable fields
without restarting the node. Immutable field changes (kind, pgdata)
are logged as warnings — a restart is required.
## pgaftest compose: uniform containers
compose_gen.c now generates:
- monitor_node.ini + <name>_node.ini per data node (in workdir)
- Every service uses the same image and same command:
pg_autoctl node run /etc/pgaf/node.ini
- The per-node ini is bind-mounted :ro at /etc/pgaf/node.ini
- No bespoke per-node command in docker-compose.yml
cluster { } block gains new options:
image, ssl, auth, formation, num-sync, monitor [port N]
test_runner.c calls compose_gen_write_monitor_ini() and
compose_gen_write_node_ini() before docker compose up.
Add three new documentation items:
docs/ref/pg_autoctl_node.rst
Full reference for the pg_autoctl node subcommand tree
(run / apply / show / check) and the pg_autoctl_node.ini file
format. Covers every section and field, immutable-vs-mutable
distinction, live-reconfiguration semantics (inotify on Linux,
mtime poll elsewhere), and Docker Compose / Kubernetes usage
patterns.
docs/pgaftest.rst
Reference chapter for the .pgaf test specification DSL:
- cluster { } block with all options (image, ssl, auth,
formation, num-sync, monitor port, node declarations)
- setup / teardown / step / sequence blocks
- Full DSL command reference: exec, wait until, assert,
sql, expect, network disconnect/connect, sleep, compose down
- TAP output format and exit codes
- Running a test suite (schedule file, single file, interactive)
- Environment variables (PGAF_IMAGE)
- How pgaftest and pg_autoctl node interact (live reconfig)
- Complete example spec (basic failover test)
docs/index.rst
Add 'Testing' toctree section pointing at pgaftest.rst and
add pg_autoctl_node.rst to the Manual Pages section.
…s], ini file renames, auto work dir
- Redesign cluster block DSL: cluster → monitor + formation(s) → nodes
Formation block: formation [name] [num-sync N] { nodename [kind] [opts...] }
Node kind (optional, default postgres): postgres | coordinator | worker
Node options: async, candidate-priority N, group N
Multiple formations supported (Citus use case)
- Add TestFormation struct; TestCluster.formations[] replaces flat nodes[]+formation+numSync
- num-sync is now per-formation; runner applies it per formation after compose-up
- compose_gen_write_node_ini() takes formation parameter for formation name in ini
- Rename [create] → [options] in nodespec.c/h and all ini generators
- Rename monitor_node.ini → monitor.ini, <name>_node.ini → <name>.ini
- Remove [settings] section from monitor.ini (monitors have no candidate_priority)
- Work dir auto-derived from spec filename: $TMPDIR/pgaftest/<testname>
derive_work_dir() strips path + .pgaf extension; TMPDIR defaults to /tmp
pgaftest step and pgaftest down now accept spec path arg to derive work dir
--work-dir flag still works as an override
- nodespec.c: fix PUSH("create") regression from [create]→[options] rename
- Docs: update cluster{} syntax, formation examples, work-dir defaults
…SL; multi-row expect
- Fix collect_block_content() to use depth-aware brace matching so inline
blocks like { { 1 } { 2 } } find the correct closing brace instead of
the first } encountered.
- Add expand_tuple_expect(): converts inline tuple format
'{ r1 } { r2 }' to the newline-separated form that psql
--tuples-only --no-align produces (called after collect_block_content
for every expect command).
- Extend TestNode with new create-time fields:
noMonitor, listen, citusSecondary, citusClusterName, pgPort,
debianCluster, ssl (node-level override), auth (node-level override).
- Extend parse_node_line() to handle all new node options including
both 'key value' and 'key=value' forms (candidate-priority, group,
port, citus-cluster-name, debian-cluster, ssl, auth-method, etc.)
and replication-quorum=false as a synonym for async.
- Extend parse_cluster_block() monitor handler to parse inline options
(port, ssl, auth-method) in both 'key value' and 'key=value' forms.
- Update compose_gen_write_node_ini() to emit the new TestNode fields:
per-node port, debian_cluster, no-monitor (omits [monitor] section),
citus_secondary, citus_cluster_name, listen option.
- compose_gen_write(): make monitor service conditional on withMonitor;
skip depends_on: monitor when cluster has no monitor; skip
monitor_data volume when withMonitor is false.
- Convert all 25 .pgaf spec files that used the old flat node/
citus-coordinator/citus-worker lines to the new:
formation [name] [num-sync N] { ... }
hierarchy. Conversions preserve all semantics:
* auth → cluster-level 'auth' directive
* ssl → cluster-level 'ssl' directive
* citus-coordinator X → X coordinator
* citus-worker X group=N → X worker group N
* replication-quorum=false → async
* candidate-priority=N → candidate-priority N
* formation=NAME → separate formation NAME { } block
- Update docs/pgaftest.rst: document new node options, monitor
inline options, and all three expect forms (single, multi-line,
inline tuple).
Replaces multi-line expect blocks:
expect {
1
2
3
}
and the older broken-but-common variant where the closing } was inline
with the last row:
expect { 1
2
3 }
with the new single-line tuple form:
expect { { 1 } { 2 } { 3 } }
The inline form is parsed by the depth-aware collect_block_content()
then expanded by expand_tuple_expect() into the newline-separated
string that psql --tuples-only --no-align produces.
Files updated: basic_citus_operation, basic_operation,
basic_operation_listen_flag, citus_multi_standbys, multi_async,
multi_maintenance, multi_standbys.
Two new DSL commands for asserting expected failures:
expect error [SQLSTATE]
Follows a 'sql' command and asserts that the query raised a SQL error.
The optional SQLSTATE (5-char PostgreSQL error code) constrains the
check to a specific error class, e.g. 25006 for
read_only_sql_transaction.
The preceding 'sql' is marked at parse time (allowError = true) so
it does not fail the step when SQL errors; instead it records the
SQLSTATE into runner state (lastSqlFailed / lastSqlState) which
CMD_EXPECT_ERROR then validates.
SQLSTATE extraction relies on psql's VERBOSITY=verbose output format:
ERROR: XXXXX: message text
ON_ERROR_STOP=1 is now always set so psql exits non-zero on SQL
errors, enabling reliable exit-code detection in exec_sql_on_service.
exec-fails <svc> <args>
Like 'exec' but asserts the command exits non-zero. Fails the step
if the command unexpectedly succeeds (exit 0).
Implementation:
- TestCmdKind: add CMD_EXEC_FAILS, CMD_EXPECT_ERROR
- TestCmd: add allowError bool field
- TestRunner: add lastSqlFailed, lastSqlState, lastSqlService fields
- test_spec_parse.y: parse 'exec-fails' and 'expect error [CODE]';
post-processing pass over each step's cmd list sets allowError=true
on any CMD_SQL immediately followed by CMD_EXPECT_ERROR
- test_runner.c: escape_sql_for_shell() helper; parse_sqlstate() helper;
exec_sql_on_service() now uses ON_ERROR_STOP=1 + VERBOSITY=verbose;
CMD_SQL soft-fails when allowError; new CMD_EXEC_FAILS, CMD_EXPECT_ERROR
cases in runner_exec_cmd()
basic_operation.pgaf: add four new steps ported from Python @raises tests:
write_to_secondary_fails — INSERT on standby → expect error 25006
read_secondary_unchanged — data unchanged after rejected write
maintenance_primary_rejected — enable maintenance without --allow-failover
write_to_old_primary_fails — INSERT on new secondary after failover
failover_single_node_rejected — perform failover with no standby
Five .pgaf files updated to include steps that correspond to Python test functions decorated with @raises(Exception): basic_operation_listen_flag.pgaf: test_005_writes_to_node2_fail — INSERT to standby → expect error 25006 test_010_writes_to_node1_fail — INSERT to new secondary → expect error 25006 basic_citus_operation.pgaf: test_004_002_writes_via_coordinator_fail — INSERT routing to failed worker2 shard before failover completes; Citus cannot contact the shard → expect error (no specific SQLSTATE — Citus connection failure, not a SQL read-only error). Also adds 'wait until worker2b state = primary' before the succeeding write, reflecting the Python test_004_004 / test_004_005 split. citus_multi_standbys.pgaf: test_004_002_writes_via_coordinator_fail — same Citus-routing failure pattern as basic_citus_operation, inserted between the disconnect step and the wait for failover completion. multi_maintenance.pgaf: test_009a_enable_maintenance_on_primary_fails — exec-fails: enabling maintenance on node3 (primary, with --allow-failover) is rejected because node1 and node2 are already in maintenance, leaving no standby for number-sync-standbys=1. test_017_primary_to_maintenance_fails — converted from 'exec + assert state' to 'exec-fails': the monitor rejects putting the primary into maintenance when only one standby would remain (below number-sync-standbys=1). nonha_citus_operation.pgaf: test_007_fail_when_disabling_with_secondaries — exec-fails: disabling secondary support on the non-ha formation is rejected while secondary nodes still exist. Two Python tests not ported (by design): test_create_standby_with_pgdata.py test_004: requires pg_ctl initdb in a separate step before pg_autoctl create — not expressible in the DSL since all containers start simultaneously; the pgaf file notes this explicitly. test_citus_skip_pg_hba.py test_002b: requires intercepting the automatic pg_autoctl create call that runs at container startup — also not directly expressible; the pgaf file handles this via the retry-after-HBA-edit path.
…gdata and citus_skip_pg_hba
no-autostart node attribute
A node declared with 'no-autostart' in the cluster block has its
compose service command overridden to 'sleep infinity' instead of
'pg_autoctl node run /etc/pgaf/node.ini'. The node.ini is still
generated and bind-mounted so that manual exec steps can use it.
Typical use:
exec-fails nodeN pg_autoctl node run /etc/pgaf/node.ini
exec nodeN (fix something)
exec nodeN pg_autoctl node run --background /etc/pgaf/node.ini
This models Python test sequences that call node.create() + node.run()
separately with state-inspection or fixup steps in between.
Implementation:
test_spec.h: bool noAutostart in TestNode
test_spec_parse.y: parse 'no-autostart' keyword in parse_node_line()
compose_gen.c: emit 'command: ["sleep", "infinity"]' for such nodes
create_standby_with_pgdata.pgaf (full port of Python tests 003-006)
node2 no-autostart
test_003: pg_ctl initdb on node2 to create a conflicting PGDATA
test_004: exec-fails — pg_autoctl node run detects system_identifier
mismatch and exits non-zero
test_005: rm -rf node2's PGDATA to clean up
node3 no-autostart
test_006: pg_basebackup from node1 (without -R so pg_autoctl manages
recovery config), then pg_autoctl node run --background,
then wait for secondary/primary states
citus_skip_pg_hba.pgaf (port Python test_002b @raises)
worker1a no-autostart
test_000: monitor HBA: manually append 'host all all 0.0.0.0/0 trust'
so pg_autoctl data nodes can reach the monitor with auth=skip
test_002b: exec-fails — pg_autoctl registers worker1a but coordinator's
master_activate_node call is blocked by default pg_hba.conf
test_002d: append HBA rules to worker1a's pg_hba.conf, then run keeper;
keeper retries activation which now succeeds
test_002e: assert hba-edited = false (pg_autoctl never touched it)
worker1b no-autostart (for ordering: starts after worker1a is primary)
test_003: pg_autoctl node run --background; inherits worker1a's HBA
via pg_basebackup streaming replication setup
Adds two GitHub Actions workflows: - .github/workflows/run-tests.yml: modernized from @V3 to @v4 actions, otherwise identical to the existing matrix (pg13-18 × multi/single/ monitor/ssl/citus + tablespaces + linting). - .github/workflows/run-pgaftest.yml: new workflow for the pgaftest suite. A single build job produces pgaf:run (Docker image) and the pgaftest binary as reusable artifacts; 20 test specs then run concurrently as a matrix; the upgrade spec runs in a separate job with its own image build (pgaf:next + pgaf:current from the previous release tag). Also included in this commit: - tests/upgrade/install-extension.sh: replaces four sudo cp lines with a single script that uses pg_config to find extension/library paths at runtime, removing the hardcoded "/16/" paths from the spec. - tests/upgrade/Dockerfile.current: COPYs install-extension.sh into the image, drops the RUN printf approach. - tests/tap/specs/upgrade.pgaf: adds 'pg_autoctl manual service restart postgres' after the symlink flip so PostgreSQL reloads the new v2.2 .so; without this the old library stays resident and every keeper call fails. Uncomments 'upgrade' in tests/tap/schedule. All 10 upgrade test steps pass locally.
…pdate spec - compose_gen.c: chmod(0666) ini files after writing so container user can update them at runtime (Permission denied fix) - workflow: use PGVERSION=17 for the regular test image so config_get_set spec's hardcoded /usr/lib/postgresql/17/ paths match - extension_update.pgaf: add extension-version "dummy" to cluster block so PG_AUTOCTL_EXTENSION_VERSION=dummy is injected in the monitor container, enabling the ALTER EXTENSION auto-update test
…mage - compose_gen.c: SSL key files on the host use chmod 0644 so the container's docker user (uid 1000) can read them for the cp step; the container immediately re-applies chmod 0600 on the copy (CA key stays 0600 — it is not mounted into containers) - replace_monitor.pgaf: add 'monitor newmonitor initially stopped' to cluster block so newmonitor service exists in compose; add --force to pg_autoctl disable monitor since the monitor is stopped - installcheck.pgaf: add 'image pgaf:testrun' cluster directive - workflow: move installcheck out of matrix into its own dedicated job that builds pgaf:testrun inline (has source tree + pg_regress)
- Use 'compose start newmonitor' to start the stopped container (docker compose exec doesn't work on stopped containers) - 'pg_autoctl inspect pgsetup wait' to ensure monitor is ready before nodes re-enable against it - 'set monitor newmonitor' switches the runner's LISTEN/NOTIFY connection so subsequent wait-until watches the new monitor
The cluster takes extra time to stabilize after the v2.1→v2.2 upgrade (keeper self-restarts, reconnections) before a failover can complete within the original 120-second window.
'pg_autoctl enable monitor' takes a positional URI argument, not --monitor flag. Remove the spurious --monitor option.
replace_monitor:
- 'pg_autoctl enable monitor' takes a positional URI, not --monitor
skip_pg_hba:
- 'auth skip' was set at cluster level, including monitor;
monitor needs trust auth so nodes can connect to it
- Use per-node 'auth skip' on node1/node2 instead of cluster-level
- Restore setup block's hba-lan steps (needed for monitor
to health-check the nodes)
…ate/timeout fixes - auth.pgaf: add monitor/node passwords (required for md5 auth to work) - compose_gen.c: skip healthcheck/depends_on for no-monitor clusters; nodes start in parallel and FSM is driven manually via test steps; also increase start_period from 5s to 15s for slower CI machines - multi_standbys test_014_002: fix state expectation from 'primary' to 'wait_primary' (with both sync standbys down, node1 blocks writes) - multi_alternate test_003_002: increase draining/report_lsn timeouts 90→120s - multi_async test_011: increase report_lsn timeout 60→120s - enable_ssl test_007: add sleep 5s before pgsetup wait for secondary SSL restart
- Add ssl off to replace_monitor, multi_maintenance, multi_standbys, multi_alternate, multi_async: these tests don't exercise SSL and the default ssl=self-signed causes monitor->node health checks to fail with (unhealthy) in CI Docker networking - Fix skip_pg_hba: pg_autoctl override pgsetup hba-lan doesn't exist; the command is pg_autoctl do pgsetup hba-lan - Fix auth test_003: bare pg_autoctl perform failover runs on the host, not inside the monitor container; prefix with 'exec monitor'
…kip_pg_hba command - Increase workflow job timeout 20→25min, step timeout 15→18min (multi_maintenance needs it) - Increase no-monitor healthcheck: retries 30→60, start_period 15→30s (monitor_disabled flaky) - enable_ssl test_007: explicit manual service restart postgres before pgsetup wait so ssl takes effect on the secondary node which needs a restart not just a reload - multi_alternate test_003_002: remove redundant sleep+second report_lsn wait; node3 can move past report_lsn before the second check - multi_standbys test_014_002: increase catchingup/wait_primary timeouts 120→180s - multi_async test_014_003: increase fast_forward timeout 120→180s - replace_monitor enable_monitor: add autoctl_node@ user to monitor URL — without explicit user, psql falls back to PGUSER=demo which can't access pg_auto_failover db, causing keeper_register_again to loop indefinitely - upgrade test_008: use perform promotion --name node2 instead of perform failover to ensure node2 specifically becomes primary; increase timeout 240→300s
- skip_pg_hba: use pg_autoctl do pgsetup hba-lan (do subcommands are
always available); prior change to inspect was wrong — hba-lan is
not in the inspect pgsetup subgroup
- monitor_disabled: add setup{} block to wait for postgres to accept
connections on all three no-monitor nodes before the first step;
without this the exec commands race against postgres startup
- enable_ssl: add pg_autoctl manual service restart postgres + inspect
pgsetup wait to test_005 (monitor) and test_006 (primary) since
ssl is not a hot-reload GUC — postgres must restart for SHOW ssl to
return 'on'
- .gitattributes: exclude src/bin/pgaftest/** from citus-style checks; pgaftest is a standalone test-runner tool, not production pg_autoctl code, and does not need to follow the banned-API policy - cli_do_misc.c: replace bare snprintf() with sformat() in the new pgsetup wait --read-write path - cli_node.c: replace direct getenv() with env_exists() wrapper
- Replace fprintf/fopen with fformat/IGNORE-BANNED in new files - Add IGNORE-BANNED to atoi() calls in CLI option parsing - Fix .gitattributes: exclude test_spec_parse.c (Bison output) and all src/bin/pgaftest/** from citus-style checks - Run citus_indent on all modified files - Pin pyroute2<0.7.0 in Dockerfile for Python 3.9 asyncio compatibility
multi_ifdown: add 'ssl off' to cluster block (prevents monitor health check failures in CI); remove test_015_prep_hba step which was a bad port containing a literal %CIDR% placeholder and hostssl HBA rule — the original Python test has no HBA manipulation. multi_alternate: wait for node3 to reach secondary state in test_003_001 before killing node2 (test_003_002). Without this, the monitor cannot complete the failover cleanly because node3 may still be transitioning when node2 is killed.
Switch Makefile to invoke citus_indent via the official Docker image (citus/stylechecker:no-py) so that local 'make indent' and 'make lint' use exactly the same formatter version as CI. Re-run citus_indent via Docker to fix 24 files that differed between the local binary and the Docker image.
CI installs citus_indent via ci/tools.mk before running make lint, so the plain binary call works there. Add docker-indent and docker-check targets for local use with the authoritative citus/stylechecker:no-py image.
…→inspect/manual in tests - run-tests.yml: replace linting matrix entry with dedicated style_checker job using container: citus/stylechecker:no-py (matches pgcopydb approach). This ensures CI uses the exact same citus_indent binary as the Docker image used for local formatting, eliminating version-mismatch false failures. - Makefile: make spellcheck use $(CITUS_INDENT_DOCKER) so local 'make lint' also uses the Docker binary; clean up dead linting branches from ci-test/test - pgautofailover_utils.py: replace all 'pg_autoctl do ...' calls with the new sub-command names (do pgsetup → inspect pgsetup, do fsm state → inspect fsm, do fsm assign/step/nodes → manual fsm, do service → manual service) - skip_pg_hba.pgaf: same rename (pg_autoctl do pgsetup → inspect pgsetup)
…hostname
Fix a cluster of issues found while running pgaftest specs locally:
1. wait until <node> assigned-state = X now correctly waits for the
monitor's goalState to become X, without requiring the node to have
transitioned its reportedState. Previously the implementation
ignored the checkAssigned flag and waited for full convergence
(both reported and assigned == target), which caused test_015_003
in multi_standbys to time out: the primary is assigned goalState
'primary' but cannot complete the transition to reported 'primary'
until disconnected standbys reconnect and sync up.
A new runner_wait_assigned_goal() function handles this case: it
succeeds as soon as the monitor's goalState matches, without any
reported-state requirement. This matches the Python test's
wait_until_assigned_state() semantics.
2. No-monitor clusters (monitor_disabled.pgaf): compose no longer
generates a healthcheck for no-monitor data nodes. Those nodes
start pg_autoctl in INIT state (postgres not yet running), so
pg_isready would always fail and Docker would mark the container
unhealthy. Subsequent nodes using depends_on with service_healthy
would then fail to start. Fix: omit healthcheck for no-monitor
nodes; use service_started as the depends_on condition.
The setup{} block was also changed from 'inspect pgsetup wait'
(requires postgres running) to 'sleep 5s' so pg_autoctl has time
to initialise the data directory before test steps begin.
3. pg_autoctl inspect pgsetup hba-lan: the hba-lan subcommand was
resolving the wrong hostname for LAN CIDR lookups. It used
pgSetup->pghost (a Unix socket path like /var/run/postgresql)
as the hostname argument to findHostnameLocalAddress(), causing
a FATAL DNS lookup failure. Fix: load KeeperConfig to get
config.hostname (the node's network hostname, e.g. 'node1').
4. multi_standbys test_014_002: when both standby containers are
compose-stopped, node1 stays in 'primary' FSM state (blocking
writes via sync rep) until number-sync-standbys is explicitly set
to 0. Changed the wait from 'wait_primary' to 'primary'.
5. multi_async test_014_003: removed transient fast_forward
assigned-state check (may have already passed before pgaftest
starts listening).
6. enable_ssl test_007: uses compose stop/start instead of a broken
sequence.
7. keeper MAINTENANCE_STATE: skip postgres restart in
keeper_ensure_configuration; ensure postgres is running via the
write-status-file path in keeper_ensure_current_state.
8. compose start: use 'docker compose up -d --no-recreate --no-deps'
instead of 'docker compose start' to avoid following the
dependency chain and restarting unrelated containers.
9. runner_wait_notify_goal: for unhealthy (dead) nodes, accept a
goalState-alone match so dead containers (where exec would fail)
don't block the wait.
10. Node.js 20 deprecation: updated CI action references to v4
(already on Node 24 runners).
cli_common_pgsetup_init() calls ProbeConfigurationFileRole() which FAILs with FATAL when the keeper config file does not yet exist (node still initializing when setup block runs in parallel). Rewrite keeper_cli_pgsetup_hba_lan() to: - use keeperOptions.pgSetup.pgdata directly (no config file needed) - wait up to 60 s for pg_hba.conf to appear before writing rules - determine hostname from: keeper config → gethostname() → pghost - use &keeperOptions.pgSetup for pg_setup_wait_until_is_ready() - use keeperOptions.pgSetup.pg_ctl for pg_ctl_reload() gethostname() inside Docker returns the container service name (e.g. "node2") which resolves correctly on the Docker LAN network, covering the race window before the keeper config file is written. Add #include <unistd.h> for gethostname() on Linux.
Fix C formatting issues that citus_indent --check reported across the files changed in this PR. No logic changes. Also add Dockerfile touch of bison/flex generated files to prevent system bison from regenerating test_spec_parse.c with a different bison version than the one used to pre-generate the committed files.
Dockerfile: make touch conditional in pgaf:current-base build git archive PREV_TAG does not include src/bin/pgaftest/ (pgaftest is new in this branch), so the current Dockerfile's touch step would fail when building pgaf:current-base from a previous release tag. Use a shell loop with [ -f ] guard so missing files are skipped with a warning rather than aborting the build. pgaftest/Makefile: add 'make generate' convenience target Rename the auto-regeneration rules to be more explicit about the intent; the committed .c/.h files are what gets built by default. multi_async: wait for assigned-state at report_lsn The failover races through report_lsn→join_secondary in ms. Using 'wait until assigned-state = report_lsn' fires as soon as the monitor assigns the state (goalState), giving the test more window to network-disconnect node4 before the transition completes.
…ry timeout Three CI fixes found by running pgaftest: 1. cli_common.c: add signal(SIGHUP, SIG_IGN) before kill() in cli_pg_autoctl_reload(). The daemon's supervisor_reload_services() broadcasts SIGHUP to all supervised children. A freshly exec'd one-shot command (e.g. 'pg_autoctl set node metadata') can receive that SIGHUP via PID reuse in the container's small PID namespace; since one-shot commands don't install a SIGHUP handler, the default disposition (terminate) kills the command before it returns, producing exit code 129 (128+SIGHUP). SIG_IGN is safe here because all callers are one-shot CLI commands. 2. multi_async.pgaf test_011: change 'wait until node4 assigned-state = report_lsn' to 'wait until node4 state is report_lsn'. The old check fires as soon as the monitor assigns the goal state, before node4 has stopped Postgres and reported its LSN. Disconnecting node4 at that point leaves the monitor waiting for a LSN report from a node that can no longer talk to it, blocking the entire promotion. 'state is' waits for the convergence event (reported == goal), so node4 has already reported its LSN when we disconnect it. 3. basic_operation.pgaf test_025: increase 'wait until node3 stopped' from 30s to 60s. Dropping the primary requires stopping Postgres (including WAL senders to the secondary), writing DROPPED twice to the monitor, and supervisor teardown — this regularly exceeds 30s in CI but fits comfortably within 60s.
The service_keeper.c changes on this branch delete the state file when the keeper exits after completing the DROP protocol (two confirmed node_active contacts in DROPPED state). This is correct: it prevents a restarted process from trying to re-register a dropped node. However, 'pg_autoctl drop node' (without --no-wait) also calls keeper_ensure_node_has_been_dropped() AFTER waiting for the running service to stop. That function reads the state file to get the node ID and then queries the monitor. If the service already exited cleanly and deleted the state file, the read fails and drop reports EXIT_CODE_MONITOR. Fix: after keeper_ensure_node_has_been_dropped() fails, check whether the state file is simply missing and we had a running service (pid != 0). The absence of the state file in that path is authoritative — the service only removes it after successfully completing the DROPPED protocol — so treat the drop as successful.
The loop using $$f was incorrectly expanding in /bin/sh where $$ is the shell PID (e.g. 1 in a container), producing '1f' instead of the variable value — causing the touch step to silently fail, letting bison regenerate the parser from scratch. Replace with explicit paths.
The upgrade test builds from v2.1 via git-archive, which has no src/bin/pgaftest/ directory. Unconditional touch fails with exit 1. Wrap in an if-d guard so the step is a no-op on older tags while still preventing bison from regenerating the committed .c/.h files on the current branch.
…it_until When the monitor postgres restarts (e.g. during an upgrade), the pgaftest LISTEN connection is killed. If the connection reconnects AFTER the state transitions fire, those pg_notify events are lost — allSatisfied is never set and the wait loop blocks for the full timeout. Fix: also call monitor_check_formation_converged every ~5 wait-socket cycles (~25 s) in the notifyConnected branch, so missed notifications are caught by direct monitor polling. Also increase the upgrade spec's post-restart sleep from 20 s to 45 s to give slow CI runners enough margin for the keeper self-restart cycle.
When allSatisfied is true from LISTEN notifications, return immediately. The previous code called monitor_check_formation_converged() as a double-check, but this races with fast post-convergence transitions: in multi_async step 8 the batch contains both the report_lsn convergence notification AND node4's immediate move to join_secondary/secondary. By the time the subprocess queries the monitor, node4 is already at secondary, producing a false negative and a 120s timeout. The periodic (every ~25s) subprocess poll is kept as a fallback for missed notifications (e.g. LISTEN reconnects after the upgrade test's monitor Postgres restart), where allSatisfied never becomes true from notifications alone.
Contributor
Author
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
Replaces the Python/pytest test suite with pgaftest, a purpose-built test runner that uses a declarative
.pgafspec format backed by Docker Compose.What's in this PR
New test infrastructure
src/bin/pgaftest/— new binary: DSL lexer (Flex), parser (Bison), compose generator, test runner, TAP outputtests/tap/specs/— all 21 test specs ported from Python (basic operation, failover, SSL, maintenance, multi-standby, extension update, upgrade, …)tests/tap/schedule— ordered list of specs forpgaftest run --schedulesrc/bin/pg_autoctl/— addspg_autoctl inspectandpg_autoctl manualsub-command groupsGitHub Actions workflows
.github/workflows/run-tests.yml— existing matrix (pg13–18 × multi/single/monitor/ssl/citus), modernized to@v4actions.github/workflows/run-pgaftest.yml— new workflow:buildjob producespgaf:run(Docker image tarball) and thepgaftestbinary as reusable artifactsupgradejob buildspgaf:next+pgaf:current(from the previous release tag) and runs the live-upgrade specLive upgrade test (
tests/tap/specs/upgrade.pgaf)pgaf:current(v2.1, fromPREV_TAG) andpgaf:next(current branch), both baked into one container via symlink flipHow the matrix reuses images
Running locally