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

Ubuntu ESP grub.cfg config broken by grub-mkconfig #51

Open
cedws opened this issue Apr 1, 2020 · 10 comments · May be fixed by #65
Open

Ubuntu ESP grub.cfg config broken by grub-mkconfig #51

cedws opened this issue Apr 1, 2020 · 10 comments · May be fixed by #65

Comments

@cedws
Copy link

cedws commented Apr 1, 2020

On vanilla Ubuntu installs, the EFI GRUB config in /boot/efi/EFI/ubuntu/grub.cfg contains a stub GRUB config which loads the real config from /boot/grub/grub.cfg

On Ubuntu, the recommended way of updating GRUB configs after making changes is to run update-grub rather than grub-mkconfig directly. update-grub only updates the config located at /boot/grub/grub.cfg as far as I can tell.

The effect of the ESP config being broken is that the system no longer boots. Not actually sure why that is. Either way, the ESP config shouldn't be updated.

@olifre
Copy link
Contributor

olifre commented Jul 23, 2021

I observe the very same. It seems this was in parts broken by #57 , but already not fully correct for Debian before.

Note that #57 addresses this for RHEL systems by replacing all files, which is the wrong approach for Debian-based systems which use (and automatically update) only one file, and all other grub configurations are stubs referring to that one.

After 63c2c4b now the list of files which are modified are all in one place:
https://github.com/hercules-team/augeasproviders_grub/blob/63c2c4bc085baebb7b29e61257a86ba7946913dd/lib/puppetx/augeasproviders_grub/util.rb#L143-L150
So I wonder whether an OS flavour dependency should be introduced there (only working on /boot/grub/grub.cfg if on an OS of the Debian family).

@trevor-vaughan , @traylenator What do you think? If the general approach is acceptable, I could certainly try to do a PR (and try my best to also adapt the test suite).

@olifre
Copy link
Contributor

olifre commented Jul 23, 2021

Another approach could be to refer to the size of the files, e.g. by checking if the number of lines is larger than 5 lines — if it is not, this should be a stub configuration. For reference, on Debian I find:

search.fs_uuid SOME_UUID root 
set prefix=($root)'/boot/grub'
configfile $prefix/grub.cfg

This might be more resilient against other OS families adopting this pattern (since nothing with a menuentry will realistically be shorter than 5 lines, this should be quite safe).

@olifre
Copy link
Contributor

olifre commented Jul 23, 2021

I've developed a patch based on this last idea in the branch at https://github.com/unibonn/puppet-augeasproviders_grub/tree/grubcfg-skip-stub-confs .
I'm currently testing this in various combinations of CentOS and Ubuntu / Debian in Legacy and BIOS boot and will open a PR in case I find no regressions.

@cedws
Copy link
Author

cedws commented Jul 23, 2021

It's an interesting idea but doing it based on line count seems a bit fragile. I think it would be preferable to outright not touch the ESP config if the distro is Debian or Ubuntu.

@olifre
Copy link
Contributor

olifre commented Jul 23, 2021

It's an interesting idea but doing it based on line count seems a bit fragile.

That's true, on the other hand, it might be more portable — the large list of files indicates the variety between the different OS's. If another OS switches to the "Debian model", the file length check would automatically do the correct thing, without having to maintain an OS family / edition / version list.

That's why I implemented this approach in the PR, and will test it in our configurations, but of course the decision on the best approach is then up to the maintainers, and I could also implement the other approach (I don't have access to all distributions in https://github.com/hercules-team/augeasproviders_grub/blob/master/metadata.json , though, to confirm which versions are affected).

@olifre
Copy link
Contributor

olifre commented Aug 2, 2021

I've now created PR #65 for the "detect stub file by length of file" approach.

Let's wait for the opinion of @trevor-vaughan and @traylenator on which approach seems most easiest to maintain and stable (I believe this is not obvious).

@trevor-vaughan
Copy link
Contributor

@olifre I'll try to review this tomorrow. This whole thing gets complicated based on how all of the different vendors continue to make a mess of GRUB config processing.

@olifre
Copy link
Contributor

olifre commented Aug 2, 2021

@trevor-vaughan Indeed, that's why I am unsure what is the most stable solution — so input is very welcome. I tested the implementation I PRed with Debian 10 and CentOS 7 in legacy boot (whch should be unaffected either way) and then Debian 10 and Ubuntu 18.04 with EFI booting, all working fine.

@trevor-vaughan
Copy link
Contributor

@olifre Can you try it on CentOS 8.4? EL 8 made dramatic changes to how things are managed. Try using geneirc/centos8 via Vagrant.

@olifre
Copy link
Contributor

olifre commented Aug 2, 2021

@trevor-vaughan I have also tested it on CentOS 8.4 with non-EFI boot, actually (hardware machines and VMs).
However, I don't have access to an EFI booting CentOS 8 machine right now (and I think the vagrant box also uses non-UEFI boot).

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 a pull request may close this issue.

3 participants