-
Notifications
You must be signed in to change notification settings - Fork 290
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: Don't add menu entries if GRUB supports parsing BLS snippets #2044
grub2: Don't add menu entries if GRUB supports parsing BLS snippets #2044
Conversation
Hi @martinezjavier. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
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.
Awesome!
We should definitely test all this before merging by e.g. provisioning an old Silverblue and upgrading it to f31, then running grub-switch-to-blscfg
. I can look into that. (Or did you already do that test?)
@@ -26,6 +26,15 @@ if ! test -d /ostree/repo; then | |||
exit 0 | |||
fi | |||
|
|||
# Gracefully exit if the grub2 configuration has BLS enabled, | |||
# and the installed version has support for the blscfg module. | |||
# Since there is no need to create menu entries for that case. |
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.
Can we link to https://src.fedoraproject.org/rpms/grub2/c/7c2bab5e98d1f376ef9bea2cf2f8f0a6db3dc933 here?
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.
Can we link to https://src.fedoraproject.org/rpms/grub2/c/7c2bab5e98d1f376ef9bea2cf2f8f0a6db3dc933 here?
Done.
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.
cp ${grub_binary} ${grubdir} || exit 1
Eeek! You know that cp
by default does open(... O_TRUNC)
right? So if interrupted you'll get a half-written file...
From shell script the install
binary is usually a better choice.
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.
Eeek! You know that
cp
by default doesopen(... O_TRUNC)
right? So if interrupted you'll get a half-written file...
You are right. I'll fix that in the grub2
package.
7c5af04
to
786d9cc
Compare
Yes, what I did was to test was to get a Fedora Silverblue, update so the GRUB binary in
and then verified that But if you can do a more extensive testing then that would be great. Thanks! |
Awesome, that definitely raises confidence levels! :) I think the main metric I'm concerned with here is whether there's any possible way this can break booting for people. For example, is there in your mind any way that Edit: so just while perusing https://fedoraproject.org/wiki/Changes/BootLoaderSpecByDefault, |
Yes, it's a corner case (the reason why that env var was introduced is because btrfs users may have the BLS snippets in a subvolume under /boot, and GRUB is only able to read the top level volume, so the path would be /subvolume_name/loader/entries). But I agree to be extra cautious and only drop that file if the blsdir variable is not set. Still a user could set it afterwards, but I guess that's up to the user since it would only be set by grub2-mkconfig if the /boot filesystem is btrfs or zfs and /boot/loader/entries is in a subvolume (and that would be the correct thing to do or otherwise GRUB won't be able to find the BLS snippets). The consequence of this would be that people using btrfs or zfs in /boot would have duplicated entries and will require manual intervention for them, but I guess that setup shouldn't be that common so is acceptable.
I can't think of one besides the blsdir variable being set that you mentioned. It may be that aren't picked due a bug in the GRUB blscfg module, but that may also be true for a regression in parsing the menuentry commands. |
OK cool! Let me know when you've added that patch. As a last test before merging this, I'll run through the workflow on my own Silverblue laptop. Probably what we'll want to do once this is merged and both ostree and grub2 are bumped in f31 is to update the common bugs entry and maybe send out an email on the devel mailing list or something. At the same time as we talk about this to others, it's probably also worth raising awareness of |
I pushed the change you suggeste to F31, F32 and Rawhide. I tested doing the following on a Silverblue F32 VM:
Yes, and for
The reason why I didn't suggest to make the And they may not want to do that if for example want to use the GRUB from another distro. In this case they won't have duplicated entries since the other distro would not have support to parse the BLS files so they will only have the entries that were added by the
Indeed. |
@jlebon and we could also add notes for I think that for OF is the same instructions than for |
/approve |
src/boot/grub2/grub2-15_ostree
Outdated
# See: https://src.fedoraproject.org/rpms/grub2/c/7c2bab5e98d | ||
. /etc/default/grub | ||
if test -f /boot/grub2/.grub2-blscfg-supported && \ | ||
test ${GRUB_ENABLE_BLSCFG} = "true"; then |
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.
test ${GRUB_ENABLE_BLSCFG} = "true"; then | |
test "${GRUB_ENABLE_BLSCFG}" = "true"; then |
to avoid a syntax error and crash if the value isn't set. (Yes, shell script is an awful programming language)
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.
to avoid a syntax error and crash if the value isn't set. (Yes, shell script is an awful programming language)
Indeed, I usually run shellcheck
to catch these but forgot this time. Pushed that change and also rebased the branch. Thanks a lot.
This is another attempt to avoid having duplicated menu entries caused by GRUB having support to parse BLS snippets and the 15_ostree script adding menu entries as well. The previous attempt was in commit 985a141 ("grub2: Exit gracefully if the configuration has BLS enabled") but that lead to users not having menu entries at all, due having an old GRUB version that was not able to parse the BLS snippets. This happened because the GRUB bootloader is never updated in the ESP as a part of the OSTree upgrade transaction. The logic is similar to the previous commit, the 15_ostree script exits if able to determine that the bootloader can parse the BLS snippets directly. But this time it will not only check that a BLS configuration was enabled, but also that a /boot/grub2/.grub2-blscfg-supported file exists. This file has to be created by a component outside of OSTree that also takes care of updating GRUB to a version that has proper BLS support.
786d9cc
to
768eee8
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, martinezjavier The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sorry for the delay in testing this myself too. Just did that now on my main FSB machine and it's working well! One gotcha worth mentioning is that I initially also did Thanks a lot Javier! |
@jlebon Thanks a lot for testing, I'm glad that it's working well for you. I've also fixed the issue pointed out by @cgwalters about using |
This is another attempt to avoid having duplicated menu entries caused by
GRUB having support to parse BLS snippets and the 15_ostree script adding
menu entries as well.
The previous attempt was in commit 985a141 ("grub2: Exit gracefully if
the configuration has BLS enabled") but that lead to users not having menu
entries at all, due having an old GRUB version that was not able to parse
the BLS snippets.
This happened because the GRUB bootloader is never updated in the ESP as
a part of the OSTree upgrade transaction.
The logic is similar to the previous commit, the 15_ostree script exits if
able to determine that the bootloader can parse the BLS snippets directly.
But this time it will not only check that a BLS configuration was enabled,
but also that a /boot/grub2/.grub2-blscfg-supported file exists. This file
has to be created by a component outside of OSTree that also takes care of
updating GRUB to a version that has proper BLS support.