From e5fa905ee2de4da79f57a5a80309e9ac5dc3b3a4 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:22:43 -0500 Subject: [PATCH] ci: verify the reboot, split machinery failures from test failures Acts on pre-push review findings from codex + correctness/security subagents. --- .github/workflows/package-currency.yml | 44 +++++-- .../scripts/prepare-test-environment.sh | 46 ++++++-- .../scripts/prepare-test-environment.test.sh | 111 +++++++++++++++--- .../infrastructure/scripts/run-integration.sh | 12 +- 4 files changed, 179 insertions(+), 34 deletions(-) diff --git a/.github/workflows/package-currency.yml b/.github/workflows/package-currency.yml index bacef8f..af5beea 100644 --- a/.github/workflows/package-currency.yml +++ b/.github/workflows/package-currency.yml @@ -34,9 +34,12 @@ on: type: boolean default: false +# packages: write is granted per-job to container-image, the only job that +# pushes. The self-hosted jobs get read-only tokens: they SSH into freshly +# upgraded nodes and should not be able to overwrite published GHCR images. permissions: contents: read - packages: write + packages: read env: SCRIPTS_DIR: tests/infrastructure/scripts @@ -74,6 +77,9 @@ jobs: container-image: runs-on: ubuntu-latest timeout-minutes: 20 + permissions: + contents: read + packages: write steps: - uses: actions/checkout@v7 @@ -107,7 +113,12 @@ jobs: - /opt/pve-integration:/opt/pve-integration steps: + # persist-credentials: false on the self-hosted jobs — nothing downstream + # uses git, and the token would otherwise be written into .git/config on + # a runner that outlives the job's container. - uses: actions/checkout@v7 + with: + persist-credentials: false - name: Provision PVE instances and dist-upgrade shell: bash @@ -156,6 +167,8 @@ jobs: steps: - uses: actions/checkout@v7 + with: + persist-credentials: false - name: Download module artifact uses: actions/download-artifact@v8 @@ -163,24 +176,33 @@ jobs: name: module-currency path: ./publish/netstandard2.0/ - # continue-on-error is the report-only decision made concrete: a module - # broken against current PVE must not turn this cron red. + # Report-only applies to TEST failures, not to the lane's own machinery. + # cmd_test returns 3 for genuine Pester failures and 4 when it cannot + # reach or authenticate to a node. A blanket continue-on-error would + # swallow 4 as well — and an unreachable node is exactly the symptom of a + # reboot gone wrong, so the lane would go green having learned nothing. - name: Run integration tests (PVE ${{ matrix.pve_version }}) id: suite - continue-on-error: true shell: bash env: PVE_PASSWORD: ${{ secrets.PVE_TEST_PASSWORD }} MODULE_ARTIFACT: ./publish/netstandard2.0 - run: bash ${SCRIPTS_DIR}/run-integration.sh test ${{ matrix.pve_version }} - - - name: Record suite outcome - shell: bash run: | - echo "Suite outcome against current PVE: ${{ steps.suite.outcome }}" + set +e + bash ${SCRIPTS_DIR}/run-integration.sh test ${{ matrix.pve_version }} + rc=$? + echo "suite_rc=${rc}" >> "$GITHUB_OUTPUT" + if [ "${rc}" -eq 0 ]; then + echo "Suite passed against current PVE." + elif [ "${rc}" -eq 3 ]; then + echo "::warning title=Suite failed against current PVE::Report-only - tests failed (rc=3). See the uploaded results." + else + echo "::error title=Currency lane machinery failed::run-integration.sh exited ${rc}, which is not a test failure. The lane learned nothing about package currency." + fi + [ "${rc}" -eq 0 ] || [ "${rc}" -eq 3 ] - name: Diagnose cluster state - if: steps.suite.outcome == 'failure' + if: always() && steps.suite.outputs.suite_rc != '0' shell: bash env: PVE_PASSWORD: ${{ secrets.PVE_TEST_PASSWORD }} @@ -209,6 +231,8 @@ jobs: steps: - uses: actions/checkout@v7 + with: + persist-credentials: false - name: Cleanup PVE instances shell: bash diff --git a/tests/infrastructure/scripts/prepare-test-environment.sh b/tests/infrastructure/scripts/prepare-test-environment.sh index a1b8012..b19047c 100755 --- a/tests/infrastructure/scripts/prepare-test-environment.sh +++ b/tests/infrastructure/scripts/prepare-test-environment.sh @@ -35,25 +35,57 @@ if [[ "${DIST_UPGRADE}" == "1" ]]; then if [[ -n "${PKG_OUT}" ]]; then echo "Recording package set to ${PKG_OUT}..." - ${SSH_CMD} "dpkg-query -W -f='\${binary:Package}\t\${Version}\n' | sort" > "${PKG_OUT}" + # pipefail must be set in the REMOTE shell. ssh returns the remote + # pipeline's status, which is sort's — and sort succeeds on the empty + # input a failed dpkg-query produces, so a broken query would otherwise + # leave a zero-byte file and still exit 0. + ${SSH_CMD} "set -o pipefail; dpkg-query -W -f='\${binary:Package}\t\${Version}\n' | sort" > "${PKG_OUT}" + if [[ ! -s "${PKG_OUT}" ]]; then + echo "ERROR: empty package set from ${NESTED_IP}" >&2 + exit 1 + fi fi # A PVE dist-upgrade pulls proxmox-kernel-*; without a reboot the node runs # new userspace on the old kernel. Reboot unconditionally rather than # testing /var/run/reboot-required — that file comes from # update-notifier-common, which is not guaranteed on a PVE node. + # + # boot_id is the evidence that the reboot happened. Without it the `|| true` + # below swallows every ssh failure, the node stays up, and wait-for-api.sh + # matches the still-running pre-reboot pveproxy on its first poll. + boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")" + echo "Rebooting after dist-upgrade..." ${SSH_CMD} "systemctl reboot" || true - # The API stays up for a few seconds after the reboot is issued, so polling - # immediately would match the pre-reboot node and return at once. - sleep 30 + # Order matters: prove the reboot first (ssh returns before pveproxy does), + # then wait for the API, then for pmxcfs. + boot_after="" + for _ in $(seq 1 60); do + boot_after="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id" 2>/dev/null || true)" + [[ -n "${boot_after}" && "${boot_after}" != "${boot_before}" ]] && break + sleep 5 + done + if [[ -z "${boot_after}" || "${boot_after}" == "${boot_before}" ]]; then + echo "ERROR: ${NESTED_IP} did not reboot (boot_id unchanged)" >&2 + exit 1 + fi + bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600 - # The running kernel is the point of the reboot: the package set alone - # cannot show whether the node actually booted what it installed. + # wait-for-api.sh only proves pveproxy answers. `pvesm set` below writes + # /etc/pve/storage.cfg, which needs pmxcfs to have mounted /etc/pve — on a + # freshly rebooted node those are seconds apart. + for _ in $(seq 1 30); do + ${SSH_CMD} "test -f /etc/pve/storage.cfg" 2>/dev/null && break + sleep 5 + done + + # printf, not echo: bash's builtin echo does not interpret \t without -e, + # which would make this the one row in the file without a real tab. if [[ -n "${PKG_OUT}" ]]; then - ${SSH_CMD} "echo \"# running-kernel\t\$(uname -r)\"" >> "${PKG_OUT}" + ${SSH_CMD} "printf '# running-kernel\t%s\n' \"\$(uname -r)\"" >> "${PKG_OUT}" fi fi diff --git a/tests/infrastructure/scripts/prepare-test-environment.test.sh b/tests/infrastructure/scripts/prepare-test-environment.test.sh index 7e44d31..d34ec3d 100755 --- a/tests/infrastructure/scripts/prepare-test-environment.test.sh +++ b/tests/infrastructure/scripts/prepare-test-environment.test.sh @@ -1,9 +1,12 @@ #!/usr/bin/env bash # Self-check for prepare-test-environment.sh's opt-in dist-upgrade branch. # -# Stubs sshpass/curl/sleep on PATH so both paths run offline in ~0s, then +# Stubs sshpass/curl/sleep on PATH so every path runs offline in ~0s, then # asserts on the commands the script actually issued. # +# The stub is deliberately stateful: boot_id must differ across the reboot, and +# case 3 pins the failure by returning the SAME boot_id twice. +# # Run: bash tests/infrastructure/scripts/prepare-test-environment.test.sh set -euo pipefail @@ -16,19 +19,45 @@ trap 'rm -rf "$TMP"' EXIT mkdir -p "$TMP/bin" -# Fake sshpass: log every invocation, emit a plausible dpkg-query result. +# Fake sshpass. Logs every invocation; emits plausible output per command. +# BOOT_ID_STUCK=1 makes it return an unchanging boot_id, simulating a node that +# never rebooted. cat > "$TMP/bin/sshpass" <<'STUB' #!/usr/bin/env bash echo "$*" >> "$STUB_LOG" case "$*" in - *dpkg-query*) echo -e "proxmox-kernel-6.14\t6.14.11-1\npve-manager\t9.2.1" ;; + *boot_id*) + if [[ "${BOOT_ID_STUCK:-0}" == "1" ]]; then + echo "11111111-1111-1111-1111-111111111111" + else + n=0 + [[ -f "$STUB_STATE/boot_calls" ]] && n=$(cat "$STUB_STATE/boot_calls") + n=$((n + 1)) + echo "$n" > "$STUB_STATE/boot_calls" + if [[ "$n" -le 1 ]]; then + echo "11111111-1111-1111-1111-111111111111" + else + echo "22222222-2222-2222-2222-222222222222" + fi + fi + ;; + *dpkg-query*) + # DPKG_EMPTY=1 simulates a failed query whose output is swallowed by + # the remote `| sort`, which succeeds on empty input. + [[ "${DPKG_EMPTY:-0}" == "1" ]] || printf 'proxmox-kernel-6.14\t6.14.11-1\npve-manager\t9.2.1\n' + ;; + *uname*) + printf '# running-kernel\t6.14.11-1-pve\n' + ;; esac exit 0 STUB -# wait-for-api.sh greps curl output for "version"; sleep must not really sleep. +# wait-for-api.sh greps curl output for "version"; log the call so the test can +# assert the wait actually ran. sleep must not really sleep. cat > "$TMP/bin/curl" <<'STUB' #!/usr/bin/env bash +echo "curl $*" >> "$STUB_LOG" echo '{"data":{"version":"9.2.1"}}' STUB cat > "$TMP/bin/sleep" <<'STUB' @@ -38,11 +67,13 @@ STUB chmod +x "$TMP/bin/"* export PATH="$TMP/bin:$PATH" +export STUB_STATE="$TMP" fail=0 check() { local desc="$1" haystack="$2" needle="$3" want="$4" - if grep -q -- "$needle" "$haystack"; then found=yes; else found=no; fi + local found=no + grep -q -- "$needle" "$haystack" && found=yes if [[ "$found" == "$want" ]]; then echo " ok: $desc" else @@ -50,41 +81,91 @@ check() { fail=1 fi } +pass() { echo " ok: $1"; } +fatal() { echo " FAIL: $1"; fail=1; } echo "case 1: no dist-upgrade argument — lane 1 path must be untouched" export STUB_LOG="$TMP/log1" -: > "$STUB_LOG" +: > "$STUB_LOG"; rm -f "$TMP/boot_calls" bash "$TARGET" 10.0.0.1 secret > "$TMP/out1" 2>&1 check "no dist-upgrade issued" "$STUB_LOG" "dist-upgrade" no check "no reboot issued" "$STUB_LOG" "systemctl reboot" no check "no package set recorded" "$STUB_LOG" "dpkg-query" no +check "no boot_id probe" "$STUB_LOG" "boot_id" no check "storage still configured" "$STUB_LOG" "pvesm set local" yes echo "case 2: dist-upgrade requested" export STUB_LOG="$TMP/log2" -: > "$STUB_LOG" +: > "$STUB_LOG"; rm -f "$TMP/boot_calls" bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages.txt" > "$TMP/out2" 2>&1 check "dist-upgrade issued" "$STUB_LOG" "dist-upgrade" yes check "reboot issued" "$STUB_LOG" "systemctl reboot" yes check "package set recorded" "$STUB_LOG" "dpkg-query" yes +check "boot_id checked" "$STUB_LOG" "boot_id" yes +check "waited for the API" "$STUB_LOG" "api2/json/version" yes +check "waited for pmxcfs" "$STUB_LOG" "/etc/pve/storage.cfg" yes check "storage still configured" "$STUB_LOG" "pvesm set local" yes +# The stub replaces the remote shell, so it cannot observe what that shell does +# with a command — only which command was sent. These two assert at that level, +# because both defects live in the command string itself: +# - without `set -o pipefail`, a failed remote dpkg-query is masked by `sort`, +# which succeeds on empty input and makes ssh return 0. +# - bash's builtin `echo` does not interpret \t without -e, so `echo` here +# would write the one row in the file lacking a real tab. +check "query sets remote pipefail" "$STUB_LOG" "set -o pipefail" yes +check "kernel capture uses printf" "$STUB_LOG" "printf '# running-kernel" yes + # The reboot must be issued after the upgrade, or the node records a package # set it never booted. upgrade_line=$(grep -n "dist-upgrade" "$STUB_LOG" | head -1 | cut -d: -f1) reboot_line=$(grep -n "systemctl reboot" "$STUB_LOG" | head -1 | cut -d: -f1) -if [[ "$reboot_line" -gt "$upgrade_line" ]]; then - echo " ok: reboot ordered after dist-upgrade" +api_line=$(grep -n "api2/json/version" "$STUB_LOG" | head -1 | cut -d: -f1) +[[ "$reboot_line" -gt "$upgrade_line" ]] \ + && pass "reboot ordered after dist-upgrade" \ + || fatal "reboot ordered before dist-upgrade" +[[ "$api_line" -gt "$reboot_line" ]] \ + && pass "API wait ordered after reboot" \ + || fatal "API wait ordered before reboot" + +# The running-kernel row must carry a REAL tab, like every dpkg-query row. +# `echo "...\t..."` in bash emits a literal backslash-t and would fail here. +if grep -q '^# running-kernel' "$TMP/packages.txt"; then + pass "running kernel recorded" + if grep -qP '^# running-kernel\t' "$TMP/packages.txt" 2>/dev/null \ + || awk -F'\t' '/^# running-kernel/ && NF == 2 {found=1} END {exit !found}' "$TMP/packages.txt"; then + pass "running-kernel row uses a real tab" + else + fatal "running-kernel row has a literal backslash-t, not a tab" + fi else - echo " FAIL: reboot ordered before dist-upgrade" - fail=1 + fatal "running kernel not recorded" fi -if [[ -s "$TMP/packages.txt" ]]; then - echo " ok: package file non-empty" +[[ -s "$TMP/packages.txt" ]] && pass "package file non-empty" || fatal "package file empty or missing" + +echo "case 3: node never rebooted — must be fatal" +export STUB_LOG="$TMP/log3" +: > "$STUB_LOG"; rm -f "$TMP/boot_calls" +if BOOT_ID_STUCK=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages3.txt" > "$TMP/out3" 2>&1; then + fatal "script exited 0 despite an unchanged boot_id" else - echo " FAIL: package file empty or missing" - fail=1 + pass "unchanged boot_id fails the run" + grep -q "did not reboot" "$TMP/out3" \ + && pass "failure names the cause" \ + || fatal "failure message does not mention the reboot" +fi + +echo "case 4: dpkg-query produced nothing — must be fatal" +export STUB_LOG="$TMP/log4" +: > "$STUB_LOG"; rm -f "$TMP/boot_calls" +if DPKG_EMPTY=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages4.txt" > "$TMP/out4" 2>&1; then + fatal "script exited 0 despite an empty package set" +else + pass "empty package set fails the run" + grep -q "empty package set" "$TMP/out4" \ + && pass "failure names the cause" \ + || fatal "failure message does not mention the package set" fi if [[ "$fail" -eq 0 ]]; then diff --git a/tests/infrastructure/scripts/run-integration.sh b/tests/infrastructure/scripts/run-integration.sh index 8b9594b..351cc9f 100644 --- a/tests/infrastructure/scripts/run-integration.sh +++ b/tests/infrastructure/scripts/run-integration.sh @@ -238,6 +238,11 @@ cmd_provision() { ci_mask "$PVE_PASSWORD" mkdir -p "$WORK_DIR" "$CACHE_DIR" + # WORK_DIR is a persistent shared mount. Stale package sets from an earlier + # run would otherwise be uploaded as if they were current — the artifact + # guard checks existence, not freshness. + rm -f "$WORK_DIR"/*-packages.txt + # Ensure base ISOs (one per version, not per node) for v in $provision_versions; do log "Ensuring base ISO for PVE $v..." @@ -523,7 +528,9 @@ cmd_test() { -d "username=root@pam&password=${PVETEST_PASSWORD}" \ "https://${PVETEST_HOST}:${PVETEST_PORT}/api2/json/access/ticket" | grep -q '"ticket"'; then ci_error "Cannot authenticate to PVE $v node A at ${PVETEST_HOST}:${PVETEST_PORT}" - overall_exit=3 + # 4, not 3: this is a machinery failure, not a test failure. The + # currency lane suppresses 3 (report-only) but must fail on 4. + overall_exit=4 continue fi @@ -534,7 +541,7 @@ cmd_test() { -d "username=root@pam&password=${PVETEST_PASSWORD}" \ "https://${PVETEST_HOST_B}:${PVETEST_PORT}/api2/json/access/ticket" | grep -q '"ticket"'; then ci_error "Cannot authenticate to PVE $v node B at ${PVETEST_HOST_B}:${PVETEST_PORT}" - overall_exit=3 + overall_exit=4 continue fi fi @@ -733,6 +740,7 @@ cmd_force_cleanup() { # Remove work artifacts, including locally cached auto-install ISOs rm -f "$CONFIG_FILE" "$WORK_DIR"/instances.tfvars.json "$TARGET_NODE_FILE" rm -f "$WORK_DIR"/*-auto-*.iso "$WORK_DIR"/*-http-auto.iso + rm -f "$WORK_DIR"/*-packages.txt log "Force cleanup complete. Next provision will start from scratch." }