efi: change `--update-firmware` to match current Anaconda logic #665

HuijingHei · 2024-06-05T12:03:16Z

Copy Timothée's comment:
We want to be able to use bootupd backend install --update-firmware
in Anaconda to setup the boot order on EFI systems.

The issue is that when called with --update-firmware, bootupd
currently removes the BootCurrent boot entry, and then adds a
new BootEntry for the system being installed.

The current Anaconda behavior is to remove all boot entries that
match the product name, then create a new boot entry using the
product name and set it as the first one in the boot order.

To sync with Anaconda behavior, when called with --update-firmware,
bootupd will remove all boot entries that match the product name.

See #658

HuijingHei · 2024-06-06T10:28:04Z

CI failed as the ubuntu does not have /etc/system-release, so can not get product name

---- efi::tests::test_get_product_name stdout ----
Error: Get product name

Caused by:
    0: Failed to read /etc/system-release
    1: No such file or directory (os error 2)

cgwalters · 2024-06-06T11:47:45Z

/etc/system-release

That's a deprecated path, the preferred file is /usr/lib/os-release (also available as /etc/os-release)

travier · 2024-06-06T12:24:35Z

The initial idea was to match Anaconda behavior exactly but that's indeed not ideal.

Maybe we should use the NAME entry from /etc/os-release. I wonder if spaces are allowed in EFI boot entries as that would let us use the NAME entry as is.

travier · 2024-06-06T12:27:40Z

Apart from that the code looks good. Thanks a lot!

cgwalters · 2024-06-07T00:57:18Z

src/efi.rs

+
+    #[test]
+    fn test_get_product_name() -> Result<()> {
+        let product_name = get_product_name()?;


We shouldn't read from the host system in a unit test. The best thing to do here is what you did in test_parse_boot_entries() - the unit test is testing parsing, leaving the "load the data" part outside of the test.

That all said, I do think we should be reading from the os-release file anyways, and actually for that...hmm, there are a few crates out there, not sure of maintenance status of any of them. Maybe not worth pulling in something external. We may even have something in our ecosystem. Ah yep, see https://github.com/coreos/rpm-ostree/blob/fec15f5e24ce2c44f97f602152ac913134bd3173/rust/src/countme.rs#L67

src/efi.rs

cgwalters

Overall this is looking pretty good, but we definitely could use some manual testing. I will try to do that tomorrow.

How much testing did you do with this on virtual or ideally some physical hardware?

HuijingHei · 2024-06-07T01:20:33Z

Thanks @cgwalters @travier very much for the suggestion, it sounds more reasonable to use NAME in /etc/os-release, it looks good for rhel (NAME="Red Hat Enterprise Linux") and centos (NAME="CentOS Stream"), but for fedora, get NAME="Fedora Linux", this will not align with Anaconda (using Fedora), should we continue with "Fedora Linux"?

HuijingHei · 2024-06-07T01:28:51Z

How much testing did you do with this on virtual or ideally some physical hardware?

I did testing using bootc install to-filesystem in vm, as this will trigger creating new boot entry, WDYT?

The steps what I did:

# start fedora vm with uefi (remove cloud-init and set root passwd first)
virt-customize --root-password password:xxx --uninstall cloud-init -a fcloud.qcow2
virt-install --boot uefi --import --name rhel9 --ram 4096 --disk path=/home/fedora/data/fcloud.qcow2 --os-variant rhel9-unknown --network default --nographics

# add more efibootmgr list and check, default get Fedora
efibootmgr -c --disk /dev/vda --part 2 --loader \\EFI\\fedora\\shimx64.efi --label fedora
efibootmgr -c --disk /dev/vda --part 2 --loader \\EFI\\fedora\\shimx64.efi --label FEDORA

# build bootupd
dnf install -y podman; dnf clean all
podman run -it --name fcos-build --net=host -v ./:/srv -w /srv --security-opt=label=disable --privileged quay.io/coreos-assembler/fcos-buildroot:testing-devel bash

git clone -b sync-anaconda https://github.com/HuijingHei/bootupd.git
cd bootupd && make
podman rmi quay.io/coreos-assembler/fcos-buildroot:testing-devel

# build new bootc container to include the bin bootupd file
cat <<EOF > Containerfile
FROM quay.io/centos-bootc/fedora-bootc-dev:eln
RUN rm -f /usr/lib/systemd/system/bootupd.service && \
    rm -f /usr/lib/systemd/system/bootupd.socket && \
    rm -f /usr/libexec/bootupd && \
    rm -f /usr/bin/bootupctl
COPY bootupd/target/release/bootupd /usr/libexec/bootupd
RUN ln -f /usr/libexec/bootupd /usr/bin/bootupctl
EOF

podman build -t test .

# run and check the log
podman run --rm --privileged -v /var/lib/containers:/var/lib/containers -v /:/target --pid=host --security-opt label=disable localhost/test:latest env BOOTC_BOOTLOADER_DEBUG=1 bootc install to-filesystem --skip-fetch-check --replace=alongside /target

Copy Timothée's comment: We want to be able to use `bootupd backend install --update-firmware` in Anaconda to setup the boot order on EFI systems. The issue is that when called with `--update-firmware`, bootupd currently removes the `BootCurrent` boot entry, and then adds a new BootEntry for the system being installed. The current Anaconda behavior is to remove all boot entries that match the product name, then create a new boot entry using the product name and set it as the first one in the boot order. To sync with Anaconda behavior, when called with `--update-firmware`, bootupd will remove all boot entries that match the product name. See coreos#658

HuijingHei · 2024-06-07T06:53:49Z

Hold on, after running bootc podman run --rm --privileged -v /var/lib/containers:/var/lib/containers -v /:/target --pid=host --security-opt label=disable localhost/test:latest env BOOTC_BOOTLOADER_DEBUG=1 bootc install to-filesystem --skip-fetch-check --replace=alongside /target, vm failed to start after reboot, need to do more research.

HuijingHei · 2024-06-07T09:38:13Z

Start vm with secure boot disabled --boot firmware=efi,firmware.feature0.enabled=no,firmware.feature0.name=secure-boot, seems /boot/loader/entries/ostree-1.conf missing console parameter, add ignition.platform.id=qemu console=tty0 console=ttyS0,115200n8 after bootc before reboot, then reboot, hang at

[FAILED] Failed to start systemd-vconsole-s…up.service - Virtual Console Setup.
See 'systemctl status systemd-vconsole-setup.service' for details.

[    **] Job dev-disk-by\x2duuid-8424dfd0\x2…start running (8min 3s / no limit)

Find /dev/vda is only 5 GiB, maybe it is not big enough, will enlarge the disk and try again.

travier · 2024-06-07T11:15:29Z

should we continue with "Fedora Linux"?

Yes, we'll slightly differ from what Anaconda does but that's fine.

How much testing did you do with this on virtual or ideally some physical hardware?

It will indeed need a lot of manual testing on real hardware. We should likely only push this code to Fedora Rawhide/41 so that we get some time to test it there. I can also make test containers/ISOs.

HuijingHei · 2024-06-07T14:50:31Z

[FAILED] Failed to start systemd-vconsole-s…up.service - Virtual Console Setup.
See 'systemctl status systemd-vconsole-setup.service' for details.

[    **] Job dev-disk-by\x2duuid-8424dfd0\x2…start running (8min 3s / no limit)

Ignore the hang error, maybe it is because of fedora vm using btrfs default (see containers/bootc#192).

Change vm to centos image, can boot successfully, but can not login with root & password after reboot, but anyway, the efibootmgr can create new efi boot entry.

Start centos image, run efibootmgr before bootc:

BootCurrent: 0003
Timeout: 0 seconds
BootOrder: 0003,0001,0000,0002
Boot0000* UiApp
Boot0001* UEFI Misc Device
Boot0002* EFI Internal Shell
Boot0003* CentOS Linux

Run efibootmgr after bootc install bootc-centos container (with new bootupd):

BootCurrent: 0003
Timeout: 0 seconds
BootOrder: 0004,0003,0001,0000,0002
Boot0000* UiApp
Boot0001* UEFI Misc Device
Boot0002* EFI Internal Shell
Boot0003* CentOS Linux
Boot0004* CentOS Stream

Another issue is we missed /boot/grub2/console.cfg that is used by grub.cfg (see 38b9716) after bootc install, which might cause systemd-vconsole-setup failed.

cgwalters · 2024-06-07T17:44:28Z

Boot0003* CentOS Linux
Boot0004* CentOS Stream

OK yeah though we can see the difference here in using the os-release data vs system-release. I guess a consequence of this is that we'll "leak" the old bootentry in the use case of a bootc install on top of a system installed from Anaconda. Which...will actually be a somewhat common case. I'm not an expert in this but I think at least some uefi firmware will GC entries at some point that don't have backing files?

cgwalters · 2024-06-07T18:52:06Z

OK so logistically we have a bit of a trap because:

bootupd: call bootupctl with --update-firmware rhinstaller/anaconda#5508 still means this code won't be called until we patch anaconda. It's kind of tempting to add some kind of environment variable or at least to force this mode on
If we hit a regression from this, it could be useful in theory also to have a way to easily force off this new efibootmgr logic

So I think we could add something like env BOOTUPD_FORCE_EFIBOOTMGR=on|off or so?

What I'm a bit uncertain is how easy it is to get environment variables in to the anaconda process itself.

Well, honestly I think my vote is: Let's reopen rhinstaller/anaconda#5508 and merge both it and this to rawhide, after at least some basic testing of both pre-merge.

cgwalters · 2024-06-07T19:39:00Z

OK I've at least tested this doesn't break the virt installation case. Let's go ahead and get this in so it shows up in https://copr.fedorainfracloud.org/coprs/g/CoreOS/continuous/package/bootupd/ for easier integration testing.

HuijingHei force-pushed the sync-anaconda branch 2 times, most recently from 0cee452 to 90b31ab Compare June 5, 2024 12:12

HuijingHei added the do-not-merge/work-in-progress label Jun 5, 2024

HuijingHei changed the title ~~efi: change --update-firmware to match current Anaconda logic~~ WIP: efi: change --update-firmware to match current Anaconda logic Jun 5, 2024

HuijingHei force-pushed the sync-anaconda branch 3 times, most recently from 5941438 to 34b1ae4 Compare June 6, 2024 10:19

HuijingHei changed the title ~~WIP: efi: change --update-firmware to match current Anaconda logic~~ efi: change --update-firmware to match current Anaconda logic Jun 6, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 6, 2024

cgwalters reviewed Jun 7, 2024

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

cgwalters reviewed Jun 7, 2024

View reviewed changes

HuijingHei force-pushed the sync-anaconda branch from 34b1ae4 to 482d159 Compare June 7, 2024 03:14

HuijingHei added the do-not-merge/hold label Jun 7, 2024

cgwalters approved these changes Jun 7, 2024

View reviewed changes

cgwalters mentioned this pull request Jun 7, 2024

bootupd: call bootupctl with --update-firmware rhinstaller/anaconda#5508

Merged

cgwalters merged commit 6e37e52 into coreos:main Jun 7, 2024
9 checks passed

HuijingHei deleted the sync-anaconda branch June 10, 2024 04:26

travier mentioned this pull request Jun 10, 2024

bootupctl backend install --update-firmware should match current Anaconda logic #658

Closed

HuijingHei mentioned this pull request Jul 10, 2024

New bootupd release #682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

efi: change `--update-firmware` to match current Anaconda logic #665

efi: change `--update-firmware` to match current Anaconda logic #665

HuijingHei commented Jun 5, 2024 •

edited

Loading

HuijingHei commented Jun 6, 2024

cgwalters commented Jun 6, 2024

travier commented Jun 6, 2024 •

edited

Loading

travier commented Jun 6, 2024

cgwalters Jun 7, 2024

cgwalters left a comment

HuijingHei commented Jun 7, 2024

HuijingHei commented Jun 7, 2024 •

edited

Loading

HuijingHei commented Jun 7, 2024

HuijingHei commented Jun 7, 2024

travier commented Jun 7, 2024

HuijingHei commented Jun 7, 2024

cgwalters commented Jun 7, 2024

cgwalters commented Jun 7, 2024

cgwalters commented Jun 7, 2024

efi: change --update-firmware to match current Anaconda logic #665

efi: change --update-firmware to match current Anaconda logic #665

Conversation

HuijingHei commented Jun 5, 2024 • edited Loading

HuijingHei commented Jun 6, 2024

cgwalters commented Jun 6, 2024

travier commented Jun 6, 2024 • edited Loading

travier commented Jun 6, 2024

cgwalters Jun 7, 2024

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

HuijingHei commented Jun 7, 2024

HuijingHei commented Jun 7, 2024 • edited Loading

HuijingHei commented Jun 7, 2024

HuijingHei commented Jun 7, 2024

travier commented Jun 7, 2024

HuijingHei commented Jun 7, 2024

cgwalters commented Jun 7, 2024

cgwalters commented Jun 7, 2024

cgwalters commented Jun 7, 2024

efi: change `--update-firmware` to match current Anaconda logic #665

efi: change `--update-firmware` to match current Anaconda logic #665

HuijingHei commented Jun 5, 2024 •

edited

Loading

travier commented Jun 6, 2024 •

edited

Loading

HuijingHei commented Jun 7, 2024 •

edited

Loading