ci: boot every image variant a build produces - #628
ci: boot every image variant a build produces#628Christopher Obbard (obbardc) wants to merge 8 commits into
Conversation
Some callers omit the variant input. Give it an explicit "default" value so an omitted input selects the default image variant directly rather than relying on the empty-string fallback. No functional change. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Label each build job with its suite and variant so matrix jobs are easier to identify in the Actions UI. Show "default" when no variant is specified. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
PREFIX is a very generic name for what is really the filename prefix shared by every artifact a build publishes. Rename it to ARTIFACT_PREFIX to better describe the variable. Expand the expression building it over several lines to improve readability and prepare for future expansion. While here document ARTIFACT_PREFIX better. Currently every workflow's build jobs all upload to the same S3 directory, so this prefix is the only thing keeping one suite's or variant's artifacts apart from another. Any other job consuming an artifact has to reconstruct it from the suite and variant it wants. No functional change: the prefix built for a given suite and variant is the same as before. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The LAVA templates currently select images by suite only, so they can only boot the default variant. Add a variant input to lava-test.yml and use the same artifact prefix as debos.yml to select the requested suite/variant image. The variant defaults to "default", so existing callers keep the current behaviour. Scope result artifacts by the same prefix so tests of multiple variants from one suite do not overwrite or collect each other's results. Update test-on-pr.yml to use the new artifact names. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
{{SUITE}} lost its last user in the previous commit, {{DEVICE_TYPE}}
and {{BUILD_FILE_NAME}} were never used: all three are substituted by
lava-test.yml and lava-schema-check.yml but appear in no
ci/lava/*/boot.yaml template.
Drop the substitutions and variables that fed them so the sed blocks
only replace placeholders that exist.
Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Since multi-variant builds landed build.yml has been building the weston-multimedia variant of both suites without ever testing either one: the LAVA templates could only boot a suite's default image. The variant has had no LAVA test coverage at all and a test failure would have gone unnoticed. Now that lava-test.yml takes a variant give the test job the same suite/variant matrix the build job already has so every image built is tested. This doubles the number of LAVA tests; from two to four. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
9274c85 to
124eb14
Compare
| JOB_ID=$(cat "$json_file" | jq -r ".id") | ||
| echo " * [Job $JOB_ID on $DEVICE_TYPE]($URL)" | ||
| echo " * [Job $JOB_ID on $DEVICE_TYPE]($URL)" >> pr-comment.txt | ||
| # more than one image is booted, so say which one this job ran. |
There was a problem hiding this comment.
I am not 100% sure about this change and it's not overly simple to validate changes to workflows triggered by on: pull_request.
There was a problem hiding this comment.
I looked at this in more detail and have manually checked the logic. At least from inspection it appears to do the right thing...
"Build on PR" builds both variants of both suites for every pull request but the tests that follow only ever booted trixie's default image: the LAVA templates could not boot anything else. So the same gap the daily build had applies to pull requests and on the workflow where a regression is cheapest to catch, before it reaches main. Boot all four images the build produces, using the same suite/variant matrix the build job uses. Each entry is a full LAVA sweep across every board, taking pull request testing from one sweep to four, but that is simply what validating four images costs: an image that is built and never booted buys nothing and the variants differ in the graphical session that has to come up before a board reaches a login prompt, which is the part a pull request is most likely to break. This workflow publishes results from its own job rather than through lava-test.yml, because it needs the pull request from the triggering run's payload. That job now collects the results of every image the test job booted instead of naming one prefix, so the list of images lives in the test matrix alone with nothing here to keep in sync: results are replaced in place by name and so are always the latest per board and image and job details are still scoped to the run attempt because they keep the LAVA job id and are never replaced. The pull request comment now also includes which image each job booted. lava-action names a job-details file after the test_job_file_name_prefix it is given, which lava-test.yml builds from the artifact prefix, so the image is recovered from the filename rather than tracked separately. That name ends in "-<run attempt>-test-job-<job id>", so dropping those two tails leaves the prefix, whatever the run attempt happens to be: trixie-1-test-job-1000123 -> trixie forky-weston-multimedia-7-test-job-1000127 -> forky-weston-multimedia Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
build-on-push.yml builds images for both trixie and forky, but its test job currently only tests the trixie image. Add forky to the test job matrix to enable testing all of the images built in the workflow. Also include the suite variable in the job names. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
124eb14 to
0a43430
Compare
Loïc Minier (lool)
left a comment
Loïc Minier (lool)
left a comment
There was a problem hiding this comment.
Hey, I did my own review once again, I was happy with most things and left just one comment. Then I also asked for a claude review; I'm pasting the tail of it below as it's not too long.
I'm a little concerned about LAVA pressure, but we can address this as a followup (lead with these jobs, then do these other jobs).
I haven't time to fully digest these yet
-
One failed matrix leg now discards the entire PR report (test-on-pr.yml:89). publish-test-results is needs: test with no if:. fail-fast: false stops cancellation but doesn't satisfy a needs gate. Going 1 leg → 4 means any single leg failure — a rejected LAVA submission, a submit-job timeout — yields no test-results check and no job-links comment, despite three images having produced good results. lava-test.yml:191 is ungated the same way. Wants if: ${{ !cancelled() }} on both.
-
The prefix is duplicated across two files resolved from different refs (test-on-pr.yml:80). Both sites carry a "keep in sync" comment, but test-on-pr.yml is workflow_run-triggered, so it — and the ./.github/workflows/lava-test.yml it calls — comes from main, while debos.yml runs from the PR head. Change the naming scheme in a future PR and the test half computes the old prefix: every qcomflash download 404s, all boards fail, no obvious cause.
This bites the PR in front of you: the job-links comment on #628 lists 7 jobs with no image label, i.e. main's test-on-pr.yml ran. None of this PR's test-on-pr.yml changes have been exercised by CI — the prefix-recovery loop and both new download patterns are untested. That's the thing I'd want addressed before merge, less for the bug than because the riskiest file in the series is unvalidated and structurally can't self-validate.
-
Four nested reports land on main instead of the PR (test-on-pr.yml:63). report_test_results_externally defaults true and isn't overridden, so all four nested publish jobs report externally alongside the new aggregate: five check runs named "Test Results", and since the nested ones pass no commit they default to github.sha — the default-branch head under workflow_run. build-debian.yml:80 already sets this false.
-
LAVA job_name no longer identifies the image (ci/lava/*/boot.yaml:65, all nine). Four jobs per board per run with byte-identical name and metadata — in the LAVA UI, the queue, and lavacli jobs list you can't tell which image failed without opening the job and reading the qcomflash URL. {{ARTIFACT_PREFIX}} is already substituted into these files, so this is nearly free.
-
PR hardware cost quadruples (test-on-pr.yml:74). ~28 concurrent LAVA jobs against a lab with one device per type. Four serialise per board while four runners sit in wait_for_job: true; job timeout 210 min vs. step timeout 240 min, so contention can time a leg out — which by finding 1 then discards everything. The commit message argues the cost honestly rather than hiding it, so this is a capacity call for you, not a defect.
Also noted but not filed: needs: build in build.yml:73 / build-on-push.yml:33 requires success, so one broken variant build skips hardware testing for all variants (pre-existing, costlier now); build-on-push.yml:47 reads artifacts_url from a matrix job where outputs collide, benign only because every leg uploads to the same S3 directory — which is what makes the whole prefix scheme work, so it deserves a sentence in the debos.yml comment; and lava-test.yml:235's near-identical loop wasn't updated alongside the test-on-pr.yml one, so the two have diverged.
| echo " * [Job $JOB_ID on $DEVICE_TYPE]($URL)" | ||
| echo " * [Job $JOB_ID on $DEVICE_TYPE]($URL)" >> pr-comment.txt | ||
| # more than one image is booted, so say which one this job ran. | ||
| # lava-action names the file "<ARTIFACT_PREFIX>-<run attempt>-test |
There was a problem hiding this comment.
Can we rely on this as part of the lava-action contract/API?
2 participants
The LAVA tests currently only ever boot a suite's default image, so any
non-default variant a build produces (e.g.
weston-multimedia) currently gets builtbut never booted. This series lets callers name the image to test and uses that to give the
weston-multimediavariants (and pull requests) real hardware coverage.