[pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems

[pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems

[Stoiko Ivanov]

v1->v2:
* adapted Friedrich's feedback (huge thanks!)
** fixed the wrongly negated check for installed grub-efi-amd64 in the
   boot-tool hook.
** Rephrased the error-message in pve7to8 to 2 sentences. I tried adding a
   newline as well, however this results in the message not being printed
   in the warning color anymore (most likely due to [0]) - and I felt this
   to be more important than having it on a separate line.

[0] https://perldoc.perl.org/Term::ANSIColor#RESTRICTIONS

original cover-letter for v1:
The following patchset is a followup to the one for the installer:
https://lists.proxmox.com/pipermail/pve-devel/2023-September/059270.html

As suggested by Thomas - adding the check to proxmox-kernel-helper seems
like a good idea. While adding it to d/postinst I thought that this might
not be the best place - and that getting the warning upon every
kernel-upgrade would be better vs. upon every upgrade of
proxmox-kernel-helper (which are far less often).
(Can gladly send the version with d/postinst as well)

If the pve-manager patch gets applied - I'd push the equivalent change to
pmg and provide one for pbs.

Tested on legacy and uefi VMs installed with pve-8.0 iso and
grub-efi-amd64 (and systemd-boot) removed vs. installed.

proxmox-kernel-helper:
Stoiko Ivanov (2):
  proxmox-boot-tool: do not exit early in kernel-hook
  proxmox-boot-tool: check if correct grub metapackage is installed

 src/proxmox-boot/zz-proxmox-boot | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

pve-manager:
Stoiko Ivanov (1):
  pve7to8: check for proper grub meta-package for bootmode

 PVE/CLI/pve7to8.pm | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook

[Stoiko Ivanov]

update_esps is called first in the actual execution below - exiting
early does not work for systems that don't use proxmox-boot-tool if a
check added later needs to work there too.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/proxmox-boot/zz-proxmox-boot | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/proxmox-boot/zz-proxmox-boot b/src/proxmox-boot/zz-proxmox-boot
index 793882b..1adc1b1 100755
--- a/src/proxmox-boot/zz-proxmox-boot
+++ b/src/proxmox-boot/zz-proxmox-boot
@@ -44,7 +44,7 @@ fi
 update_esps() {
 	if [ ! -f "${ESP_LIST}" ]; then
 	    warn "No ${ESP_LIST} found, skipping ESP sync."
-	    exit 0
+	    return
 	fi
 	if [ -f /etc/kernel/cmdline ]; then
 		# we can have cmdline files with multiple or no new line at all, handle both!
-- 
2.39.2

[pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed

[Stoiko Ivanov]

this part of the hook applies only to systems not using pbt for
bootmangement.

Currently our ISO installs grub-pc unconditionally - and never the
conflicting grub-efi-amd64. Both packages are responsible for
running grub-install (for the appropriate disks) upon an upgrade of
grub.

This results in grub currently not getting updated on uefi-booted
systems (which do not use proxmox-boot-tool).

The patch causes a warning to be printed to notify the user.

Also considered putting the check+warning in d/postinst - but this way
it will get triggered more often (upon every
kernel-upgrade/update-initramfs, instead of only on
proxmox-kernel-helper updates, which are less often), increasing the
chances of being noticed.

checking for the changelog-presence was chosen, over `dpkg-query` for
the status, for consistency with the similar patch for pve7to8 (and
potentially a small speed-gain).

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/proxmox-boot/zz-proxmox-boot | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/proxmox-boot/zz-proxmox-boot b/src/proxmox-boot/zz-proxmox-boot
index 1adc1b1..4dfa765 100755
--- a/src/proxmox-boot/zz-proxmox-boot
+++ b/src/proxmox-boot/zz-proxmox-boot
@@ -215,6 +215,23 @@ disable_systemd_boot_hook() {
 
 }
 
+check_grub_efi_package() {
+
+	if [ -f "${ESP_LIST}" ]; then
+		return
+	fi
+
+	if [ ! -d /sys/firmware/efi ]; then
+		return
+	fi
+
+	if [ -f /usr/share/doc/grub-efi-amd64/changelog.Debian.gz ]; then
+		return
+	fi
+	warn "uefi-booted system, without grub-efi-amd64 package - /boot/efi will not be updated"
+
+}
+
 set -- $DEB_MAINT_PARAMS
 mode="${1#\'}"
 mode="${mode%\'}"
@@ -228,6 +245,7 @@ case $0:$mode in
 		BOOT_KVERS="$(boot_kernel_list "$@")"
 		update_esps
 		disable_systemd_boot_hook
+		check_grub_efi_package
 	;;
 	 */postrm.d/*:|*/postrm.d/*:remove)
 		reexec_in_mountns "$@"
@@ -235,6 +253,7 @@ case $0:$mode in
 		BOOT_KVERS="$(boot_kernel_list)"
 		update_esps
 		disable_systemd_boot_hook
+		check_grub_efi_package
 	;;
 esac
 
-- 
2.39.2

Re: [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed

[Friedrich Weber]

Tested-by: Friedrich Weber <f.weber@proxmox.com>

Can confirm that with this patch,

* the warning appears after installing a new kernel on a UEFI-booted
system with root on LVM
* the warning does *not* appear after installing a new kernel on
** a UEFI-booted system with root on ZFS (using systemd-boot)
** a legacy-booted system with root on LVM or ZFS

On 09/10/2023 14:52, Stoiko Ivanov wrote:
> this part of the hook applies only to systems not using pbt for
> bootmangement.
> 
> Currently our ISO installs grub-pc unconditionally - and never the
> conflicting grub-efi-amd64. Both packages are responsible for
> running grub-install (for the appropriate disks) upon an upgrade of
> grub.
> 
> This results in grub currently not getting updated on uefi-booted
> systems (which do not use proxmox-boot-tool).
> 
> The patch causes a warning to be printed to notify the user.
> 
> Also considered putting the check+warning in d/postinst - but this way
> it will get triggered more often (upon every
> kernel-upgrade/update-initramfs, instead of only on
> proxmox-kernel-helper updates, which are less often), increasing the
> chances of being noticed.
> 
> checking for the changelog-presence was chosen, over `dpkg-query` for
> the status, for consistency with the similar patch for pve7to8 (and
> potentially a small speed-gain).
> 
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/proxmox-boot/zz-proxmox-boot | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/src/proxmox-boot/zz-proxmox-boot b/src/proxmox-boot/zz-proxmox-boot
> index 1adc1b1..4dfa765 100755
> --- a/src/proxmox-boot/zz-proxmox-boot
> +++ b/src/proxmox-boot/zz-proxmox-boot
> @@ -215,6 +215,23 @@ disable_systemd_boot_hook() {
>  
>  }
>  
> +check_grub_efi_package() {
> +
> +	if [ -f "${ESP_LIST}" ]; then
> +		return
> +	fi
> +
> +	if [ ! -d /sys/firmware/efi ]; then
> +		return
> +	fi
> +
> +	if [ -f /usr/share/doc/grub-efi-amd64/changelog.Debian.gz ]; then
> +		return
> +	fi
> +	warn "uefi-booted system, without grub-efi-amd64 package - /boot/efi will not be updated"
> +
> +}
> +
>  set -- $DEB_MAINT_PARAMS
>  mode="${1#\'}"
>  mode="${mode%\'}"
> @@ -228,6 +245,7 @@ case $0:$mode in
>  		BOOT_KVERS="$(boot_kernel_list "$@")"
>  		update_esps
>  		disable_systemd_boot_hook
> +		check_grub_efi_package
>  	;;
>  	 */postrm.d/*:|*/postrm.d/*:remove)
>  		reexec_in_mountns "$@"
> @@ -235,6 +253,7 @@ case $0:$mode in
>  		BOOT_KVERS="$(boot_kernel_list)"
>  		update_esps
>  		disable_systemd_boot_hook
> +		check_grub_efi_package
>  	;;
>  esac
>  

Re: [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed

[Friedrich Weber]

On 11/10/2023 11:39, Friedrich Weber wrote:
> Can confirm that with this patch,
> 
> * the warning appears after installing a new kernel on a UEFI-booted
> system with root on LVM

Just to be clear: This is if grub-pc is installed. If i install
grub-efi-amd64 instead, the warning does not appear anymore.

[pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode

[Stoiko Ivanov]

This should catch installations from our ISO on non-ZFS in uefi mode,
which won't get the updated grub efi binary installed upon upgrade,
because grub-pc is installed instead of grub-efi-amd64.

Adding this to pve7to8 should make this even more visible, than the
corresponding patch for promxox-kernel-helper (warnings printed during
regular package upgrades might be overlooked more easily than
a yellow line in the major upgrade checkscript)

The if/else order was chosen to limit the nesting level of the long
messages.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/CLI/pve7to8.pm | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/PVE/CLI/pve7to8.pm b/PVE/CLI/pve7to8.pm
index d1a71eff..ff7825b3 100644
--- a/PVE/CLI/pve7to8.pm
+++ b/PVE/CLI/pve7to8.pm
@@ -1302,29 +1302,36 @@ sub check_time_sync {
 
 sub check_bootloader {
     log_info("Checking bootloader configuration...");
-    if (!$upgraded) {
-	log_skip("not yet upgraded, no need to check the presence of systemd-boot");
-	return;
-    }
 
-    if (! -f "/etc/kernel/proxmox-boot-uuids") {
-	log_skip("proxmox-boot-tool not used for bootloader configuration");
+    if (! -d '/sys/firmware/efi') {
+	log_skip("System booted in legacy-mode - no need for additional packages");
 	return;
     }
 
-    if (! -d "/sys/firmware/efi") {
-	log_skip("System booted in legacy-mode - no need for systemd-boot");
-	return;
-    }
-
-    if ( -f "/usr/share/doc/systemd-boot/changelog.Debian.gz") {
-	log_pass("systemd-boot is installed");
-    } else {
+    if ( -f "/etc/kernel/proxmox-boot-uuids") {
+	if (!$upgraded) {
+	    log_skip("not yet upgraded, no need to check the presence of systemd-boot");
+	    return;
+	}
+	if ( -f "/usr/share/doc/systemd-boot/changelog.Debian.gz") {
+	    log_pass("bootloader packages installed correctly");
+	    return;
+	}
 	log_warn(
 	    "proxmox-boot-tool is used for bootloader configuration in uefi mode"
-	    . "but the separate systemd-boot package, existing in Debian Bookworm  is not installed"
-	    . "initializing new ESPs will not work until the package is installed"
+	    . " but the separate systemd-boot package, existing in Debian Bookworm is not installed"
+	    . " initializing new ESPs will not work until the package is installed"
+	);
+	return;
+    } elsif ( ! -f "/usr/share/doc/grub-efi-amd64/changelog.Debian.gz" ) {
+	log_warn(
+	    "System booted in uefi mode but grub-efi-amd64 meta-package not installed"
+	    . " new grub versions will not be installed to /boot/efi!"
+	    . " Install grub-efi-amd64."
 	);
+	return;
+    } else {
+	log_pass("bootloader packages installed correctly");
     }
 }
 
-- 
2.39.2

Re: [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode

[Friedrich Weber]

On 09/10/2023 14:52, Stoiko Ivanov wrote:
> +    } elsif ( ! -f "/usr/share/doc/grub-efi-amd64/changelog.Debian.gz" ) {
> +	log_warn(
> +	    "System booted in uefi mode but grub-efi-amd64 meta-package not installed"
> +	    . " new grub versions will not be installed to /boot/efi!"
> +	    . " Install grub-efi-amd64."
>  	);

I do like the exclamation mark, but I still think some punctuation (if
not newline) between "[...] not installed" and "new grub versions [...]"
would be good. Currently, the message reads like this:

WARN: System booted in uefi mode but grub-efi-amd64 meta-package
notinstalled new grub versions will not be installed to /boot/efi! Install
grub-efi-amd64.

which is a bit hard to parse -- the following seems easier to parse
(note the extra comma):

WARN: System booted in uefi mode but grub-efi-amd64 meta-package not
installed, new grub versions will not be installed to /boot/efi! Install
grub-efi-amd64.

Sorry for obsessing over the punctuation here, but I suspect there are a
lot of UEFI-booted PVE 7 installs with LVM root, so it would be good to
reduce the potential for confusion as much as possible.

The exact phrasing aside, consider this:

Tested-by: Friedrich Weber <f.weber@proxmox.com>

Can confirm that with this patch,

* pve7to8 prints the warning on UEFI-booted system with root on LVM and
grub-pc installed
* pve7to8 does *not* print the warning on
** the same system when grub-efi-amd64 is installed
** UEFI-booted system with root on ZFS (using systemd-boot)
** legacy-booted system with root on LVM or ZFS