Add support for installing to devices with multiple parents#1068
Add support for installing to devices with multiple parents#1068ckyrouac wants to merge 9 commits intocoreos:mainfrom
Conversation
|
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
I didn't do a deep review, only superficial. There's some nontrivial regression risk here - CI covers some basics, but far from everything.
ckyrouac
force-pushed
the
multi-device-commit-cleanup
branch
3 times, most recently
from
0b44eec to
5ff878b
Compare
Align cap-std-ext with the version used by bootc-internal-blockdev, eliminating duplicate cap-std/cap-primitives dependency chains. Assisted-by: Claude Code (Opus 4) Signed-off-by: ckyrouac <ckyrouac@redhat.com>
ckyrouac
force-pushed
the
multi-device-commit-cleanup
branch
5 times, most recently
from
10341ba to
d868b99
Compare
Signed-off-by: ckyrouac <ckyrouac@redhat.com>
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>
ckyrouac
force-pushed
the
multi-device-commit-cleanup
branch
from
d868b99 to
1365054
Compare
|
marking as ready for review but waiting on bootc 1.14.0 to release to remove the |
ckyrouac
force-pushed
the
multi-device-commit-cleanup
branch
3 times, most recently
from
612732a to
b9b9176
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Offhand this is looking plausible. This may touch on e.g. bootc-dev/bootc#1648 and similar issues, I hope we can try to see if we've fixed them with this.
| } | ||
| } | ||
|
|
||
| /// Get the block device backing the current root by trying `/boot` first, |
There was a problem hiding this comment.
I think it'd be cleaner to use /sysroot if it exists and is a mountpoint, otherwise use /.
In bootc/ostree we call this "physical root" - there's similar code there.
There was a problem hiding this comment.
This pattern was copied from the removed blockdev.rs code. I'm not entirely sure why it was done this way originally, but I think your suggestion makes sense.
There was a problem hiding this comment.
Consider that if boot and root partitions are on separate disks, see bootc-dev/bootc#1916, maybe /boot makes sense?
| [dependencies] | ||
| anyhow = "1.0" | ||
| bootc-internal-blockdev = "0.1.0" | ||
| bootc-internal-blockdev = { git = "https://github.com/bootc-dev/bootc", rev="99da62b6f09ed42841396ac66298030f25daf439"} |
There was a problem hiding this comment.
Unfortunately this will reintroduce the problem that bootupd doesn't use vendored deps in Fedora, so we either need to publish the updated crate (i can look) or fix bootupd to use vendored deps even in Fedora.
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. Assisted-by: Claude Code (Opus 4) Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Signed-off-by: ckyrouac <ckyrouac@redhat.com>
ckyrouac
force-pushed
the
multi-device-commit-cleanup
branch
from
b9b9176 to
42500c3
Compare
3 participants
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).