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

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

Closed
travier opened this issue May 15, 2024 · 15 comments
Closed
Assignees
Labels
enhancement New feature or request jira For syncing with Jira triaged This issue was triaged

Comments

@travier
Copy link
Member

travier commented May 15, 2024

See discussions in:

@travier travier added jira For syncing with Jira enhancement New feature or request triaged This issue was triaged labels May 27, 2024
@HuijingHei
Copy link
Member

What I understood is, according to rhinstaller/anaconda#5508 (comment)

The current issue in bootupd:

  • with --update-firmware, it runs efibootmgr with --create (which is the same as -c, it creates an entry and adds it to the boot order)
  • without --update-firmware, it does not run efibootmgr at all.

There's no way to make it create an entry but not add it to the boot order.

Change to:

  • keep with --update-firmware as it is
  • without --update-firmware, run efibootmgr -C (-C creates an entry but does not add it to the boot order)

Or

  • make --update-firmware as default to run efibootmgr -c to create an entry and add it to boot order
  • if there is explicit --update-firmware false, run efibootmgr -C to create an entry but not add it to the boot order

@HuijingHei HuijingHei self-assigned this Jun 3, 2024
@travier
Copy link
Member Author

travier commented Jun 3, 2024

We want to be able to use bootupd backend install --update-firmware in Anaconda to setup the boot order on EFI systems. This is rhinstaller/anaconda#5508 and ideally we'll re-open and merge this PR once the issue is fixed here.

The issue is that when called with --update-firmware, bootupd currently:

  • unconditionally removes the BootCurrent boot entry
    pub(crate) fn clear_efi_current() -> Result<()> {
  • and then adds a new BootEntry for the system being installed
    pub(crate) fn set_efi_current(device: &str, espdir: &openat::Dir, vendordir: &str) -> Result<()> {
    but not using the same name as Anaconda does: Fedora for Fedora instead of fedora for bootupd which uses the name of the vendor dir (name of the directory in the EFI partition):
    vendordir,

The current Anaconda behavior (and the one we want to reproduce in bootupd) is https://github.com/rhinstaller/anaconda/blob/6e4f976f25f9d7668fa62a7d3141735f9828ec6a/pyanaconda/modules/storage/bootloader/efi.py#L141:

Anaconda finds the product name from https://github.com/rhinstaller/anaconda/blob/6e4f976f25f9d7668fa62a7d3141735f9828ec6a/pyanaconda/core/product.py#L67. As we don't have those files in our case, we should figure out where we should read it from.

We could read it from BOOTX54.CSV (EFI/fedora/BOOTX64.CSV file in the ESP) or decide on something else.

Then rework

pub(crate) fn clear_efi_current() -> Result<()> {
to delete all matching entries only (and not necessarily the current one).

Then we use the product name to properly setup the boot entry.

That should be enough to let us enable that option in Anaconda again and then re-include bootupd in Atomic Desktops.


For a later issue, the idea is that we should consider implementing all the options (only those that make sense) from https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html#bootloader so that the bootupd path in Anaconda using a kickstart config matches the one for the classic non-bootupd path.

@travier
Copy link
Member Author

travier commented Jun 3, 2024

Example result from a Kinoite 40 installation in a virtual machine:

$ sudo efibootmgr 
BootCurrent: 0003
Timeout: 0 seconds
BootOrder: 0003,0002,0000
Boot0000* UiApp FvVol(7cb8bdc9-f8eb-4f34-aaea-3ee4af6516a1)/FvFile(462caa21-7614-4503-836e-8ab6f4662331)
Boot0002* UEFI Misc Device      PciRoot(0x0)/Pci(0x2,0x3)/Pci(0x0,0x0){auto_created_boot_option}
Boot0003* Fedora        HD(1,GPT,ae02f19e-99ce-4a8a-8d08-379c45b5048b,0x800,0x12c000)/\EFI\fedora\shimx64.efi

@travier
Copy link
Member Author

travier commented Jun 3, 2024

So several things:

  • We need to decide where we read the boot entry name from. Either we keep reading it from the vendor dir (as bootupd does right now) or we need to read it from os-release for example. We can probably keep the existing logic for now.
  • For the rework of the clear boot entry step, we should match boot entries without looking at the case to remove both Fedora and fedora for example.

@travier
Copy link
Member Author

travier commented Jun 3, 2024

The most important part for the removal is that we only remove "our" boot entries and not other boot entries. Better to remove less than too much.

@HuijingHei
Copy link
Member

HuijingHei commented Jun 3, 2024

Boot0003* Fedora HD(1,GPT,ae02f19e-99ce-4a8a-8d08-379c45b5048b,0x800,0x12c000)/\EFI\fedora\shimx64.efi

Thanks @travier for the clarification.
A little confused, the bootupd is using fedora (vendordir), why we see this as Fedora, does this mean it is using fb, which reads product name from EFI/fedora/BOOTX64.CSV? Does this mean by default, coreos/bootupd will not create boot entry and rely on fb to create the boot entry?

@HuijingHei
Copy link
Member

So several things:

  • We need to decide where we read the boot entry name from. Either we keep reading it from the vendor dir (as bootupd does right now) or we need to read it from os-release for example. We can probably keep the existing logic for now.

Or maybe read from BOOTX64.CSV to align the name as Fedora with anaconda ?

  • For the rework of the clear boot entry step, we should match boot entries without looking at the case to remove both Fedora and fedora for example.

If we have the same boot entry name Fedora, then only clear the same boot entry name, does this make sense?

@travier
Copy link
Member Author

travier commented Jun 4, 2024

A little confused, the bootupd is using fedora (vendordir), why we see this as Fedora

The output above is from a Fedora Kinoite 40 installation that was not done via bootupd but via Anaconda.

@travier
Copy link
Member Author

travier commented Jun 4, 2024

What's fb?

@travier
Copy link
Member Author

travier commented Jun 4, 2024

Or maybe read from BOOTX64.CSV to align the name as Fedora with anaconda ?

We would have to get clarification on how the product names can be set for Anaconda as I don't think it comes from this place after more thoughts. We likely don't expect Fedora Remixes or variants to change the shim package to change branding.

If we have the same boot entry name Fedora, then only clear the same boot entry name, does this make sense?

I think it's reasonable to remove both Fedora and fedora entries for the boot list. The case does not really matter here. This would let us keep our current setup and split this change into a later issue/PR.

@HuijingHei
Copy link
Member

Or maybe read from BOOTX64.CSV to align the name as Fedora with anaconda ?

We would have to get clarification on how the product names can be set for Anaconda as I don't think it comes from this place after more thoughts. We likely don't expect Fedora Remixes or variants to change the shim package to change branding.

It is the fallback.efi that reads label from BOOTX64.CSV, see https://www.rodsbooks.com/efi-bootloaders/fallback.html, add comment https://bugzilla.redhat.com/show_bug.cgi?id=2268505#c20 to get pointer where Fedora comes.

I think it's reasonable to remove both Fedora and fedora entries for the boot list. The case does not really matter here. This would let us keep our current setup and split this change into a later issue/PR.

SGTM.

@travier
Copy link
Member Author

travier commented Jun 4, 2024

It is the fallback.efi that reads label from BOOTX64.CSV, see rodsbooks.com/efi-bootloaders/fallback.html, add comment bugzilla.redhat.com/show_bug.cgi?id=2268505#c20 to get pointer where Fedora comes.

OK. The fallback case happens when we don't use the --update-firmware option to bootupd, which then does not touches the boot entries and leaves that to the fallback path on first boot. This is how Fedora CoreOS works right now.

In this case, we are looking at the path with --update-firmware as this is more commonly needed for desktops use cases where there might be an existing boot entry for another operating system set as default and we want to make sure that we set our freshly installed system as the default first boot entry.

@HuijingHei
Copy link
Member

Anaconda gets the product name from get_product_name() which is from product.py#L107, and value is from ANACONDA_PRODUCTNAME product.py#L85, and definition ANACONDA_PRODUCTNAME=$(sed -r -e 's/ *release.*//' /etc/system-release) from liveinst#L46, so it is from /etc/system-release, maybe can align it to bootupd in the later PR.

HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 5, 2024
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.

With this patch, when called with `--update-firmware`, bootupd
will remove all boot entries that match the current vendor name(
this will be changed to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 5, 2024
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.

With this patch, when called with `--update-firmware`, bootupd
will remove all boot entries that match the current vendor name(
this will be changed to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 5, 2024
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.

With this patch, when called with `--update-firmware`, bootupd
will remove all boot entries that match the current vendor name(
this will be changed to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 5, 2024
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 current vendor
name(will be changed to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 5, 2024
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 current vendor
name(will be changed to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 6, 2024
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 current vendor
name (will be updated to use product name in the later PR).

See coreos#658
HuijingHei added a commit to HuijingHei/bootupd that referenced this issue Jun 6, 2024
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 added a commit to HuijingHei/bootupd that referenced this issue Jun 6, 2024
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 added a commit to HuijingHei/bootupd that referenced this issue Jun 7, 2024
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
@travier
Copy link
Member Author

travier commented Jun 10, 2024

With #665 merged, we can consider this one fixed.

@travier travier closed this as completed Jun 10, 2024
@travier
Copy link
Member Author

travier commented Jun 10, 2024

Thanks @HuijingHei

evan-goode pushed a commit to evan-goode/workstation-ostree-config that referenced this issue Jul 24, 2024
With the following issues now fixed:
- coreos/bootupd#630
- coreos/bootupd#658
- coreos/bootupd#551

And corresponding support in Anaconda:
- rhinstaller/anaconda#5508

We can now (re-)enable bootupd for the bootable containers.

After a bit of testing, we will enable it for the classic ostree ones.

See: https://gitlab.com/fedora/ostree/sig/-/issues/1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request jira For syncing with Jira triaged This issue was triaged
Projects
None yet
Development

No branches or pull requests

2 participants