Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+159 −61
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/bios.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,16 @@ impl Bios {
}

// check bios_boot partition on gpt type disk
#[cfg(target_arch = "x86_64")]
fn get_bios_boot_partition(&self) -> Option<String> {
match blockdev::get_single_device("/") {
Ok(device) => {
let bios_boot_part =
blockdev::get_bios_boot_partition(&device).expect("get bios_boot part");
return bios_boot_part;
}
Err(e) => log::warn!("Get error: {}", e),
Err(e) => log::warn!("Get single device: {}", e),
}
log::debug!("Not found any bios_boot partition");
None
}
}
Expand Down Expand Up @@ -149,7 +149,7 @@ impl Component for Bios {

fn query_adopt(&self) -> Result<Option<Adoptable>> {
#[cfg(target_arch = "x86_64")]
if crate::efi::is_efi_booted()? && self.get_bios_boot_partition().is_none() {
if self.get_bios_boot_partition().is_none() {
log::debug!("Skip BIOS adopt");
return Ok(None);
}
Expand Down
2 changes: 0 additions & 2 deletions src/blockdev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ pub fn get_single_device<P: AsRef<Path>>(target_root: P) -> Result<String> {

/// Find esp partition on the same device
/// using sfdisk to get partitiontable
#[allow(dead_code)]
pub fn get_esp_partition(device: &str) -> Result<Option<String>> {
const ESP_TYPE_GUID: &str = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B";
let device_info: PartitionTable = bootc_blockdev::partitions_of(Utf8Path::new(device))?;
Expand All @@ -52,7 +51,6 @@ pub fn get_esp_partition(device: &str) -> Result<Option<String>> {
}

/// Find all ESP partitions on the devices with mountpoint boot
#[allow(dead_code)]
pub fn find_colocated_esps<P: AsRef<Path>>(target_root: P) -> Result<Vec<String>> {
// first, get the parent device
let devices = get_devices(&target_root).with_context(|| "while looking for colocated ESPs")?;
Expand Down
167 changes: 111 additions & 56 deletions src/efi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rustix::fd::BorrowedFd;
use walkdir::WalkDir;
use widestring::U16CString;

use crate::blockdev;
use crate::filetree;
use crate::model::*;
use crate::ostreeutil;
Expand Down Expand Up @@ -57,28 +58,7 @@ pub(crate) struct Efi {
}

impl Efi {
fn esp_path(&self) -> Result<PathBuf> {
self.ensure_mounted_esp(Path::new("/"))
.map(|v| v.join("EFI"))
}

fn open_esp_optional(&self) -> Result<Option<openat::Dir>> {
if !is_efi_booted()? && self.get_esp_device().is_none() {
log::debug!("Skip EFI");
return Ok(None);
}
let sysroot = openat::Dir::open("/")?;
let esp = sysroot.sub_dir_optional(&self.esp_path()?)?;
Ok(esp)
}

fn open_esp(&self) -> Result<openat::Dir> {
self.ensure_mounted_esp(Path::new("/"))?;
let sysroot = openat::Dir::open("/")?;
let esp = sysroot.sub_dir(&self.esp_path()?)?;
Ok(esp)
}

// Get esp device via legacy
fn get_esp_device(&self) -> Option<PathBuf> {
let esp_devices = [COREOS_ESP_PART_LABEL, ANACONDA_ESP_PART_LABEL]
.into_iter()
Expand All @@ -93,11 +73,26 @@ impl Efi {
return esp_device;
}

pub(crate) fn ensure_mounted_esp(&self, root: &Path) -> Result<PathBuf> {
let mut mountpoint = self.mountpoint.borrow_mut();
// Get esp device list on all devices
fn get_esp_devices(&self) -> Option<Vec<String>> {
let mut esp_devices = vec![];
if let Some(esp_device) = self.get_esp_device() {
esp_devices.push(esp_device.to_string_lossy().into_owned());
} else {
esp_devices = blockdev::find_colocated_esps("/").expect("get esp devices");
};
Comment on lines +79 to +83
Copy link
Contributor

@champtar champtar Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Have you considered / tried the case with 2 disks / 2 separate OS ? (we do that in our lab, 2 or even 3 rpm-ostree based OS, each one on one disk, all installed via anaconda)
I'm asking because quickly looking at the code, I would use find_colocated_esps("/") first and get_esp_device() as a fallback.

The safest might even be:

  1. mounted ESP(s)
  2. colocated ESP(s)
  3. get_esp_device

If you have a bootc installed OS, and on a second disk an anaconda installed OS (not even ostree based), right now you are going to always pick the anaconda ESP

Copy link
Member Author

@HuijingHei HuijingHei Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would use find_colocated_esps("/") first and get_esp_device() as a fallback.

  • get_esp_device() will find CoreOS ESP label /dev/disk/by-partlabel/EFI-SYSTEM, or Anaconda ESP label /dev/disk/by-partlabel/EFI\\x20System\\x20Partition

  • find_colocated_esps("/") will list ESP partitions on the devices with mountpoint /boot, but it does not mean it is always mounted, for example, coreos does not mount esp after booted, see doc

Consider that on single disk, can easily get ESP label /dev/disk/by-partlabel/EFI-SYSTEM via get_esp_device(), but on multiple disks, it would be /dev/disk/by-label/esp-1 & /dev/disk/by-label/esp-2, does this make sense?

Copy link
Member Author

@HuijingHei HuijingHei Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The safest might even be:

mounted ESP(s)
colocated ESP(s)
get_esp_device

What I did: when running update, get_esp_devices() would list the all esp devices, then in the loop device will check mounted ESP first, if not, will mount the esp then upgrade. I think you meant that need to check mounted firstly in the beginning, will try that, thanks!

Edit: For multiple disks, we still need the loop for each esp device. Will keep it as it is, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

get_esp_devices() would list the all esp devices

Maybe I don't understand rust code, but for me it first call get_esp_device() which returns 1 device based on a weak heuristic, if that fails then it tries to find multiple devices

Here 3 test cases, second OS (sdb) booted, running get_esp_devices():
sda: normal anaconda install
sdb: ESP not mounted, not using anaconda/coreos label
-> returns sda ?

sda: normal anaconda install
sdb & sdc: raid 1 mirrored, ESP not mounted, not using anaconda/coreos label
-> returns sda ?

sda: normal coreos install
sdb: also a coreos install, ESP not mounted
-> not sure which PARTLABEL wins, likely random at each boot

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not have such env, but IMU, if second OS (sdb) booted, sda will not be mounted from the booted OS, in this case, we do not scan sda.

sda: normal anaconda install
sdb: ESP not mounted, not using anaconda/coreos label

skip sda, and get_esp_devices() will call find_colocated_esps("/") to find the mount point /boot device, then get ESP part on the same device

sda: normal anaconda install
sdb & sdc: raid 1 mirrored, ESP not mounted, not using anaconda/coreos label

skip sda, this is the case that we need to resolve actually, find_colocated_esps("/") to find the mount point /boot device which is /dev/md126, then need to get the backing devices list sdb & sdc and find ESP part on each device

sda: normal coreos install
sdb: also a coreos install, ESP not mounted

When I tried to boot VM with 2 coreos, failed with:

[    9.373774] rdcore[957]: Error: System has 2 devices with a filesystem labeled 'boot': ["/dev/vdb3", "/dev/vda3"]
[FAILED] Failed to start CoreOS Ignition Ensure Unique Boot Filesystem.

So we can skip this env.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @travier to confirm about my reply, not sure if my understanding is correct, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

On a server with 3 disks / 3 installs side by side

# udevadm info /dev/sda1 | grep '^S: disk/by-partlabel'
S: disk/by-partlabel/EFI\x20System\x20Partition
# udevadm info /dev/sdb1 | grep '^S: disk/by-partlabel'
S: disk/by-partlabel/EFI\x20System\x20Partition
# udevadm info /dev/sdc1 | grep '^S: disk/by-partlabel'
S: disk/by-partlabel/EFI\x20System\x20Partition

All 3 wants to have the by-partlabel symlink, only the first one gets it, I'm not aware of any ordering/priorities

In case 1&2 we do not mount sda, but udev definitely scans it

Copy link
Member Author

Choose a reason for hiding this comment

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

All 3 wants to have the by-partlabel symlink

If booted sda, then it will find ESP part in sda, I do not think it will retrieve all the devices.

if !esp_devices.is_empty() {
return Some(esp_devices);
}
return None;
}

fn check_mounted_esp<P: AsRef<Path>>(&self, root: P) -> Result<Option<PathBuf>> {
let mountpoint = self.mountpoint.borrow_mut();
if let Some(mountpoint) = mountpoint.as_deref() {
return Ok(mountpoint.to_owned());
return Ok(Some(mountpoint.to_owned()));
}
let root = root.as_ref();
for &mnt in ESP_MOUNTS {
let mnt = root.join(mnt);
if !mnt.exists() {
Expand All @@ -109,13 +104,23 @@ impl Efi {
continue;
}
util::ensure_writable_mount(&mnt)?;
log::debug!("Reusing existing {mnt:?}");
return Ok(mnt);
log::debug!("Reusing existing mount point {mnt:?}");
return Ok(Some(mnt));
}
Ok(None)
}

pub(crate) fn ensure_mounted_esp<P: AsRef<Path>>(
&self,
root: P,
esp_device: &str,
) -> Result<PathBuf> {
let mut mountpoint = self.mountpoint.borrow_mut();
if let Some(mountpoint) = mountpoint.as_deref() {
return Ok(mountpoint.to_owned());
}

let esp_device = self
.get_esp_device()
.ok_or_else(|| anyhow::anyhow!("Failed to find ESP device"))?;
let root = root.as_ref();
for &mnt in ESP_MOUNTS.iter() {
let mnt = root.join(mnt);
if !mnt.exists() {
Expand All @@ -137,6 +142,7 @@ impl Efi {
if let Some(mount) = self.mountpoint.borrow_mut().take() {
Command::new("umount")
.arg(&mount)
.arg("-l")
.run()
.with_context(|| format!("Failed to unmount {mount:?}"))?;
log::trace!("Unmounted");
Expand Down Expand Up @@ -243,8 +249,7 @@ impl Component for Efi {
}

fn query_adopt(&self) -> Result<Option<Adoptable>> {
let esp = self.open_esp_optional()?;
if esp.is_none() {
if self.get_esp_devices().is_none() {
log::trace!("No ESP detected");
return Ok(None);
};
Expand All @@ -267,16 +272,32 @@ impl Component for Efi {
anyhow::bail!("Failed to find adoptable system")
};

let esp = self.open_esp()?;
validate_esp(&esp)?;
let updated = sysroot
.sub_dir(&component_updatedirname(self))
.context("opening update dir")?;
let updatef = filetree::FileTree::new_from_dir(&updated).context("reading update dir")?;
// For adoption, we should only touch files that we know about.
let diff = updatef.relative_diff_to(&esp)?;
log::trace!("applying adoption diff: {}", &diff);
filetree::apply_diff(&updated, &esp, &diff, None).context("applying filesystem changes")?;
let esp_devices = self
.get_esp_devices()
.expect("get esp devices before adopt");
let sysroot = sysroot.recover_path()?;

for esp_dev in esp_devices {
let dest_path = if let Some(dest_path) = self.check_mounted_esp(&sysroot)? {
dest_path.join("EFI")
} else {
self.ensure_mounted_esp(&sysroot, &esp_dev)?.join("EFI")
};

let esp = openat::Dir::open(&dest_path).context("opening EFI dir")?;
validate_esp(&esp)?;

// For adoption, we should only touch files that we know about.
let diff = updatef.relative_diff_to(&esp)?;
log::trace!("applying adoption diff: {}", &diff);
filetree::apply_diff(&updated, &esp, &diff, None)
.context("applying filesystem changes")?;
self.unmount().context("unmount after adopt")?;
}
Ok(InstalledContent {
meta: updatemeta.clone(),
filetree: Some(updatef),
Expand All @@ -298,9 +319,17 @@ impl Component for Efi {
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))?;
let destdir = if let Some(destdir) = self.check_mounted_esp(dest_root)? {
destdir
} else {
let esp_device = self
.get_esp_device()
.ok_or_else(|| anyhow::anyhow!("Failed to find ESP device"))?;
let esp_device = esp_device.to_str().unwrap();
self.ensure_mounted_esp(dest_root, esp_device)?
};

let destd = &openat::Dir::open(destdir)
let destd = &openat::Dir::open(&destdir)
.with_context(|| format!("opening dest dir {}", destdir.display()))?;
validate_esp(destd)?;

Expand Down Expand Up @@ -339,12 +368,25 @@ impl Component for Efi {
.context("opening update dir")?;
let updatef = filetree::FileTree::new_from_dir(&updated).context("reading update dir")?;
let diff = currentf.diff(&updatef)?;
self.ensure_mounted_esp(Path::new("/"))?;
let destdir = self.open_esp().context("opening EFI dir")?;
validate_esp(&destdir)?;
log::trace!("applying diff: {}", &diff);
filetree::apply_diff(&updated, &destdir, &diff, None)
.context("applying filesystem changes")?;
let esp_devices = self
.get_esp_devices()
.context("get esp devices when running update")?;
let sysroot = sysroot.recover_path()?;

for esp in esp_devices {
let dest_path = if let Some(dest_path) = self.check_mounted_esp(&sysroot)? {
dest_path.join("EFI")
} else {
self.ensure_mounted_esp(&sysroot, &esp)?.join("EFI")
};

let destdir = openat::Dir::open(&dest_path).context("opening EFI dir")?;
validate_esp(&destdir)?;
log::trace!("applying diff: {}", &diff);
filetree::apply_diff(&updated, &destdir, &diff, None)
.context("applying filesystem changes")?;
self.unmount().context("unmount after update")?;
}
let adopted_from = None;
Ok(InstalledContent {
meta: updatemeta,
Expand Down Expand Up @@ -392,24 +434,37 @@ impl Component for Efi {
}

fn validate(&self, current: &InstalledContent) -> Result<ValidationResult> {
if !is_efi_booted()? && self.get_esp_device().is_none() {
let esp_devices = self.get_esp_devices();
if !is_efi_booted()? && esp_devices.is_none() {
return Ok(ValidationResult::Skip);
}
let currentf = current
.filetree
.as_ref()
.ok_or_else(|| anyhow::anyhow!("No filetree for installed EFI found!"))?;
self.ensure_mounted_esp(Path::new("/"))?;
let efidir = self.open_esp()?;
let diff = currentf.relative_diff_to(&efidir)?;

let mut errs = Vec::new();
for f in diff.changes.iter() {
errs.push(format!("Changed: {}", f));
}
for f in diff.removals.iter() {
errs.push(format!("Removed: {}", f));
let esps = esp_devices.ok_or_else(|| anyhow::anyhow!("No esp device found!"))?;
let dest_root = Path::new("/");
for esp_dev in esps.iter() {
let dest_path = if let Some(dest_path) = self.check_mounted_esp(dest_root)? {
dest_path.join("EFI")
} else {
self.ensure_mounted_esp(dest_root, &esp_dev)?.join("EFI")
};

let efidir = openat::Dir::open(dest_path.as_path())?;
let diff = currentf.relative_diff_to(&efidir)?;

for f in diff.changes.iter() {
errs.push(format!("Changed: {}", f));
}
for f in diff.removals.iter() {
errs.push(format!("Removed: {}", f));
}
assert_eq!(diff.additions.len(), 0);
self.unmount().context("unmount after validate")?;
}
assert_eq!(diff.additions.len(), 0);
if !errs.is_empty() {
Ok(ValidationResult::Errors(errs))
} else {
Expand Down
7 changes: 7 additions & 0 deletions tests/kola/raid1/config.bu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
variant: fcos
version: 1.5.0
boot_device:
mirror:
devices:
- /dev/vda
- /dev/vdb
1 change: 1 addition & 0 deletions tests/kola/raid1/data/libtest.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../data/libtest.sh
37 changes: 37 additions & 0 deletions tests/kola/raid1/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#!/bin/bash
## kola:
## # additionalDisks is only supported on qemu.
## platforms: qemu
## # Root reprovisioning requires at least 4GiB of memory.
## minMemory: 4096
## # Linear RAID is setup on these disks.
## additionalDisks: ["10G"]
## # This test includes a lot of disk I/O and needs a higher
## # timeout value than the default.
## timeoutMin: 15
## description: Verify updating multiple EFIs with RAID 1 works.

set -xeuo pipefail

# shellcheck disable=SC1091
. "$KOLA_EXT_DATA/libtest.sh"

srcdev=$(findmnt -nvr /sysroot -o SOURCE)
[[ ${srcdev} == "/dev/md126" ]]

blktype=$(lsblk -o TYPE "${srcdev}" --noheadings)
[[ ${blktype} == "raid1" ]]

fstype=$(findmnt -nvr /sysroot -o FSTYPE)
[[ ${fstype} == "xfs" ]]
ok "source is XFS on RAID1 device"


mount -o remount,rw /boot

rm -f -v /boot/bootupd-state.json

bootupctl adopt-and-update | grep "Adopted and updated: EFI"

bootupctl status | grep "Component EFI"
ok "bootupctl adopt-and-update supports multiple EFIs on RAID1"
Loading