feat: add image profiles with initial debug and performance profiles - #607
feat: add image profiles with initial debug and performance profiles#607Christopher Obbard (obbardc) wants to merge 14 commits into
Conversation
Christopher Obbard (obbardc)
changed the title
|
Loïc asked claude for a review, please find it below Overall this is a clean, well-decomposed series: the mechanism ( Things verified rather than assumed, since they're the load-bearing bits:
Substantive1. The performance profile gets no hardware boot validation. Related caveat: forcing 2. A whole extra rootfs for a cmdline-only difference. For 3. On a tree with an existing default 4. The image recipe never validates the profile name, only agreement with the rootfs. So Robustness / duplication5. The valid-profile list lives in three places — the 6. Dead branch in the image recipe: 7. The rootfs validation runs unconditionally, unlike the "Validate snapshot timestamp" precedent it's modelled on, which is wrapped in 8. Style drift on reading buildinfo. The recipe uses 9. Docs and commit messages10. 11. The new PREFIX comment cites 12. 13. Trivia: None of the above is a blocker except arguably #3, which will bite the first person who follows the README example. #1 is the one most worth the author's view on before merge, since it determines whether this profile is ever actually booted on hardware. |
Loïc Minier (lool)
left a comment
Loïc Minier (lool)
left a comment
There was a problem hiding this comment.
Hey, so I went through all the commits, really nice split and logical incremental changes – everything made sense and is inline with exactly what I would expect for these features. TL;DR: I think this is close to what we should merge for an initial version of this.
I didn't review line by line, I mostly connected with the approach and found this sound.
In review comments, I'm only flagging a minor cosmetic thing in a comment with naming of variables.
Otherwise it also bugs me a bit that we "variants" and "profiles" and that people might find this confusing. Saying "default" might also be confusing to refer to one or the other default, perhaps we should pick unique names for each of the defaults.
The Makefile duplication is also not something new and unrelated to this PR, but it's starting to get worse.
I also asked claude to review the PR, and it was insightful (will attach), albeit most bits were harmless. From the Claude notes, point 1), 2), 10), 11), 12) are worth considering IMO.
One thing made me pause. We're on a path to grow our CI significantly, yet while the changes are easy to review and maintain, they are the opposite of what we usually thrive for with Debian: an universal OS that adapts to different situations. So I was wondering if we should handle some of these more dynamically at build time or even at runtime. For instance, we could have a command to select between profiles and it would set the kernel cmdline and kernel flavor. Leaving this thought here as
435f81c to
ff46d7c
Compare
ff46d7c to
95c67d1
Compare
I agree, I think for
Yeah, I am not sure the best way to handle this. Maybe we need to consider cleaning that up very shortly.
I'll look at those one-by-one. Very detailed. I've split the "debug" into a separate branch locally, since that depends on another kernel. That can be proposed/merged after this one which adds "performance" (no kernel changes needed).
Yeah, I think that this needs to be considered as part of #507 - my vision is that as we get bigger, we should create a shared/generic minimal "rootfs", then extend it more as the recipes increase (may be other options too). I think that cleanup should come as soon as this is in though. |
Christopher Obbard (obbardc)
changed the title
0d0d8a8 to
4020da3
Compare
|
Loïc Minier (@lool) in response to your very in-depth claude review: Review 1 - The performance profile gets no hardware boot validationThis is a genuine issue to fix now (and also was an issue with multimedia images!). I have fixed that in this PR, but I will move that to its own PR once validated, it doesn't belong here. Review 2 - A whole extra rootfs for a cmdline-only differenceYeah, this was deliberate. The long-term intent is actually the opposite: profile is expected to affect only the image recipe (e.g. kernel cmdline) not rootfs contents. It's currently passed to the rootfs anyway while the profile set is still settling. Once the profiles have settled and the rootfs is demonstrably profile-independent, we can remove the profile from the rootfs (e.g. the default rootfs can be shared) and only the image and flash stages need repeating per profile. I'd rather do that as a deliberate cleanup after all profiles are implemented. Review 3 - stale
|
Christopher Obbard (obbardc)
changed the title
4020da3 to
c05878b
Compare
c05878b to
910ee1d
Compare
Loïc Minier (lool)
left a comment
Loïc Minier (lool)
left a comment
There was a problem hiding this comment.
I've left minor remarks, I understand we can't land this yet due to deps on kernels
The main thing I'd like to discuss/revisit is how we go about building the set of LAVA jobs. My gut feeling is that we want to convey the build set information from the moment we decide what to build down to the moment we want to send things for testing, rather than maintaining list in multiple places. I do want the ability to add/enable or skip/disable specific combos at various points, but by default everything should be tested.
Because of the kernel deps and the complexity of this PR, I'd like to propose merging it in 2-3 PRs:
- first few commits of general cleanups can go in right now (minor comments left)
- image variant concept can IMO land now as well
- LAVA / CI I feel requires further discussion, but could potentially land now
- only the debug profile needs to be deferred to landing later, when the kernel lands
| - suite: forky | ||
| # boot the Weston/multimedia session rather than the Xfce desktop | ||
| - suite: trixie | ||
| artifact_prefix: trixie-weston-multimedia |
There was a problem hiding this comment.
kind of unfortunate to repeat the suite there, I wonder if you should use:
variant: [default, weston-multimedia]
and compute artifact_prefix from suite + variant
| headers: | ||
| QLIAuthorization: Q_QLI_S3_TOKEN | ||
| url: "{{BUILD_DOWNLOAD_URL}}/{{SUITE}}-flash-emmc.tar.gz" | ||
| url: "{{BUILD_DOWNLOAD_URL}}/{{ARTIFACT_PREFIX}}-flash-emmc.tar.gz" |
There was a problem hiding this comment.
I did some similar cleanup in a draft some time back, I think we want to bring as little internal variables to these templates. Upstream LAVA changes might not be going in this direction yet, since they start capturing specific list of UFS partitions that should be flashed with qdl method for instance, but I'd like to move towards having a single high-level LAVA template for all boards.
Anyway, I think this is fine, it's one variable being swapped for another here, so fine with me, just wanted to share the perspective in context :)
| # request is where a regression is cheapest to catch. Keep this list in | ||
| # step with the build matrix in build-on-pr.yml. artifact_prefix is | ||
| # omitted for a suite's default image, whose prefix is just the suite. | ||
| matrix: |
There was a problem hiding this comment.
So that's yet another copy of our target matrix; here's another strategy:
why don't we upload the image set along with the build as a metadata file, then use it as input for the tests?
Here's yet another strategy:
why don't we query GH workflows for what was built to decide what to test?
But if I think of implementation, what I'm vaguely picturing is:
- build-on-pr triggers some event as soon as any of the builds is completed
- test-on-pr fires on any of these events, triggers a corresponding LAVA test unless we want to skip testing
- build-on-pr triggers some "all builds complete" event
- test-on-pr fires on this event and triggers a test summary generation
Thanks! |
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>
"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>
| # needs the custom qcom-next-debug kernel package from | ||
| # https://github.com/qualcomm-linux/qcom-deb-images/issues/568 and is left to a | ||
| # follow-up; the kernel command line below is in place for it. | ||
| {{- if eq $profile "debug" }} |
There was a problem hiding this comment.
For debug, we also need qcom_scm.download_mode=1 and RuntimeWatchdogSec=10s; see #568 (comment)
Add a "profile" recipe variable selecting the intended runtime configuration of the image. Only "default" is accepted for now; later commits will add "debug" and "performance" profiles. Both recipes check that the profile is valid and the rootfs recipe records the chosen profile in /etc/buildinfo as "PROFILE=<profile>". The image recipe checks that rootfs.tar was built with the profile it is invoked with; as the two recipes are separate debos invocations they can easily disagree. A rootfs.tar with no /etc/buildinfo or no PROFILE= entry is rejected as well, as its profile cannot be established. In CI, the profile is validated before the build, passed to every debos invocation and (unless it is the default profile) appended to ARTIFACT_PREFIX so that profiles of the same suite and variant don't overwrite each other's artifacts. lava-test.yml derives the same prefix to pick the image it boots, so extend its profile input and its copy of the rule at the same time to keep the two expressions in sync. The profile is passed to both recipes even though it is expected to only affect the image recipe in the long run. Threading it through the rootfs as well is intentional while the profile set is still settling: the rootfs records PROFILE= so that the image recipe can verify the two independent debos invocations agree on it, as otherwise an image built from a stale or default rootfs.tar would be indistinguishable from a real profile build. Once the profiles have settled and the rootfs is profile-independent, the record and the check can be dropped and one rootfs build can shared between all profiles. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Move the kernel command line passed to filesystem-deploy into a $kernelcmdline variable alongside the other recipe variables. This makes it easier for later commits to extend the command line without changing the filesystem-deploy action directly. No functional change. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Add a "performance" profile. It is similar to the performance flavour meta-qcom builds from its ci/performance.yml. It builds with the default kernel configuration: no kernel options are changed for this profile as none are needed for now. It appends "quiet systemd.tty.term.console=linux" to the kernel command line so that the kernel doesn't print the boot log to the (slow) console and so that systemd still formats its own console output once that boot log is hidden. This reduces verbose kernel output during boot, suppresses driver and module initialisation messages, filesystem checks and other non-critical logs to improve overall boot time. Fixes: #604 Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Build a separate image in the daily build with the performance profile, only for trixie and the weston-multimedia variant to keep the number of CI jobs minimal and add it to the list of images LAVA tests. This deliberately runs a full extra build - rootfs, both disk images and the flash bundle - even though the performance profile currently differs from the default only by the kernel command line. "Minimal" here means the number of jobs rather than the work inside them; sharing the rootfs across profiles depends on the profile variable becoming image-only and is left as a follow-up improvement. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Add a "debug" profile. It is similar to the debug flavour meta-qcom builds from its ci/debug.yml. It enables ftrace from the very start of the boot by appending "ftrace=tracing_on trace_buf_size=5M trace_event=<events>" to the kernel command line, where <events> covers the timer, irq, workqueue, sched, power, regulator, thermal and rpmh tracepoints meta-qcom traces. The events leading to an early hang or a slow boot are therefore already recorded by the time userspace comes up, rather than only from the point a tracing session is started by hand. The profile is not ready for use yet: it still installs the default kernel, which is built without the debug options the tracepoints above are only half the story without. That needs the custom qcom-next-debug kernel package and installing it will be added in a follow-up pull request. This commit puts the profile and its kernel command line in place so that image can be built and published under its own name. Related: #568 Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Build a separate image in the daily build with the debug profile, only for trixie and the weston-multimedia variant to keep the number of CI jobs minimal, like the performance profile, and add it to the list of images LAVA tests. The profile is not ready for use yet - it still installs the default kernel rather than the qcom-next-debug package tracked by #568 - but building and booting it from now on means the follow-up that installs that kernel lands on a job which is already known to produce a bootable image. Booting it is worth a LAVA sweep of its own: tracing every timer, irq, workqueue and scheduler event from the start of the boot is exactly the kind of change that can slow a board down or keep it from reaching a login prompt, and that only hardware can tell apart from the default image. As with the performance profile this runs a full extra build - rootfs, both disk images and the flash bundle - even though the debug profile currently differs from the default only by the kernel command line. Sharing the rootfs across profiles remains a follow-up improvement. Related: #568 Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
910ee1d to
b3583f3
Compare
Christopher Obbard (obbardc)
changed the title
| {{- $suite = "unstable" }} | ||
| {{- end }} | ||
|
|
||
| # Validate $profile; keep this list in sync with the list in the |
There was a problem hiding this comment.
This currently builds a separate rootfs for each profile, even though the profile currently only affects the image. We could change that in this PR so profiles reuse the same rootfs; Loïc Minier (@lool) would you prefer that here or as a follow-up? It will need some additional actions plumbing.
2 participants
Add a
profiledebos variable to select the intended runtimeconfiguration of an image, as well as initial
performanceanddebugprofiles.Notes:
This PR depends on ci: boot every image variant a build produces #628
The debug profile is created but isn't yet ready for use, since it depends on the custom
qcom-next-debugkernel package (see Builddebugkernel variant withqcom_debug.configfragment pkg-linux-qcom#69 and Create a new "debug" debian build for Glymur #568). Installing the debug kernel will be added in a follow-up PR, or possibly the debug profile will be added in its own PR, once this PR is in a good state.Currently, this PR builds a separate rootfs/image pair for each profile. Since profiles should only affect the image, a future change will allow images with different profiles to reuse the same rootfs. That could be done in this PR as well; I’d be interested in your preference on whether to include it here or keep this change smaller.