-
Notifications
You must be signed in to change notification settings - Fork 160
install: read boot entry from /etc/fstab and write to new created bootc deployment
#1856
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
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 enhances the install to-filesystem command to correctly handle systems where /boot is a subvolume, such as on Fedora Rawhide. It achieves this by reading the /boot entry from the target filesystem's /etc/fstab to preserve its mount options. The implementation is clean, using or_else to fall back to the previous logic of using the boot partition's UUID if an fstab entry isn't found. This change is consistent with similar logic in install_reset. The code is correct and well-implemented.
bootc deployment On rawhide, the /boot is subvol instead of separate partition, we need to use the same option in the created deployment. Fixes bootc-dev#1849 Signed-off-by: Huijing Hei <hhei@redhat.com>
bee35a7 to
985f7bd
Compare
cgwalters
left a comment
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.
Thanks so much for digging into this! It looks like there's a backup in the testing farm queue but we can merge after seeing the rawhide branch be green.
| boot_uuid | ||
| .as_deref() | ||
| .map(|boot_uuid| MountSpec::new_uuid_src(boot_uuid, "/boot")) | ||
| // Read /etc/fstab to get boot entry, but only use it if it's UUID-based |
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.
Hum...I think we can unconditionally propagate this; I don't think we need to filter to UUID only.
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.
Yes, I tried without condition firstly, but the install test failed with
$ sudo podman run --rm --privileged --pid=host -v /:/target -v /tmp/xshell-tmp-dir-1/test_authorized_keys:/test_authorized_keys localhost/bootc-integration-install bootc install to-filesystem --acknowledge-destructive --karg=foo=bar --replace=alongside --root-ssh-authorized-keys=/test_authorized_keys /target
test replace=alongside with ssh keys and a karg, and SELinux disabled ... Installing image: docker://localhost/bootc-integration-install:latest
Digest: sha256:f718c825e84eb8f5f3854b815a282ef80773a5f2320e5effe0c885a3146a5e83
error: Installing to filesystem: /boot is not specified via UUID= (this is currently required)
See https://github.com/bootc-dev/bootc/actions/runs/20265544961/job/58187786591
On rawhide, the /boot is subvol instead of separate partition, we need to use the same option in the created deployment.
Fixes #1849