Allow to skip BIOS component installation for x86_64 #1010

martinezjavier · 2025-09-25T12:47:32Z

The bootc images generated by automotive-image-builder for x86_64 are only meant to be booted using an EFI firmware and the ukiboot boot protocol.

The images don't support neither legacy BIOS nor grub2. But currently, the osbuild manifests used by automotive-image-builder must include the grub2 packages just to avoid bootupd to fail when attempting to generate the update metadata and install the listed components for x86_64.

This merge request relaxes this requirement, making the BIOS component metadata update and installation optional. Instead of bailing out if the expected grub2 tools are not present or there is no update metadata for the BIOS component.

Currently, the `bootupctl backend generate-update-metadata` command tries to generate an update layout for each component, and it bails out if one of the components can't be installed. There are systems in which some of the bootupd components are not needed (e.g.: x86_64 EFI-only systems). But if one of the requirements for one of the components is not meet (e.g.: grub2-install is missing for BIOS), this will lead to a fatal error instead of just skipping the component that is not supported. To prevent this, let's make the update layout generation to be optional. Signed-off-by: Javier Martinez Canillas <[email protected]>

openshift-ci · 2025-09-25T12:47:43Z

Hi @martinezjavier. 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.

gemini-code-assist

Code Review

This pull request refactors the boot component handling to allow skipping BIOS component installation when grub2 is not present, which is useful for EFI-only images. The changes correctly make generate_update_metadata return an Option and handle this in the calling code.

My review includes two main points:

In src/bootupd.rs, there's a potential bug where errors from query_update are silently ignored. I've suggested a fix to properly propagate these errors.
In src/bios.rs, I've recommended using the log crate instead of println! for a diagnostic message to improve logging consistency and control.

Overall, the changes are in the right direction to achieve the goal. Addressing these points will improve the robustness and maintainability of the code.

gemini-code-assist · 2025-09-25T12:49:12Z

src/bootupd.rs

+        if let Ok(None) = component.query_update(&source_root_dir) {
+            println!(
+                "Skip installing component {} without update metadata",
+                component.name()
+            );
+            continue;
+        }


The use of if let Ok(None) = ... here can silently swallow errors. If component.query_update() returns an Err, the condition is false, and execution proceeds to component.install(), which will likely fail again but with a potentially more obscure error message. This makes debugging harder. It's better to handle the Err case explicitly, for instance by using the ? operator to propagate the error.

Suggested change

if let Ok(None) = component.query_update(&source_root_dir) {

println!(

"Skip installing component {} without update metadata",

component.name()

);

continue;

}

if component.query_update(&source_root_dir)?.is_none() {

println!(

"Skip installing component {} without update metadata",

component.name()

);

continue;

}

Did the suggested change.

gemini-code-assist · 2025-09-25T12:49:12Z

src/bios.rs

+            println!("Failed to find {:?}", grub_install);
+            return Ok(None);


Using println! for this diagnostic message can add noise to the standard output, which is primarily used for user-facing progress messages. It's better to use the log crate for such messages. This provides more control over verbosity and separates diagnostic output from application output. The calling function in bootupd.rs already prints a user-friendly message about skipping the component.

Note: You will need to add use log; at the top of the file.

Suggested change

println!("Failed to find {:?}", grub_install);

return Ok(None);

log::info!("Skipping BIOS metadata generation; grub2-install not found at {:?}", grub_install);

return Ok(None);

I used the println!() macro to be consistent with how other similar messages were printed.

cgwalters · 2025-09-25T13:08:59Z

Thanks for filing this! This also relates to #978

Now that update metadata generation is optional for some components, skip installing the components that don't have an update metadata.

Instead of making it a fatal error, just make the component to be skipped. This allows to generate the update metadata for other components, instead of failing due not being able to generate the metadata for BIOS component.

martinezjavier · 2025-09-25T16:39:06Z

Thanks for filing this! This also relates to #978

Note that this MR is only about handling the legacy BIOS case since for UEFI we have a different workaround. But yes, decoupling the EFI component from grub2 (and shim?) would be ideal too.

cgwalters

Thanks! One minor nit we can just change post merge.

cgwalters · 2025-09-26T19:52:38Z

src/bios.rs

        let grub_install = Path::new(sysroot_path).join(GRUB_BIN);
        if !grub_install.exists() {
-            bail!("Failed to find {:?}", grub_install);
+            println!("Failed to find {:?}", grub_install);


How about: Skipping, executable not found: {:?} (it's not a failure) - as you've used elsewhere in the PR

cgwalters · 2025-09-26T20:06:18Z

src/bootupd.rs

            continue;
        }

+        // skip components that don't have an update metadata


Hmm this one is interesting. I think actually the problem is we're iterating over the static list of components, when actually I think we should just accept whatever the updated components in the updates set are to start with or so.

Although it raises a different point - it would probably make sense in the future to handle the case where a component is removed. (e.g. an OS update drops out BIOS support and we would then in theory clear out the BIOS partition e.g.)

And to do that, we do need to iterate over the components we know (as this is doing) and intersect them with the update set.

cgwalters · 2025-09-26T20:06:43Z

/ok-to-test

openshift-ci bot added the needs-ok-to-test label Sep 25, 2025

gemini-code-assist bot reviewed Sep 25, 2025

View reviewed changes

martinezjavier added 2 commits September 25, 2025 18:17

component: Skip installing components without update metadata

81edbae

Now that update metadata generation is optional for some components, skip installing the components that don't have an update metadata.

bios: Don't bail out if grub2-install is not present

f9de0fd

Instead of making it a fatal error, just make the component to be skipped. This allows to generate the update metadata for other components, instead of failing due not being able to generate the metadata for BIOS component.

martinezjavier force-pushed the optional-update-metadata branch from 6afe758 to f9de0fd Compare September 25, 2025 16:23

cgwalters approved these changes Sep 26, 2025

View reviewed changes

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Sep 26, 2025

cgwalters enabled auto-merge September 26, 2025 20:06

cgwalters merged commit 2352767 into coreos:main Sep 26, 2025
12 checks passed

		println!("Failed to find {:?}", grub_install);
		return Ok(None);

Allow to skip BIOS component installation for x86_64 #1010

Allow to skip BIOS component installation for x86_64 #1010

Conversation

martinezjavier commented Sep 25, 2025

Uh oh!

openshift-ci bot commented Sep 25, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

martinezjavier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

martinezjavier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Sep 25, 2025

Uh oh!

martinezjavier commented Sep 25, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Sep 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants