Skip to content

Integration for GrubCC and SystemdBoot#1113

Open
Johan-Liebert1 wants to merge 20 commits into
coreos:mainfrom
Johan-Liebert1:grub-cc
Open

Integration for GrubCC and SystemdBoot#1113
Johan-Liebert1 wants to merge 20 commits into
coreos:mainfrom
Johan-Liebert1:grub-cc

Conversation

@Johan-Liebert1

@Johan-Liebert1 Johan-Liebert1 commented Jun 12, 2026

Copy link
Copy Markdown
Member

Add Bootloader enum

Now that we are going to support multiple bootloaders, we need a way to
keep track of which one we're operating on


For bootupd generate-update-metadata we now generate metadata for all
found bootloaders. The final json will look like

{
  "timestamp": "2026-06-10T09:52:58.107743026Z",
  "version": "grub-cc-1:2.12-59.fc45,grub2-1:2.12-58.fc44,shim-16.1-5",
  "versions": [
    { "name": "grub-cc", "rpm_evr": "1:2.12-59.fc45" },
    { "name": "grub2", "rpm_evr": "1:2.12-58.fc44" },
    { "name": "shim", "rpm_evr": "16.1-5" }
  ]
}

This allows us to have multiple bootloaders in a single image and
install/update based on preference.

During install/update, we check to bootloader and filter out any and all
entries from the update metadata that do not match or are not required
for the bootloader


For grub we store the statefile in /sysroot/boot/bootupd.json. For
grub-cc, and in future systemd-boot, we won't have a /boot, so we now
store the state pre bootloader.

Grub2: Statefile is still in /sysroot/boot/bootupd.json
GrubCC: Statefile is stored in all the ESPs

Store the sysroot_path in StateLockGuard which we require for mounting
the ESP. Also, mount the ESP at /boot which is what the BLS spec suggest


See: #1080

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

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 support for multiple bootloaders, specifically adding a new GrubCC option alongside the default Grub. It updates the saved state, component installation, update, and status mechanisms to dynamically detect and filter metadata based on the active bootloader, including handling state files stored in the ESP for GrubCC. The review feedback identifies a potential mount leak where the was_mounted flag is not reset upon unmounting, several instances of unnecessary double referencing, and opportunities to make the code more idiomatic by simplifying OnceLock usage and removing redundant iterator adapters.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/efi.rs
Comment thread src/efi.rs Outdated
Comment thread src/backend/statefile.rs Outdated
Comment thread src/backend/statefile.rs
Comment thread src/backend/statefile.rs
Comment thread src/bootloader.rs
Comment thread src/model.rs
@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-cc branch 2 times, most recently from 2ffcc16 to e8da148 Compare June 12, 2026 08:56
@Johan-Liebert1 Johan-Liebert1 marked this pull request as draft June 12, 2026 09:31
@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-cc branch 5 times, most recently from 77321d0 to fb028b8 Compare June 14, 2026 03:19
@Johan-Liebert1 Johan-Liebert1 marked this pull request as ready for review June 15, 2026 05:10
@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-cc branch 2 times, most recently from aa72462 to e7bf29d Compare June 15, 2026 05:35
Comment thread src/cli/bootupd.rs Outdated
Comment thread src/backend/statefile.rs
Comment thread src/backend/statefile.rs Outdated
Comment thread src/backend/statefile.rs Outdated
Comment thread src/backend/statefile.rs Outdated
Comment thread src/backend/statefile.rs
Comment thread src/cli/bootupd.rs Outdated
Comment thread src/bootloader.rs
Now that we are going to support multiple bootloaders, we need a way to
keep track of which one we're operating on

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Generate metadata for only a particular bootloader, either Grub or
GrubCC

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Add `is_bootloader_supported` function to figure out if bootloader is
supported by the component or not. Skip installs and metadata generation
for the component if it does not support the bootloader

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
For grub we store the statefile in `/sysroot/boot/bootupd.json`. For
grub-cc, and in future systemd-boot, we won't have a `/boot`, so we now
store the state pre bootloader.

Grub2: Statefile is still in `/sysroot/boot/bootupd.json`
GrubCC: Statefile is stored in all the ESPs

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Store the sysroot_path in StateLockGuard which we require for mounting
the ESP. Also, prevent mounting ESP at a tempdir, instead just mount it
at `/boot` which is what the BLS spec suggest

Fix issue with composefs systems where lsblk would fail to find the
backing device for "/" due to it being mounted as a virtual filesystem.
Instead we now use `/sysroot` or `/boot` to find the baking device

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
For `bootupd generate-update-metadata` we now generate metadata for all
found bootloaders. The final json will look like

