UKI Cleanup#2200
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the UKI (Unified Kernel Image) build process to support passing explicit kernel and initramfs paths via CLI arguments, reducing reliance on auto-discovery within the rootfs. Key changes include updating the seal-uki and finalize-uki scripts to use named arguments, modifying Dockerfile stages to extract and clean up kernel components, and extending the Rust library and CLI to handle the new parameters. Review feedback identified a potential path resolution bug in the Rust file existence checks, a filename mismatch in the upgrade test Dockerfile, and suggested improvements for error handling and validation in the seal-uki script.
cb42d78 to
8a3ed64
Compare
66dc0e3 to
3a9dc2b
Compare
| cat >> /tmp/Containerfile.drop-lbis <<-EOF | ||
| FROM base as kernel | ||
| RUN <<-RUNEOF | ||
| objcopy -O binary --only-section=.initrd /boot/EFI/Linux/*.efi /boot/initramfs.img |
There was a problem hiding this comment.
We were discussing maybe having this in bootc - bootc internals uki extract?
| how: fmf | ||
| test: | ||
| - /tmt/tests/tests/test-37-install-no-boot-dir | ||
| extra-fixme_skip_if_composefs: true |
There was a problem hiding this comment.
How is this related?
From live chat - we probably want to test at a basic level that composefs tests actually install that way
There was a problem hiding this comment.
Oh, this is failing for another reason. I kinda lumped it with the bootloader=none one
There was a problem hiding this comment.
Actually this is failing for composefs because bootloader=none is not supported for composefs and this test fails when trying to install bootloader for composefs. For ostree, the bootloader installation is simply skipped with bootloader=none
I did end up fixing a containers-storage permission issue while debugging this
3a9dc2b to
8e581d4
Compare
8e581d4 to
503b07a
Compare
| RUN --network=none --mount=type=tmpfs,target=/run --mount=type=tmpfs,target=/tmp \ | ||
| --mount=type=bind,from=sealed-uki,src=/,target=/run/sealed-uki \ | ||
| /usr/bin/finalize-uki /run/sealed-uki/out | ||
| cat >> /tmp/Containerfile.drop-lbis <<-EOF |
There was a problem hiding this comment.
This isn't about dropping lbis?
There was a problem hiding this comment.
This is kinda confusing but the original Dockerfile is for removing lbis and we just append UKI stuff to it
| kver=$(bootc container inspect --rootfs /run/base-penultimate-src --json | jq -r '.kernel.version') | ||
|
|
||
| rm -v "/usr/lib/modules/$kver/vmlinuz" | ||
| rm -v "/usr/lib/modules/$kver/initramfs.img" | ||
| fi |
There was a problem hiding this comment.
Since this one would end up being closer to user-facing, I would prefer to streamline this more.
I think what would work best is to have our "base" build process be a stage that generates a single intermediate image with /rootfs and /kernel or so. This could be part of bootc-base-imagectl, or we could have it be part of something more like bootc container split-root-and-uki?
There was a problem hiding this comment.
sgtm. By the "base" build process, are you referring to the Dockerfile (in bootc) or is it about something else?
There was a problem hiding this comment.
The pattern we apply to this should generalize to https://github.com/cgwalters/rhel-bootc-examples/tree/sealed/sealing as well etc.
|
I think we should have the kernel and initrd as required arguments and thus only support the case where they are not in the rootfs anymore as I don't see a use case where someone would want a sealed image with both a UKI and split out kernel and initrd. Edit: That would break the current |
|
Hmm. I kind of lean towards not breaking it at least right away, it seems really easy to continue to support what we have now too. We could mark it deprecated though. |
503b07a to
8a5c7ae
Compare
|
Failures are transient & volatile related. Maybe from #2201? |
8a5c7ae to
cc8a2d8
Compare
While building a sealed UKI image we'd want to remove the original kernel + initramfs from the final image and have only the final UKI present. This was not possible before as `bootc container ukify` expected kernel + initramfs to be present in `usr/lib/modules` of container root Fixes: bootc-dev#2185 Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com> Mark `bootc container ukify` without `--kernel` and `--initramfs` as deprecated
Now that we can pass kernel and initrd paths to `bootc ukify`, rework our UKI Dockerfile to remove kernel + initrd from the final layer and only keep the UKI This still will not *remove* the kernel + initrd from the tarball but have whiteout instead See bootc-dev#2027 (comment) Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
vmlinuz and intrd should not be present in UKI images; add test for the same Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Bootloader set to none is not supported with the composefs backend so we skip tests with this option for composefs backend Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We were defaulting to unprivileged user "nobody" when pulling an image, but pulling from containers-storage was failing as it requires extra privileges. Default to the current user, usually root, when pulling from containers-storage Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
f2c3424 to
667a0fd
Compare
Don't know if clap has a way to do this, if it does I couldn't find it. We just print our custom warning if |
667a0fd to
4d6bed6
Compare
Baseconfigs tests were failing in CI because the initrd which had the expected baseconfigs was being deleted in the same layer it was being created in. Move the deletion to a completely separate layer Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
4d6bed6 to
20e2fed
Compare
| }), | ||
|
|
||
| (None, None) => { | ||
| eprintln!( |
There was a problem hiding this comment.
I am having second thoughts, it should not be too onerous for us to just continue to support what we shipped before. I think we can drop the eprintln!
|
|
||
| /// Path to the kernel | ||
| #[clap(long, requires = "initramfs", requires = "kver")] | ||
| kernel: Option<Utf8PathBuf>, |
There was a problem hiding this comment.
What I am thinking here is we could make this --kernel-dir which is defined to be a path to something like /usr/lib/modules/$kver.
That solves three problems:
- We expect kernel+initramfs in one place, so we don't need separate args for them
- We also require the directory to be named the same as a modules dir, so we don't need a separate kver argument
ukify: Allow passing custom kernel, initramfs
While building a sealed UKI image we'd want to remove the original
kernel + initramfs from the final image and have only the final UKI
present. This was not possible before as
bootc container ukifyexpected kernel + initramfs to be present in
usr/lib/modulesofcontainer root
Fixes: #2185
dockerfile/uki: Rework to remove kernel + initrd
Now that we can pass kernel and initrd paths to
bootc ukify, reworkour UKI Dockerfile to remove kernel + initrd from the final layer
and only keep the UKI
This still will not remove the kernel + initrd from the tarball but
have whiteout instead
See #2027 (comment)
test/integration: Test vmlinuz non-existence with UKI
vmlinuz and intrd should not be present in UKI images; add test for the
same