Skip to content
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

grub2-15_ostree: Graceful exit if /etc/default/grub doesn't exist #3150

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

travier
Copy link
Member

@travier travier commented Jan 31, 2024

grub2-15_ostree: Graceful exit if /etc/default/grub doesn't exist

With the new bootupd installation path in Anaconda, the
/etc/default/grub config file is not written anymore as we are only
using BLS configs with new enough bootloaders.

We thus don't need to generate (duplicated) legacy boot entries.

We still need to keep this logic in place in Atomic Desktops
(Silverblue, etc.) until we've actually landed bootupd there and forced
a bootloader update for everybody.

See: fedora-silverblue/issue-tracker#530
See: fedora-silverblue/issue-tracker#120
See: https://fedoraproject.org/wiki/Changes/FedoraSilverblueBootupd


grub2-15_ostree: Fix whitespace

With the new bootupd installation path in Anaconda, the
`/etc/default/grub` config file is not written anymore as we are only
using BLS configs with new enough bootloaders.

We thus don't need to generate (duplicated) legacy boot entries.

We still need to keep this logic in place in Atomic Desktops
(Silverblue, etc.) until we've actually landed bootupd there and forced
a bootloader update for everybody.

See: fedora-silverblue/issue-tracker#530
See: fedora-silverblue/issue-tracker#120
See: https://fedoraproject.org/wiki/Changes/FedoraSilverblueBootupd
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine...however, the real fix here is anything using bootupd really must be setting sysroot.bootloader=none.

This is a messy bootupd-ostree integration issue, which is kind of sad given the two projects are designed to be used together 😄

One bikeshed here...at a practical level ostree could probably detect that bootupd+static grub configs is in use and automatically do the equivalent of bootloader=none.

@cgwalters
Copy link
Member

One stance to take is that anything using bootupctl backend install --with-static-configs s should probably rm /usr/bin/grub2-mkconfig too in their builds. Then ostree can say "ok there's no grub2-mkconfig, I'm going to assume bootloader=none".

Anyways hmmm...it's actually kind of annoyingly hard to detect right now as bootupd doesn't really leave any explicit trace on the system that static configs are in use right now.

@jlebon
Copy link
Member

jlebon commented Jan 31, 2024

This is nice because it works as that marker we were looking for to know that GRUB2 is new enough. (The issue IIRC with .grub2-blscfg-supported is that it was only added by the BLS migration script, and never taught to Anaconda.)

