Repository navigation
Cleanup: BDD CI speedup, web-ui quoting, binary-upgrade docs, NO_COLOR, EL7/EL8 RPM - #251
Merged
Merged
Conversation
added 2 commits
May 13, 2026 14:12
… web-ui
cucumber-rs `{string}` parameter parser does not unescape `\"` inside double-quoted Gherkin strings, so the nine `should contain` assertions on `/api/auth/config` and `/api/config` were searching the curl output for literal `\"key\":\"value\"` substrings with backslashes that never appear in the JSON body. The first three Web UI scenarios (`sso_enabled`, `current_user`, `role`/`source`, four bind-address keys, `changeable`) fail in BDD: Web UI of the post-merge run because of this.
Swap the outer Gherkin quotes to single quotes so the embedded double quotes pass through verbatim and the substring search hits the JSON body. Scenario semantics are unchanged.
…matrix Each of the 23 entries in the `bdd-tests` matrix used to start from a cold workspace, run `cargo download` (1-2 min) and recompile `pg_doorman` plus the BDD test crate (4-9 min) inside the test-runner image before the actual tags ran. The fan-out wasted ~150 CPU-min of duplicate compile per CI run and stretched the wall clock to ~30 minutes even at 6 parallel suites. Add a `prebuild-bdd` job between `prepare-tests` and `bdd-tests`. It checks out the source, restores the cache, pulls the same test-runner image, and compiles `cargo test --test bdd --no-run`, `cargo test --test patroni_proxy_bdd --no-run`, and the `--features tls-migration` variant. `CARGO_HOME=/workspace/.cargo` keeps the registry and the build artefacts inside `github.workspace`, so `actions/cache@v4` stores `.cargo/` and `target/` without touching runner-level paths. Each matrix entry now `needs: prebuild-bdd`, restores the same cache, and reuses the prebuilt binaries. cargo resolves the test against the cached `target/`, so the per-suite cost drops to a link plus the scenario runtime.
vadv
changed the title
added 8 commits
May 13, 2026 14:55
…unit to Type=notify `cp pg_doorman_new /usr/bin/pg_doorman` rewrites the live inode the running pg_doorman is memory-mapped from. The process can take SIGBUS or SIGSEGV mid-upgrade because its text segment is being overwritten in place. `install -m 0755` writes to a temporary file and renames it onto the target, so the running inode survives and only new processes pick up the new file. The shipped `pg_doorman.service` already uses `Type=notify`, but the tutorial example showed `Type=simple`. With `Type=simple` systemd marks the unit `active` immediately after exec without waiting for `READY=1`, and the new process spawned by `SIGUSR2` cannot hand off `MainPID` to itself. `Type=notify` + `NotifyAccess=all` lines up the tutorial with the real readiness contract: pg_doorman sends `READY=1` once listeners are bound and `MAINPID=<new_pid>` during an upgrade, both via `sd_notify`. `ExecReload=SIGHUP` covers config reload; `kill -USR2` and `UPGRADE;` still drive the binary upgrade.
…rial Four operator-facing issues left over after the cp/Type=notify fix: - `kill -USR2 $(pgrep -f /usr/bin/pg_doorman)` matches command line and signals every pg_doorman process on the host. On a multi-tenant box or after a stuck previous upgrade the cleanup signal lands on neighbours. Switch the Quick start to `systemctl kill -s SIGUSR2 pg_doorman.service`, which targets the single tracked MainPID, and add a stand-alone tip for the PID-file path. - Verification step relied on `pgrep -f /usr/bin/pg_doorman`, which cannot tell the old PID from the new one once both are alive during the drain window. Replace it with `systemctl show -p MainPID --value`, the same field `Type=notify` updates from `MAINPID=<new_pid>`. - The shipped systemd example in "Daemon vs foreground" lacked the production knobs every operator ends up adding: `Restart=on-failure` + `RestartSec=5s` to absorb a single crash without a tight loop, `LimitNOFILE=1048576` to clear the default 1024 fd cap that connection-heavy poolers hit first, `User=pg_doorman`/`Group=pg_doorman` for a non-root listener, `KillMode=mixed` so SIGUSR2 reaches the parent and the children, and `TimeoutStopSec=120` aligned with `shutdown_timeout`. Each setting has a one-line comment explaining why it is there. - Operational checklist still asked operators to verify `ExecReload=/bin/kill -SIGUSR2 $MAINPID`. The new unit uses SIGHUP for reload; SIGUSR2 is the binary-upgrade trigger and lives outside `ExecReload`. Updated the checklist item to match. - Added a tip above Quick start naming `apt-get install --only-upgrade pg-doorman` / `dnf upgrade pg-doorman` as the preferred path when a package is in scope, so direct-binary `install -m 0755` is positioned as the fallback rather than the recommendation. EN and RU tutorials updated in lock-step.
…onour NO_COLOR `TextLogger` writes `\x1b[31m`-style colour escapes to stderr whenever `args.no_color` is false. Under systemd that stderr is the journal pipe, not a terminal, so the escape bytes ended up inside the `MESSAGE` field and journalctl rendered every record as `[NNN blob data]`. Operators reading `journalctl -u pg-pooler` could not see the actual log text without remembering to pass `--no-color` or pin `Environment=NO_COLOR=...`. `clap`'s `env` attribute on the bool flag also made `NO_COLOR=1` (the canonical https://no-color.org/ value) crash startup because the parser only accepts `true`/`false`. Wire `should_use_color` between args and `TextLogger::new`. The pure `resolve_color(no_color_flag, env_no_color, stderr_is_tty)` predicate now gates the colour escapes on all three signals: explicit `--no-color`, any non-empty `NO_COLOR` value, and `stderr.is_terminal()`. The four cases of that predicate are covered by unit tests. Drop the `env` attribute on `args.no_color` so a `NO_COLOR=1` env var no longer fails clap parsing; the logger reads the standard's spelling directly via `var_os`.
…no longer fails the whole job `docker/login-action@v3` calls `docker login` exactly once. When ghcr.io's auth endpoint times out mid-handshake (`Error response from daemon: Get \"https://ghcr.io/v2/\": net/http: request canceled (Client.Timeout exceeded while awaiting headers)`) the entire BDD job, the prebuild job, or the image-check job dies before its real work starts. Outer retries on `docker pull` already handle the pull leg of the same flake, but the login step was a single point of failure. Replace every `Log in to Container Registry` step in `bdd-tests.yml` with `nick-fields/retry@v3` wrapping a plain `docker login ... --password-stdin`. Three attempts with a 10-second back-off match the policy we already use for `docker pull`, and the credential is passed via stdin so it never appears on the command line or in the process listing.
…logger behaviour After the previous commit dropped the `env` attribute on `--no-color` and added auto-TTY detection plus `NO_COLOR` handling, the `--help` snapshots in `basic-usage.md` (EN and RU) still showed `disable colors in the log output [env: NO_COLOR=]`. Update the snapshot to the new help text and expand the option description to call out the auto-disable triggers (non-TTY stderr, non-empty `NO_COLOR`).
… COPR builds
`pg-doorman.spec` listed `perl-FindBin` and `perl-IPC-Cmd` as `BuildRequires`. On RHEL/CentOS Stream 7 and 8 these modules ship only inside the modular `perl:5.30` stream, which COPR's mock filters out by default ("package perl-FindBin-... is filtered out by modular filtering"). COPR builders have no network access, so we cannot `dnf module enable perl` inside the chroot, and the rhel-8, centos-stream-8, epel-8, and epel-7 chroots fail at `dnf builddep` before pg_doorman compiles. AlmaLinux 9 / Rocky 9 / Fedora / Amazon Linux 2023 are unaffected because they expose the same modules outside modular streams.
Apply the same offline-tarball pattern we already use for Rust:
- The CI prepare-srpm step now installs `perl-FindBin` and `perl-IPC-Cmd` in the Fedora container, copies the `.pm` files from `@INC` into `perl_modules/`, and tarballs the directory as `perl_modules.tar.gz`.
- The spec adds `Source3: perl_modules.tar.gz`, swaps the two missing `BuildRequires` for `perl-interpreter`, extracts the tarball into `_builddir/perl_modules`, and exports `PERL5LIB="_builddir/perl_modules:$PERL5LIB"` in `%build` so the openssl-sys Configure path resolves `use FindBin` and `use IPC::Cmd` from the bundle on every chroot, modular filtering or not.
The pure-Perl module list covers FindBin plus the dependency closure IPC::Cmd actually walks at use-time (Module::Load::Conditional, Module::Load, Module::Metadata, Params::Check, Locale::Maketext::Simple, ExtUtils::MakeMaker, version, IPC::Open3). Modules without architecture-specific XS bits, so the tarball is portable across the EL7/EL8/EL9/Fedora/Amazon mix.
…s saving an empty tarball `prebuild-bdd` runs `cargo test --no-run` inside the test-runner container. The container runs as root, so the `target/` and `.cargo/` files it produces are root-owned on the host. `actions/cache@v4`'s post-step runs as the runner user (`uid 1001`) and silently fails to read the root-owned files; in the recent PR #251 runs it saved a roughly empty tarball in ~13 s and the next run's `Restore cargo cache` step finished in 0 s with nothing to restore. Result: every `prebuild-bdd` paid full 5-7 minutes of cargo build even though the cache key matched. Add a `Chown cargo cache so actions/cache can read it` step after the two docker compile steps. `sudo chown -R "$(id -u):$(id -g)" .cargo target` is enough on GitHub-hosted ubuntu-latest (sudo is available without password) to flip ownership back to the runner user before the cache post-step tars the directories. With the chown the save step takes its real time (single-digit minutes on a fully-built target), and the next run's restore actually populates `.cargo/` and `target/`.
With prebuild-bdd populating the cache, each matrix entry is now bound by link time and the scenario itself (~1-2 min) rather than the full cargo build (~7-9 min). The CPU contention that drove the previous cap of 6 (sleep-heavy lifecycle and SCRAM passthrough reconnect losing their timing margin) does not apply to the link-only path, so we can safely double the fan-out. Wall clock on a green PR roughly halves; the timing-sensitive suites keep their `nick-fields/retry@v3` outer policy.
vadv
changed the title
…pattern User flagged that their production unit differs from the tutorial. Adjust the example so it matches what is actually deployed, while keeping the comments load-bearing. - `NotifyAccess=exec` instead of `all`: the upgrade child is spawned via Command::spawn (fork + execve), so exec scope is enough; `all` was an over-broad surface. - `ExecReload=/bin/kill -SIGUSR2 $MAINPID` instead of SIGHUP: `pg_doorman -t` runs first under SIGUSR2 and aborts the reload on a bad config, so `systemctl reload` becomes the single command that drives a full safe binary upgrade. Quick start now uses `systemctl reload pg_doorman.service` accordingly. - `Restart=always` instead of `on-failure`: the production expectation is that the pool never stays down on the host, even after an explicit `systemctl stop`. - `LimitNOFILE=65536` instead of `1048576`: the 1M default was over-cautious; 65536 covers most OLTP pools and the comment now explains how to size it from `pool_size * num_pools` plus clients. - `User=postgres` / `Group=postgres` instead of a dedicated `pg_doorman` account: many deployments already have the postgres account, reusing it keeps file ownership aligned with PostgreSQL. - `SyslogIdentifier=pg-pooler`: matches the production journal identifier so `journalctl -u pg_doorman -t pg-pooler` is the documented lookup path. - `ExecStop=/bin/kill -SIGTERM $MAINPID`: redundant with the default but explicit makes the stop path obvious next to the reload path. - Drop `KillMode=mixed`: the production setup runs without it and works because Type=notify already updates MainPID during the handoff window; keeping it pulled out lines up with the deployed unit. Mention `MemoryMax/Nice/CPUAffinity` as orthogonal resource controls in a trailing paragraph. Operational checklist and step 3 of Quick start updated to match. EN and RU edited in lock-step.
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
Cleanup pass on the BDD CI pipeline that surfaced after merging #250, plus two operator-facing fixes that landed in the same review window.
What changes
BDD: Web UIwas failing on nineshould containassertions about/api/auth/configand/api/config. The cucumber-rs{string}parameter parser does not unescape\"inside double-quoted Gherkin strings, so the steps were searching the curl output for literal\"key\":\"value\"substrings with backslashes that never appear in the JSON body. Swap the outer Gherkin quotes to single quotes so the embedded"pass through verbatim.prebuild-bddjob +actions/cache@v4ontarget/and.cargo/. Each entry in the 23-waybdd-testsmatrix used to start from a cold workspace and pay a fullcargo download + compilecycle (~5-10 minutes per suite) inside the test-runner image before the actual scenarios ran. The new job compiles the BDD binaries once for default features and once for--features tls-migration; the matrix restores the same cache and reuses the prebuilt artifacts. The cache had to be chowned back to the runner user after eachdocker run, otherwiseactions/cache@v4was silently saving an empty tarball (root-owned files unreadable to uid 1001).bdd-testsmatrixmax-parallel: 6 → 12. With the cache, each entry is now link-time + scenario, not full compile, so the CPU pressure that drove the previous cap is gone.docker login ghcr.iois wrapped innick-fields/retry@v3in every step that needed it (check-image,build-and-push-image,prebuild-bdd,bdd-testsmatrix).docker/login-action@v3callsdocker loginonce; ghcr.io's auth handshake occasionally times out under GHA load (Client.Timeout exceeded while awaiting headers) and was killing whole jobs.tutorials/binary-upgrade.md(EN+RU):cp pg_doorman_new /usr/bin/pg_doorman→install -m 0755.cprewrites the inode the running pg_doorman is mapped from and risks SIGBUS/SIGSEGV mid-upgrade;installwrites to a tempfile and renames. systemd unit example flipped fromType=simpletoType=notifywithNotifyAccess=all,ExecReload=SIGHUP,Restart=on-failure,RestartSec=5s,LimitNOFILE=1048576,User=pg_doorman,KillMode=mixed,TimeoutStopSec=120. Quick-startkill -USR2 $(pgrep -f ...)replaced withsystemctl kill -s SIGUSR2; verification withsystemctl show -p MainPID. Apt/dnf package install added as the preferred path.TextLoggerauto-disables ANSI colours when stderr is not a TTY and onNO_COLOR. Under systemd the unit's stderr was the journal pipe; the colour escapes were leaking intoMESSAGEand journalctl rendered every record as[NNN blob data]. Theclapenvattribute on--no-coloralso madeNO_COLOR=1crash startup; dropped, and the logger now readsNO_COLORdirectly per https://no-color.org/.perl-FindBin/perl-IPC-Cmdmodular filtering. The CI workflow bundles the pure-Perl modules (FindBin + IPC::Cmd dependency closure) intoperl_modules.tar.gzasSource3; the spec extracts it and exportsPERL5LIBso the openssl-sys Configure path resolves the modules regardless of distro, matching the offline-tarball pattern we already use for the Rust toolchain.Risk
${{ runner.os }}-bdd-cargo-${{ image-tag }}-${{ hashFiles('Cargo.lock', 'src/**', 'tests/**', 'build.rs') }}with restore-keys falling back first to any prebuild for the current image tag, then to any prebuild on this OS. GHA cache limit is 10 GB; debugtarget/compresses to ~500-800 MB.--no-color:NO_COLOR=falsenow disables colour (was: colour on). Conforms to no-color.org. Operator surface stays the same:--no-colorflag, or any non-emptyNO_COLOR..pmfiles only; no XS, portable across EL7/EL8/EL9/Fedora/Amazon Linux.Test plan
BDD: Web UIgreen on this branch.prebuild-bddshowsCache restoredon the second run with the sameCargo.lock/src/testshash; matrix entries finish noticeably faster than master.actions/cachesave step takes more than the previous ~13 s (saving a populated cache, not an empty tarball).Publish to Fedora COPRPR-stagetest-rpm-buildmatrix is green andUpload to Fedora COPRsucceeds on the next release tag for all 18 chroots (including rhel-8, epel-8, centos-stream-8, epel-7).