install: Automatically configure aboot #1532
Conversation
![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 introduces support for configuring the bootloader during installation via a new bootloader option in the install configuration. This is primarily to support aboot. The changes also include a fix to use the process ID instead of /proc/self when creating the ostree sysroot, which is necessary for aboot-deploy to function correctly. The implementation looks solid, with corresponding updates to configuration merging and tests. I've left one minor comment regarding a docstring that should be updated for clarity.
alexlarsson
force-pushed
the
aboot-support
branch
from
09b35a2 to
3bd779c
Compare
|
Note: The install config file was the most obvious way to allow bootloader to be configured, it may not be the best approach. |
/proc/self doesn't resolve correctly for subprocesses that may be spawned. In particular, if ostree spawns aboot-deploy to update the aboot symlinks that will fail.
alexlarsson
force-pushed
the
aboot-support
branch
from
3bd779c to
423e237
Compare
| let sysroot = { | ||
| let path = format!("/proc/self/fd/{}", rootfs_dir.as_fd().as_raw_fd()); | ||
| let path = format!( | ||
| "/proc/{}/fd/{}", |
There was a problem hiding this comment.
I'm OK with this but I think the real fix here is that everything in ostree should be using fd-relative access.
There was a problem hiding this comment.
There was a problem hiding this comment.
The issue here is that the path passed to ostree here ends up in a commandline when ostree spawns aboot-deploy. I don't see how using fd-relative access inside ostree itself can fix that.
There was a problem hiding this comment.
I don't see how using fd-relative access inside ostree itself can fix that.
Because ostree turns the provided path into a fd which we should be using instead as the above PR does, right?
There was a problem hiding this comment.
What happens currently (without this change) is that the ostree deploy code will spawn /usr/bin/aboot-deploy -r /proc/self/fd/12. And then when aboot-deploy tries to read this, /proc/self/fd/12 no longer points to the correct thing.
For sure, we could make ostree::Sysroot work with a dirfd for the root path, but it would run into the same problem when it tried to spawn aboot-deploy.
crates/lib/src/install/config.rs
Outdated
| /// Supported architectures for this configuration | ||
| pub(crate) match_architectures: Option<Vec<String>>, | ||
| /// Bootloader backend to use | ||
| pub(crate) bootloader: Option<String>, |
There was a problem hiding this comment.
Hmm, ideally there's a way we can detect this automatically instead.
In ostree wouldn't it be as simple as saying "if there's a /usr/bin/aboot-deploy" then use it?
There was a problem hiding this comment.
I'll have a look at what is possible here. But sure, that would be nice.
There was a problem hiding this comment.
I don't think we want to auto-detect the aboot bootloader if bootloader is set to "none" (maybe if its "auto", but that is not what bootc is doing), because it is fundamentally a pretty destructive operation. I.e. if aboot is enabled the "ostree deploy will potentially dd stuff to a raw partition, and even though its unlikely to happen accidentally I don't fee like it is worth risking it. Not to mention that it is a large change in ostree behavior.
However, I think you're right that we want to automatically detect this by just looking at the image. But we should probably do that in "bootc install" instead. I'll have a look at this.
There was a problem hiding this comment.
I pushed a new version that does this instead
There was a problem hiding this comment.
I pushed a new version that does this instead
I don't see any changes like that?
I don't think we want to auto-detect the aboot bootloader if bootloader is set to "none" (maybe if its "auto", but that is not what bootc is doing), because it is fundamentally a pretty destructive operation.
Yes agree re historical ostree behavior, but note that bootc is basically overriding it by always setting none today. I think it does make sense to re-consider how we do autodetection at least in the bootc context because (unlike ostree) there actually is an explicit install operation.
There was a problem hiding this comment.
This all said, I'm overall OK to just merge this as is
There was a problem hiding this comment.
I pushed a new version that does this instead
I don't see any changes like that?
I'm pretty sure I pushed it? Weird. Anyway, pushed it now.
Aboot images need to have the ostree bootloader backend set to "aboot", otherwise the deploy during bootc install will not create the correct boot A/B symlinks, and additionally once booted will not correctly deploy to the aboot partition during an update. To see whethere an image is using aboot, we look for the "aboot.img" file in the kernel modules dir. NOTE: In order to correctly handle running bootc from a different container than the to-be-installed container we look for aboot.img in the actual commit, not just in the running container.
alexlarsson
force-pushed
the
aboot-support
branch
from
423e237 to
7d276a2
Compare
crates/lib/src/install/config.rs
Outdated
| /// Supported architectures for this configuration | ||
| pub(crate) match_architectures: Option<Vec<String>>, | ||
| /// Bootloader backend to use | ||
| pub(crate) bootloader: Option<String>, |
There was a problem hiding this comment.
This all said, I'm overall OK to just merge this as is
cgwalters
changed the title
|
At first I thought this was a flake
But thinking about it a bit more I don't know how we'd hit that unless there was actually an incorrect image on the registry. But I can't reproduce it pulling locally. |
2 participants
This allows you to set a non-default ostree bootloader backend. In particular, this is needed for aboot images, as otherwise the deploy will not shell out to aboot-deploy to create the required A/B boot symlinks.
PR includes a fix to make aboot-deploy actually work.