efi: Install grub efi modules needed for UKIs (in case they are not b… - #1165
alexlarsson wants to merge 1 commit into
Conversation
…uilt in) We're running into issues with bootc with UKIs on aarch64, because the chain.mod file is not built in to the grub efi file. And anyway we don't know how the grub in the image is built, so just in case we always install fat and chain modules that are needed for UKI menu entries to boot. Signed-off-by: Alexander Larsson <alexl@redhat.com>
|
Hi @alexlarsson. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: coreos/bootupd/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (23)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe change adds architecture-specific GRUB EFI module directories and copies available ChangesGRUB EFI module copying
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was identified; the GRUB module copy paths match the supported boot layout. 🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title describes the GRUB EFI module change, but it does not follow the required format because the description begins with uppercase "Install" instead of lowercase text after the colon. The visible title is also truncated. Full details: Commit Message ConventionExplanation The PR contains one non-merge commit with subject
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Rolv-Apneseth
left a comment
Rolv-Apneseth
left a comment
There was a problem hiding this comment.
LGTM. Might also ask that @Johan-Liebert1 take a look though
| /// Copy the external modules for booting UKIs to /boot when the image provides | ||
| /// them. A GRUB EFI binary may already contain these modules. | ||
| fn install_grub_efi_modules(source_root: &Dir, target_root: &Dir) -> Result<()> { | ||
| let target_dir = format!("boot/grub2/{GRUB_EFI_MODULE_DIR}"); |
There was a problem hiding this comment.
Could we use the GRUB2DIR from grubconfigs.rs here? Cross-distro support is one of the goals for the future, and this would just make it easier to track down where to update this hard-coded path in the future.
|
/ok-to-test |
|
|
||
| target_root.create_dir_all(&target_dir)?; | ||
| let target = target_root.open_dir(&target_dir)?; | ||
| source_root |
There was a problem hiding this comment.
Ideally we'd want to use atomic write APIs
| const GRUB_EFI_MODULE_DIR: &str = "arm64-efi"; | ||
|
|
||
| #[cfg(target_arch = "x86_64")] | ||
| const GRUB_EFI_MODULE_DIR: &str = "x86_64-efi"; |
There was a problem hiding this comment.
I wonder if this is correct. In the image quay.io/fedora/fedora-bootc:rawhide I see
/usr/lib/grub$ uname -m
x86_64
/usr/lib/grub$ ls -a
. .. i386-pcThere was a problem hiding this comment.
Thats the 32bit modules. The 64bit ones are in the grub2-efi-x64-modules rpm, which you probably don't need as the x86-64 grub includes a ton of modules.
There was a problem hiding this comment.
My question was regarding this directory itself. In my non-coreos system I see both i386-pc and x86_64-efi inside of /usr/lib/grub, but on coreos and bootc images I only see i386-pc. Wouldn't this always skip the module copy on x86-64 systems?
There was a problem hiding this comment.
The grub2-efi-x64-modules rpm is not installed in the coreos x86-64 images, because it is not needed, because on this arch the modules are built-in (on fedora at least). So, indeed, this would skip the copy on x86-64, but that is fine. The problem is on aarch64 where these modules are not built in. Or any non-fedora system where we don't know what is built in or not.
3 participants
…uilt in)
We're running into issues with bootc with UKIs on aarch64, because the chain.mod file is not built in to the grub efi file. And anyway we don't know how the grub in the image is built, so just in case we always install fat and chain modules that are needed for UKI menu entries to boot.