all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems
@ 2023-10-09 12:52 Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook Stoiko Ivanov
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stoiko Ivanov @ 2023-10-09 12:52 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook
  2023-10-09 12:52 [pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems Stoiko Ivanov
@ 2023-10-09 12:52 ` Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode Stoiko Ivanov
  2 siblings, 0 replies; 7+ messages in thread
From: Stoiko Ivanov @ 2023-10-09 12:52 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed
  2023-10-09 12:52 [pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook Stoiko Ivanov
@ 2023-10-09 12:52 ` Stoiko Ivanov
  2023-10-11  9:39   ` Friedrich Weber
  2023-10-09 12:52 ` [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode Stoiko Ivanov
  2 siblings, 1 reply; 7+ messages in thread
From: Stoiko Ivanov @ 2023-10-09 12:52 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode
  2023-10-09 12:52 [pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook Stoiko Ivanov
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed Stoiko Ivanov
@ 2023-10-09 12:52 ` Stoiko Ivanov
  2023-10-11  9:38   ` Friedrich Weber
  2 siblings, 1 reply; 7+ messages in thread
From: Stoiko Ivanov @ 2023-10-09 12:52 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode
  2023-10-09 12:52 ` [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode Stoiko Ivanov
@ 2023-10-11  9:38   ` Friedrich Weber
  0 siblings, 0 replies; 7+ messages in thread
From: Friedrich Weber @ 2023-10-11  9:38 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stoiko Ivanov

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed
  2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed Stoiko Ivanov
@ 2023-10-11  9:39   ` Friedrich Weber
  2023-10-11 10:05     ` Friedrich Weber
  0 siblings, 1 reply; 7+ messages in thread
From: Friedrich Weber @ 2023-10-11  9:39 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stoiko Ivanov

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
>  




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed
  2023-10-11  9:39   ` Friedrich Weber
@ 2023-10-11 10:05     ` Friedrich Weber
  0 siblings, 0 replies; 7+ messages in thread
From: Friedrich Weber @ 2023-10-11 10:05 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stoiko Ivanov

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.





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-10-11 10:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-09 12:52 [pve-devel] [PATCH kernel-helper/manager v2] check for fitting grub-meta package on uefi systems Stoiko Ivanov
2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 1/2] proxmox-boot-tool: do not exit early in kernel-hook Stoiko Ivanov
2023-10-09 12:52 ` [pve-devel] [PATCH kernel-helper v2 2/2] proxmox-boot-tool: check if correct grub metapackage is installed Stoiko Ivanov
2023-10-11  9:39   ` Friedrich Weber
2023-10-11 10:05     ` Friedrich Weber
2023-10-09 12:52 ` [pve-devel] [PATCH manager v2 1/1] pve7to8: check for proper grub meta-package for bootmode Stoiko Ivanov
2023-10-11  9:38   ` Friedrich Weber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal