Polish E2E pipeline: fast shellcheck, self-describing evidence, collision-free ports - #36
Merged
Conversation
shellcheck ran only in os-e2e.yml, where it sits behind ~3 min of runner setup and behind that workflow's `paths:` filter on os/**. A shell syntax error therefore surfaced minutes late, and a change that broke a script without touching os/** was never linted at all. Run it in ci.yml alongside the other static guards, with the same glob -- including os/scripts/lib/*.sh, which an earlier version of that glob omitted, leaving the shared libraries unlinted. Install shellcheck only when the runner does not already provide it, so a missing binary is a named install rather than "command not found". The os-e2e copy stays: it costs ~0.02 min and keeps that workflow able to fail on its own inputs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two robustness fixes to the same harness path. Evidence: the CI bundle recorded no host toolchain, so an artifact could not say what produced it -- and OVMF/QEMU versions silently change UEFI and device behaviour, which is precisely what this test exercises. Write diagnostics/host/versions.txt before the long install (emulator, firmware paths and their sha256, ISO sha256, uname), mirroring the host-environment file the GCP path already produces. Best-effort, so a missing optional tool cannot fail the run. Ports: the update server hardcoded 127.0.0.1:8080. That is fine on an ephemeral runner but collides wherever a host is reused or shared -- the GCP path keeps one instance alive across runs -- and surfaces as an "Address already in use" race against an unrelated server rather than as anything about ports. Bind port 0, read the kernel-assigned port back from the server banner, and hand it to the guest over the fw_cfg channel that already carries the update SHA-256. `python3 -u` is load-bearing: without it the banner is buffered when stdout is a file and the port cannot be read back at all. The guest falls back to 8080 when the fw_cfg entry is absent. Unlike the SHA-256, which is a trust anchor and stays a hard requirement, the port is only routing: an image from this tree still works with an older harness, and a wrong port merely fails the download -- the archive is SHA-verified regardless of which port delivered it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Finishes the remaining P2 items from
docs/reviews/e2e-pipeline-review.md.Done here
#9 — shellcheck runs in
ci.yml. It ran only inos-e2e.yml, behind ~3 min of runner setup and behind that workflow'spaths:filter onos/**. So a shell syntax error surfaced minutes late, and a change breaking a script without touchingos/**was never linted. The glob matches os-e2e's exactly, includingos/scripts/lib/*.sh— an earlier version of that glob omittedlib/, leaving the shared libraries unlinted. shellcheck is installed only if the runner lacks it.#11 — the evidence bundle names its own toolchain.
diagnostics/host/versions.txtnow records emulator/firmware versions, OVMF paths + sha256, ISO sha256 anduname, written before the long install. OVMF and QEMU versions silently change UEFI and device behaviour — and UEFI boot is what this test exercises — so an artifact that cannot name them is not reproducible evidence. Mirrors thehost-environment.txtthe GCP path already writes. Lands inside the existingoutput/diagnostics/**upload glob.#16 — no fixed port, no collision. The update server hardcoded
127.0.0.1:8080; fine on an ephemeral runner, but it collides wherever a host is reused (the GCP path keeps one instance alive across runs), showing up as an "Address already in use" race rather than as anything about ports. Now binds port 0 and passes the kernel-assigned port to the guest over the fw_cfg channel already carrying the update SHA-256.python3 -uis load-bearing: without it Python buffers the "Serving HTTP on … port N" banner when stdout is a file, and the port cannot be read back. Verified locally — the unbuffered form parses and serves (HTTP 200); the buffered form yields an empty log.The guest falls back to 8080 when the fw_cfg entry is absent. Unlike the SHA-256 — a trust anchor, still a hard requirement — the port is only routing: an image from this tree keeps working with an older harness, and a wrong port merely fails the download, which the SHA check catches regardless.
Already done on main — not redone
#10 (ISO no longer uploaded with
if: always()), #12 (docker-images: truekept, with written justification), #15 (GCP path runs the static guards), #17 (deadtest-daily-driver.shreference fixed).Deliberately not done
#14 (parallelize the 3 matrix profiles) — under KVM, the path CI actually uses, this saves ~1.4 min. Doing it means turning
qemu_pidinto an array and reworking the cleanup trap that currently guarantees no QEMU process or nbd device leaks on failure. Trading that guarantee for 1.4 min is a bad deal; the TCG case where it would matter (~3h → ~1h) is already fail-fast blocked. Left serial.#13 (shared-lib consolidation) —
lib/assert.shandlib/markers.shalready cover the duplication that actually caused a bug (the CR/ANSI fix landing in one poller and not the other). The remaining overlap is either genuinely divergent or the guest-sideemit()inos/files/usr/libexec/*, which ships inside the image and would need a new install path in the Containerfile to share. Noted as follow-up rather than forced.Validation
shellcheck clean over the full glob;
bash -non both edited scripts; both workflows parse; platform-guard (14 cases), layer-budget, and the timeout-budget guard (build=71/75 lifecycle=135/140 guest=42/45) all still pass — the last matters because it is sensitive to workflow edits.New markers
ANDROMEDA_UPDATE_FETCH_START/ANDROMEDA_UPDATE_PORT_INVALIDwere checked against the strict marker validator: it asserts the 10 required markers occur exactly once and in order and rejects*FAILED; extra markers are tolerated and neither new one matches that pattern.QEMU/KVM and the ISO build cannot run on this host (darwin, no
/dev/kvm) — os-e2e on this PR is the real test, and it exercises the new port path end to end.