Repository navigation
bootc: Test ostree against the Rust libcomposefs - #4
cgwalters-bot wants to merge 4 commits into
Conversation
6c5d7c1 to
c80424d
Compare
c80424d to
3c6dedb
Compare
3c6dedb to
0ec9b99
Compare
cgwalters-bot
left a comment
There was a problem hiding this comment.
Review guide for head 0ec9b999b3: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Review only the last two commits: ef413b3 (pack.sh PACK_VENDOR=cargo) and 0ec9b99 (just bootc/test-ostree, plus the ostree matrix entry in bootc-revdep). The rest is #7 and upstream commits that the stale forge main doesn't have yet. Risk is low, because the workflow is still behind upstream's if: never(), so nothing runs in CI. Once that is lifted, both matrix entries need a bootc that builds against ExternalPath (from #7). Without it, both fail at compile time whether or not continue-on-error is set, so dropping continue-on-error from the ostree entry is fine. The check that libostree and the initramfs load our libcomposefs is the part that makes the test meaningful. Worth reading closely.
Hotspots
- look-closely · logic —
contrib/packaging/pack.sh:41-62(ef413b3): The cargo branch relies on the sed below rewriting cargo vendor's absolute directory path to 'vendor'. Unlike the filterer path, its vendor tarball isn't created with --sort/--mtime, so it isn't reproducible. That's fine for tests but worth a comment if release builds might use it. - note · logic —
bootc/Justfile:108-120(0ec9b99): This clones HEAD of the checkout, and _require-clean makes that the tested tree. On GitHub the checkout is shallow; a local clone of a shallow repo works, but the version pack.sh derives from git should be checked there. - look-closely · logic —
bootc/Justfile:128-157(0ec9b99): This is what proves ostree runs on our library. It relies on bootc's local-repo priority, so check that it fails loudly (it does, by comparing the rpm owner) if the distro's C composefs wins. The initramfs glob assumes a single kernel. - note · logic —
.github/workflows/bootc-revdep.yml:25-39(0ec9b99):if: never()is upstream's and invalid per actionlint (there's no never() function); it isn't this PR's to fix. Two matrix entries each building bootc plus RPMs and VMs within timeout-minutes: 120 is untested on GitHub runners.
Safe to skim
bootc/build-composefs-rpms: straightforward rpmbuild wrapper, tested on devspacecrates/composefs/src/tree.rs: from #7, reviewed therecrates/composefs/src/erofs/reader.rs: from #7, reviewed therecrates/composefs/src/erofs/writer.rs: from #7, reviewed therecrates/composefs/src/dumpfile.rs: from #7, reviewed therecrates/composefs-capi/src/convert.rs: from #7, reviewed there
708ba90 to
7985b2b
Compare
Avoid indexing arrays when parsing the split cmdline, instead use CStr::from_bytes_until_nul() and iterators. Signed-off-by: Alexander Larsson <alexl@redhat.com>
7985b2b to
01c35c4
Compare
ostree gives each file a libcomposefs payload of `xx/<checksum>.file` and sets the fsverity digest separately. The capi turned the payload into an ObjectID (dropping `.file`) or ignored it when a digest was set, so the redirect pointed at a file that doesn't exist and an ostree deployment on the Rust libcomposefs couldn't boot. ExternalPath carries an optional redirect as given plus an optional verity digest, which is how libcomposefs models it. RegularFile::external() builds it from the pair and keeps the usual composefs layout (redirect is the digest's object path) as External, so the capi, the EROFS reader and the dumpfile parser all normalize the same way. A digest without a payload now stays that way everywhere, as in C: no redirect is made up for it, and the dumpfile parser accepts it. composefs-info lists the redirect as the object path, like C's, so ostree images don't lose their objects there. In V2 images ExternalPath gets the same single null chunk index as External. Note Item::Regular's path in dumpfile_parse is now an Option, and a redirect with an interior NUL fails the capi conversion instead of being dropped. The new test builds an image like ostree does via the C API and pins its digest; `just test-capi` compares the same image against the C library. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
pack.sh needs cargo-vendor-filterer, which isn't packaged anywhere and gets installed unpinned with `cargo install`. `PACK_VENDOR=cargo` uses `cargo vendor` instead, for builds that only need a working RPM and would rather not fetch and build another tool; the vendor tarball is bigger since it keeps every platform's crates. Prep for building RPMs in the bootc revdep test. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
ostree is the main C consumer of libcomposefs, and nothing tests it against our implementation yet: the capi job only runs the C library's own test suite. For composefs-rs to replace the C composefs package, ostree's composefs support (writing images at deploy time, mounting them from ostree-prepare-root) has to work on the Rust library. The new `just bootc/test-ostree` builds the composefs RPMs from this checkout with pack.sh and composefs.spec (vendoring with plain cargo) in a CentOS Stream 10 buildroot, and adds them to the packages bootc installs into its test image. bootc installs those from a local repository that takes priority over the distribution's, so they replace the C composefs. It checks that libostree loads our libcomposefs, and that the initramfs has the same library, then runs bootc's readonly and upgrade-with-reboot tests with the ostree backend. The revdep workflow runs it as a second matrix entry (still behind the workflow's temporary `if: never()`). Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
|
Signed off 3 commit(s) with |
01c35c4 to
ffb38b1
Compare
|
Opened upstream as composefs#408. Closing this review draft. |
Stacked on #7 (
ExternalPath, which keeps ostree'sxx/<checksum>.fileredirects), whose commit is the first one here; review only the last two. composefs#401 (the mount fixes it was stacked on before) is merged, so its commits are gone from this branch.Adds
just bootc/test-ostree, a second matrix entry in the bootc revdep workflow. It builds the composefs RPMs from the tree (pack.sh + composefs.spec) in a CentOS Stream 10 buildroot and puts them into the packages bootc installs into its test image, so they replace the C composefs. It checks that libostree loads our libcomposefs and that the initramfs has the identical file, then runs bootc'sreadonlyandimage-upgrade-rebootplans on the ostree backend (BOOTC_variant=ostree, set explicitly).A prep commit adds
PACK_VENDOR=cargoto pack.sh, so the test vendors with the distribution'scargo vendorinstead of installing rustup and an unpinned cargo-vendor-filterer. pack.sh now also fails if vendoring printed no source replacement config. The RPM build runs withCARGO_NET_OFFLINE=true, so it has to use the vendored crates.The workflow keeps upstream's temporary
if: never()(waiting onbootc-dev/bootc#2490), so neither matrix entry runs until that's lifted. The ostree entry is no longercontinue-on-error, since it passes with #7.Tested on a 16-core RHEL 10 devspace (kernel 6.12),
just bootc/test-ostreeat3d53788(the same tree as this head708ba90, which only rewords #7's commit message). bootc wasbootc-dev/bootc#2490plus the adaptation frombot/composefs-externalpathon cgwalters-bot/bootc, since bootc doesn't build against #7 without it:PACK_VENDOR=cargo;libostree uses /usr/lib64/libcomposefs.so.1.4.0 from composefs-libs-202609290233.g3d53788b2b-1.el10.x86_64, and the initramfs has the same file;plan-01-readonly,plan-24-image-upgrade-reboot): the ostree deployment boots through ostree-prepare-root on the Rust libcomposefs and survives an upgrade with a reboot.Related: composefs#323
Generated-by: https://github.com/cgwalters/#llms
Review draft in cgwalters-forge, not upstream yet. This section is removed when the PR is opened upstream.
composefs/composefs-rs, basemainPVTI_lAHOAQ_SPs4Bj2Gizg8qbJITo review:
/promoteon a line of its own, to open it upstream, ready for review. Either covers only the commits pushed so far.Signed-off-by: Colin Walters <walters@verbum.org>to the commits lacking it (the bot's and yours; anyone else's only if you ask), with you as committer./draftline (in the same comment or before) to open it upstream as a draft (/readyundoes that).