Re: [pve-devel] [PATCH pve-kernel-meta v2 2/4] proxmox-boot: fix #3671 add pin/unpin for kernel-version

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 4, 2022 7:45 pm, Stoiko Ivanov wrote:
> The 2 commands follow the mechanics of p-b-t kernel add/remove in
> writing the desired abi-version to a config-file in /etc/kernel and
> actually modifying the boot-loader configuration upon p-b-t refresh.
> 
> A dedicated new file is used instead of writing the version (with some
> kind of annotation) to the manual kernel list to keep parsing the file
> simple (and hopefully also cause fewer problems with manually edited
> files)
> 
> For systemd-boot we write the entry into the loader.conf on the ESP(s)
> instead of relying on the `bootctl set-default` mechanics (bootctl(1))
> which write the entry in an EFI-var. This was preferred, because of a
> few reports of unwriteable EFI-vars on some systems (e.g. DELL servers
> have a setting preventing writing EFI-vars from the OS). The rationale
> in `Why not simply rely on the EFI boot menu logic?` from [0] also
> makes a few points in that direction.
> 
> For grub the following choices were made:
> * write the pinned version (or actually the menu-path leading to it)
>   to a snippet in /etc/default/grub.d instead of editing the grub.cfg
>   files on the partition. Mostly to divert as little as possible from
>   the grub-workflow I assume people are used to.
> * the 'root-device-id' part of the menu-entries is parsed from
>   /boot/grub/grug.cfg since it was stable (the same on all ESPs and in
>   /boot/grub), saves us from copying the part of "find device behind
>   /, mangle it if zfs/btrfs, call grub_probe a few times" part of
>   grub-mkconfig - and seems a bit more robust
> 
> Tested with a BIOS and an UEFI VM with / on ZFS.
> 
> [0] https://systemd.io/BOOT_LOADER_SPECIFICATION/
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  bin/proxmox-boot-tool        | 46 ++++++++++++++++++++++++++++++++----
>  proxmox-boot/functions       | 37 +++++++++++++++++++++++++++++
>  proxmox-boot/zz-proxmox-boot |  5 ++++
>  3 files changed, 83 insertions(+), 5 deletions(-)
> 
> diff --git a/bin/proxmox-boot-tool b/bin/proxmox-boot-tool
> index 93760fb..31342a6 100755
> --- a/bin/proxmox-boot-tool
> +++ b/bin/proxmox-boot-tool
> @@ -280,12 +280,16 @@ list_kernels() {
>  	if [ -z "$manual_kernels" ]; then
>  		manual_kernels="None."
>  	fi
> +	pinned_kernel="$(get_first_line "$PINNED_KERNEL_CONF")"
>  
>  	echo "Manually selected kernels:"
>  	echo "$manual_kernels"
>  	echo ""
>  	echo "Automatically selected kernels:"
>  	echo "$boot_kernels"
> +	echo ""
> +	echo "Pinned kernel:"
> +	echo "${pinned_kernel:-None}"
>  }
>  
>  usage() {
> @@ -295,8 +299,8 @@ usage() {
>  	warn "       $0 init <partition>"
>  	warn "       $0 clean [--dry-run]"
>  	warn "       $0 refresh [--hook <name>]"
> -	warn "       $0 kernel <add|remove> <kernel-version>"
> -	warn "       $0 kernel list"
> +	warn "       $0 kernel <add|remove|pin> <kernel-version>"
> +	warn "       $0 kernel <list|unpin>"
>  	warn "       $0 status [--quiet]"
>  	warn "       $0 help"
>  }
> @@ -318,14 +322,16 @@ help() {
>  	echo ""
>  	echo "    refresh all configured EFI system partitions. Use --hook to only run the specified hook, omit to run all."
>  	echo ""
> -	echo "USAGE: $0 kernel <add|remove> <kernel-version>"
> +	echo "USAGE: $0 kernel <add|remove|pin> <kernel-version>"
>  	echo ""
>  	echo "    add/remove pve-kernel with ABI <kernel-version> to list of synced kernels, in addition to automatically selected ones."
> -	echo "    NOTE: you need to manually run 'refresh' once you're finished with adding/removing kernels from the list"
> +	echo "    pin pve-kernel with ABI <kernel-version> sets it as the default entry to be booted."
> +	echo "    NOTE: you need to manually run 'refresh' once you're finished with adding/removing/pinning kernels from the list"

I wonder if this should be split? e.g.,

 	echo "USAGE: $0 kernel <add|remove> <kernel-version>"
 	echo ""
 	echo "    add/remove pve-kernel with ABI <kernel-version> to list of synced kernels, in addition to automatically selected ones."
 	echo "    NOTE: you need to manually run 'refresh' once you're finished with adding/removing kernels from the list"
+	echi ""
+	echo "USAGE: $0 kernel pin <kernel-version>"
+	echo "    pin pve-kernel with ABI <kernel-version> as the default entry to be booted."
+	echo "    NOTE: you need to manually run 'refresh' once you're finished with adding/removing/pinning kernels from the list"

and then adding next-boot and unpin to this section? while pin/next-boot 
share the argument with add/remove, they are kind of a different 
operation?

e.g., the full thing could then be:

	echo ""
	echo "USAGE: $0 kernel <add|remove> <kernel-version>"
	echo ""
	echo "    add/remove pve-kernel with ABI <kernel-version> to list of synced kernels, in addition to automatically selected ones."
	echo ""
	echo "USAGE: $0 kernel <pin|next-boot> <kernel-version>"
	echo "    pin pve-kernel with ABI <kernel-version> sets the default entry to be booted until unpinned."
	echo "    next-boot pve-kernel with ABI <kernel-version> sets the kernel version for the next boot."
	echo "    NOTE: you need to manually run 'refresh' once you're finished with pinning kernels"
	echo ""
	echo "USAGE: $0 kernel unpin [--next-boot]"
	echo ""
	echo "    unpin removes pinned and next-boot kernel settings. Use --next-boot to only remove a next-boot setting."
	echo "    NOTE: you need to manually run 'refresh' once you're finished with unpinning kernels"
	echo ""
	echo "USAGE: $0 kernel list"
	echo ""
	echo "    list kernel versions currently selected for inclusion on ESPs."

and reading it like that - maybe it makes sense to have 'pin 
--next-boot' like with unpin?

>  	echo ""
> -	echo "USAGE: $0 kernel list"
> +	echo "USAGE: $0 kernel <list|unpin>"
>  	echo ""
>  	echo "    list kernel versions currently selected for inclusion on ESPs."
> +	echo "    unpin sets the latest kernel as the default entry (undoes a previous pin)"
>  	echo ""
>  	echo "USAGE: $0 status [--quiet]"
>  	echo ""
> @@ -392,6 +398,28 @@ status() {
>  	fi
>  }
>  
> +pin_kernel() {
> +	ver="$1"
> +
> +	if [ -z "$ver" ]; then
> +		warn "E: <kernel-version> is mandatory"
> +		warn ""
> +		exit 1
> +	fi
> +
> +	if [ ! -e "/boot/vmlinuz-$ver" ]; then
> +		warn "E: no kernel image found in /boot for '$ver', not setting default."
> +		exit 1
> +	fi
> +	echo "$ver" > "$PINNED_KERNEL_CONF"
> +	echo "Set kernel '$ver' $PINNED_KERNEL_CONF. Use the 'refresh' command to update the ESPs."
> +}
> +
> +unpin_kernel() {
> +	rm -f "$PINNED_KERNEL_CONF"
> +	echo "Removed $PINNED_KERNEL_CONF. Use the 'refresh' command to update the ESPs."
> +}
> +
>  if [ -z "$1" ]; then
>      usage
>      exit 0
> @@ -460,6 +488,14 @@ case "$1" in
>  				list_kernels
>  				exit 0
>  			;;
> +			'pin')
> +				pin_kernel "$2"
> +				exit 0
> +			;;
> +			'unpin')
> +				unpin_kernel "$2"
> +				exit 0
> +			;;
>  			*)
>  				warn "E: invalid 'kernel' subcommand '$cmd'."
>  				warn ""
> diff --git a/proxmox-boot/functions b/proxmox-boot/functions
> index 27da363..d97a7a1 100755
> --- a/proxmox-boot/functions
> +++ b/proxmox-boot/functions
> @@ -5,11 +5,13 @@ ESP_LIST="/etc/kernel/proxmox-boot-uuids"
>  ESPTYPE='c12a7328-f81f-11d2-ba4b-00a0c93ec93b'
>  
>  MANUAL_KERNEL_LIST="/etc/kernel/pve-efiboot-manual-kernels"
> +PINNED_KERNEL_CONF="/etc/kernel/proxmox-boot-pin"
>  
>  MOUNTROOT="${TMPDIR:-/var/tmp}/espmounts"
>  # relative to the ESP mountpoint
>  PMX_ESP_DIR="EFI/proxmox"
>  PMX_LOADER_CONF="loader/loader.conf"
> +GRUB_PIN_SNIPPET="/etc/default/grub.d/proxmox-kernel-pin.cfg"
>  
>  # adapted from /etc/kernel/postinst.d/apt-auto-removal as present in
>  # debian's apt package:
> @@ -21,6 +23,7 @@ PMX_LOADER_CONF="loader/loader.conf"
>  #  - the second-latest kernel version
>  #  - the latest kernel version of each series (e.g. 4.13, 4.15, 5.0) by
>  #    marking the meta-packages
> +#  - the currently pinned kernel if any
>  
>  kernel_keep_versions() {
>  	eval "$(apt-config shell DPKG Dir::bin::dpkg/f)"
> @@ -56,6 +59,8 @@ kernel_keep_versions() {
>  		manual_kernels="$(cat "$MANUAL_KERNEL_LIST")"
>  	fi
>  
> +	pinned_kernel="$(get_first_line "$PINNED_KERNEL_CONF")"
> +
>  	kernels="$(cat <<-EOF
>  		$running_version
>  		$install_version
> @@ -63,6 +68,7 @@ kernel_keep_versions() {
>  		$latest_2_versions
>  		$series_metapackages
>  		$oldseries_latest_kernel
> +		$pinned_kernel
>  		EOF
>  	)"
>  
> @@ -114,3 +120,34 @@ get_first_line() {
>  	done < "${file}"
>  	echo "$line"
>  }
> +
> +set_grub_default() {
> +	kver="$1"
> +
> +	if [ -z "${kver}" ]; then
> +		rm -f "${GRUB_PIN_SNIPPET}"
> +	else
> +		# grub menu entry ids contain the internal root-device id
> +		# (e.g. for zfs the GUID of the pool printed in hex) as this
> +		# as this is independent of the ESP (or grub location) take

nit: doubled 'as this'

> +		# it from /boot/grub/grub.cfg
> +		root_devid=$(sed -rn "s/.*gnulinux-advanced-(.+)['] \{$/\1/p" \
> +			/boot/grub/grub.cfg)
> +		entry="gnulinux-advanced-${root_devid}>gnulinux-${kver}-advanced-${root_devid}"
> +		echo "GRUB_DEFAULT=\"${entry}\"" > "${GRUB_PIN_SNIPPET}"
> +	fi
> +}
> +
> +set_systemd_boot_default() {
> +	mountpoint="$1"
> +	kver="$2"
> +	if [ -z "${kver}" ]; then
> +		entry="proxmox-*"
> +	else
> +		entry="proxmox-${kver}.conf"
> +	fi
> +
> +	sed -ri "/^default /{h;s/ .*\$/ ${entry}/};\${x;/^$/{s//default ${entry}/;H};x}" \
> +		"${mountpoint}/$PMX_LOADER_CONF"
> +
> +}
> diff --git a/proxmox-boot/zz-proxmox-boot b/proxmox-boot/zz-proxmox-boot
> index db73166..7958a5d 100755
> --- a/proxmox-boot/zz-proxmox-boot
> +++ b/proxmox-boot/zz-proxmox-boot
> @@ -90,9 +90,14 @@ update_esp_func() {
>  	fi
>  	warn "Copying and configuring kernels on ${path}"
>  	copy_and_config_kernels "${mountpoint}"
> +
> +	pinned_kernel=$(get_first_line "${PINNED_KERNEL_CONF}")
> +
>  	if [ -d /sys/firmware/efi ]; then
> +		set_systemd_boot_default "${mountpoint}" "${pinned_kernel}"
>  		remove_old_kernels_efi "${mountpoint}"
>  	else
> +		set_grub_default "${pinned_kernel}"
>  		remove_old_kernels_legacy "${mountpoint}"
>  		mount --bind "${mountpoint}" "/boot"
>  		update-grub
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>