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

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

[Stoiko Ivanov]

v2->v3:
* adapted Friedrich's feedback (huge thanks for the patience and attention
  to semantically important details!!) - so that the pve7to8 warning is
  actually understandable

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 v3 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 v3 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

[pve-devel] [PATCH manager v3 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..b34c8362 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 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 kernel-helper/manager v3] check for fitting grub-meta package on uefi systems

[Friedrich Weber]

Looks good to me, thanks a lot!

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

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

[Thomas Lamprecht]

Am 11/10/2023 um 15:23 schrieb Stoiko Ivanov:
> v2->v3:
> * adapted Friedrich's feedback (huge thanks for the patience and attention
>   to semantically important details!!) - so that the pve7to8 warning is
>   actually understandable
> 
> 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(-)
> 


applied, with Friedrich's R-b/T-b tags, thanks!

I did some rewording for the kernel-helper, aligning it a bit more with the
one from pve6to7