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

bootloader: rely on unified GRUB configuration for UEFI and BIOS #5996

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Nov 14, 2024

This patch simplifies the EFI GRUB configuration generation by replacing the manual config file writing with a grub2-mkconfig command. By doing so, we avoid direct file handling and path resolution issues, leveraging grub2-mkconfig to set up the correct paths and UUIDs.

Additionally, this change enables OS Prober in grub.cfg generation by default, ensuring that other operating systems are detected and added to the boot menu automatically. This is particularly useful for multi-boot environments, as it allows the system to recognize existing OS installations without additional configuration.

Relevant to: https://issues.redhat.com/browse/INSTALLER-3977

@pep8speaks
Copy link

pep8speaks commented Nov 14, 2024

Hello @KKoukiou! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 372:100: E501 line too long (128 > 99 characters)
Line 380:100: E501 line too long (104 > 99 characters)

Comment last updated at 2024-11-20 17:27:36 UTC

@github-actions github-actions bot added the f42 Fedora 42 label Nov 14, 2024
@KKoukiou KKoukiou changed the title bootloader: use grub2-mkconfig for EFI boot entry generation bootloader: rely on unified GRUB configuration for UEFI and BIOS Nov 14, 2024
@jkonecny12
Copy link
Member

Oh, sorry. I didn't meant the whole including other calls. but only the one you have created with the first reduction. I think we need/want the other want.

My point was to remove:

def write_config(self):
    super.write_config()

because it does nothing at all.

Copy link

@marta-lewandowska marta-lewandowska left a comment

Choose a reason for hiding this comment

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

lgtm. grub2-common will handle grub stub generation.

Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

This breaks the UEFI dual boot WebUI test. I see after rebooting it get's stuck in the grub menu.

https://cockpit-logs.us-east-1.linodeobjects.com/pull-515-b3e1fd86-20241115-094407-fedora-rawhide-boot-anaconda-pr-5996-rhinstaller-anaconda-webui/log.html#13-2

Needs further investigation

@KKoukiou KKoukiou added the blocked Don't merge this pull request! label Nov 15, 2024
Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@marta-lewandowska can you give some insight on how grub2-common package should autohandle the grub generation?
Which script is supposed to run on reboot?

@marta-lewandowska
Copy link

marta-lewandowska commented Nov 15, 2024

grub2-common contains this posttrans scriptlet: looks for a mountpoint at /boot/efi and if it finds it, then it looks for a config file in /boot/grub2 and writes one if not found. then it greps for certain keywords in the grub.cfg that's in /boot/efi/EFI/$distro (it should be checking for the existence of the file first... I'm pretty sure it used to) and then if it doesn't find those keywords, it writes the stub cfg.

here's the code:

%posttrans common
set -eu

EFI_HOME=%{efi_esp_dir}
GRUB_HOME=/boot/grub2
ESP_PATH=/boot/efi

if ! mountpoint -q ${ESP_PATH}; then
    exit 0 # no ESP mounted, nothing to do
fi

if test ! -f ${GRUB_HOME}/grub.cfg; then
    # there's no config in GRUB home, create one
    grub2-mkconfig -o ${GRUB_HOME}/grub.cfg
else
    GRUB_CFG_MODE=$(stat --format="%a" ${GRUB_HOME}/grub.cfg)
    if ! test "${GRUB_CFG_MODE}" = "600"; then
        # when upgrading from <=2.06-126 to newer versions, the grub config stub
        # may have different mode than 0600, so set the latter if this is the case
        chmod 0600 ${GRUB_HOME}/grub.cfg
    fi
fi

if (((grep -q "configfile" ${EFI_HOME}/grub.cfg && grep -q "root-dev-only" ${EFI_HOME}/grub.cfg) || grep -q "source" ${EFI_HOME}/grub.cfg) && ! grep -q "# It is automatically generated by grub2-mkconfig using templates" ${EFI_HOME}/grub.cfg); then
    exit 0 #Already unified
fi

# create a stub grub2 config in EFI
BOOT_UUID=$(grub2-probe --target=fs_uuid ${GRUB_HOME})
GRUB_DIR=$(grub2-mkrelpath ${GRUB_HOME})

cat << EOF > ${EFI_HOME}/grub.cfg.stb
search --no-floppy --root-dev-only --fs-uuid --set=dev ${BOOT_UUID}
set prefix=(\$dev)${GRUB_DIR}
export \$prefix
configfile \$prefix/grub.cfg
EOF

if test -f ${EFI_HOME}/grubenv; then
    cp -a ${EFI_HOME}/grubenv ${EFI_HOME}/grubenv.rpmsave
    mv --force ${EFI_HOME}/grubenv ${GRUB_HOME}/grubenv
fi

mv ${EFI_HOME}/grub.cfg.stb ${EFI_HOME}/grub.cfg

@marta-lewandowska
Copy link