@cgwalters The static config stance makes sense (though in that case, couldn't --with-static-configs just directly set sysroot.bootloader=none?), though in the general case wouldn't this break dual boot systems?

@cgwalters
Copy link
Member

@cgwalters The static config stance makes sense (though in that case, couldn't --with-static-configs just directly set sysroot.bootloader=none?),

At the moment, bootupd isn't really in a position to affect the ostree global configuration.

though in the general case wouldn't this break dual boot systems?

static configs inherently do that, yes.

@jlebon
Copy link
Member

jlebon commented Jan 31, 2024

static configs inherently do that, yes.

I'm talking specifically about the general case. Basically, this statement:

the real fix here is anything using bootupd really must be setting sysroot.bootloader=none.

@cgwalters
Copy link
Member

Yeah agree, we want to support bootupd with dynamic configs as well.

@jlebon
Copy link
Member

jlebon commented Jan 31, 2024

This is nice because it works as that marker we were looking for to know that GRUB2 is new enough. (The issue IIRC with .grub2-blscfg-supported is that it was only added by the BLS migration script, and never taught to Anaconda.)

Though if it's a bug that Anaconda isn't writing /etc/default/grub in the bootupd path, then we should fix that, and while we're there make Anaconda add .grub2-blscfg-supported.

@travier
Copy link
Member Author

travier commented Jan 31, 2024

Yeah agree, we want to support bootupd with dynamic configs as well.

Though if it's a bug that Anaconda isn't writing /etc/default/grub in the bootupd path, then we should fix that, and while we're there make Anaconda add .grub2-blscfg-supported.

If I understand correctly, this is OK, but we should also fix it in Anaconda to support dynamic GRUB configs? Do we really want to support non BLS configs?

@travier
Copy link
Member Author

travier commented Jan 31, 2024

This is nice because it works as that marker we were looking for to know that GRUB2 is new enough. (The issue IIRC with .grub2-blscfg-supported is that it was only added by the BLS migration script, and never taught to Anaconda.)

Unfortunately it's only a marker that Anaconda detected bootupd in the image and is installing the system in this mode.

In general, systems installed with bootupd have a recent enough GRUB2 so that works, but there is no guarantee.

@travier
Copy link
Member Author

travier commented Jan 31, 2024

CI failed but looks green in Jenkins UI so I don't know what happened.

@travier
Copy link
Member Author

travier commented Jan 31, 2024

This is totally fine...however, the real fix here is anything using bootupd really must be setting sysroot.bootloader=none.

We will only be able to set that for everybody once we've installed bootupd everywhere and forced a bootloader update.

Or we could have Anaconda set this directly?

@travier
Copy link
Member Author

travier commented Jan 31, 2024

Maybe I could do something like rhinstaller/anaconda#4240 and set sysroot.bootloader=none in the ostree repo config when we detect bootupd in the image?

@travier
Copy link
Member Author

travier commented Jan 31, 2024

I've just found out that we have a script to switch systems to BLS and we never used it AFAIK: grub2-switch-to-blscfg (https://src.fedoraproject.org/rpms/grub2/c/7c2bab5e98d)

@ericcurtin
Copy link
Collaborator

We do this interesting thing with Android Bootloader support in that although the bootloader has no BLS concept from the OSTree userspace perspective it still uses the BLS files.

cgwalters added a commit to cgwalters/bootupd that referenced this pull request Jan 31, 2024
Currently our static configs don't directly support updates.
(They really should)

We also have a use case around simply introspecting the state
that static configs were enabled (xref ostreedev/ostree#3150)
where we want to have ostree not run grub2-mkconfig in this case.

In preparation for both of these things, add tracking of
our *own* version of the static configs into the metadata.

In theory of course, static configs could include other components
(ignition, greenboot, etc.) and we should track those too.  For
now this at least gets us basic metadata.
cgwalters added a commit to cgwalters/bootupd that referenced this pull request Jan 31, 2024
Currently our static configs don't directly support updates.
(They really should)

We also have a use case around simply introspecting the state
that static configs were enabled (xref ostreedev/ostree#3150)
where we want to have ostree not run grub2-mkconfig in this case.

In preparation for both of these things, add tracking of
our *own* version of the static configs into the metadata.

In theory of course, static configs could include other components
(ignition, greenboot, etc.) and we should track those too.  For
now this at least gets us basic metadata.
cgwalters added a commit to cgwalters/bootupd that referenced this pull request Jan 31, 2024
Currently our static configs don't directly support updates.
(They really should)

We also have a use case around simply introspecting the state
that static configs were enabled (xref ostreedev/ostree#3150)
where we want to have ostree not run grub2-mkconfig in this case.

In preparation for both of these things, add tracking of
our *own* version of the static configs into the metadata.

In theory of course, static configs could include other components
(ignition, greenboot, etc.) and we should track those too.  For
now this at least gets us basic metadata.
@travier
Copy link
Member Author

travier commented Feb 1, 2024

11:27:17  FAIL: iso-live-login.bios (10m0s)
11:27:17      timed out after 10m0s
11:27:17  Running test: iso-live-login.uefi
11:37:24  FAIL: iso-live-login.uefi (10m0.006s)
11:37:24      timed out after 10m0s
11:37:24  Running test: iso-live-login.uefi-secure
11:47:15  FAIL: iso-live-login.uefi-secure (10m0.008s)
11:47:15      timed out after 10m0s

@travier
Copy link
Member Author

travier commented Feb 1, 2024

I've verified that this fixes the installation for Silverblue.

@travier
Copy link
Member Author

travier commented Feb 1, 2024

Looks like those tests failed on previous merge requests as well.

@cgwalters
Copy link
Member

/override continuous-integration/jenkins/pr-merge

Copy link

openshift-ci bot commented Feb 1, 2024

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge

In response to this:

/override continuous-integration/jenkins/pr-merge

Instructions 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/test-infra repository.

@cgwalters cgwalters merged commit 7404131 into ostreedev:main Feb 1, 2024
24 checks passed
@travier travier deleted the grub2-15_ostree-atomic-desktops branch February 1, 2024 13:54
@travier
Copy link
Member Author

travier commented Feb 1, 2024

Thanks

@AdamWill
Copy link

AdamWill commented Mar 14, 2024

This is nice because it works as that marker we were looking for to know that GRUB2 is new enough. (The issue IIRC with .grub2-blscfg-supported is that it was only added by the BLS migration script, and never taught to Anaconda.)

Though if it's a bug that Anaconda isn't writing /etc/default/grub in the bootupd path, then we should fix that, and while we're there make Anaconda add .grub2-blscfg-supported.

Did we ever decide this?

I realized today openQA is running up against this. On aarch64 installs, at the end of install before we boot for the first time, we actually need to add a kernel parameter to see anything on the screen after the reboot (console=tty0). We currently (try to) do this by editing the file etc/default/grub in the installed system and then re-running grub2-mkconfig, but since IoT switched to bootupd, this doesn't work because that file isn't there.

I looked into it a bit, and while the PR commit message says "the /etc/default/grub config file is not written anymore as we are only using BLS configs with new enough bootloaders. We thus don't need to generate (duplicated) legacy boot entries", I'm not sure that follows? Looking at /etc/grub.d/10_linux, /etc/default/grub doesn't seem to be tied to only "legacy boot entries". update_bls_cmdline() in that file replaces the options line in all BLS entries it finds with a string that includes GRUB_CMDLINE_LINUX_DEFAULT, which (IIUC) is the contents of the GRUB_CMDLINE_LINUX line in /etc/default/grub.

anaconda writes various things in that file which we intend to be "Fedora defaults" - the timeout, the distributor, the rhgb quiet default args, things like that - so if we're not writing those into /etc/default/grub on the bootupd path, are they just not being applied at all? Is that what we want?

If not /etc/default/grub, is there some other way I can change the kernel params for a bootupd-based install before first boot, other than just straight up editing the grub config file directly? Or should I just do that?

@cgwalters
Copy link
Member

Note that there's a difference between "bootupd" and "bootupd --with-static-configs". We're talking about the static configs path.

If not /etc/default/grub, is there some other way I can change the kernel params for a bootupd-based install before first boot

Anaconda today uses an ostree CLI that I was never very happy with to do this (which is the backend for the bootloader verb) but:

other than just straight up editing the grub config file directly?

The idea here is that the /boot/loader entries are a stable "API" that multiple tools can read and write. But we also ended up wrapping it with rpm-ostree kargs which is only designed to operate live.

@cgwalters
Copy link
Member

What would definitely make sense is to not even install grub2-mkconfig or /etc/default/grub if bootupd static configs are in use; that would hopefully make things clearer.

@AdamWill
Copy link

Right now, on an install of an osbuild-built IoT image that uses bootupd, /etc/default/grub is not present, but grub2-mkconfig is.

@ericcurtin
Copy link
Collaborator

/boot/loader

I do think editing these entries is the most straightforward path today, but if they don't exists at the points described above, maybe it's worth reconsidering, 🤷

@AdamWill
Copy link

AdamWill commented Mar 15, 2024

Thanks, that is what I've made openQA do. It does work.

@ericcurtin
Copy link
Collaborator

Thanks, that is what I've made openQA do. It does work.

Yay for standardized BLS configs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants