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

src/bios.rs

@@ -95,21 +95,6 @@ impl Bios {
         }
         Ok(())
     }
-
-    // check bios_boot partition on gpt type disk
-    #[allow(dead_code)]
-    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),
-        }
-        log::debug!("Not found any bios_boot partition");
-        None
-    }
 }
 
 impl Component for Bios {
@@ -167,18 +152,11 @@ impl Component for Bios {
             return Ok(None);
         };
 
-        let mut parent_devices = rootcxt.devices.iter();
-        let Some(parent) = parent_devices.next() else {
-            anyhow::bail!("Failed to find parent device");
-        };
-
-        if let Some(next) = parent_devices.next() {
-            anyhow::bail!(
-                "Found multiple parent devices {parent} and {next}; not currently supported"
-            );
+        for parent in rootcxt.devices.iter() {
+            self.run_grub_install(rootcxt.path.as_str(), &parent)?;
+            log::debug!("Installed grub modules on {parent}");
         }
-        self.run_grub_install(rootcxt.path.as_str(), &parent)?;
-        log::debug!("Installed grub modules on {parent}");
+
         Ok(Some(InstalledContent {
             meta: update.clone(),
             filetree: None,
@@ -195,20 +173,11 @@ impl Component for Bios {
             .query_update(&rootcxt.sysroot)?
             .expect("update available");
 
-        let mut parent_devices = rootcxt.devices.iter();
-        let Some(parent) = parent_devices.next() else {
-            anyhow::bail!("Failed to find parent device");
-        };
-
-        if let Some(next) = parent_devices.next() {
-            anyhow::bail!(
-                "Found multiple parent devices {parent} and {next}; not currently supported"
-            );
+        for parent in rootcxt.devices.iter() {
+            self.run_grub_install(rootcxt.path.as_str(), &parent)?;
+            log::debug!("Installed grub modules on {parent}");
         }
 
-        self.run_grub_install(rootcxt.path.as_str(), &parent)?;
-        log::debug!("Install grub modules on {parent}");
-
         let adopted_from = None;
         Ok(InstalledContent {
             meta: updatemeta,

src/blockdev.rs

@@ -1,39 +1,39 @@
 use camino::Utf8Path;
 use std::path::Path;
 
-use anyhow::{bail, Context, Result};
+use anyhow::{Context, Result};
 use bootc_blockdev::PartitionTable;
 use fn_error_context::context;
 
-#[context("get parent devices from mount point boot")]
+#[context("get parent devices from mount point boot or sysroot")]
 pub fn get_devices<P: AsRef<Path>>(target_root: P) -> Result<Vec<String>> {
     let target_root = target_root.as_ref();
-    let bootdir = target_root.join("boot");
-    if !bootdir.exists() {
-        bail!("{} does not exist", bootdir.display());
+    let mut source = None;
+
+    for path in ["boot", "sysroot"] {
+        let target_path = target_root.join(path);
+        if !target_path.exists() {
+            continue;
+        }
+
+        let target_dir = openat::Dir::open(&target_path)
+            .with_context(|| format!("Opening {}", target_path.display()))?;
+        if let Ok(fsinfo) = crate::filesystem::inspect_filesystem(&target_dir, ".") {
+            source = Some(fsinfo.source);
+            break;
+        }
     }
-    let bootdir = openat::Dir::open(&bootdir)?;
-    // Run findmnt to get the source path of mount point boot
-    let fsinfo = crate::filesystem::inspect_filesystem(&bootdir, ".")?;
-    // Find the parent devices of the source path
-    let parent_devices = bootc_blockdev::find_parent_devices(&fsinfo.source)
-        .with_context(|| format!("while looking for backing devices of {}", fsinfo.source))?;
-    log::debug!("Find parent devices: {parent_devices:?}");
-    Ok(parent_devices)
-}
 
-// Get single device for the target root
-#[allow(dead_code)]
-pub fn get_single_device<P: AsRef<Path>>(target_root: P) -> Result<String> {
-    let mut devices = get_devices(&target_root)?.into_iter();
-    let Some(parent) = devices.next() else {
-        anyhow::bail!("Failed to find parent device");
+    let source = match source {
+        Some(s) => s,
+        None => anyhow::bail!("Failed to inspect filesystem from boot or sysroot"),
     };
 
-    if let Some(next) = devices.next() {
-        anyhow::bail!("Found multiple parent devices {parent} and {next}; not currently supported");
-    }
-    Ok(parent)
+    // Find the parent devices of the source path
+    let parent_devices = bootc_blockdev::find_parent_devices(&source)
+        .with_context(|| format!("While looking for backing devices of {}", source))?;
+    log::debug!("Found parent devices: {parent_devices:?}");
+    Ok(parent_devices)
 }
 
 /// Find esp partition on the same device

src/efi.rs

@@ -10,6 +10,7 @@ use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use anyhow::{bail, Context, Result};
+use bootc_utils::CommandRunExt;
 use cap_std::fs::Dir;
 use cap_std_ext::cap_std;
 use fn_error_context::context;
@@ -22,7 +23,7 @@ use widestring::U16CString;
 use crate::bootupd::RootContext;
 use crate::model::*;
 use crate::ostreeutil;
-use crate::util::{self, CommandRunExt};
+use crate::util;
 use crate::{blockdev, filetree};
 use crate::{component::*, packagesystem};
 
@@ -256,32 +257,30 @@ impl Component for Efi {
             return Ok(None);
         };
 
-        let esp_devices = esp_devices.unwrap_or_default();
-        let mut devices = esp_devices.iter();
-        let Some(esp) = devices.next() else {
-            anyhow::bail!("Failed to find esp device");
-        };
-
-        if let Some(next_esp) = devices.next() {
-            anyhow::bail!(
-                "Found multiple esp devices {esp} and {next_esp}; not currently supported"
-            );
-        }
-        let destpath = &self.ensure_mounted_esp(rootcxt.path.as_ref(), Path::new(&esp))?;
-
-        let destdir = &openat::Dir::open(&destpath.join("EFI"))
-            .with_context(|| format!("opening EFI dir {}", destpath.display()))?;
-        validate_esp(&destdir)?;
         let updated = rootcxt
             .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(&destdir)?;
-        log::trace!("applying adoption diff: {}", &diff);
-        filetree::apply_diff(&updated, &destdir, &diff, None)
-            .context("applying filesystem changes")?;
+
+        let esp_devices = esp_devices.unwrap_or_default();
+        for esp in esp_devices {
+            let destpath = &self.ensure_mounted_esp(rootcxt.path.as_ref(), Path::new(&esp))?;
+
+            let efidir = openat::Dir::open(&destpath.join("EFI")).context("opening EFI dir")?;
+            validate_esp_fstype(&efidir)?;
+
+            // For adoption, we should only touch files that we know about.
+            let diff = updatef.relative_diff_to(&efidir)?;
+            log::trace!("applying adoption diff: {}", &diff);
+            filetree::apply_diff(&updated, &efidir, &diff, None)
+                .context("applying filesystem changes")?;
+
+            // Do the sync before unmount
+            efidir.syncfs()?;
+            drop(efidir);
+            self.unmount().context("unmount after adopt")?;
+        }
         Ok(Some(InstalledContent {
             meta: updatemeta.clone(),
             filetree: Some(updatef),
@@ -312,7 +311,7 @@ impl Component for Efi {
 
         let destd = &openat::Dir::open(destpath)
             .with_context(|| format!("opening dest dir {}", destpath.display()))?;
-        validate_esp(destd)?;
+        validate_esp_fstype(destd)?;
 
         // TODO - add some sort of API that allows directly setting the working
         // directory to a file descriptor.
@@ -354,24 +353,21 @@ impl Component for Efi {
         let Some(esp_devices) = blockdev::find_colocated_esps(&rootcxt.devices)? else {
             anyhow::bail!("Failed to find all esp devices");
         };
-        let mut devices = esp_devices.iter();
-        let Some(esp) = devices.next() else {
-            anyhow::bail!("Failed to find esp device");
-        };
 
-        if let Some(next_esp) = devices.next() {
-            anyhow::bail!(
-                "Found multiple esp devices {esp} and {next_esp}; not currently supported"
-            );
+        for esp in esp_devices {
+            let destpath = &self.ensure_mounted_esp(rootcxt.path.as_ref(), Path::new(&esp))?;
+            let destdir = openat::Dir::open(&destpath.join("EFI")).context("opening EFI dir")?;
+            validate_esp_fstype(&destdir)?;
+            log::trace!("applying diff: {}", &diff);
+            filetree::apply_diff(&updated, &destdir, &diff, None)
+                .context("applying filesystem changes")?;
+
+            // Do the sync before unmount
+            destdir.syncfs()?;
+            drop(destdir);
+            self.unmount().context("unmount after update")?;
         }
-        let destpath = &self.ensure_mounted_esp(rootcxt.path.as_ref(), Path::new(&esp))?;
-
-        let destdir = &openat::Dir::open(&destpath.join("EFI"))
-            .with_context(|| format!("opening EFI dir {}", destpath.display()))?;
-        validate_esp(&destdir)?;
-        log::trace!("applying diff: {}", &diff);
-        filetree::apply_diff(&updated, &destdir, &diff, None)
-            .context("applying filesystem changes")?;
+
         let adopted_from = None;
         Ok(InstalledContent {
             meta: updatemeta,
@@ -430,31 +426,26 @@ impl Component for Efi {
             .as_ref()
             .ok_or_else(|| anyhow::anyhow!("No filetree for installed EFI found!"))?;
 
+        let mut errs = Vec::new();
         let esp_devices = esp_devices.unwrap_or_default();
-        let mut devices = esp_devices.iter();
-        let Some(esp) = devices.next() else {
-            anyhow::bail!("Failed to find esp device");
-        };
+        for esp in esp_devices.iter() {
+            let destpath = &self.ensure_mounted_esp(Path::new("/"), Path::new(&esp))?;
 
-        if let Some(next_esp) = devices.next() {
-            anyhow::bail!(
-                "Found multiple esp devices {esp} and {next_esp}; not currently supported"
-            );
-        }
-        let destpath = &self.ensure_mounted_esp(Path::new("/"), Path::new(&esp))?;
+            let efidir = openat::Dir::open(&destpath.join("EFI"))
+                .with_context(|| format!("opening EFI dir {}", destpath.display()))?;
+            let diff = currentf.relative_diff_to(&efidir)?;
 
-        let efidir = &openat::Dir::open(&destpath.join("EFI"))
-            .with_context(|| format!("opening EFI dir {}", destpath.display()))?;
-
-        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));
+            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);
+            drop(efidir);
+            self.unmount().context("unmount after validate")?;
         }
-        assert_eq!(diff.additions.len(), 0);
+
         if !errs.is_empty() {
             Ok(ValidationResult::Errors(errs))
         } else {
@@ -492,7 +483,7 @@ impl Drop for Efi {
     }
 }
 
-fn validate_esp(dir: &openat::Dir) -> Result<()> {
+fn validate_esp_fstype(dir: &openat::Dir) -> Result<()> {
     let dir = unsafe { BorrowedFd::borrow_raw(dir.as_raw_fd()) };
     let stat = rustix::fs::fstatfs(&dir)?;
     if stat.f_type != libc::MSDOS_SUPER_MAGIC {

src/util.rs

@@ -5,20 +5,6 @@ use std::process::Command;
 use anyhow::{bail, Context, Result};
 use openat_ext::OpenatDirExt;
 
-pub(crate) trait CommandRunExt {
-    fn run(&mut self) -> Result<()>;
-}
-
-impl CommandRunExt for Command {
-    fn run(&mut self) -> Result<()> {
-        let r = self.status()?;
-        if !r.success() {
-            bail!("Child [{:?}] exited: {}", self, r);
-        }
-        Ok(())
-    }
-}
-
 /// Parse an environment variable as UTF-8
 #[allow(dead_code)]
 pub(crate) fn getenv_utf8(n: &str) -> Result<Option<String>> {

tests/kola/raid1/config.bu

@@ -0,0 +1,7 @@
+variant: fcos
+version: 1.5.0
+boot_device:
+  mirror:
+    devices:
+      - /dev/vda
+      - /dev/vdb

tests/kola/raid1/data/libtest.sh

@@ -0,0 +1 @@
+../../data/libtest.sh

tests/kola/raid1/test.sh

@@ -0,0 +1,42 @@
+#!/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 using RAID 1 works.
+
+set -xeuo pipefail
+
+# shellcheck disable=SC1091
+. "$KOLA_EXT_DATA/libtest.sh"
+
+tmpdir=$(mktemp -d)
+cd ${tmpdir}
+
+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 | tee out.txt
+assert_file_has_content out.txt "Adopted and updated: BIOS: .*"
+assert_file_has_content out.txt "Adopted and updated: EFI: .*"
+
+bootupctl status | tee out.txt
+assert_file_has_content_literal out.txt 'Component BIOS'
+assert_file_has_content_literal out.txt 'Component EFI'
+ok "bootupctl adopt-and-update supports multiple EFIs on RAID1"