-
Notifications
You must be signed in to change notification settings - Fork 129
install: store boot assets in ESP for composefs bls install #1541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: composefs-backend
Are you sure you want to change the base?
install: store boot assets in ESP for composefs bls install #1541
Conversation
Can confirm with this patch set (plus coreos/bootupd#978 & the below), we are able to produce a bootable Chainguard Wolfi disk image with sd-boot.
Additional bootc patch, dependent on bootupd:
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly refactors setup_composefs_bls_boot
to store boot assets in the ESP, which is essential for systemd-boot
compatibility. The approach of locating and mounting the ESP partition is sound and aligns with the existing setup_composefs_uki_boot
function. However, I've found a critical issue in the implementation of the mounting logic that will cause failures. My review includes a specific code suggestion to fix this.
22a1106
to
fdc7199
Compare
8faa04c
to
346fa79
Compare
I tried to implement this in a slightly different way - with the vmlinuz and initrd in rootfs, and the boot entries in ESP that point to them. Thinking it might not have been a mistake directing these files to rootfs. But unfortunately this failed to boot. Unsuccessful patch (applied ontop of fdc7199):
This does make sense though, as we're aiming for bls-compliance, and the bls specification requires all assets referenced in a entry file to be on the ESP.
|
fdc7199
to
4fcc336
Compare
Signed-off-by: Robert Sturla <[email protected]>
4fcc336
to
3d58280
Compare
Yes I think this is the ideal we want to get to, is that grub can be made to just implement BLS. |
Thank you so much for adding these tags! It's very helpful to know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're in a state right now where we don't have automated tests on the composefs backend branch at all which we need to fix pretty soon and as we start to think about handling both grub and systemd-boot we'll need to do that. But skimming this looks sane to me.
crates/lib/src/install.rs
Outdated
}; | ||
|
||
let boot_dir = root_path.join("boot"); | ||
let mounted_esp: PathBuf = root_path.join("esp").into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recommended ESP mount point is /efi
, not /esp
https://uapi-group.org/specifications/specs/boot_loader_specification/#mount-points
Regardless though it's a bit ugly for us to mutate the target disk image for this; we can mount it to a temporary directory instead.
(Actually though, what would be even nicer is to use the new mount API to get a detached FD instead...hmm, could add a helper that does that but falls back to mkdtemp + mount + openat + umount(MNT_DETACH)
if we don't have open_tree
or so...)
Thanks for the reviews!
Perfect! Glad there's not going to need to be much forking out based on the bootloader - the majority of the implementation will be identical between the two. I wanted to get this confirmation first before I spent too much more time looking at this. |
Let's not block on that, just mounting to a tmpdir I think is totally fine. |
Plus additional review comments: - Created constant for EFI/LINUX - Switched from Task to Command - Create efi_dir as Utf8PathBuf Signed-off-by: Robert Sturla <[email protected]>
Have pushed a new commit with the updates. Will squash after I've confirmed this still works as expected. May follow this up with a separate PR after I learn what the new mount API is. |
Please note: I do not currently have an environment to test this with GRUB.
The existing code copied the boot entries, kernel and initrd into the rootfs rather than the ESP. This meant systemd-boot was unable to see them, and is not compliant with the BLS spec. From my brief investigation, it seems like grub handles this fine, however systemd-boot does not.
I am aware this is different to how the existing ostree backend works today, however this change makes the implementation more BLS-compliant (which it seems like we're wanting to be for the composefs backend) due to the following requirement:
In my mind I would wonder, if this was an intentional decision, why we would want bootc to even need to be aware of whether sd-boot or grub is being used after the initial install as my understanding is that grub can act in a BLS-compliant way. Therefore, a single BLS-compliant implementation that works across grub and sd-boot would be simpler overall (though I'm more than happy to be corrected on this).
The majority of this code was created referencing the existing
setup_composefs_uki_boot()
function.Also, please note that I don't really know what I'm doing when it comes to bootloaders, so there may be a better approach to solve this problem.
Assisted by: ChatGPT (for conceptual knowledge)
Assisted by: GitHub Copilot gpt-4.1 (for code completion and rust syntax help)