Ok @KKoukiou I know what's wrong ... there was a change to that posttrans scriptlet not too long ago. It used to check for the grub.cfg in EFI_HOME, create it if it wasn't there and then move it to GRUB_HOME and then do the grepping to see whether or not is needs to write the stub. But now it looks like what I posted above.
The fact that anaconda was writing out the stub was saving the day, but now that you've taken it out, grub has no config file in the ESP...
We will fix this asap and then your change should also work!

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Nov 18, 2024

Ok @KKoukiou I know what's wrong ... there was a change to that posttrans scriptlet not too long ago. It used to check for the grub.cfg in EFI_HOME, create it if it wasn't there and then move it to GRUB_HOME and then do the grepping to see whether or not is needs to write the stub. But now it looks like what I posted above. The fact that anaconda was writing out the stub was saving the day, but now that you've taken it out, grub has no config file in the ESP... We will fix this asap and then your change should also work!

OK let me know when I can retry the tests. Please link a PR or a Jira ticket so that I can track it if possible.

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Nov 20, 2024

The current approach for generating /boot/efi/EFI/fedora/grub.cfg using the grub2-common posttrans script does not work as expected for all Anaconda installation scenarios, particularly in environments like live images or during custom installation workflows (e.g., %liveimg custom payload). This happens because in these environments, the RPMs are rsynced rather than installed using DNF or RPM, meaning the posttrans scriptlet is not triggered.

I propose creating a reusable helper script to generate the EFI grub.cfg, which can be shared and used by both Anaconda and the grub2-common posttrans script. Is this posttrans script written for anaconda or for other use cases as well?

Anyhow, we need to ensure that we have a common way to generate grub.cfg during the installation process in all scenarios, including live image installations, without relying on RPM triggers.

Let me know your thoughts on this. Here or https://bugzilla.redhat.com/show_bug.cgi?id=2327644
@marta-lewandowska @jkonecny12

… file generation

Our backend now executes the grub2-common posttrans script to generate
grub config files. This is an ugly hack, but still better than having
this logic on our side.
I requested that this logic get's extracted to some sanely reusable
helper scripts: https://bugzilla.redhat.com/show_bug.cgi?id=2327644
@marta-lewandowska
Copy link

We have talked about making the stub config just a file, owned by grub2-x64-efi, instead of being generated by the posttrans. Right now it comes up as not belonging to any package and intuitively people try reinstalling the efi rpm if there are issues with that config because it's pretty weird that grub2-common is creating it. Would that work for you?

@@ -143,6 +143,8 @@ Requires: python3-pid
Requires: crypto-policies
Requires: crypto-policies-scripts

Requires: grub2-common
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be in install-env-deps (eq. the package that pulls in dependencies needed to do a a full installation run) as otherwise it could get pulled in when just the Anaconda package gets installed, say a an initial-setup dependency.

IIRC some people potentially use something else than grub (systemd-boot ? at least there seem to be enough users to complain on fedora-devel when its broken by grub specifics). If it adds post-transaction scriptlets, I guess it could actively break something.

# FIXME: https://bugzilla.redhat.com/show_bug.cgi?id=2327644

# Drop the first line as it's the posttrans scriptlet header
script = util.execWithCapture("rpm", ["-q", "--scripts", "grub2-common"], root=conf.target.system_root).splitlines()[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will work only as long as the rootfs deployed by the payload:

  • has an RPM database
  • has the rpm binary
    For very minimal systems this might not be the case - eq. the expectation is they will be re-installed instead of updated/modified in place.

Also I'm not sure about immutable system like ostree or bootc/image mode. Eq. if they even have scriptlets like this or if they can/expect changes being done in chroot.

Lastly I wonder about ordering or re-entrancy. Eq. can it depend on some other scritplet being called & can it survive being called more times (or do we need to make sure not to call it twice on regular RPM installs).

f.write(script)

# Run the script in the chroot
rc = util.execWithRedirect("/bin/sh", ["/tmp/grub2-posttrans.sh"], root=conf.target.system_root)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main issue with this would be that is basically a black box & implementation detail of the GRUB 2 package - eq. some changes unless properly coordinated with us could potentially break things due to changes in GRUB, its packaging or Anaconda. Also unlike a more widely used tool, it would be IMHO quite hard (to impossible) to test, especially outside of this very specific environment.

In comparison, the proposed "shared script" solution could be called by this scriplet (making it much simpler), could be versions (with both GRUB 2 & Anaconda being able to see & control the installed version) as well as presumably being much easier to test a debug separately ("this folder is now /boot, do your thing").

@KKoukiou
Copy link
Contributor Author

We have talked about making the stub config just a file, owned by grub2-x64-efi, instead of being generated by the posttrans. Right now it comes up as not belonging to any package and intuitively people try reinstalling the efi rpm if there are issues with that config because it's pretty weird that grub2-common is creating it. Would that work for you?

Sounds reasonable. Note that whatever solution your team decides on, will have to be backported to both RHEL9 and RHEL10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Don't merge this pull request! f42 Fedora 42
Development

Successfully merging this pull request may close these issues.

5 participants