```json
{
  "timestamp": "2026-06-10T09:52:58.107743026Z",
  "version": "grub-cc-1:2.12-59.fc45,grub2-1:2.12-58.fc44,shim-16.1-5",
  "versions": [
    { "name": "grub-cc", "rpm_evr": "1:2.12-59.fc45" },
    { "name": "grub2", "rpm_evr": "1:2.12-58.fc44" },
    { "name": "shim", "rpm_evr": "16.1-5" }
  ]
}
```

This allows us to have multiple bootloaders in a single image and
install/update based on preference.

During install/update, we check to bootloader and filter out any and all
entries from the update metadata that do not match or are not required
for the bootloader

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Only allow Grub as the bootloader for powerpc and s390x

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-cc branch 4 times, most recently from 1c646b3 to 1106373 Compare June 25, 2026 10:26
@Johan-Liebert1 Johan-Liebert1 changed the title Integration for GrubCC Integration for GrubCC and SystemdBoot Jun 25, 2026
@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-cc branch 6 times, most recently from 3a39aed to 4dd892b Compare June 29, 2026 11:15
@Johan-Liebert1 Johan-Liebert1 requested a review from cgwalters June 29, 2026 11:45
@Johan-Liebert1

Copy link
Copy Markdown
Member Author

I've tested SystemdBoot and GrubCC installation with bootc and it seems to work fine. Having this test in the CI would be helpful but we have a circular dependency between bootc and this PR

This works exactly like GrubCC
We expect systemd-boot efi to be in

`/usr/lib/efi/systemd-boot/<evr>/fedora/grubx64.cc`

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We don't have e2e upgrade tests for now, but we can atleast test if the
installations are working well

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We generate the final filetree from `/usr/lib/efi` which worked well
with only one bootloader, but now that we have three which are stored in
the following places

Grub2
- /usr/lib/efi/grub2/<evr>/EFI/fedora/grubx64.efi

GrubCC
- /usr/lib/efi/grub-cc/<evr>/EFI/fedora/grubx64.efi

SystemdBoot
- /usr/lib/efi/systemd-boot/<evr>/EFI/fedora/grubx64.efi

The problem arises as all the distinct efi binaries have the same name
(grubx64.efi - due to the particular name being baked into shim), we
cannot blindly create a filesystem tree out of this as later values will
override the previous ones.

To mitigate this, introduce a param `skip_dirs` which will filter
directories not required for the bootloader being installed

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
A utility cfg macro defined in build.rs so that we can deny EFI only
bootloaders - GrubCC and SystemdBoot, on non-EFI systems at compile time

Also, only allow setting default bootloaders for EFI systems

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Comment thread src/bootupd.rs Outdated
Comment thread src/bootloader.rs
Use this enum in match statements so that if we add a new component in
future, we'll get compile time errors in places where we need to handle
the new component. This also gets rid of some hardcoded strings

Also remove `#[default]` from Bootloader enum. Having a "default" in the
enum doesn't make sense as we never construct this enum

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
@Johan-Liebert1

Copy link
Copy Markdown
Member Author

A lot of changes here. I'll test it out in fcos as an override

Comment thread Dockerfile.bls
grubName="grubaa64.efi"
fi

cp /usr/lib/systemd/boot/efi/systemd-boot*.efi "/usr/lib/efi/systemd-boot/${evr}/EFI/$ID/$grubName"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, is this really required? I guess it must be - it's us taking over bootctl install and bootctl update.

That feels...unnecessary?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I spend a bit of time on this, is this case actually "install systemd-boot pretending to be grub" so it gets chainloaded by shim? That looks like what this is.

That feels like a clearly distinct case of things here and really worth clearly documenting.

OK right, and we're doing the same with grub-cc?

Hmmm, I guess it's kind of an assumption woven into things here that we're using shim, which is probably the right one by default, but I think we do ultimately need to handle non-shim cases too.

I guess this isn't blocking per se, but we should have a note about this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I spend a bit of time on this, is this case actually "install systemd-boot pretending to be grub" so it gets chainloaded by shim? That looks like what this is.

yes, that's what it is. We will probably hardlink the actual systemd-boot binary at /usr/lib/efi/systemd-boot/** so that all bootloader live in the same place

OK right, and we're doing the same with grub-cc?

yes

but I think we do ultimately need to handle non-shim cases too

We do. On a call with @travier we decided to handle that case later

@travier travier Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the EFI binary loaded by shim is hardcoded in shim: https://src.fedoraproject.org/rpms/shim-unsigned-x64/blob/rawhide/f/shim-unsigned-x64.spec#_154

There are options to change it but they have a lot of downsides: #1080 (comment)

Pretending to be grub$arch.efi is apparently the simplest option.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We settled (at least for now) on leaving standalone systemd-boot (or potentially standalone grub-cc) support to systemd's bootctl as we don't need all the complexity from bootupd for those cases: #1080 (comment)

@Rolv-Apneseth Rolv-Apneseth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions and suggestions but wow, this is cool

;;
grub)
EFI_DIR_NAME=grub2
;;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a failure for the default case?

Comment thread src/backend/statefile.rs
use std::io::prelude::*;
use std::path::Path;

fn parse_statefile(statusf: cap_std::fs::File) -> Result<Option<SavedState>> {

@Rolv-Apneseth Rolv-Apneseth Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about doing something like:

fn parse_statefile(statusf: cap_std::fs::File) -> Result<SavedState> {
    let mut bufr = BufReader::new(statusf);
    let mut s = String::new();
    bufr.read_to_string(&mut s)?;
    let state: serde_json::Result<SavedState> = serde_json::from_str(s.as_str());

    match state {
        Ok(s) => Ok(s),
        Err(orig_err) => {
            let legacy_state: serde_json::Result<crate::model_legacy::SavedState01> =
                serde_json::from_str(s.as_str());
            legacy_state
                .map(crate::model_legacy::SavedState01::upconvert)
                .map_err(|_| orig_err.into())
        }
    }
}

And at the call sites:

root.open_optional(&path)
    .map_err(Into::into)
    .and_then(|o| o.map(parse_statefile).transpose())

Comment thread src/backend/statefile.rs
// According to BLS, the ESP should be mounted at /boot or /boot/efi
// which the following method already checks
let mounted = efi
.ensure_mounted_esp(Path::new("/"), Path::new(&esp.path()))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using root_path in the case of an install to e.g. /var/mnt?

Comment thread src/backend/statefile.rs
root_path: impl AsRef<Path>,
bootloader: Bootloader,
) -> Result<()> {
let saved_state = SavedState::load_from_disk(&root_path, Some(bootloader))?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_from_disk calls device.find_first_colocated_esp() for grub-cc and systemd-boot - do we need to handle the case where no ESP is found?

Comment thread src/backend/statefile.rs
// Do the sync before unmount
fsfreeze_thaw_cycle(dir.reopen_as_ownedfd()?)?;
drop(dir);
efi.unmount().context("unmount after update")?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended that this unmounts even when was_mounted = true due to how Efi::unmount sets was_mounted = false?

Comment thread src/bootloader.rs
} else {
Bootloader::Grub
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
Some(info) => match info.to_lowercase() {
i if i.contains("grub cc") => Bootloader::GrubCC,
i if i.contains("systemd-boot") => Bootloader::Systemd,
_ => Bootloader::Grub,
},

Comment thread src/bootupd.rs
//
// We can't install GrubCC for BIOS as it's not supported
// So we just default to installing GrubCC
(Some(_), Some(eb)) => eb,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(Some(_), Some(eb)) => eb,
(_, Some(eb)) => eb,

Comment thread src/model.rs

/// Returns bootloaders are available for install
#[cfg(efi_arch)]
pub(crate) fn num_bootloader_available(&self) -> Vec<Bootloader> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be renamed since it doesn't actually return a number

Comment thread src/model.rs
Comment on lines +58 to +63
#[cfg(efi_arch)]
pub(crate) fn bootloader_available(&mut self, bootloader: Bootloader) -> bool {
self.version
.split(",")
.any(|v| v.starts_with(bootloader.efi_component_name()))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg(efi_arch)]
pub(crate) fn bootloader_available(&mut self, bootloader: Bootloader) -> bool {
self.version
.split(",")
.any(|v| v.starts_with(bootloader.efi_component_name()))
}
#[cfg(efi_arch)]
pub(crate) fn is_bootloader_available(&self, bootloader: Bootloader) -> bool {
self.version
.split(',')
.any(|v| v.starts_with(bootloader.efi_component_name()))
}

Comment thread Dockerfile.bls

. /etc/os-release

# Create the expected directory structure at /usr/lib/efi/grub-cc/<evr>/EFI/fedora/

@Rolv-Apneseth Rolv-Apneseth Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was meant to be the systemd-boot path

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants