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

cc_grub_dpkg: Added UEFI support #2029

Merged
merged 11 commits into from Mar 6, 2023
Merged

cc_grub_dpkg: Added UEFI support #2029

merged 11 commits into from Mar 6, 2023

Conversation

BirknerAlex
Copy link
Contributor

@BirknerAlex BirknerAlex commented Feb 22, 2023

Proposed Commit Message

summary: Fixed cc_grub_dpkg on UEFI systems

On Debian and Ubuntu based systems the cc_grub_dpkg module
handles the needed change of the disk device name / path between
the pre created image and the real hardware system.

Currently it seems only BIOS mode is supported. This adds UEFI support as
well to change the configuration keys for UEFI.

Test Steps

  • Unit tests fixed
  • Tested on different bare metal UEFI machines
  • Tested on a legacy BIOS machine

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@BirknerAlex
Copy link
Contributor Author

@holmanb Is there any chance to get this inside the upcoming version or too late? This bricks UEFI systems after a new grub-efi-amd64-signed package has been released and apt-get tries to upgrade it.

@BirknerAlex BirknerAlex changed the title Fixed cc_grub_dpkg on UEFI systems cc_grub_dpkg: Added UEFI support Feb 22, 2023
@BirknerAlex
Copy link
Contributor Author

BirknerAlex commented Feb 22, 2023

Closed for now, found another issue which I need to investigate.

//edit: Should be fine now. 👍🏻

@BirknerAlex BirknerAlex reopened this Feb 22, 2023
@BirknerAlex
Copy link
Contributor Author

BirknerAlex commented Feb 22, 2023

I just found another issue with NVME disks. While SATA works fine the NVME and UEFI combination seems to fail. Fix is coming.

// edit: I can't reproduce anymore. Will test further but maybe someone can help testing on UEFI too with another hardware type?

Test procedure on Ubuntu:

# Set invalid device, try to reinstall the package, apt-get throws an error, confirm the new disk
echo "set grub-efi/install_devices /dev/notexists1" | debconf-communicate
apt-get update && apt-get reinstall grub-efi-amd64-signed

# Set invalid device again, run cloud-init (with my changes), try reinstall again, no error
echo "set grub-efi/install_devices /dev/notexists1" | debconf-communicate
cloud-init --debug --force single --frequency always --name cc_grub_dpkg
apt-get update && apt-get reinstall grub-efi-amd64-signed

@holmanb
Copy link
Member

holmanb commented Feb 23, 2023

@holmanb Is there any chance to get this inside the upcoming version or too late?

It is too late, but if something is truly broken we can re-release / SRU.

This bricks UEFI systems after a new grub-efi-amd64-signed package has been released and apt-get tries to upgrade it.

By this do you mean that cloud-init is rendering the hardware unusable? So re-install fails after this occurs?

@BirknerAlex
Copy link
Contributor Author

BirknerAlex commented Feb 23, 2023

@holmanb If it's an EFI machine, the BIOS grub-pc/install_devices keys are definitely wrong here, cloud-init itself does not break the machine configuration (it's already broken before booting the first time). But cloud-init creates unused config entries (which are intended for BIOS mode).

So cc_grub_dpkg should fix the debconf settings but can't since it has no UEFI support and it seems the way how apt-get requires these settings are different between BIOS and UEFI.

But maybe I am completely wrong and I misunderstood the whole thing here, at least with this change the errors disappearing on a fresh install in my case.

Sadly I can't find any documentation on Debian or Ubuntu pages which explains the differences between grub-pc/install_devices and grub-efi/install_devices.

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @BirknerAlex, overall this looks really good.

I reproduced this issue and verified the solution today in a VM with OVMF.

I left a few minor comments inline. Once they're fixed, I'll merge.

cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
cloudinit/config/cc_grub_dpkg.py Outdated Show resolved Hide resolved
@BirknerAlex
Copy link
Contributor Author

@holmanb Thank you for the review. I am really happy that you were able to reproduce the issue on UEFI.

I did fix all your comments and I'm thankful tor the feedback, let me know if you find anything else.

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.

None yet

2 participants