ci: add OCE compliance test (DHCP VIVSO discovery + install)

[bhouse-nexthop]

What this adds

ci: add OCE compliance test (DHCP VIVSO discovery + install) — a new CI gate that runs ONIE's own OCE (ONIE Compliance Environment, contrib/oce) against the freshly built kvm_x86_64 image, exercising the real OCP discovery path end-to-end:

• New oce-test job chained after install-test (build → boot-test → install-test → oce-test), so it runs against the same validated image.
• Boots ONIE headless with an L2 tap (not user-mode SLIRP), stands up the OCP discovery service backends (DHCP incl. VIVSO/opt-125, TFTP, HTTP), and has OCE drive discovery → fetch → full install (and updater mode for the relevant tests), asserting success over the serial console.
• workflow_dispatch gains an oce_scope input: default (a fast representative subset, also what push/PR runs) or full (the entire OCE installer+updater sweep).
• Harness: emulation/ci-oce-test.sh.

[mshych]

Three of the notes below are about the code this PR #1126
adds; the other eleven are about the CI it extends, which landed with
#1114 and which I went through in detail at the time.

On #1126 itself: the approach looks right, and it adds the gate the other
jobs don't provide — install-test hands ONIE the installer URL directly,
so real VIVSO discovery was untested until now. My three notes are
hardening rather than objections: an uncapped job, a vCPU count, and error
handling on the QEMU launches. None of them make it report a false PASS.

I built the image and ran the #1114 harnesses against the
real artifacts, so notes 1-11 are backed by execution.

On the eleven from #1114: nothing that has landed since has fixed any of
them, and all three files they touch are byte-identical between the #1114
commit and current onie-modernization-2026, so they still apply verbatim.
Two of them cost more because of this PR, which is why I'm raising them
here rather than separately — see notes 2 and 4.

Two attachments: ci-review-fixes.patch and the test suite described at the
end. The patch is based on your PR branch (5f9e62c).
It carries all fourteen notes, which is convenient for trying them
but is NOT how I would land them: notes 12-14
belong in #1126, and notes 1-11 fix already-merged code and belong in a
separate commit or PR. Every change carries a "FIX (review note N)"
comment matching the numbering below, so splitting it is mechanical — grep
the tag. Please review and take whichever parts you agree with.

###############################################

