efi: rewrite `ensure_mounted_esp()` #932

HuijingHei · 2025-05-21T10:01:35Z

This is the preparation for #855.

The main target for this PR is: "efi: rewrite ensure_mounted_esp()", but find there are much to update consequently.

efi: rewrite ensure_mounted_esp()

Split into 2 parts:

get_mounted_esp() to get mounted point esp
mount_esp_device() to mount the passed esp device, return
the mount point

Then in ensure_mounted_esp() call the above 2 functions:
firstly check if esp is already mounted, if not, mount the passed
esp device.

efi: update install() and update() according to split change

adopt_update: pass RootContext as parameter

efi: update validate() according to the split change

openshift-ci · 2025-05-21T10:01:39Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

src/efi.rs

HuijingHei · 2025-05-22T13:56:44Z

Ready for review now, thanks!

cgwalters

Looks reasonable to me overall, I just have some nits

src/efi.rs

cgwalters

It looks like the cosa build is legitimately failing.

HuijingHei · 2025-05-26T11:22:16Z

Seems the try_fold() iter all items without return at the first match, ~~checking more~~ fix this with return early.

HuijingHei · 2025-05-26T12:39:23Z

/test test

Split into 2 parts: - `get_mounted_esp()` to get mounted point esp - `mount_esp_device()` to mount the passed esp device, return the mount point Then in `ensure_mounted_esp()` call the above 2 functions: firstly check if esp is already mounted, if not, mount the passed esp device.

cgwalters · 2025-05-27T15:34:38Z

src/efi.rs

        log::debug!("Found metadata {}", meta.version);
        let srcdir_name = component_updatedirname(self);
        let ft = crate::filetree::FileTree::new_from_dir(&src_root.sub_dir(&srcdir_name)?)?;
-        let destdir = &self.ensure_mounted_esp(Path::new(dest_root))?;


Why can't we keep calling this one to do the "mount if not mounted" logic?

Good question. After change, the function ensure_mounted_esp() will check if esp is already mounted, else mount the passed esp device, return the mount point and append path /EFI.

For installation, during bootc install to-disk, the esp device is not mounted and will call get_esp_device() to get esp device via /dev/disk/by-partlabel, but when running cosa build, not sure if it would also call bootc, find that esp device is already mounted, and can not find esp device via /dev/disk/by-partlabel (do not why). Before I use esp_device = self.get_esp_device() before ensure_mounted_esp(), but cosa build failed.

If failed get_esp_device() (via /dev/disk/by-partlabel), change to get_esp_partition(device) (via sfdisk).

According to comment, use blockdev to find the partition, instead of partlabel.

src/blockdev.rs

src/bios.rs

src/bootupd.rs

src/component.rs

src/bios.rs

src/efi.rs

dustymabe

LGTM - a few small suggestions

src/efi.rs

instead of `partlabel` Copy Dusty's comment from https://github.com/coreos/bootupd/pull/932/files#r2112567301: During `bootc install to-disk` the esp device is not mounted but we can call `get_esp_device()` and it will find the device via the `/dev/disk/by-partlabel/` symlink. When running `cosa build` and building a disk image using `OSBuild` the EFI partitions is mounted but no udev `/dev/disk/by-partlabel/` symlinks exist because it's just a chroot and not a full linux environment. In that case fallback to using `blockdev` to find the partition. Even better, get rid of `self.get_esp_device()` altogether and just use `blockdev`

dustymabe

LGTM - merge when ready

openshift-ci bot added the do-not-merge/work-in-progress label May 21, 2025

HuijingHei force-pushed the split-ensure-mounted-esp branch 5 times, most recently from aacbb09 to d34bf3b Compare May 21, 2025 12:49

HuijingHei changed the title ~~Split ensure mounted esp~~ efi: rewrite ensure_mounted_esp() May 21, 2025

HuijingHei marked this pull request as ready for review May 21, 2025 13:12

openshift-ci bot removed the do-not-merge/work-in-progress label May 21, 2025

HuijingHei added the do-not-merge/work-in-progress label May 21, 2025

HuijingHei commented May 21, 2025

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

HuijingHei mentioned this pull request May 22, 2025

efi: support updating multiple EFIs in mirrored setups (RAID1) #855

Merged

HuijingHei force-pushed the split-ensure-mounted-esp branch from 314c7ba to ea09938 Compare May 22, 2025 03:45

HuijingHei requested a review from cgwalters May 22, 2025 03:46

HuijingHei removed the do-not-merge/work-in-progress label May 22, 2025

cgwalters reviewed May 23, 2025

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

src/efi.rs Outdated Show resolved Hide resolved

HuijingHei force-pushed the split-ensure-mounted-esp branch 2 times, most recently from 9c9c6c7 to a85552e Compare May 23, 2025 14:32

cgwalters reviewed May 23, 2025

View reviewed changes

HuijingHei force-pushed the split-ensure-mounted-esp branch 2 times, most recently from 3ff06fe to 950feab Compare May 26, 2025 12:04

HuijingHei force-pushed the split-ensure-mounted-esp branch from 950feab to ed38ae0 Compare May 26, 2025 13:23

HuijingHei added 4 commits May 26, 2025 22:26

efi: update install() and update() according to split change

b648fc0

adopt_update: pass RootContext as parameter

fe53bab

efi: update validate() according to the split change

a825409

HuijingHei force-pushed the split-ensure-mounted-esp branch from ed38ae0 to 53ea47f Compare May 26, 2025 14:26

cgwalters reviewed May 27, 2025

View reviewed changes

Back to use for loop in get_mounted_esp() for error propagation

9ef528d

HuijingHei force-pushed the split-ensure-mounted-esp branch 2 times, most recently from c17b92c to e0cfb73 Compare May 28, 2025 03:16

dustymabe reviewed May 28, 2025

View reviewed changes

Update sysroot to rootcxt to make the var consistent

53db0e9

HuijingHei force-pushed the split-ensure-mounted-esp branch 2 times, most recently from d2716e4 to 5998d92 Compare May 29, 2025 02:40

dustymabe approved these changes May 29, 2025

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

src/efi.rs Outdated Show resolved Hide resolved

HuijingHei force-pushed the split-ensure-mounted-esp branch from 5998d92 to e4b7a71 Compare May 29, 2025 14:50

HuijingHei force-pushed the split-ensure-mounted-esp branch from e4b7a71 to f6ef3d1 Compare May 29, 2025 14:54

dustymabe approved these changes May 29, 2025

View reviewed changes

HuijingHei merged commit 50adce8 into coreos:main Jun 1, 2025
12 checks passed

HuijingHei deleted the split-ensure-mounted-esp branch June 1, 2025 02:22

efi: rewrite ensure_mounted_esp() #932

efi: rewrite ensure_mounted_esp() #932

Uh oh!

Conversation

HuijingHei commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented May 21, 2025

Uh oh!

Uh oh!

HuijingHei commented May 22, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

HuijingHei commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuijingHei commented May 26, 2025

Uh oh!

cgwalters May 27, 2025

Choose a reason for hiding this comment

Uh oh!

HuijingHei May 28, 2025

Choose a reason for hiding this comment

Uh oh!

HuijingHei May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuijingHei May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dustymabe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dustymabe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

efi: rewrite `ensure_mounted_esp()` #932

efi: rewrite `ensure_mounted_esp()` #932

HuijingHei commented May 21, 2025 •

edited

Loading

HuijingHei commented May 26, 2025 •

edited

Loading

HuijingHei May 28, 2025 •

edited

Loading