ci: overhaul CI infrastructure - #1148
Merged
Merged
Conversation
dimitri
changed the title
dimitri
added
Developer productivity
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
…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.
- Dockerfile: COPY Makefile* ./ instead of explicit Makefile Makefile.docker so that old source archives (e.g. v2.1 with Makefile.azure) work with the current Dockerfile without modification - tests/upgrade/Makefile: drop the cp Makefile.docker override — the old build system from the PREV_TAG archive is used as-is, which is correct since build dependencies have not changed across releases - ci.yml: pin docker/login-action to v3.4.0 (first release built against Node.js 24, eliminating the Node.js 20 deprecation warning)
Actions warnings: - docker/login-action@v3 → v3.4.0 (Node.js 24) - docker/setup-buildx-action@v3 → v3.10.0 (Node.js 24) - Remove deprecated 'install: true' from setup-buildx-action stanzas pytest monitor PG17 flaky race (test_003_002_stop_primary): - The keeper uses LISTEN/NOTIFY (monitor_wait_for_state_change) when the monitor is enabled, so state changes from the monitor wake the keeper immediately — no polling delay. - The real fix is accepting draining or demote_timeout: the monitor moves node2 from draining to demote_timeout so fast that a 100ms Python poll can miss draining entirely. citus_skip_pg_hba.pgaf / test_002d_run_worker: - Root cause: pg_autoctl create citus-worker --auth skip fails when the coordinator cannot call master_add_node (pg_hba blocks), which caused cli_node_run to exit the container even though pg_autoctl.cfg was written - Fix: use XDG config path (not PGDATA/pg_autoctl.cfg) for warm-start detection; when node_do_init fails but config was written, fall through to exec pg_autoctl run so the keeper can retry activation after HBA fix - The spec's pgsetup wait + hba-lan + wait-for-single flow is now correct pgaftest log capture on failure: - Emit docker compose logs for all containers when a step or setup block fails, before teardown destroys the evidence
dimitri
force-pushed
the
fix/citus-python-teardown
branch
from
July 14, 2026 15:55
e523819 to
89ff563
Compare
dimitri
changed the title
Actions Node.js 20 deprecation warnings: - docker/login-action@v3/v3.4.0 → v4.4.0 (Node.js 24) in ci.yml and base-image.yml - docker/setup-buildx-action@v3/v3.10.0 → v4.2.0 (Node.js 24) in ci.yml and base-image.yml - Remove deprecated 'install: true' from setup-buildx-action in base-image.yml pytest / tablespaces (PG14) — 401 Unauthorized on ghcr.io: - run-test-prebuilt for TEST=tablespaces was calling 'make test teardown' in tablespaces/Makefile, whose 'test' target depends on 'build', which calls 'build-pytest-pg14' — rebuilding the image from scratch via a docker build that tries to pull the ghcr.io base image without a login step - Fix: add 'run-test-prebuilt' target to tablespaces/Makefile that only runs 'docker compose build' (FROMs the already-loaded local image) plus the test steps plus teardown, skipping the build-pytest-pgN rebuild entirely - Update tests/Makefile to call 'make -C tablespaces run-test-prebuilt'
keeper_config_write_file and monitor_config_write_file both opened the
config file with fopen(..., "w"), which truncates the file to 0 bytes
immediately and then writes the new content. Any concurrent reader
(e.g. pg_autoctl inspect pgsetup wait) that ran between the truncation
and fclose would see an empty file, fail to find the [pg_autoctl] section,
and exit non-zero.
This caused the pytest/ssl test_006_enable_ssl_primary failure:
stop_pg_autoctl() -- process exits
enable_ssl() -- pg_autoctl enable ssl rewrites the config
run() -- starts keeper in the background; keeper writes
the config early in its init (truncates it)
wait_until_pg_is_running() -- pgsetup wait reads the config and sees
an empty file during the truncation window
Fix: write to <path>.tmp first, then rename() to the final path.
rename() is atomic on POSIX -- readers always see either the old complete
file or the new complete file, never an empty file.
dimitri
force-pushed
the
fix/citus-python-teardown
branch
from
July 14, 2026 16:52
b1f9840 to
98dda07
Compare
…ailover
perform_failover's GetNodeToFailoverFromInGroup requires both reportedstate
and goalstate to equal an eligible primary state (primary/single/join_primary).
wait_until_state("primary") only polls reportedstate — there is a window where
the keeper just reported primary but the monitor's goalstate column has not yet
been updated to match.
Fix: before calling perform_failover, poll pgautofailover.node until at least
one node satisfies reportedstate = goalstate IN ('primary','single','join_primary'),
mirroring the exact C predicate in GetNodeToFailoverFromInGroup.
… state When node3 is the sole remaining standby after node2 fails, the monitor assigns report_lsn then immediately assigns prepare_promotion in the same node_active() transaction (both events share the same millisecond timestamp). node3's keeper never reports reportedstate=report_lsn because it transitions to prepare_promotion before the next poll round. wait_until_state() polls reportedstate — it never observes report_lsn. Fix: use wait_until_assigned_state(target_state='report_lsn', or_state='prepare_promotion') to match the goalstate column instead, mirroring the draining/demote_timeout fix already applied for node2 in 89ff563.
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. A core goal of this PR is eliminating all flaky test failures so the CI green/red signal is reliable.
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→v4.4.0(Node.js 24) pinned across all jobsdocker/setup-buildx-action→v4.2.0(Node.js 24); deprecatedinstall: trueremoved5.
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 wiringdocker compose logs --timestampsis emitted after any failing step or setup block, before teardown destroys the evidence7. Test reliability fixes
All changes in this section target known or observed flaky failures. The goal is that every CI failure on this branch is a real regression, not a race condition.
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_namestest_003_002_stop_primaryflaky race: the monitor moves node2 fromdraining→demote_timeoutfast enough that a 100 ms Python poll can missdrainingentirely; fixed by acceptingor_state="demote_timeout"as successcitus_skip_pg_hba.pgaf / test_002d_run_worker:pg_autoctl create citus-worker --auth skipfails when the coordinator'smaster_add_nodeis blocked bypg_hba, butpg_autoctl.cfgis written before the failure. Fixed incli_node.c: warm-start detection now uses the real XDG config path (notPGDATA/pg_autoctl.cfg), and whennode_do_initfails but the config was written the process falls through toexec pg_autoctl runso the keeper retries activation after HBA rules are added.pytest / sslconfig write race:keeper_config_write_fileandmonitor_config_write_fileusedfopen("w")which truncates the file to 0 bytes before writing — concurrentpgsetup waitcould read an empty file and fail with "Failed to find section pg_autoctl". Fixed with atomic temp-file +rename()in both functions.test_auth / test_004_failoverflaky race:Monitor.failover()was callingperform_failoverimmediately afterwait_until_state("primary"). The monitor'sGetNodeToFailoverFromInGrouprequiresreportedstate == goalstate(both stable atprimary), but a keeper that just transitioned and sent its firstnode_activecan briefly havereportedstate = primarywhile the monitor has not yet written the matchinggoalstate. Fixed by waiting inMonitor.failover()for the monitor's ownpgautofailover.nodetable to showreportedstate = goalstatein an eligible state before callingperform_failover.Testing
CI will run the full matrix on push. The upgrade test runs against PG16 (latest version supported by the previous release tag).