On this PR (#1126) — the OCE job and harness

###############################################

These come first but are numbered 12-14: notes 1-11 keep the numbers they
had when I wrote them against #1114, so that the "FIX (review note N)"
tags in the patch stay stable.

12. The new oce-test job has no timeout-minutes, and it is the one job
    whose length depends on an input.

    Worst case is bounded by the harness's own caps — a 240s embed plus
    PER_TEST_TIMEOUT (180s) per test:

    default 25 tests -> 240s + 25180s ~= 80 min
    full 119 tests -> 240s + 119180s ~= 361 min

    So "full" as configured can exceed GitHub's hard 6-hour job ceiling.
    That is the case I would care about: a sweep in which several methods
    hang gets killed by the platform rather than by the workflow, and a
    platform kill tells you nothing about which test it died on — which is
    exactly what you want to know. Expected-case runtime is far below both
    numbers; these are worst case only.

    Patch: 120 min for default, 350 for full, via
    "${{ inputs.oce_scope == 'full' && 350 || 120 }}". If "full" is meant
    to be routinely usable rather than an occasional manual sweep, then
    splitting it across matrix jobs is the better answer and a timeout is
    only a stopgap.

13. -smp 2 with accel=kvm:tcg, in both QEMU invocations.

    Same pattern as note 11 below, and I want to be careful about what I'm
    claiming: I measured a two-vCPU TCG guest failing to reach userspace at
    all (rcu_sched stalls) on the Secure-Boot OVMF boot in ci-boot-test.sh,
    NOT on these direct -kernel boots. I have no evidence these hang.

    It still matters here because every test is capped by
    PER_TEST_TIMEOUT and the sweep is fail-fast: under TCG a merely slow
    guest turns a working discovery method into a timeout and stops the
    run. CI runners have /dev/kvm, so this only affects local runs against
    the accel=kvm:tcg fallback the header advertises.

    Patch: one vCPU when /dev/kvm is not writable, matching the other
    harness.

14. A QEMU that never starts is reported as a failed install, and
    wait_marker picks up its pid implicitly.

    Both launches end ">/dev/null 2>&1 &", so a QEMU that dies on startup
    (bad args, a missing device model, a tap it cannot open) leaves an
    empty serial log — indistinguishable from "ONIE booted but never
    installed", and reported as the latter. Better than a false pass, but
    undiagnosable. Keeping stderr also surfaces "Could not access KVM
    kernel module ... falling back to tcg", which is otherwise invisible
    and would explain an inexplicably slow sweep.

    Separately, wait_marker reads the pid from a bare $! inside the
    function body, i.e. "the shell's most recent background job". That is
    correct at both call sites today only because QEMU happens to be
    started last — run_one backgrounds four service processes (dhcpd,
    nginx, tftpd, dnsmasq) immediately before it. Any reordering there
    would silently make the harness poll, and then kill, the wrong
    process. Passing the pid explicitly costs nothing and removes the
    coupling.

    One wrinkle if you implement this yourself: the liveness check cannot
    live inside wait_marker, because wait_marker runs in a command
    substitution — an exit there ends only the subshell, and the caller
    continues with a bogus elapsed time. The patch puts it in a
    qemu_started_or_die helper called at the launch site. I hit the same
    trap in ci-boot-test.sh's negative control.

###################################################

From #1114 — still open, and two now cost more

###################################################

These eleven are grouped by how soon I'd act on them, not by severity.
They fix already-merged code, so they belong in their own commit rather
than in #1126 — see the note about splitting the patch above.

--- Worth a PR now: all small diffs, all compounding cost ---

1. The cross-toolchain cache key omits inputs that change the toolchain.
   Highest practical impact of anything here.

   xtools.make derives
   XTOOLS_VERSION = $(ONIE_ARCH)-g$(GCC_VERSION)-lnx$(LINUX_RELEASE)-...
   and seds CT_LINUX_VERSION into the generated toolchain .config. But
   LINUX_RELEASE, LINUX_VERSION and LINUX_MINOR_VERSION are defined in
   kernel-download.make, and ONIE_ARCH in the arch/machine makefiles —
   none of which are hashed into the key.

   My build produced
   build/x-tools/x86_64-g8.3.0-lnx5.4.86-uClibc-ng-1.0.38
   so the kernel release is literally part of the directory name. After a
   kernel bump the key still hits, the restored toolchain is for the old
   directory, "make xtools" rebuilds from scratch (~23 min measured here),
   and the save step is gated on a cache miss so it never stores the new
   one. Every subsequent run pays the same 23 minutes, permanently, with
   no signal saying why and no way out except touching a hashed file.

   I'm flagging this first because it's a cost bug rather than a
   correctness bug and those tend to get waved through — but this is a
   modernization patchset, so bumping the kernel is more or less the
   point, and the failure is silent and self-perpetuating.

   Worth adding that the very next step already leans on this. "Mark
   restored toolchain up to date" justifies itself, in its own comment,
   with "A cache hit means the key matched, i.e. every toolchain input is
   unchanged and the restored toolchain is valid" — which is exactly the
   guarantee the key does not currently provide. That step is right to
   want it; the key just has to earn it. On a stale hit today the step
   dutifully touches the stamps of the old toolchain forward, and make
   builds the new one anyway.

   Patch: adds kernel-download.make, the arch makefile and
   machine/kvm_x86_64/machine.make to the key, plus a cheap guard that
   records the restored toolchain dirs and emits a ::warning if
   "make xtools" built one the cache did not contain.

2. push: and pull_request: both unfiltered run the whole pipeline twice
   for every push to a branch with an open PR.

   concurrency can't collapse them because the two events carry different
   refs (refs/heads/ vs refs/pull//merge). Restricting push to
   the integration branches keeps post-merge coverage at half the cost.
   Small change, immediate saving on every push.

   The patch lists both onie-modernization-2026 and master, in the branch
   filter and in the cancel-in-progress guard. Listing only master would
   remove CI from the branch this series actually integrates on, which is
   the opposite of the intent.

   This is worth more now than when I first raised it: ci: add OCE compliance test (DHCP VIVSO discovery + install) #1126 adds a fourth
   chained VM-heavy stage, and it is the longest of the four, so it roughly
   doubles what a duplicated run costs. I can't put a real number on it
   since I didn't run the OCE job — but whatever the pipeline costs, this
   pays it twice on every push to a branch with an open PR.

3. --privileged is ineffective and can be dropped.

   The Dockerfile ends with USER build, so the build runs unprivileged and
   cannot use any capability --privileged grants. I ran the complete build
   without it — xtools, signing-keys-generate, shim-self-sign and the
   image all succeeded — so this is a free removal of an escalation.

4. No "permissions:" block and no timeout-minutes.

   The job token inherits the repository default scope although nothing
   here writes to the repo, and a wedged build can burn the 6-hour
   default. "contents: read" plus 180/45/60 on the three jobs covers it.

   This PR is the second reason to do it: your new oce-test job has no cap
   either, and it is the longest of the four — that is note 12. The
   "permissions:" half needs nothing extra, since it is workflow-level and
   so covers oce-test for free.

5. The Dockerfile will not build for a fair number of developers.

   groupadd -g $GID fails when the host GID already exists in debian:11.
   My primary GID is 30, which is "dip", so the build aborts immediately.
   The low GID range is full of system groups (30 dip, 50 staff, 100
   users), which is common with central/NFS accounts. CI is unaffected
   because the runner's GID 1001 happens to be free, so this will never
   show up in Actions — it just quietly stops people reproducing CI
   locally.

   Reusing the existing group when $GID is taken fixes it. Note that
   "chown build:build" then also has to become "chown $UID:$GID", since
   the group "build" no longer exists in that case — I hit that second
   failure immediately after fixing the first.

6. secureboot mode never checks that Secure Boot was actually enforced.

   The positive run only asserts "it booted". If the varstore fails to
   enrol, the guest boots with SB off and the assertion passes for the
   wrong reason. init-arch already prints "Info: Secure Boot: Active."
   (read from the SecureBoot EFI variable via efivar), so the guest can
   confirm enforcement directly — verified present in a real enforced
   boot. That single assertion is what turns the negative control from
   "nothing appeared" into real evidence.

7. A failed virt-fw-vars is silently ignored.

   The enrolment is piped through "| sed | grep ... || true", which drops
   its exit status twice over: the pipeline reports grep's status, and
   "|| true" swallows even that. A failed enrolment yields an un-enrolled
   varstore and the run continues into note 6 above.

8. A QEMU that never starts can be scored as a successful SB rejection.

   With a QEMU that exits immediately, the harness prints:

   FAIL  no serial output captured
   PASS  ONIE did not boot without db (as required)
   NOTE  no explicit rejection string; relying on absence-of-boot
   

   The negative control can't distinguish "firmware refused the image"
   from "QEMU never ran". A liveness check a couple of seconds after
   launch, plus keeping QEMU's stderr instead of sending it to /dev/null,
   makes that a hard error. Keeping stderr is independently useful — it's
   where "Could not access KVM kernel module ... falling back to tcg"
   shows up, which is otherwise invisible.

   Correction to my first draft, in case it reached you: I also claimed
   OVMF's rejection strings never reach ttyS0. That's wrong. A real
   negative-control boot logged

   BdsDxe: failed to load Boot0001 "UEFI QEMU DVD-ROM": Access Denied
   

   to the serial console, with an empty debug console (Ubuntu ships a
   release OVMF). Your serial-only grep does match. Capturing the OVMF
   debug console is still worth doing for debug builds, but it's
   belt-and-braces, not a fix.

--- Whenever convenient: low risk ---

9. The "ONIE userspace" milestone is matched by GRUB output.

   It greps for 'ONIE: (OS Install|Rescue) Mode|^Version :'. Both are
   echoed by GRUB itself, from the menuentry and onie_entry_end in
   build-config/recovery/grub-iso.cfg, before the kernel is loaded — so
   the check duplicates "GRUB reached" and proves nothing about userspace.

   To be clear about the impact: this does not produce a false green.
   When the kernel hung in one of my runs, the milestone was mislabelled
   but the run still failed, because "console up" failed — and those
   markers are themselves genuine userspace strings. So the suite has a
   working userspace gate; this one check is just wearing the wrong label
   and gives a misleading diagnostic when a boot fails.

   'Info: BIOS mode:' is printed unconditionally during sysinit by
   rootconf/grub-arch/sysroot-lib-onie/init-arch and cannot be produced by
   GRUB. Confirmed against a real boot: GRUB banner at lines 4-10, first
   userspace output at line 12. The clearest case was a real SB-enforced
   boot that reached GRUB and then stalled in the kernel — the harness at
   HEAD still reported "ONIE userspace" as PASS on it. I can send that
   serial capture if you want to reproduce it.

10. The /sbin:/usr/sbin PATH addition never takes effect in CI.

    It's appended to ~/.bashrc, but the workflow runs bash -lc — a
    non-interactive login shell — and Debian's default .bashrc returns
    early for non-interactive shells before reaching that line. Under
    bash -lc, mkfs.vfat, blkid and losetup are all off PATH.

    The root cause is a level deeper: Debian's /etc/profile replaces
    PATH for non-root users with one that omits both sbin dirs, which is
    presumably why ~/.bashrc was reached for in the first place. So a
    plain ENV isn't enough either — /etc/profile would undo it under
    bash -l. Setting both ENV (non-login shells) and an /etc/profile.d
    snippet (sourced after /etc/profile resets PATH) holds for every
    invocation style.

    Low priority because the kvm_x86_64 build succeeds anyway — the line
    simply isn't doing anything today.

    Adjacent, not worth acting on now: sfdisk/fdisk/parted aren't
    installed at all (on Debian sfdisk lives in the separate "fdisk"
    package). Fine for kvm_x86_64; may matter if another machine needs
    them.

11. The documented accel=kvm:tcg fallback doesn't work for secureboot
    mode at -smp 2.

    Without KVM the SB-enforced boot dies in rcu_sched stalls and burns
    the whole timeout. Same firmware, same varstore, same ISO at -smp 1
    boots in ~31s and reports "Secure Boot: Active". GitHub runners have
    /dev/kvm so CI is unaffected — this only bites people running the
    harness locally, against a fallback the header comment advertises.
    Selecting one vCPU when /dev/kvm is unusable is enough; with that the
    secureboot job passes under TCG in 47s.

######################

What was verified

######################

Full build in your container, all four stages, no --privileged:
xtools rc=0, 1388s
signing-keys-generate rc=0, 6s
shim-self-sign rc=0, 76s
all recovery-iso demo rc=0, 192s

All five artefacts
ci-review-fixes.patch
the workflow uploads were produced under exactly the
referenced names, and all five secure-boot cert paths the boot test
expects exist. The .gitignore addition holds — a full build leaves the
tree clean.

ci-boot-test.sh relaxed PASS (31s)
ci-boot-test.sh secureboot PASS (47s) — guest reports Secure Boot:
Active, negative control rejected in 2s
with a real "Access Denied"
ci-install-test.sh PASS — embed, discovery install, demo OS
boots

[mshych]

ci-review-fixes.patch

[bhouse-nexthop]

Thanks — this is a thorough review, and the split you proposed is the right one. I went through all fourteen notes and re-verified each against the tree rather than taking them on trust; every one held up. All fourteen are applied, split exactly as you suggested:

• Notes 12–14 → this PR, folded into the existing commit (force-pushed).
• Notes 1–11 → ci: harden the kvm_x86_64 build/boot workflow and boot harness #1137, against onie-modernization-2026, since they fix already-merged ci: add GitHub Action to build and boot-test the kvm_x86_64 ONIE image #1114 code.

I did not use your patch verbatim — I reimplemented each fix so the comments explain the reasoning in the surrounding style — but the substance is yours, and the FIX (review note N) tags made the split mechanical, as intended.

On this PR (notes 12–14)

12 — uncapped oce-test job. Applied, with the scope-dependent cap you proposed: timeout-minutes: ${{ inputs.oce_scope == 'full' && 350 || 120 }}. Your worst-case arithmetic is right and the full case genuinely can exceed the 6-hour ceiling, which is the outcome worth avoiding — a platform kill says nothing about which test hung. I've kept your note about matrix-splitting full in the comment; agreed that's the real answer if it becomes a routine run rather than an occasional manual sweep.

13 — -smp 2 under TCG. Applied. I appreciate you being precise that you measured this on the SB/OVMF boot in ci-boot-test.sh and not on these direct -kernel boots — I've written the comment to say exactly that, so nobody later reads it as a fix for a hang observed here. The reason to take it anyway is the one you gave: with PER_TEST_TIMEOUT and fail-fast, a merely slow guest turns a working method into a timeout.

14 — QEMU that never starts, and the implicit $!. Applied, including your qemu_started_or_die placement — you're right that it can't live inside wait_marker, since that runs in a command substitution and an exit there would only end the subshell. Verified both halves in isolation: a stub that exits immediately now aborts with rc and stderr instead of being reported as "did not install", and with decoy background jobs started after the QEMU launch, wait_marker still watches and kills the right process (it did not before).

One addition beyond your patch: your patch puts the per-test stderr in $WORKDIR, which the EXIT trap deletes, so the "falling back to tcg" line you wanted visible would only ever appear in the never-started case. I dump it in the FAIL path too (filtering the terminating on signal 15 noise), so a slow-but-not-dead sweep can be diagnosed. I also kill the embed HTTP server from cleanup, since the new early exit 2 path could otherwise leak it.

On notes 1–11 (#1137)

All confirmed. A few where I checked the specific claim:

• 1 — confirmed the gap is exactly LINUX_RELEASE (kernel-download.make:22) and ONIE_ARCH (machine/kvm_x86_64/machine.make:10); GCC_VERSION and XTOOLS_LIBC default in crosstool-ng.make, which is already hashed, so those were the only two missing. Agreed this is the one to land soonest — kernel: move to the 6.18 LTS kernel #1130 bumps the kernel, which is precisely the trigger.
• 3 — confirmed independently that nothing in the build needs privileges: no losetup/mount anywhere, it goes through fakeroot and mtools, and onie-mk-iso.sh hardcodes /sbin/mkdosfs.
• 5 — reproduced: groupadd -g 30 build in debian:11 fails rc=4. Test-built the image at GID 30 and GID 1001; both work now.
• 9 — confirmed grub-iso.cfg:32 and :24 echo both strings the old check matched, so it was indeed a duplicate of "GRUB reached".
• 10 — reproduced the whole chain: .bashrc returns early when non-interactive, and /etc/profile lines 5–9 replace PATH for non-root before profile.d is sourced at line 27. Under bash -lc, blkid was off PATH. ENV + profile.d fixes both shell styles.
• 8 — took your correction about the OVMF rejection strings; the serial grep stays as the primary check and the debug console is additive, gated on an isa-debugcon probe.

Both PRs are pushed. CI on this one is sitting at action_required pending a maintainer approving the workflow run for the fork — could you kick that off when you get a chance?

[bhouse-nexthop]

One follow-up on note 2, since I deviated from what you suggested — everything else is applied as proposed.

I first implemented it exactly as you wrote it (push: restricted to master and onie-modernization-2026) and then found it has a blind spot: a fork's pull requests are opened against this repository, so they raise no pull_request event in the fork, and a fork's only trigger is push. Filtering push by branch name leaves every topic branch in a fork with no CI at all, and workflow_dispatch can't cover it either — that requires the workflow to be on the fork's default branch, and this one lives on onie-modernization-2026 while forks default to master.

I noticed because pushing the branch with your filter applied produced zero runs on my fork, which is where I validate the stack before submitting anything here.

So #1137 fixes the actual condition instead: a small gate job that skips the push run when an open PR in the same repository already covers the commit. Upstream that drops exactly the duplicate you identified; in a fork the query finds nothing and push CI keeps working. Confirmed on my fork:

EVENT: push  REPO: bhouse-nexthop/onie  BRANCH: bhouse.ci-review-fixes
No open pull request in bhouse-nexthop/onie for bhouse.ci-review-fixes; running.

Only build depends on the gate; the rest chain from it and skip automatically, so it costs one short job on a duplicate and nothing otherwise.

Entirely happy to switch back to the plain branch filter if you prefer the simpler workflow file — it's your call on the cost/complexity tradeoff, and the only thing lost is fork-side pre-submit CI.