ci: overhaul CI infrastructure — shared base image, pgaftest suite, Debian cluster unification, test reliability fixes#1148
Open
dimitri wants to merge 40 commits into
Open
ci: overhaul CI infrastructure — shared base image, pgaftest suite, Debian cluster unification, test reliability fixes#1148dimitri wants to merge 40 commits into
dimitri wants to merge 40 commits into
Conversation
- Raise version_compat.h upper bound to PG20 (allows PG19 compilation) - Add PG19 to PGVERSIONS in Makefile; use Citus v14.1.0 for PG18/19 - Extend CI matrices (run-tests.yml, run-pgaftest.yml) to cover PG18/19 with correct Citus version per release; bump pgaftest builder to PG18 - Fix #1116: thread dbname through ReplicationSource so primary_conninfo includes a dbname keyword, required by PG17 synchronized_standby_slots for logical replication slot failover - Guard pg_stat_wal_receiver.received_tli against NULL: PG18 introduced WALRCV_CONNECTING state where received_tli is NULL; fall back to pg_control_checkpoint() timeline_id during that transient state Fixes #1116
PG19 requires several API and build system adaptations: Build system: - Remove explicit -std=c99 from Makefile.common and monitor/Makefile; pg_config --cflags now provides the right standard per version (gnu23 for PG19, c99/c11 for older versions) - Add libnuma-dev to Dockerfile build deps (PG18+ links against libnuma) - Support CITUSTAG=none in Dockerfile to skip Citus build for PG versions not yet supported by any Citus release (PG19) - Update CI case statements: 18 -> v14.1.0, 19 -> none Signal handlers (PG19 changed SIGNAL_ARGS to two-argument SA_SIGINFO form): - Use SIGNAL_ARGS macro in all signal handler signatures (signals.c catch_reload/int/term/quit/quit_and_exit, watch.c catch_sigwinch) - Use PG_SIG_IGN instead of SIG_IGN with pqsignal() in health_check_worker.c - Add backward-compat PG_SIG_IGN/PG_SIG_DFL macros to version_compat.h for builds against PG < 19 Monitor extension (PG19 API changes): - LWLockNewTrancheId() now takes the tranche name; LWLockRegisterTranche() is removed — add LWLockNewTrancheIdCompat() wrapper in version_compat.h - ShmemInitHash() dropped the separate init/max size params — add ShmemInitHashCompat() wrapper - WAIT_EVENT_CLIENT_READ moved to utils/wait_event.h; include explicitly - storage/lock.h and storage/lmgr.h no longer pulled in transitively; add explicit includes to metadata.c and health_check_worker.c - PgStartTime renamed to MyStartTimestamp — compat define in version_compat.h - utils/timestamp.h no longer included transitively — add in version_compat.h - pg_fallthrough macro used instead of /* FALLTHROUGH */ comment; add backward-compat pg_fallthrough define in version_compat.h for PG < 12 - Fix checkPgAutoFailoverVersion() old-style function definition Regression tests: - PG19 changed pg_lsn display format (e.g. 0/0 -> 0/00000000); add expected/pg19/expected/ with adjusted output for monitor and fast_forward tests; Makefile auto-selects this dir for PG >= 19 All 9 installcheck tests pass on PG16, PG17, PG18, and PG19.
supervisor_update_pidfile() was rewriting the pidfile using O_TRUNC, which zeros the file before writing the new content. If cli_do_service restart read the pidfile in that brief window it saw an empty file and called log_fatal immediately. Primary fix: write to supervisor.pidfile.tmp then rename(2) into place. POSIX rename is atomic so readers always see a complete file. Defensive fix: retry supervisor_find_service_pid up to 10 times with 10ms sleeps in cli_do_service_restart before treating the lookup as a fatal error.
…state The previous fix for PG19's WALRCV_CONNECTING state used: WHERE status <> 'connecting' to exclude the connecting row and fall back to pg_control_checkpoint(). This is wrong when the WAL receiver restarts after having already streamed: PG19 retains receivedTLI in shared memory across WAL receiver restarts (WalRcvDie() does not reset it), so a restarting receiver in WALRCV_CONNECTING state still has received_tli=3 from the previous session, but the WHERE filter drops that row and the fallback returns timeline_id=2 from the last restartpoint. The monitor then sees node2 reporting TLI=2 while node1's last report was TLI=3, which prevents the automatic failover from completing within the 90-second timeout. Fix: filter on received_tli > 0 instead of status <> 'connecting'. This correctly handles both cases: - First-ever WAL receiver (shared memory zeroed at startup): received_tli=0 → filtered → falls back to pg_control_checkpoint() - WAL receiver restart after streaming TL3: received_tli=3 retained from previous session → not filtered → returns 3
PgStartTime (the postmaster startup time) still exists in PG19 via
utils/timestamp.h. The earlier shim incorrectly aliased it to
MyStartTimestamp, which is the *per-backend* start time set by
InitProcessGlobals() in every new process.
With the shim in place, NodeIsUnhealthy() compared health-check
timestamps against 'when did this backend connection start?' A backend
forked to handle a node_active call is always younger than the monitored
healthCheckTime, so the condition
TimestampDifferenceExceeds(PgStartTime, node->healthCheckTime, 0)
was permanently false in PG19 — the monitor marked an unhealthy primary
but the FSM never advanced to prepare_promotion because NodeIsUnhealthy
kept returning false. The startup-grace-period check was also broken by
the same substitution.
Fix: delete the shim. PgStartTime remains valid in PG19 and refers to
the same postmaster start time as in earlier releases.
Documents the investigation technique used to root-cause the PG19 failover stall: polling pg_stat_wal_receiver, pg_stat_activity, and pgautofailover.node every few seconds to observe monitor state in real time. Useful as a reusable template for debugging future FSM stalls. Not listed in tests/tap/schedule; run on demand: pgaftest run tests/tap/specs/debug_failover_pg19.pgaf
PG19 images were built with CITUSTAG=none, so the citus test job ran against an image with no Citus installed and immediately failed. Changes: - Dockerfile: when CITUSTAG=main, pass --without-pg-version-check to ./configure so Citus main builds against PG19 which has no stable Citus release yet. - Makefile BUILD_ARGS_pg19: none → main. - run-tests.yml: continue-on-error for the PG19 × citus job; Citus main may not always be stable on the latest Postgres beta. - run-pgaftest.yml: same CITUSTAG switch in all three build case statements (run, debian, testrun targets); add PG19 citus-1 and citus-2 matrix entries with allow_failure: true so they show up in CI without blocking the overall build.
Citus main has compile errors against PG19 (PageSetChecksumInplace removed, pgstat_report_vacuum signature changed, get_relation_info_hook type gone, tuplesort/tuplestore API changes). --without-pg-version-check only bypasses the version guard; it cannot fix compilation. Keep CITUSTAG=none for PG19 in the Makefile and workflow case statements. The Dockerfile --without-pg-version-check branch and the continue-on-error matrix entries for PG19 citus jobs are retained so the wiring is ready when Citus main eventually supports PG19.
Citus does not yet support Postgres 19 — no release and the main branch does not compile against it either. The PG19 citus-1/citus-2 matrix entries (with allow_failure/continue-on-error) were left behind when CITUSTAG was reverted to none. Remove them so those jobs no longer appear in CI at all.
teardown_module() is not a safe place to run DDL that depends on the cluster being in a clean state. If the final test perturbs the cluster (e.g. test_011_start_worker2b_again reattaches a failed node), Citus metadata sync may still be in progress when teardown fires immediately afterwards. The DROP TABLE then fails with: ObjectNotInPrerequisiteState: <node> is a metadata node, but is out of sync Because the exception is raised before cluster.destroy(), all background pg_autoctl and postgres processes stay alive inside the test container, keeping the docker run from exiting. The job then burns its remaining CI budget until the 15-minute hard timeout fires — consistently across PG14, 15, 16, and 17. Fix: move wait_until_metadata_sync() + DROP TABLE into a numbered test step that runs while the cluster is in a known-stable state, and reduce teardown_module() to cluster.destroy() only. - test_basic_citus_operation: test_015_drop_table (after coordinator failover) - test_citus_multi_standbys: test_012_drop_table (after worker2b rejoins as secondary) - test_citus_force_failover: test_004b_drop_table (before node-drop tests) - test_nonha_citus_operation: test_007b_drop_table (before primary failures)
1. basic_operation.pgaf test_022_detect_network_partition: sleep 3s → 30s The monitor assigns demote_timeout (and node3 reaches wait_primary) on the monitor/node3 side. Node2's local keeper detects connection loss independently via network_partition_timeout (default 20s, defaults.h). Those two clocks are decoupled: 3s was not enough for node2's keeper to fire its own partition check and call fsm_stop_postgres. 30s covers the worst case (20s timeout + keeper loop interval + Postgres shutdown). 2. wait_until_assigned_state: add or_state= keyword argument The FSM can transition draining → demote_timeout in the same wall-clock second (monitor event log confirms same timestamp). A 100ms poll loop can miss draining entirely. or_state= lets callers accept either state. Used in test_003_002_stop_primary. 3. has_needed_replication_slots: 5s retry on OperationalError A newly-joined async or sync standby node can transiently refuse connections right after its FSM state converges to secondary. wait_until_state checks pg_auto_failover state, not psycopg2 connectivity. Retry the pgmajor() call for up to 5s (with cache invalidation) before surfacing the exception. 4. DataNode.run_sql_query_retry: new helper for transient connectivity Like run_sql_query but retries on OperationalError and CannotConnectNow (ERRCODE 57P03, 'database system is starting up') for up to a caller-specified timeout with 0.5s back-off. 5. test_nonha_citus_operation test_004: use run_sql_query_retry (30s) create_distributed_table internally connects to each worker node. Even after worker pg_autoctl state reaches 'single', Postgres can still report 'system is starting up' at the Citus inter-node level. Retry for 30s instead of failing immediately.
wait_until_metadata_sync() reflects the coordinator's view of sync completion, but a recently-rejoined worker (e.g. after re-attach as secondary) may still be applying Citus metadata updates when the coordinator considers sync done. DROP TABLE then fails with: ObjectNotInPrerequisiteState: <node> is a metadata node, but is out of sync Two related fixes: 1. Add DataNode.citus_run_ddl_after_sync(query, timeout=60): a helper that calls wait_until_metadata_sync() then the DDL, and retries the whole pair with 1s back-off if ObjectNotInPrerequisiteState is raised, for up to timeout seconds. Use it everywhere DROP TABLE t1 appears after a metadata-sync wait. 2. Remove test_004b_drop_table from test_citus_force_failover: this test intentionally leaves workers in disrupted states (worker1a dead, worker1b in wait_primary with no secondary). There is no clean stable point to run Citus DDL here; DROP TABLE coverage is provided by the other citus test files. Verified: 77 tests pass cleanly across two consecutive local runs on PG16.
…ation_slot_names The 5s retry loop introduced in the previous commit only guarded pgmajor(). After that call returned, list_replication_slot_names() made a fresh connection outside the retry scope — which could still get 'Connection refused' if Postgres dropped the socket between the two calls. Reproduce: test_009_add_sync_standby in test_multi_async.py on PG14 in CI; node4 reaches FSM 'secondary' so wait_until_state passes, but Postgres transiently refuses the subsequent slot query. Fix: fold all database access inside the try/except block so any OperationalError from any call is retried uniformly for up to retry_timeout seconds. Also removes the unused 'hostname' local variable.
…octl node init
Summary table (pgaftest)
After compose down completes, pgaftest prints a pg_regress-style per-step
table to stderr showing timing and result for every step:
test test_003_create_t1 ... ok ( 432 ms)
test test_022_detect_network_partition ... FAILED ( 31204 ms)
Timing is measured with clock_gettime(CLOCK_MONOTONIC) around each
runner_exec_step() call. The table appears after teardown{} (which
includes compose down) so it does not interleave with live logging.
On fail-fast runs only the completed steps appear.
pg_autoctl drop node --destroy / --force coverage
New spec tests/tap/specs/drop_node_destroy.pgaf exercises:
- pg_autoctl drop node --no-wait (PGDATA kept)
- pg_autoctl drop node --destroy (PGDATA removed)
- pg_autoctl drop node --destroy --force (no monitor; PGDATA removed)
Added to multi-misc.sch schedule.
pg_autoctl node init
New subcommand in cli_node.c that runs pg_autoctl create <kind> without
--run, initialising a node's PGDATA without starting the supervisor.
Useful in Dockerfile build stages to pre-bake initdb into image layers.
Idempotent: exits 0 if PGDATA is already initialised.
CI image caching (run-pgaftest.yml)
Switch docker build to docker buildx build with --cache-from/--cache-to
type=gha per version so that the apt-heavy base/citus layers are reused
across CI runs when the Dockerfile hasn't changed (saves 3-6 min per
PG version per build job).
Prebuilt monitor image (Dockerfile + compose_gen.c)
New Dockerfile target 'prebuilt' extends 'run' by running pg_autoctl
node init for the monitor, baking initdb into a named layer. The
compose generator picks PGAF_PREBUILT_IMAGE for the monitor service so
pg_autoctl node run skips initdb and starts immediately. Uploaded as
a separate artifact and passed as PGAF_PREBUILT_IMAGE in test jobs.
… paths
- All containers now use command: ["pg_autoctl", "node", "run",
"/etc/pgaf/node.ini"] — no shell wrapper, no PID-cleanup or SSL-copy
baked into the compose command string.
- cli_node_run() now handles everything internally:
· deletes stale PID file before exec
· copies SSL certs (verify-ca/verify-full) via node_copy_ssl_certs()
· runs pg_createcluster for debian_cluster nodes (cold start)
· cold start: fork+exec pg_autoctl create <kind>, waitpid, then exec
pg_autoctl run — both paths now end with the same exec-run sequence
· warm start: nodespec_apply() then exec pg_autoctl run (unchanged)
- cli_node_init() shares the same init logic via node_do_init(); used
standalone for Dockerfile RUN layers and interactive pre-init.
- NodeSpec gains debian_cluster field; parsed from [options] section of
the node.ini file; no change to compose_gen node.ini generation.
- Remove Dockerfile prebuilt, debian, testrun stages — all nodes use the
single run image. pg_createcluster now runs inside pg_autoctl node run
for debian-cluster nodes, removing the need for a separate image.
- Remove CI workflow steps for prebuilt/debian image build, upload,
download, and load. Remove separate installcheck job (make installcheck
now gates the build stage itself).
- pgaftest summary now matches pg_regress TAP format:
ok N - step_name NNN ms
1..N
# All N tests passed.
…x, new spec Implement create-deferred as a second gate alongside the existing launch-deferred (run-deferred) flag. Each axis is independent: launch deferred → create immediate, run deferred create deferred → create deferred, run immediate create and launch deferred → both gates held node.ini [launch] section now writes two keys: create = deferred|immediate mode = deferred|immediate Grammar additions (test_spec_scan.l, test_spec_parse.y): - T_CREATE token added to CLUSTER_BODY scanner state - T_AND added to CLUSTER_BODY (was STEP_BODY only) - bare "deferred" now sets both createDeferred + launchDeferred - "launch deferred" = run-deferred only (unchanged semantics) - "create deferred" = create-deferred only - "create and launch deferred" = both gates cli_node_run() ordering fixed: 1. PID cleanup 2. SSL cert copy (always immediate) 3. createDeferred poll loop 4. pg_createcluster + node_do_init (cold start) 5. launchDeferred poll loop 6. setenv + execv pg_autoctl run cli_node_start() now clears both createDeferred and launchDeferred. Rename 6 specs from "launch deferred" to "create and launch deferred" since those nodes were always intended to hold both gates. Add launch_deferred_set_metadata.pgaf: ports test_basic_operation.py pattern — create-immediate/run-deferred node registers with monitor, test sets node metadata while waiting at run gate, pg_autoctl node start releases it, cluster reaches primary+secondary. TAP output: remove pre-teardown tap_plan() that duplicated the summary printed by runner_print_summary() after docker compose down. compose_gen.c: volume ro/rw mode and healthcheck depends_on conditions updated to check createDeferred || launchDeferred.
…tion Two bugs prevented debian_clusters.pgaf from working: 1. compose_gen.c: debian-cluster nodes had their named volume mounted at /var/lib/postgres (the standard pg_autoctl data home), but the Debian cluster PGDATA lives at /var/lib/postgresql/<ver>/<name>. The cluster data was being written to the ephemeral container layer rather than the named volume, so it was never persisted. Fix: mount the volume at /var/lib/postgresql for nodes that have debianCluster set. 2. cli_node.c: pg_createcluster was invoked with --user docker --group postgres, but those flags change cluster ownership without actually granting the docker user access (pg_createcluster still creates the PGDATA directory with mode 700, owned by postgres:postgres). Fix: drop the --user/--group flags and let pg_createcluster use its defaults (postgres:postgres ownership, which is what a real Debian install produces), then follow up with sudo chmod -R g+rwX on PGDATA so the docker user (a member of the postgres group) can run pg_autoctl create postgres against it.
The ini key name now matches the command it gates: pg_autoctl node run. Create and run are now symmetric: [launch] create = deferred # hold before pg_autoctl create run = deferred # hold before pg_autoctl run No behaviour change — only the ini key string changed from 'mode' to 'run'.
…dir fix runner_print_summary: emit '# <specfile.pgaf>' as the first line of the TAP output block so the reader immediately knows which spec produced it. drop_node_destroy.pgaf: add the missing sequence block listing all 9 steps in order. Without it the runner auto-sequences from the linked list of step definitions, which produced only the first 2 in one run. cli_node.c (debian cluster): extend the post-pg_createcluster chmod to the version directory (/var/lib/postgresql/17/) instead of just the cluster PGDATA (/var/lib/postgresql/17/main). pg_autoctl creates a sibling backup directory (/var/lib/postgresql/17/backup/node_N/) during registration; with chmod scoped only to PGDATA that directory was owned by postgres:postgres with no group write, causing 'Permission denied' and a registration retry loop that never converged.
Pass --allow-group-access through pg_createcluster to initdb instead of running a separate sudo chmod -R g+rwX after the fact. initdb sets PGDATA to mode 750 and data files to 640/750 atomically at cluster creation time. The docker user (member of the postgres group via adduser docker postgres in the Dockerfile) can then read and write the cluster without any post-creation chmod step. The sibling backup directory that pg_autoctl creates during registration (/var/lib/postgresql/<ver>/backup/) lives under the version directory which pg_createcluster already creates with mode 0755, so group members can write there without special handling. Removes one fork/waitpid pair and the pgverdir path-stripping logic.
…itus_operation
Three related changes:
1. Refactor post-init formation creation (service_monitor.c, cli_node.c)
The original approach forked the post-init child in cli_node_run (before
execv) and hand-off its PID via PG_AUTOCTL_POST_INIT_PID for the
supervisor to adopt. The cleaner approach: start_monitor() reads the node
spec from PG_AUTOCTL_NODESPEC (already set before execv) and directly adds
a RP_TEMPORARY 'post-init' service whose startFunction forks the child.
The supervisor owns it from the start, no env var PID hand-off needed.
2. Fix off-by-one in createDeferredStr buffer (nodespec.c)
char createDeferredStr[8] was one byte too small for 'deferred' (8 chars +
null = 9 bytes), causing the last character to be cut off so
strcmp(createDeferredStr, 'deferred') always returned non-zero and the
createDeferred flag was never set. All deferred nodes were proceeding
immediately to pg_autoctl create, racing with each other.
Fix: widen the buffer to 16.
3. nonha_citus_operation.pgaf: secondary false + DSL changes
- Add 'secondary false' keyword to the formation DSL (test_spec_scan.l,
test_spec_parse.y, and regenerated test_spec_parse.c/h test_spec_scan.c)
- compose_gen: auto-derive kind=citus from coordinator/worker node types;
emit secondary=false when disableSecondary is set
- nodespec.c: parse secondary=false in [formation name] sections
- nodespec.h, monitor_config.h: add formationDisableSecondary arrays
- monitor.c: treat 42P17 (formation not found) as a retryable registration
error so keepers retry until the post-init service creates the formation
- nonha_citus_operation.pgaf: use 'formation non-ha secondary false { }'
Tested: pgaftest run tests/tap/specs/nonha_citus_operation.pgaf — all 11 tests pass.
Introduce Dockerfile.base that builds a single long-lived image containing:
- All supported Postgres major versions (14–19) from apt.postgresql.org
- One Citus build per Postgres version (CITUSTAG_<N> ARGs, default tags
baked in, overridable for new releases)
- All build tools needed to compile pg_auto_failover and pgaftest
Per-PG pg_auto_failover builds (Dockerfile) now use FROM ${BASE} so the
heavy apt/Citus work is in ghcr.io rather than rebuilt per PR via GHA cache.
New GitHub workflows:
base-image.yml:
Builds and pushes the base image to ghcr.io. Triggered by:
- push on main when Dockerfile.base changes
- workflow_dispatch (manual, with optional force_rebuild)
- workflow_call from run-pgaftest.yml (pull-or-build on cache miss)
check-versions.yml:
Weekly cron (Monday 06:00 UTC) that:
- Inspects org.pgaf.citustag.<N> labels on the current base image
- Queries GitHub Releases API for latest tag in each Citus series
- Runs apt-cache inside the base image to detect newer PGDG packages
- Triggers a forced base image rebuild if any version has changed
run-pgaftest.yml changes:
- Adds ensure-base job (calls base-image.yml) between style_checker
and build-images; fast path is a manifest check (~5 s)
- build-images and build-pgaftest pass BASE=ghcr.io/.../pgaf-base:bookworm
and no longer pass CITUSTAG (that lives in the base image)
- GHA layer cache now only covers the pg_auto_failover compilation layer
Replace the inline fork+loop+waitpid implementation in service_post_init_start() with a simple fork+execv into the new 'pg_autoctl node post-init' command. pg_autoctl node post-init: - Reads the node spec from PG_AUTOCTL_NODESPEC (set before execv by node run) or falls back to deriving the spec path from --pgdata - Reads the MonitorConfig from <pgdata>/pg_autoctl.cfg to get a fully populated PostgresSetup (port, socket dir, etc.) - Calls pg_setup_wait_until_is_ready() with a 120 s timeout before touching the monitor, so it never races against Postgres startup - Creates each [formation <name>] section via run_program() calls to 'pg_autoctl create formation', consistent with the rest of the codebase - Exits 0 on success; the RP_TEMPORARY supervisor policy reaps it cleanly service_post_init_start(): - Reduced to 10 lines: fork + execv + setsid; all logic moved to the command - Removed sys/wait.h (no longer calls waitpid) nodespec_apply(): formation support for monitor nodes - When the spec file watcher detects a change on a monitor node (kind == NODE_KIND_UNKNOWN), apply formation-level changes live: * New [formation <name>] sections → pg_autoctl create formation * Changed secondary= setting → pg_autoctl enable/disable secondary - Formation kind changes are not applied live (immutable after creation)
Org name fix (citusdata → hapostgres):
- Dockerfile: ghcr.io/hapostgres/pg_auto_failover/pgaf-base:bookworm
- base-image.yml: same
- check-versions.yml: same
Dockerfile: BINDIR=/usr/local/bin on make install
pg_config --bindir returns /usr/lib/postgresql/<N>/bin which is the
wrong install prefix for pg_autoctl and pgaftest; override BINDIR so
both binaries land in /usr/local/bin as the COPY instructions expect.
Also update the pgaftest COPY to source from /usr/local/bin (not the
old pg_config --bindir path).
Makefile: adapt build targets to new Dockerfile
- Drop CITUSTAG vars (now baked into the base image)
- Add BASE variable (default ghcr.io/hapostgres/…/pgaf-base:bookworm)
- All BUILD_ARGS_pg* now pass --build-arg BASE=$(BASE)
- build-pg* targets add --target run (was the implicit default before)
- Remove stale build-image / build-demo targets
- Remove CITUSTAG ?= (it lives in Dockerfile.base now)
GitHub workflows: merge run-tests.yml + run-pgaftest.yml → ci.yml
Single workflow with this dependency graph:
style_checker
└── ensure_base (base-image.yml reusable workflow)
├── build_run_images (PG 14-19 → pgaf:run-pgN)
│ └── test_pgaftest (21 spec jobs)
├── build_pgaftest (pgaf:pgaftest)
│ └── test_pgaftest + upgrade
└── build_test_images (PG 14-19 → pg_auto_failover_test:pgN)
└── test_python (PG × {multi,single,monitor,ssl,citus})
- Split Makefile into Makefile (core) + Makefile.docker (shared Docker targets, included by both top-level and tests/Makefile) + Makefile.installcheck (runs inside base image via pg_virtualenv) - Add tests/Makefile with all pytest targets; remove pytest targets from top-level Makefile - Drop PG18/19 everywhere; default to PG17 (pg_auto_failover not yet supported on those versions) - Remove Makefile.citus (TESTS_CITUS folded into tests/Makefile) - Remove tmux/cluster/compose/valgrind Make targets (replaced by pgaftest) - Rename build-test-pg* -> build-pytest-pg* (--target test is pytest only) - Add build-pgaftest target in Makefile.docker - Add installcheck to Dockerfile build stage via pg_virtualenv; drop separate installcheck CI job (it now runs inside every build_run_images job) - Remove debian Dockerfile stage: pg_autoctl node run already calls pg_createcluster when debian_cluster is set in node.ini - compose_gen.c: remove PGAF_DEBIAN_IMAGE / debian image special case; forward PGVERSION as build-arg in inline build stanza so docker uses the right PG version when PGAF_IMAGE is not set - CI: drop debian image build/save/upload/download steps; drop docker tag alias (pgaf:run -> pgaf:run-pgN); pass PGAF_IMAGE=pgaf:run-pg$N directly - Dockerfile.base: add libcurl4-gnutls-dev (Citus build dep), silence pip root warning, restrict PGDG source to PG14-17
…printf with fformat in test_runner.c
- monitor.destroy() was re-raising on 'drop monitor' failure, poisoning the last test when monitor died before teardown_module() ran; drop raise - tests/tablespaces/Makefile still called build-test-pgN (old name); rename to build-pytest-pgN - upgrade job had no docker/login-action so 'make pgaf-next' failed with 401 Unauthorized pulling ghcr.io/…/pgaf-base:bookworm
dimitri
changed the title
dimitri
added
Developer productivity
…e.h style exclusion
…red launch
debian_clusters: restore --user docker --group postgres to pg_createcluster
pg_createcluster without --user docker creates PGDATA owned by postgres:postgres
with mode 750. The container user (docker) is in group postgres so has group
r-x but NOT write access, causing pg_autoctl create postgres to fail when
creating files inside PGDATA. Restore --user docker so docker owns PGDATA.
compose_gen: emit shell-wrapped command for SSL specs (SSL_COPY_CERTS_CMD)
SSL_COPY_CERTS_CMD was defined but never used. Certs were bind-mounted
read-only at /etc/pgaf/ssl/ but postgres needs them in the writable
/var/lib/postgres/ volume. Add write_node_command() helper that emits
a /bin/sh -c wrapper for any spec that uses ssl_needs_certs.
test_runner: always run teardown{} block, even when setup fails
The runner returned false immediately after setup failure without calling
the teardown block, leaving docker stacks running. Call teardown even
on setup failure so containers are always cleaned up.
test_spec: add T_TRUE token; accept T_REPLICATION_QUORUM T_TRUE | T_FALSE
'replication-quorum false' was a parse error because 'false' is lexed as
T_FALSE in CLUSTER_BODY state but the grammar only accepted T_IDENT.
Add T_TRUE token and update the grammar to accept both boolean tokens.
Regenerate bison/flex files using the base image toolchain (Bison 3.8.2).
Dockerfile: remove Makefile.installcheck from upgrade build COPY line
The upgrade build uses 'git archive v2.1' which predates that file; the
COPY line failed with 'not found'.
tests/Makefile: fix run-test and tablespaces prebuilt targets
launch_deferred_set_metadata spec: fix race in test_000 and test_001
node2 starts after node1 is healthy (depends_on: service_healthy) but
pg_autoctl create postgres takes several seconds. The old test_000
waited for node1=single (already satisfied) and test_001 immediately
queried the monitor before node2 had registered. Fix: test_000 now
waits for node1=wait_primary, which the monitor transitions to only
after node2 registers -- a reliable signal that node2 is in the node
table.
test_runner: use nodehost in psql fallback for monitor node-state lookup
The psql fallback queried pgautofailover.node WHERE nodename='<service>'.
After 'pg_autoctl set node metadata --name node_b', nodename changes but
nodehost (the container TCP hostname) stays as 'node2'. Use nodehost so
wait_until works correctly after a node rename.
compose_gen.c: write debian_cluster under [options] section in generated node.ini files. The nodespec parser reads it from [options] (line 130 of nodespec.c), but the generated ini was writing it under [node]. As a result spec.debianCluster was always empty, pg_createcluster was never called, and node1 never initialized — causing the 60 s setup timeout in the debian_clusters test. cli_node.c: guard copy_file() against src==dst. node_copy_ssl_certs() copies spec->ssl_cert_file to $HOME/server.crt. When ssl_cert_file is already at /var/lib/postgres/server.crt and HOME is /var/lib/postgres, the source and destination paths are identical; the old code opened the destination with O_TRUNC first, zeroing the file before reading from it. tests/upgrade/Makefile: copy Makefile.docker to the temp dir before building pgaf:current. Makefile.docker was added after the v2.1 tag, so git archive v2.1 does not include it, but the current Dockerfile's COPY Makefile Makefile.docker ./ requires it.
pg_createcluster creates /var/lib/postgresql/<ver>/ owned by the postgres system user with mode 755. pg_autoctl (running as the docker user, which is in group postgres) subsequently tries to create a backup directory at /var/lib/postgresql/<ver>/backup/node_N; that fails with EACCES because the group does not have write permission on the version directory. Fix: after pg_createcluster exits successfully, run sudo chown docker /var/lib/postgresql/<ver> so that the docker user owns the version directory and can create subdirectories inside it.
Extract the Debian-specific pg_createcluster + chown block from cli_node.c into pg_createcluster_for_test() in src/bin/common/debian.c, where the rest of the Debian-specific code already lives. cli_node.c now just calls pg_createcluster_for_test() with a comment that this is a pgaftest testing facility, not intended for production.
cli_drop_node.c: --no-wait ignored on monitor-side drop When 'pg_autoctl drop node --name <n> --no-wait' is run from the monitor, it went through cli_drop_node_from_monitor_and_wait() which never checked dropNoWait and always blocked for up to 60s waiting for the row to disappear. Add the same early-return guard that the local path already had. test_monitor_disabled: race on node2 config file not yet written test_004 starts node2 in the background then returns immediately. test_005 calls pg_autoctl inspect fsm state on node2 before it has finished writing its config file, causing 'missing [pg_autoctl] section'. Add wait_until_pg_autoctl_is_running() to PGNode (polls for config_file_path()) and call it at the end of test_004. test_multi_maintenance: cluster not settled before set_candidate_priority test_004 returned immediately after DML without confirming all three nodes were healthy. A monitor health-check gap during the CHECKPOINT could trigger a failover that left node1 non-primary when test_005's set_candidate_priority (which blocks for 60s) ran. Add the three wait_until_state assertions at the end of test_004. test_multi_standbys test_015_003: apply_settings cycle not waited for After set_formation_number_sync_standbys via SQL, the test used wait_until_assigned_state(primary) which returned instantly because the assigned state was already primary. The apply_settings → primary cycle that actually rewrites synchronous_standby_names had not run yet. Switch to wait_until_state(apply_settings) + wait_until_state( primary) which checks reportedstate and therefore blocks until the cycle completes.
pgaftest: add 'run' command (compose run --rm) for stopped containers
docker compose exec requires a running container; after 'pg_autoctl stop'
the container exits. Add CMD_RUN / T_RUN / 'run <svc> <args>' which
uses 'docker compose run --rm' instead, starting a fresh container with
the same volumes. Used in drop_node_destroy for the --destroy steps
where the supervisor must be stopped before calling drop node --destroy.
drop_node_destroy.pgaf: restructure all 9 steps to work correctly
- test_003/004: drop while running (keeper self-exits on next heartbeat),
verify PGDATA while container is still up, then wait for stop
- test_005/006: stop supervisor first (container exits), then 'run --rm'
for the drop --destroy command and the PGDATA-absent check
- test_007/008/009: same pattern for --destroy --force
test_multi_standbys test_015_003: correct wait after set_number_sync_standbys
The earlier fix waited for apply_settings AND primary. But with both
standbys failed, fsm_enable_sync_rep() writes synchronous_standby_names
to postgresql.conf (via fsm_apply_settings) then returns false because
no standby is in quorum sync state. Node1 is stuck in apply_settings
until test_015_004 restarts the standbys. Only wait for apply_settings,
which confirms the SSN was written; primary comes in test_015_004.
initialize_program() sets prog->program = args[0] verbatim — no PATH
lookup — so access("sudo", ...) fails with ENOENT. Use the absolute
path /usr/bin/sudo in both pg_createcluster and chown calls.
Also fix compose_gen.c: the debian_cluster ini entry was written in a
separate [options] block before the ssl/auth/pg_hba_lan block, producing
two [options] sections. Move debian_cluster = %s into the existing
combined [options] block so the generated node.ini has a single section.
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.
Overview
A ground-up overhaul of the pg_auto_failover CI and build infrastructure, consolidating years of accumulated complexity into a clean, maintainable system. This is the foundation that the PG17/18/19 support PR will rebase on top of.
1. Shared base image (
Dockerfile.base)ghcr.io/hapostgres/pg_auto_failover/pgaf-base:bookwormpg_config, andpostgresql-commontoolingdocker buildcalls use--build-arg BASE=...and get a fast incremental build — no redundant apt installs per CI jobrm -rf2. Unified Debian image (
pg_autoctl node runhandles cluster creation)pg_autoctl node runalready calls intopg_autoctl node initcode paths which callpg_createclusterwhendebian_clusteris set innode.ini(cli_node.c:499). The separatedebianDocker stage andPGAF_DEBIAN_IMAGEenv var have been removed — all images are already Debian images.initdb --allow-group-accessadded for Debian clusterscompose_gen.csimplified: no morePGAF_DEBIAN_IMAGEspecial-case; inlinebuild:stanza now passesPGVERSIONas a build arg so the correct single-version image is selected at compose-gen timepg_createclusterinvocation fixed3.
installcheckruns inside the Docker build stagemake installcheckruns viapg_virtualenvinside thebuildDockerfile stage — no separate CI job needed. TheinstallcheckCI job has been removed.4. Consolidated single CI workflow
The CI has been merged into a single
ci.ymlwith a clear job dependency graph:style_checkerandensure_baserun in parallel; all build jobs gate onneeds: [style_checker, ensure_base].Key changes:
build-test-pg*→build-pytest-pg*rename completed everywhere (Makefile.docker, tests/Makefile, tests/tablespaces/Makefile)docker tag pgaf:run-pgN pgaf:runalias; jobs usePGAF_IMAGE=pgaf:run-pg${{ matrix.PGVERSION }}directlyupgradejob getsdocker/login-action@v3somake pgaf-nextcan pull the ghcr.io basedocker/login-action@v3added to all build jobs that pull from ghcr.io5.
Makefile.docker— usable from any directoryversiontarget andVERSION_FILElogic moved intoMakefile.dockerusing$(REPO_ROOT)anchored to the Makefile's own location. Docker build contexts updated to$(REPO_ROOT)somake -C tests run-test PGVERSION=17works without a priormake versionfrom the root.6. pgaftest improvements
drop_node_destroysequencecreateDeferred+launchDeferred)\nrestored in compose_gen command/healthcheck stanzas (regression from\\→\nfix)pg_autoctl node runas the single entry point (unifies init/run paths)node post-initproper supervisor service wiring7. Test reliability fixes
monitor.destroy()no longer unconditionally re-raises on failure — monitor may already be gone at teardown time, poisoning the last test's resultDROP TABLEmoved out ofteardown_module()into numbered test steps (avoids metadata-sync race)has_needed_replication_slotsretry extended to coverlist_replication_slot_namesTesting
CI will run the full matrix on push. The upgrade test runs against PG16 (latest version supported by the previous release tag).