-
Notifications
You must be signed in to change notification settings - Fork 28
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
Import GRUB static migration code #790
Conversation
Skipping CI for Draft Pull Request. |
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 for this!
src/bootupd.rs
Outdated
@@ -489,6 +496,88 @@ pub(crate) fn client_run_validate() -> Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
pub(crate) fn client_run_migrate() -> Result<()> { | |||
// Used to condition execution of this unit at the systemd level | |||
let stamp_file="/var/lib/.fedora_atomic_desktops_static_grub"; |
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.
Missing a cargo fmt
.
Also... now that this is part of bootupd calling the stamp file name "fedora_atomic_desktops_" seems odd.
Bikeshedding things a bit more...bootupd already has its own little database where we could store this state.
I'm also fine just keeping it as a stamp file, but how about e.g. .bootupd-static-migration-complete
? Also since this is about data in /boot
I think we should probably keep the stamp file there?
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.
If we want to have this represented in bootup state directly then we would have to add a new mode to distinguish between the bootupd managed static grub configs and the ones that we imported from a system that are still user managed because they may contain arbitrary changes, os-prober systems, etc. and we don't want bootupd to override those on static config updates (when we'll implement that).
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.
Hmm...I more meant that we add a new key to the JSON file
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.
So in the end, I have not done that yet as:
- I would have to figure out how to do it and test it
- Having a stamp file makes it easy to skip running this later during boot using systemd
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 have not done that yet but I removed the stamp file and the logic now relies only on the ostree bootloader repo config option.
I immediately ran into fedora-selinux/selinux-policy#2444 while testing this. |
0436363
to
ed15ca7
Compare
Overall LGTM, I can help to do some testing if you have some instructions. |
How I'm (manually) testing this change:
|
ec3c25f
to
e17023d
Compare
So this works as far as I've tested but we have no tests for it yet. |
Do testing on BIOS VM and UEFI VM, then upgrade to f41, copy the new |
e17023d
to
4a65f3b
Compare
4a65f3b
to
06132d0
Compare
Ask a silly question, is there any reason should do the migration only when Check on silverblue40 (using efi), it is symlink to |
As far I know, this is the marker that lets us tell a system with a dynamic config from one with a static one.
On Silverblue 40, GRUB is setup using a dynamic config like package mode Fedora and on Silverblue 41, it is setup by bootupd with a static config. |
src/bootupd.rs
Outdated
.context("Failed to exchange symlink with current GRUB config")?; | ||
|
||
// Remove the now unused symlink (optional cleanup, ignore any failures) | ||
_ = dirfd.remove_file("grub.cfg.current"); |
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.
Should we also delete the target of the symlink (/boot/loader/grub.cfg
) ?
It'll be delete on the next ostree deploy, but it's cleaner IMO.
Or we could just mv $(readlink -ne /boot/grub2/grub.cfg) /boot/grub2/grub.cfg
I would also add a sync
call after the mv
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.
Should we also delete the target of the symlink (
/boot/loader/grub.cfg
) ?
It'll be delete on the next ostree deploy, but it's cleaner IMO.
We could, but it's harmless and as you said it will be "removed" (not generated) for the next deployment. Keeping it makes things easier for debugging for now.
Or we could just
mv $(readlink -ne /boot/grub2/grub.cfg) /boot/grub2/grub.cfg
AFAIK that would not be atomic.
I would also add a
sync
call after themv
We should indeed do that. I'll add this.
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 that would not be atomic.
both files are under a single fs /boot so I don't see why it would not be atomic
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 though I had read somewhere that only renames of files in the same directory would be atomic but looks like I misread of misunderstood something as I can not find that in: https://manpages.debian.org/testing/manpages-dev/renameat2.2.en.html
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.
In the ERRORS section:
EXDEV
oldpath and newpath are not on the same mounted filesystem. (Linux permits a filesystem to be mounted at multiple points, but rename() does not work across different mount points, even if the same filesystem is mounted on both.)
Thinking about this more, this means that we can not take the approach as written in this current PR as the grub configuration generation would fail as part of the generation on the first boot on F42 with composefs enabled. We have multiple options:
|
06132d0
to
460bf23
Compare
I reworked this PR to do both the static GRUB config migration and enable composefs via the karg. Instead of calling I manually tested this change using the same instructions as #790 (comment). The only difference is the new name of the hidden argument:
This would be enabled on IoT & Atomic Desktops by adding a systemd drop-in config with:
to the When running on a Fedora 41 born system:
|
Hum, unfortunately I think I've found another case where that will not work:
So this means that we need to enable day 1 composefs in the image for F42 to make sure that ostree from F41 generates the composefs blob and hope that the migration works :/ |
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.
Marking requested-changes per discussion
If we want to avoid that then this means that we can not rely on the kernel argument for the transition. If I understand correctly ostreedev/ostree#3353, it means that before this patch, the composefs blob was only generated if the destination system had it explicitly enabled. This means that a freshly installed Fedora 41, not updated, which includes such an older ostree version, will only generate the composefs metadata if it's enabled in the Fedora 42 image. I don't know how it would react to a config in the F42 image set to If older If older Overall, the safest path seems to be setting composefs to enabled in F42 and directly blocking rpm-ostree operations on the migration being successful, via an |
460bf23
to
081cf03
Compare
Add a hidden subcommand that migrates existing systems using a dynamic GRUB config to a static one. This command is expected to be run after a successful bootloader update. One way to do that is to add it as a droppin unit config for the `bootloader-update.service` unit included in this repo: ``` $ cat /usr/lib/systremd/system/bootloader-update.service.d/migrate-static-grub-config.conf [Service] ExecStart=/usr/bin/bootupctl migrate-static-grub-config ``` This will be used on Atomic Desktops & IoT systems to migrate systems to a static GRUB config before enabling composefs as GRUB curently does not interact well with it [1]. [1] https://bugzilla.redhat.com/show_bug.cgi?id=2308594 See: https://gitlab.com/fedora/ostree/sig/-/issues/35 See: https://pagure.io/workstation-ostree-config/pull-request/591 See: https://fedoraproject.org/wiki/Changes/ComposefsAtomicDesktops Fixes: coreos#789
081cf03
to
c4eaadb
Compare
Alright, thanks for the reviews, I've updated that again to remove the composefs part and keep this only about the static GRUB config migration following my investigations detailed above. |
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.
One last comment but not a blocker
FYI, the new command is:
as it's only doing the static GRUB config change and no longer anything regarding composefs. |
I've tested all the scenarios I could think of with that code and things appears to be working as expected. I would appreciate if we could get this in before the F42 beta freeze. |
migrate-static-grub-config: Add GRUB static migration subcommand
Add a hidden subcommand that migrates existing systems using a dynamic
GRUB config to a static one.
This command is expected to be run after a successful bootloader update.
One way to do that is to add it as a droppin unit config for the
bootloader-update.service
unit included in this repo:This will be used on Atomic Desktops & IoT systems to migrate systems to
a static GRUB config before enabling composefs as GRUB curently does not
interact well with it [1].
[1] https://bugzilla.redhat.com/show_bug.cgi?id=2308594
See: https://gitlab.com/fedora/ostree/sig/-/issues/35
See: https://pagure.io/workstation-ostree-config/pull-request/591
See: https://fedoraproject.org/wiki/Changes/ComposefsAtomicDesktops
Fixes: #789