Add support for installing to devices with multiple parents#1068
Add support for installing to devices with multiple parents#1068ckyrouac wants to merge 6 commits intocoreos:mainfrom
Conversation
Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Instead of referencing devices as a &str, use Device from the blockdev crate and its related functions. Signed-off-by: ckyrouac <ckyrouac@redhat.com>
This adds a new --filesystem flag that will use the blockdev crate to determine the parent devices of the root filesystem and install the bootloader to each device. The devices can also be explicitly specified via multiple --device flags. Signed-off-by: ckyrouac <ckyrouac@redhat.com>
When running install-to-filesystem, the host may already have the ESP mounted (e.g. /dev/sda15 at /boot/efi). Previously mount_esp_device would unconditionally call mount, which fails with "already mounted". Check if the mount point already contains a vfat filesystem on a different device than its parent (i.e. is a real mount point) before attempting to mount, and reuse the existing mount if so. Co-Authored-By: Claude <noreply@anthropic.com>
|
Hi @ckyrouac. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring to support installing bootloaders to multiple devices, such as in a RAID setup. The migration to the bootc-internal-blockdev crate and the Device struct is consistently applied across the codebase. The new CLI options --device and --filesystem are sensible additions. The logic for handling multiple devices, especially for EFI where it filters for ESPs, is robust. I've also noticed some nice correctness improvements, like verifying that a VFAT partition is a true mount point. I have a couple of suggestions to fix a potential panic when dealing with relative paths, and one for code simplification.
| // subdirectory (e.g. /boot/efi on a vfat /boot) could be | ||
| // misidentified as a mounted ESP. | ||
| let path_dev = std::fs::metadata(&path)?.dev(); | ||
| let parent_dev = std::fs::metadata(path.parent().unwrap_or(&path))?.dev(); |
There was a problem hiding this comment.
There's a potential panic here. path.parent() can return Some("") if path is a relative path without a directory separator (e.g., "efi"). This would cause std::fs::metadata("") to be called, which fails. This can happen if root is relative. To make this more robust, you should handle the empty path case, for example by treating it as the current directory ..
| let st = rustix::fs::statfs(&mnt)?; | ||
| if st.f_type == libc::MSDOS_SUPER_MAGIC { | ||
| let path_dev = std::fs::metadata(&mnt)?.dev(); | ||
| let parent_dev = std::fs::metadata(mnt.parent().unwrap_or(&mnt))?.dev(); |
There was a problem hiding this comment.
| let with_esp: Vec<Option<&Device>> = devices | ||
| .iter() | ||
| .filter(|dev| match dev.find_partition_of_esp() { | ||
| Ok(_) => true, | ||
| Err(e) => { | ||
| log::warn!("Skipping device {} for EFI: {:#}", dev.path(), e); | ||
| false | ||
| } | ||
| }) | ||
| .map(|dev| Some(dev)) | ||
| .collect(); |
There was a problem hiding this comment.
This block of code can be made more concise by using filter_map to combine the filtering and mapping operations into a single step. This can improve readability.
| let with_esp: Vec<Option<&Device>> = devices | |
| .iter() | |
| .filter(|dev| match dev.find_partition_of_esp() { | |
| Ok(_) => true, | |
| Err(e) => { | |
| log::warn!("Skipping device {} for EFI: {:#}", dev.path(), e); | |
| false | |
| } | |
| }) | |
| .map(|dev| Some(dev)) | |
| .collect(); | |
| let with_esp: Vec<Option<&Device>> = devices | |
| .iter() | |
| .filter_map(|dev| match dev.find_partition_of_esp() { | |
| Ok(_) => Some(Some(dev)), | |
| Err(e) => { | |
| log::warn!("Skipping device {} for EFI: {:#}", dev.path(), e); | |
| None | |
| } | |
| }) | |
| .collect(); |
See individual commits for details. Generally, this migrates to using the new blockdev crate funtions and standardize on the Device struct operating on block devices. On top of this, handle the
--filesystemand multiple--deviceflags to install the bootloader to multiple devices, e.g. a /dev/md0 RAID1 device backed by 3 physical disks, each with an ESP partition.Keeping this as a draft because it will break older bootc versions prior to bootc-dev/bootc#2048.
Leaving the TEMP code in to validate CI passes (previously validated on my fork).