Skip to content

Conversation

p5
Copy link

@p5 p5 commented Aug 8, 2025

This is nowhere near ready, and I highly doubt any of this code will make it's way into bootupd without major refactoring... I'm just trying to make something that works, without looking at the quality of code. And eventually start an early feedback cycle.

Info logs changed to warns as it was the easiest way to make them show up during bootc install.

Current state is it throws you to emergency mode when booting into a UKI. Might be due to either ostree, or lack of root=UUID options.

TODO:

  • Figure out the public API we wish to expose for enabling sd-boot
  • Correctly detect whether to install with sd-boot or grub
  • Figure out how to boot without emergency mode
  • Everything else

Copy link

openshift-ci bot commented Aug 8, 2025

Hi @p5. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

@p5 p5 changed the title [WIP] feat: enable systemd-boot support [WIP] feat: enable systemd-boot/UKIs support Aug 8, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a 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 introduces initial support for systemd-boot. It's a work-in-progress, adding a new systemd_boot_configs module and modifying the EFI component installation logic to handle systemd-boot entries and bootctl.

My review focuses on correctness and robustness. I've found a few critical issues in the new systemd_boot_configs.rs file related to incorrect path and UUID generation for the bootloader entries, which would likely prevent the system from booting. I've also pointed out a potential panic in efi.rs due to an unsafe unwrap.

Additionally, there are several places where log::warn is used for debugging; these should be reverted to more appropriate log levels. There are also some hardcoded values and logic that are marked as TODOs in the PR description, which I've also commented on for completeness.

Overall, this is a good start, and addressing the feedback will significantly improve the correctness and stability of the new functionality.

@cgwalters cgwalters marked this pull request as ready for review August 15, 2025 11:23
@cgwalters cgwalters marked this pull request as draft August 15, 2025 11:24
@cgwalters
Copy link
Member

Figure out the public API we wish to expose for enabling sd-boot

First I think we can add a build time option which entirely disables GRUB support. Then it's simple on UEFI platforms and where systemd-boot is in the target, we use it by default.

But for dynamic detection one way to do this would be to detect if the target system is both:

Also of course, if the target image doesn't have grub then we should default to systemd-boot.

@p5
Copy link
Author

p5 commented Aug 15, 2025

Would you mind if I went straight for the dynamic detection?
I'm trying to gate grub behind a feature flag, but doing so rapidly turns the codebase into a mess because it also means disabling bios and ostree (currently).

@p5 p5 force-pushed the sdboot-support branch from 9d5eb1a to 912de59 Compare August 15, 2025 12:49
@cgwalters
Copy link
Member

Would you mind if I went straight for the dynamic detection?

Yes of course that's fine!

I'm trying to gate grub behind a feature flag, but doing so rapidly turns the codebase into a mess because it also means disabling bios and ostree (currently).

Yeah, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants