-
Notifications
You must be signed in to change notification settings - Fork 168
install: Enable installing to multi device parents #1911
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully enables installing to multi-device parent filesystems, such as LVM spanning multiple disks. It correctly discovers all parent devices and, for bootupd/GRUB, installs the bootloader to all devices with an ESP partition. For bootloaders that only support single-device configurations like systemd-boot and zipl, the implementation correctly defaults to using the first available device. The changes are well-architected, adapting data structures and logic to handle multiple devices. A new, thorough integration test validates both single and dual ESP scenarios. Overall, this is a solid enhancement with good error handling and logging. I have one suggestion to further improve the robustness of ESP detection.
| if table.find_partition_of_esp()?.is_some() { | ||
| tracing::info!("Found ESP on device {dev}"); | ||
| esp_devices.push(table); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of ? here could cause the entire installation to fail if find_partition_of_esp() returns an error (e.g., for an unsupported partition table type). This might be undesirable, especially in a multi-device setup where one device having an unsupported format shouldn't prevent bootloader installation on other valid devices.
Consider handling the Result from find_partition_of_esp() explicitly to log the error and continue, similar to how errors from partitions_of() are handled. This would make the process more robust.
match table.find_partition_of_esp() {
Ok(Some(_)) => {
tracing::info!("Found ESP on device {dev}");
esp_devices.push(table);
}
Ok(None) => (),
Err(e) => {
tracing::debug!("Could not check for ESP on {dev}: {e}");
}
}| let devpath = dev.path(); | ||
| println!("Installing bootloader via bootupd to {devpath}"); | ||
| Command::new("bootupctl") | ||
| .args(["backend", "install", "--write-uuid"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will lead to a kind of last-one wins behavior for bootupd.json - but in the end they should be identical I guess?
cc @HuijingHei
We probably want to document the right way to do multi-device installs there. (and have man pages in general)
Alternatively it might be nicer to explicitly support this in bootupd by just passing each device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will lead to a kind of last-one wins behavior for
bootupd.json- but in the end they should be identical I guess?
Agree, but we need this like RAID.
Alternatively it might be nicer to explicitly support this in bootupd by just passing each device?
That will be cleaner, and we could do this only if we make bootupd not fail if the passed device does not have the esp device.
| // Locate ESP partition device | ||
| let esp_part = esp_in(&root_setup.device_info)?; | ||
| // Locate ESP partition device (use first device) | ||
| // TODO: Handle multiple devices (RAID, LVM, etc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK non-redundant multi-device composefs setups (with systemd-boot e.g.) should work where there's just one ESP.
So I think it should work here to walk the blockdevs until we find an ESP, but we would need to error out if there are multiple.
| None, | ||
| )?; | ||
| } else { | ||
| // systemd-boot only supports single device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a single ESP
f2a175a to
f7b1892
Compare
|
waiting to merge until the patch release goes out |
f6bf07c to
77b65cb
Compare
When the root filesystem spans multiple backing devices (e.g., LVM across multiple disks), discover all parent devices and find ESP partitions on each. For bootupd/GRUB, install the bootloader to all devices with an ESP partition, enabling boot from any disk in a multi-disk setup. systemd-boot and zipl only support single-device configurations. This adds a new integration test validating both single-ESP and dual-ESP multi-device scenarios. Fixes: bootc-dev#481 Assisted-by: Claude Code (Opus 4.5) Signed-off-by: ckyrouac <ckyrouac@redhat.com>
77b65cb to
d03c6fa
Compare
When the root filesystem spans multiple backing devices (e.g., LVM across multiple disks), discover all parent devices and find ESP partitions on each. For bootupd/GRUB, install the bootloader to all devices with an ESP partition, enabling boot from any disk in a multi-disk setup. systemd-boot and zipl only support single-device configurations.
This adds a new integration test validating both single-ESP and dual-ESP multi-device scenarios.
Fixes: #481
Assisted-by: Claude Code (Opus 4.5)