Conversation
There was a problem hiding this comment.
Pull request overview
Adds OpenHCL kernel binary size regression checks to the OSS GitHub CI, mirroring the existing openvmm_hcl (usermode) size-check pattern by publishing merge baselines and comparing PR outputs against them via xtask verify-size.
Changes:
- Introduces a new kernel baseline artifact publisher node (
artifact_openhcl_kernel_sizecheck) and a CI job node to publish kernel baselines on post-merge CI. - Adds a PR job node to download the last-merge baseline artifact and run
xtask verify-sizefor kernel variants (x64 main, x64 CVM, aarch64 main). - Wires the new baseline publishing + PR comparison jobs into the
checkin_gatespipeline and regeneratesopenvmm-ci.yamlaccordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| flowey/flowey_lib_hvlite/src/lib.rs | Exposes the new kernel sizecheck artifact module. |
| flowey/flowey_lib_hvlite/src/artifact_openhcl_kernel_sizecheck.rs | New node to copy the resolved kernel binary into an artifact directory for publishing. |
| flowey/flowey_lib_hvlite/src/_jobs/mod.rs | Registers the new job nodes for kernel baseline publish + PR size verification. |
| flowey/flowey_lib_hvlite/src/_jobs/check_openhcl_kernel_size.rs | New PR job to fetch last-merge baseline artifact and run xtask verify-size against the currently resolved kernel. |
| flowey/flowey_lib_hvlite/src/_jobs/build_and_publish_openhcl_kernel_baseline.rs | New CI job to resolve the kernel binary and publish it as a baseline artifact. |
| flowey/flowey_hvlite/src/pipelines/checkin_gates.rs | Adds kernel baseline publishing (CI) and kernel verify-size jobs (PR) to the pipeline graph. |
| .github/workflows/openvmm-ci.yaml | Regenerated workflow to publish the new kernel baseline artifacts from CI jobs. |
5d5100d to
44a963e
Compare
|
Still looking at this, don't think I want one runner per kernel type. |
44a963e to
2386d79
Compare
| for (label, kernel_var) in kernel_baselines { | ||
| let kernel_path = rt.read(kernel_var); | ||
| fs_err::copy(kernel_path, artifact_dir.join(&label))?; | ||
| } |
There was a problem hiding this comment.
label is used as a path component via artifact_dir.join(&label). If label ever contains path separators (e.g. "../" or an absolute path), this can write outside the artifact directory. Consider validating/sanitizing label to a safe filename (reject separators/absolute paths) before joining, or derive the filename from a trusted enum instead of a free-form String.
| let _kernel_labels: Vec<String> = kernel_checks.into_iter().map(|kc| kc.label).collect(); | ||
|
|
There was a problem hiding this comment.
_kernel_labels is computed from kernel_checks but never used. This looks like leftover scaffolding; removing it would simplify the control flow and avoid confusion about whether labels are needed later for dependency ordering.
| let _kernel_labels: Vec<String> = kernel_checks.into_iter().map(|kc| kc.label).collect(); |
| - name: extract and resolve kernel package | ||
| run: flowey e 11 flowey_lib_hvlite::resolve_openhcl_kernel_package 0 | ||
| shell: bash | ||
| - name: extract and resolve kernel package |
There was a problem hiding this comment.
These two steps have the same display name ("extract and resolve kernel package"), which makes the Actions log harder to interpret when multiple kernel variants are resolved. Consider adjusting the generator (flowey step naming) so the step name includes the request index or kernel kind/arch (e.g. main vs cvm) to disambiguate.
| - name: extract and resolve kernel package | |
| run: flowey e 11 flowey_lib_hvlite::resolve_openhcl_kernel_package 0 | |
| shell: bash | |
| - name: extract and resolve kernel package | |
| - name: extract and resolve kernel package (0) | |
| run: flowey e 11 flowey_lib_hvlite::resolve_openhcl_kernel_package 0 | |
| shell: bash | |
| - name: extract and resolve kernel package (1) |
2386d79 to
c125586
Compare
Extend the existing openhcl binary size check infrastructure to also compare kernel binaries between PRs and the last successful main merge. Kernels checked per architecture: - x64: Main, CVM - aarch64: Main The kernel baselines are included in the existing per-arch openhcl-baseline artifact (alongside the usermode binary), and the comparisons run in the existing 'verify openhcl binary size' jobs. No new CI jobs, artifacts, or files are added.
c125586 to
ecbe39c
Compare
| let original_elf = object::File::parse(&*original); | ||
| let new_elf = object::File::parse(&*new); | ||
|
|
||
| println!("Verifying size for {}:", (&self.new.display())); | ||
| let (total_diff, net_diff) = verify_sections_size(&new_elf, &original_elf)?; | ||
|
|
||
| let (total_diff, net_diff) = match (original_elf, new_elf) { | ||
| (Ok(orig), Ok(new_parsed)) => verify_sections_size(&new_parsed, &orig)?, | ||
| _ => { | ||
| // Fall back to raw file size comparison for non-object files | ||
| // (e.g. aarch64 raw kernel Image). | ||
| println!("(file is not a parseable object file, comparing raw file sizes)"); | ||
| let orig_size = original.len() as u64 / 1024; |
There was a problem hiding this comment.
The fallback to raw file-size comparison triggers on any parse failure (including cases where an ELF/PE/Mach-O is expected). That can mask real problems (e.g., corrupted/incorrect binary) and also removes the helpful file-path context that previously came from with_context. Consider only falling back when the parse error indicates an unknown/unsupported file format (or gate it on a known raw-kernel filename/extension), and otherwise return an error that includes the path being parsed.
| let orig_size = original.len() as u64 / 1024; | ||
| let new_size = new.len() as u64 / 1024; | ||
| let diff = (new_size as i64) - (orig_size as i64); | ||
| println!( | ||
| "{:20} {:>15} {:>15} {:>16}", | ||
| "raw file", orig_size, new_size, diff | ||
| ); | ||
| println!("Total Size: {new_size} KiB."); | ||
| (diff.unsigned_abs(), diff) |
There was a problem hiding this comment.
Raw-size comparison converts bytes to KiB via integer division (len() / 1024), which truncates. This can allow a binary to exceed the 50 KiB limit by up to 1023 bytes without failing. If the intent is a strict 50 KiB tolerance, compare in bytes against ALLOWED * 1024 (keeping KiB only for display), or use a ceiling conversion when computing the KiB values used for the threshold.
Add size regression checks for OpenHCL kernel binaries in the OSS CI pipeline, matching the pattern used for the usermode openvmm_hcl binary.
On post-merge CI, kernel baselines are published as artifacts. On PRs, the current kernel is compared against the baseline using xtask verify-size (50 KiB tolerance).
Kernel variants checked: