voxpupuli / puppet-augeasproviders_grub

Augeas-based grub type and providers
Apache License 2.0
9 stars 33 forks source link

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

Open cedws opened 4 years ago

cedws commented 4 years ago

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 commented 3 years ago

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 https://github.com/hercules-team/augeasproviders_grub/commit/63c2c4bc085baebb7b29e61257a86ba7946913dd 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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).