diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index 439f2bf702..94cb0e2ae5 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -38,6 +38,7 @@ IS_NIX_UPGRADE=${IS_NIX_UPGRADE:-} IS_NIX_BASED_SYSTEM="false" PGVERSION=$1 +POSTGRES_SHARE_DIR="/usr/share/postgresql/${PGVERSION}" MOUNT_POINT="/data_migration" LOG_FILE="/var/log/pg-upgrade-initiate.log" @@ -85,7 +86,13 @@ fi OLD_BOOTSTRAP_USER=$(run_sql -A -t -c "select rolname from pg_authid where oid = 10;") +# shellcheck disable=SC1091 +source "$SCRIPT_DIR/start_guard.sh" + +CLEANUP_STARTED="" + cleanup() { + CLEANUP_STARTED=1 UPGRADE_STATUS=${1:-"failed"} EXIT_CODE=${?:-0} @@ -106,6 +113,24 @@ cleanup() { enable_conflicting_timers || log "WARNING: failed to re-enable one or more timers; check 'systemctl list-timers --all' on this host" fi + # Put the old share dir back before the start block is lifted, so a start that + # slips in right after it cannot see the new version's files. + if [ -L "$POSTGRES_SHARE_DIR" ]; then + rm "$POSTGRES_SHARE_DIR" || log "WARNING: failed to remove the postgres share symlink" + + if [ -f "${POSTGRES_SHARE_DIR}.bak" ]; then + mv "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR" || log "WARNING: failed to restore the postgres share directory" + fi + + if [ -d "${POSTGRES_SHARE_DIR}.bak" ]; then + mv "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR" || log "WARNING: failed to restore the postgres share directory" + fi + fi + + # Same reasoning as the timers: lift the start block before anything else that + # can fail under set -e, or the restart below would be refused. + unblock_postgres_start || log "WARNING: failed to lift the postgres start block; check /run/systemd/system/postgresql.service.d" + if [ -d "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" ]; then log "Copying pg_upgrade output to /var/log" cp -R "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" /var/log/ || true @@ -116,18 +141,6 @@ cleanup() { ship_logs "/var/log/pg_upgrade_output.d/pg_upgrade.log" || true fi - if [ -L "/usr/share/postgresql/${PGVERSION}" ]; then - rm "/usr/share/postgresql/${PGVERSION}" - - if [ -f "/usr/share/postgresql/${PGVERSION}.bak" ]; then - mv "/usr/share/postgresql/${PGVERSION}.bak" "/usr/share/postgresql/${PGVERSION}" - fi - - if [ -d "/usr/share/postgresql/${PGVERSION}.bak" ]; then - mv "/usr/share/postgresql/${PGVERSION}.bak" "/usr/share/postgresql/${PGVERSION}" - fi - fi - log "Restarting postgresql" if [ -z "$IS_CI" ]; then systemctl enable postgresql @@ -183,6 +196,28 @@ EOF fi } +# EXIT trap: any exit that did not go through cleanup() (set -u abort, TERM/INT) +# still restores the source, since step 10 left it stopped and disabled. If +# cleanup() already ran (or died part way under set -e), only make sure the +# start block is gone. +on_exit() { + local exit_code=$? + trap - ERR + set +e + if [ -z "$CLEANUP_STARTED" ]; then + ( + trap - ERR EXIT + set -e + cleanup failed + ) + if [ "$exit_code" -eq 0 ]; then + exit_code=1 + fi + fi + unblock_postgres_start || log "WARNING: failed to lift the postgres start block; check /run/systemd/system/postgresql.service.d" + exit "$exit_code" +} + function handle_extensions { if [ -z "$IS_CI" ]; then retry 5 systemctl restart postgresql @@ -233,6 +268,11 @@ EOF } function initiate_upgrade { + # This runs in a background subshell, where the parent's traps do not apply. + trap on_exit EXIT + trap 'exit 143' TERM + trap 'exit 130' INT + # Before anything destructive: no || true here, a timer firing mid-upgrade is # exactly what this guards against, and failing now leaves the project untouched. # Interrupting an apt-daily run can leave dpkg half-configured; step 2's @@ -616,10 +656,13 @@ EOF sleep 3 systemctl stop postgresql + block_postgres_start else CI_stop_postgres fi + start_pid_lock_watcher + # Start the old PostgreSQL instance with version-specific options if [[ ${PGVERSION%%.*} -ge 16 ]]; then GRN_PLUGINS_DIR=/var/lib/postgresql/.nix-profile/lib/groonga/plugins LC_ALL=en_US.UTF-8 LANGUAGE=en_US.UTF-8 LANG=en_US.UTF-8 LOCALE_ARCHIVE=/usr/lib/locale/locale-archive su -pc "$UPGRADE_COMMAND" -s "$SHELL" postgres @@ -627,6 +670,10 @@ EOF GRN_PLUGINS_DIR=/var/lib/postgresql/.nix-profile/lib/groonga/plugins LC_ALL=en_US.UTF-8 LC_CTYPE=$SERVER_LC_CTYPE LC_COLLATE=$SERVER_LC_COLLATE LANGUAGE=en_US.UTF-8 LANG=en_US.UTF-8 LOCALE_ARCHIVE=/usr/lib/locale/locale-archive su -pc "$UPGRADE_COMMAND" -s "$SHELL" postgres fi + if ! assert_source_stayed_down; then + false # not exit: let the ERR trap run cleanup + fi + # copying custom configurations log "11. Copying custom configurations" mkdir -p "$MOUNT_POINT/conf" diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh new file mode 100755 index 0000000000..231a06040b --- /dev/null +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh @@ -0,0 +1,195 @@ +#! /usr/bin/env bash + +# Start guard for initiate.sh: keeps the source Postgres down during the pg_upgrade +# file copy. Sourced by initiate.sh and tests/pg_upgrade/test-start-guard.sh. +# Expects log, IS_CI, PGDATAOLD and PGDATANEW from the caller. + +# From step 10 until cleanup, nothing may start the source postgres: pg_upgrade +# copies the data files without a postmaster holding the directory, and a start +# in that window (Salt's `service.running`, adminapi, Restart=always, ...) +# yields a copy that is inconsistent with the dump. Two independent guards: +# 1. a runtime systemd drop-in that makes `systemctl start` fail while this +# script or pg_upgrade is alive (a SIGKILLed script must not leave the +# source blocked until a reboot); +# 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which +# makes any postmaster (systemd or direct pg_ctl) refuse to start while +# pg_upgrade lives. The file goes stale when pg_upgrade exits. +PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" +PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" +PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" +PG_UPGRADE_PID_LOCK_TMP_NAME="postmaster.pid.pg-upgrade-lock" +PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid" +PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh" +PG_UPGRADE_LOCK_WATCHER_PID="" +PG_UPGRADE_START_BLOCKED="" + +block_postgres_start() { + # systemd needs absolute paths, and a drop-in naming a missing binary would + # silently never expire: fail here instead. + local sh_path rm_path systemctl_path + sh_path=$(type -P sh) || { + log "ERROR: sh not found in PATH" + return 1 + } + rm_path=$(type -P rm) || { + log "ERROR: rm not found in PATH" + return 1 + } + systemctl_path=$(type -P systemctl) || { + log "ERROR: systemctl not found in PATH" + return 1 + } + mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" + # initiate_upgrade runs in a background subshell: $BASHPID is its own pid, $$ is + # the already exited parent. + printf '%s\n' "${BASHPID:-$$}" >"$PG_UPGRADE_INITIATE_PID_FILE" + # Refuse a start only while the pid in one of the files is alive, so a SIGKILLed + # script stops blocking once pg_upgrade is gone too. Run through sh by the + # drop-in because /run is mounted noexec. + cat >"$PG_UPGRADE_START_GUARD" <<'GUARD' +for pid_file in "$@"; do + pid=$(cat "$pid_file" 2>/dev/null) || continue + # not a number: some shells' kill treat it as a job name, not an error + case "$pid" in + '' | *[!0-9]*) continue ;; + esac + if kill -0 "$pid" 2>/dev/null; then + exit 255 + fi +done +exit 0 +GUARD + # ExecCondition runs before the unit's own ExecStartPre (postgres_prestart.sh, + # as root), which therefore does not run for a refused start. Exit 255 fails the + # start, 1-254 would only skip it silently. The unit does not run as root, so + # `+` is needed to see root-owned pids and to remove files under /run. A start + # that gets past the guard means this script and pg_upgrade are gone: the + # ExecStartPost lines then remove the drop-in so Restart=no does not outlive it. + printf '[Service]\nExecCondition=+%s %s %s %s\n' \ + "$sh_path" "$PG_UPGRADE_START_GUARD" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" >"$PG_UPGRADE_LOCK_DROPIN" + printf 'ExecStartPost=-+%s -f %s %s %s %s\nExecStartPost=-+%s daemon-reload\nRestart=no\n' \ + "$rm_path" "$PG_UPGRADE_LOCK_DROPIN" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_START_GUARD" \ + "$systemctl_path" >>"$PG_UPGRADE_LOCK_DROPIN" + PG_UPGRADE_START_BLOCKED=1 + systemctl daemon-reload +} + +# Wait for the new cluster's temp server (the old one is down from then on, +# its schema dump is done) and then claim the old data dir's postmaster.pid. +# It must not exist before: pg_upgrade aborts if it finds one at startup, so +# finding one now means a postmaster started during the copy. +# The watcher exits 0 when it took the lock or when there is nothing left to +# lock (initiate.sh and pg_upgrade are gone), and non-zero when it could not +# take the lock: assert_source_stayed_down turns that into a failed upgrade. +start_pid_lock_watcher() { + local initiate_pid=${BASHPID:-$$} + ( + # set -E would otherwise run cleanup() from this subshell + trap - ERR EXIT + set +e + until [ -e "$PGDATANEW/postmaster.pid" ]; do + # initiate.sh SIGKILLed and pg_upgrade gone: nothing left to lock + if ! kill -0 "$initiate_pid" 2>/dev/null && ! pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" >/dev/null; then + exit 0 + fi + sleep 0.2 + done + upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) + if [ -z "$upg_pid" ]; then + log "WARNING: pg_upgrade not found; not creating the pg_upgrade lock" + exit 1 + fi + if [ -e "$PGDATAOLD/postmaster.pid" ]; then + log "ERROR: $PGDATAOLD/postmaster.pid already exists; a postmaster ran during the copy" + exit 1 + fi + printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1 + # Build the file aside and link it into place: the link is atomic and fails + # if the name exists, so a watcher killed by unblock_postgres_start can never + # leave a truncated postmaster.pid behind (cmp would not match it). + lock_tmp="$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + if ! { + cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$lock_tmp" && + chown postgres:postgres "$lock_tmp" && + chmod 0600 "$lock_tmp" && + ln "$lock_tmp" "$PGDATAOLD/postmaster.pid" + }; then + rm -f "$lock_tmp" + log "ERROR: could not create the pg_upgrade PID lock" + exit 1 + fi + rm -f "$lock_tmp" + log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" + ) & + PG_UPGRADE_LOCK_WATCHER_PID=$! +} + +# Idempotent: cleanup() can run before step 10 or more than once. +unblock_postgres_start() { + if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then + kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + wait "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + PG_UPGRADE_LOCK_WATCHER_PID="" + fi + local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" + # a watcher killed between writing and linking leaves only this temp file + rm -f "${PGDATAOLD:-/nonexistent}/$PG_UPGRADE_PID_LOCK_TMP_NAME" + # Callers run this under `||` where set -e is off, so report failures through + # the return status: a failed daemon-reload leaves the block loaded in systemd. + local rc=0 + if [ -f "$PG_UPGRADE_PID_LOCK_MARKER" ]; then + if [ -f "$pidfile" ] && cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$pidfile"; then + rm -f "$pidfile" || rc=1 + fi + if [ "$rc" -eq 0 ]; then + rm -f "$PG_UPGRADE_PID_LOCK_MARKER" || rc=1 + fi + fi + if [ -z "$IS_CI" ]; then + if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then + PG_UPGRADE_START_BLOCKED=1 + rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 + rm -f "$PG_UPGRADE_INITIATE_PID_FILE" || rc=1 + rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true + fi + systemctl daemon-reload || rc=1 + # Only after a successful reload: with the drop-in still loaded, a missing + # guard exits 127, which systemd treats as a silent skip of the start. + if [ "$rc" -eq 0 ]; then + rm -f "$PG_UPGRADE_START_GUARD" || rc=1 + fi + if [ "$rc" -eq 0 ] && [ -n "$PG_UPGRADE_START_BLOCKED" ]; then + systemctl reset-failed postgresql || true + PG_UPGRADE_START_BLOCKED="" + fi + fi + return $rc +} + +# The source must have stayed down for the whole copy; if not, the copy is +# inconsistent and the upgrade has to fail (the source data is untouched). +assert_source_stayed_down() { + local state="" watcher_rc=0 + # A watcher that is still polling is fine; one that is gone must have exited 0. + # `wait` reports a child's status only once, so nothing may wait on the watcher + # before this check (unblock_postgres_start does, and runs after it). 127 means + # the status is not available (already reaped), not that the watcher failed. + if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ] && ! kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; then + wait "$PG_UPGRADE_LOCK_WATCHER_PID" || watcher_rc=$? + if [ "$watcher_rc" -ne 0 ] && [ "$watcher_rc" -ne 127 ]; then + log "ERROR: the pg_upgrade lock could not be taken; the copied data files may be inconsistent" + return 1 + fi + fi + if [ -z "$IS_CI" ]; then + state=$(systemctl is-active postgresql || true) + fi + if [ "$state" = "active" ] || [ "$state" = "activating" ] || [ "$state" = "reloading" ]; then + log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent" + return 1 + fi + if [ -f "$PGDATAOLD/postmaster.pid" ] && ! cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid"; then + log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent" + return 1 + fi +} diff --git a/ansible/tasks/internal/admin-api.yml b/ansible/tasks/internal/admin-api.yml index dcdc68a393..e7d30b71ee 100644 --- a/ansible/tasks/internal/admin-api.yml +++ b/ansible/tasks/internal/admin-api.yml @@ -74,6 +74,7 @@ - { file: "prepare.sh" } - { file: "pgsodium_getkey.sh" } - { file: "common.sh" } + - { file: "start_guard.sh" } - name: adminapi - create service file template: diff --git a/audit-specs/baselines/baseline.yml b/audit-specs/baselines/baseline.yml index d26d55543b..148dad8886 100644 --- a/audit-specs/baselines/baseline.yml +++ b/audit-specs/baselines/baseline.yml @@ -145,6 +145,12 @@ file: owner: "1006" group: "0" filetype: file + /etc/adminapi/pg_upgrade_scripts/start_guard.sh: + exists: true + mode: "0755" + owner: "1006" + group: "0" + filetype: file /etc/alternatives/README: exists: true mode: "0644" diff --git a/nix/checks.nix b/nix/checks.nix index 1a4b21a212..906eb5a3ad 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -608,6 +608,21 @@ psql_17_cli = pkgs.runCommand "run-check-harness-psql-17-cli" { } ( lib.getExe (makeCheckHarness self'.packages."psql_17_cli/bin" { isCliVariant = true; }) ); + pg_upgrade_start_guard = + let + src = lib.fileset.toSource { + root = ../.; + fileset = lib.fileset.unions [ + ../tests/pg_upgrade/test-start-guard.sh + ../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh + ../ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh + ]; + }; + in + pkgs.runCommand "pg-upgrade-start-guard-test" { } '' + bash ${src}/tests/pg_upgrade/test-start-guard.sh + touch $out + ''; # Portable CLI bundle portability checks psql_17_cli_portable = pkgs.runCommand "psql_17_cli_portable-portability-check" diff --git a/nix/docs/testing-pg-upgrade-scripts.md b/nix/docs/testing-pg-upgrade-scripts.md index 9aa9cc7306..2b6afafd83 100644 --- a/nix/docs/testing-pg-upgrade-scripts.md +++ b/nix/docs/testing-pg-upgrade-scripts.md @@ -2,6 +2,20 @@ This document describes how to test changes to the PostgreSQL upgrade scripts on a running machine. +## Startup Guard Unit Tests + +Run the deterministic startup-guard tests locally on macOS or Linux: + +```bash +bash tests/pg_upgrade/test-start-guard.sh +``` + +The suite uses temporary directories and command shims, not a live PostgreSQL +instance or systemd. It covers PID lock ownership, drop-in install/removal, +reload retries, share-directory restoration failures, and ERR/EXIT/TERM/INT +cleanup. It is run in CI as `nix flake check`. A full non-CI upgrade still requires a Linux +staging host with real systemd. + ## Prerequisites - A running PostgreSQL instance diff --git a/nix/ext/tests/pg-upgrade-start-guard.nix b/nix/ext/tests/pg-upgrade-start-guard.nix new file mode 100644 index 0000000000..e2b9154b8f --- /dev/null +++ b/nix/ext/tests/pg-upgrade-start-guard.nix @@ -0,0 +1,207 @@ +{ self, pkgs }: +let + inherit (pkgs) lib; + pg15 = self.packages.${pkgs.pkgsLinux.stdenv.hostPlatform.system}."psql_15/bin"; + pg17 = self.packages.${pkgs.pkgsLinux.stdenv.hostPlatform.system}."psql_17/bin"; + + dataOld = "/var/lib/postgresql/data"; + dataNew = "/var/lib/postgresql/data-17"; + + guard = builtins.path { + path = ../../../ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh; + name = "start_guard.sh"; + }; + + drive = pkgs.pkgsLinux.writeShellApplication { + name = "drive"; + runtimeInputs = with pkgs.pkgsLinux; [ + bash + coreutils + diffutils + procps + systemd + util-linux + ]; + checkPhase = ""; + text = '' + IS_CI="" + PGDATAOLD=${dataOld} + PGDATANEW=${dataNew} + log() { echo "[$(date +%T)] $*"; } + . ${guard} + + hold_block() { + systemctl stop postgresql + block_postgres_start + exec sleep infinity + } + + hold_lock() { + PGDATANEW=/tmp/fake-new + mkdir -p "$PGDATANEW" + runuser -u postgres -- bash -c 'sleep infinity; true' fake-pg-upgrade "--old-datadir=$PGDATAOLD" & + start_pid_lock_watcher + touch "$PGDATANEW/postmaster.pid" + wait + } + + salt_sim() { + : >/tmp/salt-states + while :; do + systemctl start postgresql 2>/dev/null || true + systemctl is-active postgresql >>/tmp/salt-states || true + sleep 0.1 + done + } + + upgrade() { + systemctl stop postgresql + block_postgres_start + runuser -u postgres -- ${pg17}/bin/initdb --no-locale -E UTF8 -U postgres -D "$PGDATANEW" + start_pid_lock_watcher + systemd-run --unit=salt-sim --collect "$0" salt_sim + cd /var/lib/postgresql + runuser -u postgres -- ${pg17}/bin/pg_upgrade \ + --username=postgres \ + --old-datadir="$PGDATAOLD" --new-datadir="$PGDATANEW" \ + --old-bindir=${pg15}/bin --new-bindir=${pg17}/bin + systemctl stop salt-sim + assert_source_stayed_down + unblock_postgres_start + } + + "$@" + ''; + }; + + initOld = pkgs.pkgsLinux.writeShellApplication { + name = "init-old"; + runtimeInputs = with pkgs.pkgsLinux; [ + coreutils + util-linux + ]; + text = '' + if [ ! -f ${dataOld}/PG_VERSION ]; then + install -d -o postgres -g postgres -m 0700 ${dataOld} + runuser -u postgres -- ${pg15}/bin/initdb --no-locale -E UTF8 -U postgres -D ${dataOld} + fi + ''; + }; +in +pkgs.testers.runNixOSTest { + name = "pg-upgrade-start-guard"; + + nodes.server = { + virtualisation = { + memorySize = 2048; + diskSize = 4096; + }; + + users = { + users.postgres = { + isSystemUser = true; + group = "postgres"; + home = "/var/lib/postgresql"; + createHome = true; + }; + groups.postgres = { }; + }; + + systemd.tmpfiles.rules = [ "d /run/postgresql 0755 postgres postgres -" ]; + + systemd.services.postgresql = { + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + Type = "notify"; + User = "postgres"; + Group = "postgres"; + ExecStartPre = "+${lib.getExe initOld}"; + ExecStart = "${pg15}/bin/postgres -D ${dataOld}"; + KillMode = "mixed"; + KillSignal = "SIGINT"; + Restart = "always"; + RestartSec = 1; + }; + }; + }; + + testScript = '' + DRIVE = "${lib.getExe drive}" + PG15 = "${pg15}/bin" + PG17 = "${pg17}/bin" + OLD = "${dataOld}" + NEW = "${dataNew}" + DROPIN = "/run/systemd/system/postgresql.service.d/pg-upgrade-lock.conf" + + def unit(name, job): + server.succeed(f"systemd-run --unit={name} --collect {DRIVE} {job}") + + def kill(name): + server.succeed(f"systemctl kill -s KILL {name}") + server.wait_until_fails(f"systemctl is-active --quiet {name}") + + def pg_ctl(bindir, action, data, opts=""): + log = "-l /tmp/pg_ctl.log" if action == "start" else "" + return f"runuser -u postgres -- {bindir}/pg_ctl {action} -D {data} -w -s {log} {opts}" + + def sql(query, bindir=PG15, port=5432): + return server.succeed( + f"{bindir}/psql -h 127.0.0.1 -p {port} -U postgres -d postgres -Atc \"{query}\"" + ).strip() + + def show(prop): + return server.succeed(f"systemctl show -p {prop} postgresql") + + start_all() + server.wait_for_unit("postgresql.service") + + with subtest("Seed the source cluster"): + sql("create table seed as select g as id, repeat(md5(g::text), 20) as pad from generate_series(1, 100000) g") + expected = sql("select count(*) || ',' || sum(id) from seed") + + with subtest("Control: with no guard the start loop brings the source up"): + server.succeed("systemctl stop postgresql") + unit("salt-sim", "salt_sim") + server.wait_until_succeeds("systemctl is-active --quiet postgresql", timeout=60) + server.succeed("systemctl stop salt-sim") + + with subtest("A systemd start is refused while the block is held"): + unit("hold-block", "hold_block") + server.wait_until_succeeds("systemctl show -p ExecCondition postgresql | grep -q start-guard", timeout=120) + server.fail("systemctl start postgresql") + server.sleep(5) + server.fail("systemctl is-active --quiet postgresql") + assert "Restart=no" in show("Restart") + + with subtest("The block expires on its own when the holder is SIGKILLed"): + kill("hold-block") + server.succeed("systemctl start postgresql") + server.wait_for_unit("postgresql.service") + server.fail(f"test -e {DROPIN}") + assert "Restart=always" in show("Restart") + assert "NeedDaemonReload=no" in show("NeedDaemonReload") + + with subtest("The pid lock stops a direct start once the new cluster is up"): + server.succeed("systemctl stop postgresql") + unit("hold-lock", "hold_lock") + server.wait_until_succeeds(f"test -s {OLD}/postmaster.pid", timeout=120) + server.fail(pg_ctl(PG15, "start", OLD, "-t 5")) + kill("hold-lock") + server.succeed(pg_ctl(PG15, "start", OLD)) + server.succeed(pg_ctl(PG15, "stop", OLD)) + server.succeed("rm -f /run/pg-upgrade-source.pid") + server.succeed("systemctl start postgresql") + server.wait_for_unit("postgresql.service") + + with subtest("A real pg_upgrade copy under a start storm"): + server.succeed(f"systemd-run --unit=fake-initiate --collect --pipe --wait {DRIVE} upgrade") + attempts = int(server.succeed("wc -l = 5, f"start loop barely ran: {attempts} attempts" + server.fail("grep -qx active /tmp/salt-states") + + with subtest("The new cluster matches the seed"): + server.succeed(pg_ctl(PG17, "start", NEW, "-o '-p 5433'")) + assert sql("select count(*) || ',' || sum(id) from seed", PG17, 5433) == expected + server.succeed(pg_ctl(PG17, "stop", NEW)) + ''; +} diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh new file mode 100644 index 0000000000..67fa160c7e --- /dev/null +++ b/tests/pg_upgrade/test-start-guard.sh @@ -0,0 +1,422 @@ +#!/usr/bin/env bash + +set -eEuo pipefail + +REPO_ROOT=$(cd "$(dirname "$0")/../.." && pwd) +TEST_ROOT=$(mktemp -d) +trap 'rm -rf "$TEST_ROOT"' EXIT + +SCRIPTS_DIR="$REPO_ROOT/ansible/files/admin_api_scripts/pg_upgrade_scripts" + +# cleanup and on_exit are part of initiate.sh itself, which cannot be sourced. +eval "$(awk -v status_file="$TEST_ROOT/pg-upgrade-status" ' + /^(cleanup|on_exit)\(\) \{/ { printing = 1 } + printing { gsub("/tmp/pg-upgrade-status", status_file); print } + printing && /^}/ { printing = 0 } +' "$SCRIPTS_DIR/initiate.sh")" + +mkdir "$TEST_ROOT/bin" +cat >"$TEST_ROOT/bin/systemctl" <<'SHIM' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$SYSTEMCTL_LOG" +case "$1" in +daemon-reload) + if [ -f "$FAIL_RELOAD_ONCE" ]; then + rm "$FAIL_RELOAD_ONCE" + exit 1 + fi + ;; +is-active) + printf '%s\n' "${SERVICE_STATE:-inactive}" + ;; +esac +SHIM +cat >"$TEST_ROOT/bin/pgrep" <<'SHIM' +#!/usr/bin/env bash +if [ -n "${PGREP_EMPTY:-}" ]; then + exit 1 +fi +printf '12345\n' +SHIM +cat >"$TEST_ROOT/bin/chown" <<'SHIM' +#!/usr/bin/env bash +exit 0 +SHIM +cat >"$TEST_ROOT/bin/rmdir" <"$TEST_ROOT/bin/rm" <>"\$SYSTEMCTL_LOG" + exit 1 +fi +exec $(type -P rm) "\$@" +SHIM +cat >"$TEST_ROOT/bin/mv" <>"\$SYSTEMCTL_LOG" + exit 1 +fi +exec $(type -P mv) "\$@" +SHIM +# The Nix build sandbox on Linux has no /usr/bin/env: point the shims at this bash. +for shim in "$TEST_ROOT/bin/"*; do + { + printf '#!%s\n' "$BASH" + tail -n +2 "$shim" + } >"$shim.tmp" + mv "$shim.tmp" "$shim" +done +chmod +x "$TEST_ROOT/bin/"* +export PATH="$TEST_ROOT/bin:$PATH" + +log() { printf '%s\n' "$*"; } +retry() { + printf 'cleanup-abort\n' >>"$SYSTEMCTL_LOG" + exit 1 +} + +# shellcheck disable=SC1091 +source "$SCRIPTS_DIR/start_guard.sh" + +setup() { + local test_dir + test_dir=$(mktemp -d "$TEST_ROOT/case.XXXXXX") + unset IS_CI + export IS_CI=${IS_CI:-} + IS_LOCAL_UPGRADE=true + PGVERSION="guard-test-$$" + PGDATAOLD="$test_dir/old" + PGDATANEW="$test_dir/new" + MOUNT_POINT="$test_dir/mount" + PG_UPGRADE_LOCK_DROPIN_DIR="$test_dir/postgresql.service.d" + PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" + PG_UPGRADE_PID_LOCK_MARKER="$test_dir/pg-upgrade-source.pid" + PG_UPGRADE_INITIATE_PID_FILE="$test_dir/pg-upgrade-initiate.pid" + PG_UPGRADE_START_GUARD="$test_dir/pg-upgrade-start-guard.sh" + PG_UPGRADE_LOCK_WATCHER_PID="" + PG_UPGRADE_START_BLOCKED="" + CLEANUP_STARTED="" + POSTGRES_SHARE_DIR="$test_dir/share" + POST_UPGRADE_EXTENSION_SCRIPT="$test_dir/extensions.sql" + POST_UPGRADE_POSTGRES_PERMS_SCRIPT="$test_dir/perms.sql" + LOG_FILE="$test_dir/upgrade.log" + export IS_LOCAL_UPGRADE PGVERSION PGDATAOLD PGDATANEW MOUNT_POINT LOG_FILE + export POSTGRES_SHARE_DIR POST_UPGRADE_EXTENSION_SCRIPT POST_UPGRADE_POSTGRES_PERMS_SCRIPT + export PG_UPGRADE_LOCK_DROPIN_DIR PG_UPGRADE_LOCK_DROPIN PG_UPGRADE_PID_LOCK_MARKER + export PG_UPGRADE_INITIATE_PID_FILE PG_UPGRADE_START_GUARD PG_UPGRADE_PID_LOCK_TMP_NAME + export PG_UPGRADE_LOCK_WATCHER_PID PG_UPGRADE_START_BLOCKED CLEANUP_STARTED + export SYSTEMCTL_LOG="$test_dir/systemctl.log" + export FAIL_RELOAD_ONCE="$test_dir/fail-reload" + export FAIL_SHARE_RESTORE="" + export SERVICE_STATE=inactive + mkdir "$PGDATAOLD" "$PGDATANEW" + : >"$SYSTEMCTL_LOG" +} + +assert() { + if ! "$@"; then + printf 'FAIL: %s\n' "$*" >&2 + exit 1 + fi +} + +test_dropin() { + setup + block_postgres_start + assert grep -qx "ExecCondition=+$(type -P sh) $PG_UPGRADE_START_GUARD $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecStartPost=-+$TEST_ROOT/bin/rm -f $PG_UPGRADE_LOCK_DROPIN $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER $PG_UPGRADE_START_GUARD" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecStartPost=-+$TEST_ROOT/bin/systemctl daemon-reload" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx 'Restart=no' "$PG_UPGRADE_LOCK_DROPIN" + assert test -s "$PG_UPGRADE_INITIATE_PID_FILE" + assert test -s "$PG_UPGRADE_START_GUARD" + unblock_postgres_start + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$PG_UPGRADE_INITIATE_PID_FILE" + assert test ! -e "$PG_UPGRADE_START_GUARD" + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + assert grep -qx 'reset-failed postgresql' "$SYSTEMCTL_LOG" + unblock_postgres_start + assert test "$(grep -c '^reset-failed postgresql$' "$SYSTEMCTL_LOG")" -eq 1 +} + +test_no_block_reset() { + setup + unblock_postgres_start + if grep -q '^reset-failed postgresql$' "$SYSTEMCTL_LOG"; then + exit 1 + fi +} + +test_block_reload_failure() { + setup + touch "$FAIL_RELOAD_ONCE" + if block_postgres_start; then + exit 1 + fi + assert test -f "$PG_UPGRADE_LOCK_DROPIN" + unblock_postgres_start + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" +} + +test_unblock_reload_retry() { + setup + block_postgres_start + touch "$FAIL_RELOAD_ONCE" + if unblock_postgres_start; then + exit 1 + fi + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + unblock_postgres_start + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 3 +} + +# Not `wait`: it reports a child's status once, and assert_source_stayed_down +# needs it. +wait_for_watcher() { + local tries=0 + while kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; do + tries=$((tries + 1)) + assert test "$tries" -lt 50 + sleep 0.1 + done +} + +test_owned_pid() { + setup + touch "$PGDATANEW/postmaster.pid" + start_pid_lock_watcher + wait_for_watcher + assert test ! -e "$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + assert cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid" + assert_source_stayed_down + unblock_postgres_start + assert test ! -e "$PGDATAOLD/postmaster.pid" + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" +} + +test_foreign_pid() { + local content + for content in '' '54321' $'54321\n/data'; do + setup + printf '12345\n' >"$PG_UPGRADE_PID_LOCK_MARKER" + printf '%s' "$content" >"$PGDATAOLD/postmaster.pid" + if assert_source_stayed_down; then + exit 1 + fi + unblock_postgres_start + assert test -f "$PGDATAOLD/postmaster.pid" + assert test "$(cat "$PGDATAOLD/postmaster.pid")" = "$content" + done + setup + printf '12345\n' >"$PGDATAOLD/postmaster.pid" + if assert_source_stayed_down; then + exit 1 + fi + unblock_postgres_start + assert test -f "$PGDATAOLD/postmaster.pid" +} + +test_watcher_existing_pid() { + setup + touch "$PGDATANEW/postmaster.pid" + printf '54321\n' >"$PGDATAOLD/postmaster.pid" + start_pid_lock_watcher + wait_for_watcher + # the watcher could not take the lock: the upgrade must fail + if assert_source_stayed_down; then + exit 1 + fi + unblock_postgres_start + assert test "$(cat "$PGDATAOLD/postmaster.pid")" = 54321 +} + +# pg_upgrade is gone by the time the new cluster's pid file shows up +test_watcher_no_pg_upgrade() { + setup + export PGREP_EMPTY=1 + touch "$PGDATANEW/postmaster.pid" + # initiate_pid is this shell, so the watcher does not abandon the wait + start_pid_lock_watcher + wait_for_watcher + unset PGREP_EMPTY + if assert_source_stayed_down; then + exit 1 + fi + assert test ! -e "$PGDATAOLD/postmaster.pid" +} + +# a watcher still polling is not a failure +test_watcher_still_polling() { + setup + start_pid_lock_watcher + assert_source_stayed_down + unblock_postgres_start + assert test -z "$PG_UPGRADE_LOCK_WATCHER_PID" +} + +# the watcher killed between writing and linking leaves no pid file behind +test_unblock_removes_lock_tmp() { + setup + printf '1\n' >"$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + unblock_postgres_start + assert test ! -e "$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + assert test ! -e "$PGDATAOLD/postmaster.pid" +} + +test_watcher_abandoned() { + local watcher_pid_file tries=0 + setup + export PGREP_EMPTY=1 + watcher_pid_file="$PGDATAOLD/../watcher.pid" + # a separate shell stands in for a SIGKILLed initiate.sh: it exits, the watcher + # stays (BASHPID is missing on bash 3.2, so a subshell would not work here) + # shellcheck disable=SC2016 + "$BASH" -c 'log() { :; }; source "$1"; start_pid_lock_watcher; printf "%s\n" "$PG_UPGRADE_LOCK_WATCHER_PID" >"$2"' _ \ + "$SCRIPTS_DIR/start_guard.sh" "$watcher_pid_file" + while kill -0 "$(cat "$watcher_pid_file")" 2>/dev/null; do + tries=$((tries + 1)) + assert test "$tries" -lt 50 + sleep 0.2 + done + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" + unset PGREP_EMPTY +} + +test_active_source() { + local state + for state in active activating reloading; do + setup + export SERVICE_STATE="$state" + if assert_source_stayed_down; then + exit 1 + fi + done +} + +test_start_guard() { + local live dead + setup + block_postgres_start + sleep 60 & + live=$! + (exit 0) & + dead=$! + wait "$dead" + guard_exit() { + local exit_code=0 + sh -c "$(sed -n 's/^ExecCondition=+//p' "$PG_UPGRADE_LOCK_DROPIN")" || exit_code=$? + printf '%s\n' "$exit_code" + } + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$dead" >"$PG_UPGRADE_INITIATE_PID_FILE" + assert test "$(guard_exit)" -eq 0 + printf '%s\n' "$live" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$live" >"$PG_UPGRADE_INITIATE_PID_FILE" + printf '%s\n' "$dead" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$dead" >"$PG_UPGRADE_INITIATE_PID_FILE" + printf '%s\n' "$dead" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + printf 'not-a-pid\n' >"$PG_UPGRADE_INITIATE_PID_FILE" + : >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + rm -f "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + kill "$live" + wait "$live" 2>/dev/null || true +} + +test_dropin_expiry() { + local exec_start_post + setup + block_postgres_start + printf '%s\n' 12345 >"$PG_UPGRADE_PID_LOCK_MARKER" + exec_start_post=$(grep -F "ExecStartPost=-+$TEST_ROOT/bin/rm " "$PG_UPGRADE_LOCK_DROPIN") + sh -c "${exec_start_post#ExecStartPost=-+}" + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$PG_UPGRADE_INITIATE_PID_FILE" + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" + assert test ! -e "$PG_UPGRADE_START_GUARD" + unblock_postgres_start +} + +test_exit_cleanup() { + local trigger exit_code expected_code + for trigger in failure TERM INT clean-exit cleanup-exit already-cleaned restore-rm restore-mv; do + setup + mkdir "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR.new" + ln -s "$POSTGRES_SHARE_DIR.new" "$POSTGRES_SHARE_DIR" + exit_code=0 + expected_code=1 + case "$trigger" in + TERM | cleanup-exit) expected_code=143 ;; + INT) expected_code=130 ;; + already-cleaned) expected_code=0 ;; + restore-rm) export FAIL_SHARE_RESTORE=rm ;; + restore-mv) export FAIL_SHARE_RESTORE=mv ;; + esac + "$BASH" -s "$trigger" <<-'TRAP_TEST' || exit_code=$? + set -eEuo pipefail + if [ "$1" = cleanup-exit ]; then + cleanup() { + printf "cleanup-abort\n" >>"$SYSTEMCTL_LOG" + exit 1 + } + fi + trap on_exit EXIT + trap "cleanup failed" ERR + trap "exit 143" TERM + trap "exit 130" INT + block_postgres_start + touch "$FAIL_RELOAD_ONCE" + case "$1" in + TERM | INT) kill -s "$1" "$$" ;; + clean-exit) exit 0 ;; + cleanup-exit) rm "$FAIL_RELOAD_ONCE"; exit 143 ;; + already-cleaned) rm "$FAIL_RELOAD_ONCE"; CLEANUP_STARTED=1; exit 0 ;; + *) false ;; + esac + TRAP_TEST + assert test "$exit_code" -eq "$expected_code" + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$FAIL_RELOAD_ONCE" + assert test "$(tail -n 2 "$SYSTEMCTL_LOG")" = $'daemon-reload\nreset-failed postgresql' + case "$trigger" in + already-cleaned) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + if grep -q '^cleanup-abort$' "$SYSTEMCTL_LOG"; then exit 1; fi + ;; + cleanup-exit) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + assert test "$(grep -c '^cleanup-abort$' "$SYSTEMCTL_LOG")" -eq 1 + ;; + *) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 3 + assert test "$(grep -c '^cleanup-abort$' "$SYSTEMCTL_LOG")" -eq 1 + case "$trigger" in + restore-rm | restore-mv) assert grep -qx "$trigger" "$SYSTEMCTL_LOG" ;; + *) assert test ! -L "$POSTGRES_SHARE_DIR" ;; + esac + ;; + esac + done +} + +export -f log retry assert block_postgres_start unblock_postgres_start assert_source_stayed_down cleanup on_exit +for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_watcher_no_pg_upgrade test_watcher_still_polling test_unblock_removes_lock_tmp test_watcher_abandoned test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do + case_output="$TEST_ROOT/$test_name.log" + trap 'exit_code=$?; cat "$case_output"; exit "$exit_code"' ERR + ( + trap - ERR + "$test_name" + ) >"$case_output" 2>&1 + printf 'PASS: %s\n' "$test_name" +done