diff --git a/DECISIONS.md b/DECISIONS.md index a11fb78..72aa810 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -590,3 +590,127 @@ var task = vmService.RebootVm(session, node, vmid, timeout); if (Wait.IsPresent) task = WaitForStatusTransition(session, node, task, vmid, "running", timeout); ``` + +--- + +## D017 — CI runs two lanes: a pinned gating lane and a report-only currency lane + +**Status**: Active +**Finding refs**: (none — arose from integration runs 176–180, root-caused 2026-09-01) +**Resolved in scan**: n/a + +### Decision +CI provisions nested PVE nodes in two distinct modes, and they must not be merged into one: + +- **Lane 1, `integration-tests.yml`** — nodes stay pinned to what the ISO ships. `first-boot.sh` + must never run `apt-get upgrade` or `dist-upgrade`. This lane gates merges. +- **Lane 2, `package-currency.yml`** — nodes are `dist-upgrade`d to current PVE and the suite runs + against them. **Report-only**: test failures do not fail the job. + +Both declare `concurrency: group: integration-tests`. They drive the same nested VMIDs on the same +parent node, so they must never run at once. + +### Rationale +`apt-get upgrade` holds back packages that need new dependencies. On the nested nodes that produced +`pve-cluster` 9.1.6 against `libpve-cluster-api-perl` 9.1.0 — a combination no real install ever +has — and its symptom was not a package error. The node's pmxcfs came back in local mode after a +cluster join, `/etc/pve/corosync.conf` never appeared, and the node reported `online=0` while +corosync itself had healthy 2-node membership. Three CI runs went into diagnosing that, and removing +the upgrade was the entire fix: run 180 was the first fully green integration run. + +So the pin is what makes lane 1 a trustworthy merge gate. But a permanently pinned CI never +exercises the module against a current PVE, and that gap is exactly where an upstream regression +would hide. Lane 2 closes it without putting the instability back into the gating path. + +Report-only is deliberate. A scheduled job that goes red on an upstream change nobody has chosen to +chase becomes noise, and a noisy cron gets ignored — which is the failure mode that makes a canary +worthless. The signal is the rolling issue and the recorded package set, not the check colour. + +**Consequence accepted**: a module genuinely broken against current PVE shows a green weekly check +plus an updated issue. Operator ruling 2026-09-01, to be revisited after a few releases. + +A failure of the lane's own machinery — provisioning, the upgrade, the reboot, an unreachable node — +still fails the job. `run-integration.sh` returns 3 for a genuine test failure and 4 when it cannot +reach or authenticate to a node; only 3 is suppressed. Suppressing both would let a botched reboot +report success while the lane learned nothing. + +### Anti-pattern (do not reintroduce) +```bash +# NEVER in first-boot.sh — this is the mismatch that left a node unclustered +apt-get update -qq +apt-get -y upgrade +``` + +### Correct pattern +```bash +# first-boot.sh installs only what provisioning needs; the ISO is the pin +apt-get update -qq +apt-get install -y -qq --no-install-recommends qemu-guest-agent open-iscsi +``` +```yaml +# package-currency.yml opts in explicitly; lane 1 never sets this +env: + PVE_DIST_UPGRADE: '1' +``` + +--- + +## D018 — The currency lane reboots after dist-upgrade, and proves it rebooted + +**Status**: Active +**Finding refs**: (none — found in pre-push review of PR #106, 2026-09-01) +**Resolved in scan**: n/a + +### Decision +After `dist-upgrade`, `prepare-test-environment.sh` reboots the node **unconditionally** and then +**verifies the reboot happened** by comparing `/proc/sys/kernel/random/boot_id` before and after. +An unchanged boot id is fatal. The reboot lives here, not in `first-boot.sh`. + +### Rationale +A PVE `dist-upgrade` pulls `proxmox-kernel-*`. Without a reboot the node runs new userspace on the +old kernel, so the lane records a package set it never actually ran and is blind to kernel +regressions — it would report "current PVE" while testing something that never booted. + +The reboot cannot live in `first-boot.sh`: that runs `ordering = "fully-up"` while the parent is +still polling, so `wait-for-pve.sh` can discover the IP, see the API, pass auth, and then have the +node reboot out from under provisioning. That presents as an intermittent network fault. + +It is unconditional rather than gated on `/var/run/reboot-required`, because that file comes from +`update-notifier-common`, which is not guaranteed present on a PVE node. + +Verification is the part that is easy to omit and was omitted in the first draft. `ssh … reboot` +returns non-zero when the connection dies, so it needs `|| true` — which swallows *every* ssh +failure, including the reboot never being issued. `wait-for-api.sh` then matches the +**still-running pre-reboot** pveproxy on its first poll and returns `responsive after 0s`. The +script exits 0 having proved nothing. A blind `sleep` before polling does not fix this; it is wrong +in both directions and verifies nothing either way. + +Order matters: prove the boot id changed first (ssh returns before pveproxy does), then wait for the +API, then wait for pmxcfs — `pvesm set` writes `/etc/pve/storage.cfg`, which needs `/etc/pve` +mounted, and on a fresh boot that lags the API by seconds. + +### Anti-pattern (do not reintroduce) +```bash +# NEVER — `|| true` hides a reboot that never happened, and the poll then +# matches the pre-reboot node and returns immediately +${SSH_CMD} "systemctl reboot" || true +sleep 30 +bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600 +``` + +### Correct pattern +```bash +boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")" +${SSH_CMD} "systemctl reboot" || true +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 +``` diff --git a/docs/lane2-change-plan.md b/docs/lane2-change-plan.md index a13deef..a3e4208 100644 --- a/docs/lane2-change-plan.md +++ b/docs/lane2-change-plan.md @@ -27,7 +27,7 @@ From the cluster/CI workstream tracker, and not reopened here: |---|---|---| | 1 | Lane home | New `.github/workflows/package-currency.yml` | | 2 | Scope | Full Pester suite; test failures reported, **not** fatal | -| 3 | Baseline store | Committed file, updated by an auto-merging PR, only when the package set actually changed | +| 3 | Baseline store | **Superseded during build** — see 3.3. An unprotected `ci/package-baseline` data branch, not a committed file in a PR | ### 3.1 Why a separate workflow @@ -39,22 +39,20 @@ Package drift that doesn't break anything is not interesting. The thing worth kn **Consequence to accept:** a genuinely broken module against current PVE is a green check with an updated issue. The issue is the signal, not the check colour. -### 3.3 Baseline: what works and what doesn't +### 3.3 Baseline: superseded during implementation -The operator asked whether the baseline could go through a PR that auto-merges and skips CI, committing only when versions differ. Three parts, and they don't all hold: +The original choice was a committed baseline file updated by an auto-merging PR. **That cannot work on this repo**, and the reason only surfaced in review: -**Commit only on change — yes.** The lane computes the set, diffs against the committed baseline, and does nothing when identical. On a pinned no-subscription repo most ticks are no-ops. +- A PR opened with `GITHUB_TOKEN` never triggers `pull_request` workflows — GitHub suppresses them to prevent recursion. `build.yml`, `unit-tests.yml` and `claude-code-review.yml` are all `pull_request`-triggered, so the baseline PR would report **zero required checks** and be permanently unmergeable under branch protection. Not "waits for a human to merge it" — cannot be merged. +- Auto-merge is enabled on the repo, but auto-merge waits for required checks that will never report, so it does not help. +- `pve_api` solves this by pushing straight to `main` as `github-actions[bot]` with `contents: write`. That works because its `main` is unprotected. This repo's is (required checks, required review, admin enforced), so the same action is rejected. +- Using a GitHub App installation token would fire the checks — but it puts output from a machine that just ran `dist-upgrade` against an upstream repo in front of the `claude-review` agent, which holds `pull-requests: write` and is instructed to end with `--approve`. That is a known [required-reviews bypass](https://medium.com/cider-sec/bypassing-required-reviews-using-github-actions-6e1b29135cc7) shape, and not worth opening for a generated data file. -**Auto-merge — yes.** `enablePullRequestAutoMerge` merges once required checks pass and required reviews are satisfied. PR #100 established that `claude[bot]`'s APPROVED alone takes `mergeable_state` to `clean` on this repo, with the operator's code-owner review still pending. So the chain closes with no human: App opens the baseline PR → `claude-review` approves → build + unit tests go green → auto-merge fires. "Allow auto-merge" is already enabled on this repo (operator confirmed 2026-09-01). +**Adopted instead: an unprotected data branch**, `ci/package-baseline`, holding exactly one file. This is the established pattern for generated data against a protected main — [github-archive-action](https://github.com/githubocto/github-archive-action) writes to an orphan branch for the same reasons. Branch protection covers `main` only, so `contents: write` plus `GITHUB_TOKEN` is sufficient: no PR, no checks, no protection conflict, and no path from node output to the review agent. -**Skip CI — no, and it's worth being precise about why.** Both mechanisms deadlock against branch protection: +It is written with git plumbing (`hash-object` → `mktree` → `commit-tree` → `push :refs/heads/…`) so the job's checkout is never touched and no local branch is created, which also makes a re-run unable to collide with itself. -- `paths-ignore` on `build.yml` / `unit-tests.yml` → the workflow never runs → the required check never reports → the PR is blocked forever, and auto-merge waits forever. -- `[skip ci]` in the commit message → GitHub skips the whole run → identical deadlock. - -The only way to skip the *work* while still satisfying protection is to keep the trigger and have each job short-circuit to an immediate `exit 0` on a baseline-only diff, so the check still reports success. That means editing `build.yml` and `unit-tests.yml` — two files with no other stake in this lane — to special-case it. - -**Recommendation: let CI run.** Both workflows trigger on every PR to `main` with no path filters today. On PR #100 the full set settled in roughly two minutes, and a baseline PR only exists on ticks where versions actually moved. Paying two minutes a handful of times a year is cheaper than a permanent special case in the two workflows that gate every merge. If the baseline turns out to churn weekly, add the short-circuit then. +Artifacts alone were considered — the operator's own suggestion — and rejected on the stated goal: artifacts expire (90 days by default), so a weekly cadence would retain roughly 13 data points and lose the history. `git log ci/package-baseline` keeps it indefinitely. ## 4. Change plan @@ -77,13 +75,18 @@ Add an opt-in third argument (default off), so lane 1's behaviour is byte-identi Schedule + `workflow_dispatch`; `concurrency: group: integration-tests`; provisions with `PVE_DIST_UPGRADE=1`; runs the full suite with `continue-on-error` on the test step; uploads the package set as an artifact. -### Commit 3 — reporting +### Commit 3 — reporting *(landed as #108, revised)* -Diff the recorded set against `tests/infrastructure/pve-package-baseline.txt`. On difference: update the rolling issue (find by label, create if absent) with the diff and the suite result, and open the baseline-bump PR with auto-merge enabled. On no difference: exit quietly. +Diff the reference node against the baseline on `ci/package-baseline`. On difference: update the data branch, then upsert the rolling issue (`pve-currency`). On no difference: exit quietly. + +Two additions beyond the plan: + +- **Node-vs-node comparison.** A package mismatch *between* the two nested nodes is the failure that left a node unclustered and cost three CI runs to diagnose (see D017). It is reported even when the set is otherwise unchanged. +- **Input validation.** The package files come from a machine that just installed from an upstream repo and their contents reach a GitHub issue body, so anything that is not a dpkg name/version pair fails the run. ### Commit 4 — `DECISIONS.md` -D017 (two-lane CI: pinned gating lane + report-only currency lane, and why `first-boot.sh` must never upgrade) and D018 (currency lane reboots unconditionally after `dist-upgrade`). +D017 (two-lane CI: pinned gating lane + report-only currency lane, and why `first-boot.sh` must never upgrade) and D018 (the currency lane reboots after `dist-upgrade` **and proves it rebooted** — the verification was missing from the first draft and is the part easiest to omit). ## 5. Convention conflict, surfaced