[pve-devel] [PATCH manager] d/postinst: lvm conf: automatically set correct default for thin_check_options

[Fiona Ebner]

The Debian package for lvm was built without the
THIN_CHECK_NEEDS_CHECK configure flag, leading to incompatible default
thin_check_options with respect to the thin_check binary. The lvm.conf
mentions that the defaults are
> thin_check_options = [ "-q", "--clear-needs-check-flag" ]
but an actual invocation of 'lvmconfig' will show that this is not the
case:
> # lvmconfig --typeconfig full global/thin_check_options
> thin_check_options=["-q"]

This leads to thin pools failing to activate when there are minor
issues that the new version of thin_check can detect, which couldn't
be detected earlier, because those issues will only be repaired when
--clear-needs-check-flag (or --auto-repair) is present.

If there is no override, explicitly set the default to the compatible
value until the issue is fixed in the LVM package.

If there is an override, check if the
'--clear-needs-check-flag' option is present and tell the user to add
it if not.

Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
---

If we go with this approach, might make sense to also add it to
pve8to9 and the upgrade guide (although having it set only
automatically makes removing it later easier).

Alternative is to rebuild the LVM package ourselves with the correct
build flags.

 debian/postinst | 71 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 12 deletions(-)

diff --git a/debian/postinst b/debian/postinst
index b6e07fd9..8568c5f0 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -8,9 +8,43 @@ set -e
 # conffile handling, and all the packages we depend of are already fully
 # installed and configured.
 
+check_lvm_thin_check_options() {
+    # silence stderr to avoid message if there is no override
+    OLD_VALUE="$(lvmconfig --typeconfig diff global/thin_check_options 2> /dev/null || true)"
+    if test -z "$OLD_VALUE"; then
+        return 1
+    fi
+    if echo "$OLD_VALUE" | grep -qv -- '--clear-needs-check-flag'; then
+        printf '\nNOTE: Detected override for thin_check_options without --clear-needs-check-flag option in /etc/lvm/lvm.conf\n'
+        printf 'Add the option to the override or thin pools with minor issues might not automatically activate anymore!\n\n'
+    else
+        echo "Good, '--clear-needs-check-flag' is already part of 'thin_check_options' in /etc/lvm/lvm.conf"
+    fi
+    return 0
+}
+
+set_lvm_thin_check_options() {
+    echo "Adding explicit thin_check_options setting to /etc/lvm/lvm.conf to allow thin pools with minor issues to be auto-repaired"
+        cat >> /etc/lvm/lvm.conf <<EOF
+global {
+     $LVM_CONF_MARKER thin check options
+     thin_check_options = [ "-q", "--clear-needs-check-flag" ]
+}
+EOF
+}
+
+validate_lvm_conf() {
+    if ! lvmconfig --validate; then
+        echo "Invalid LVM config detected - restoring from /etc/lvm/lvm.conf.bak"
+        mv /etc/lvm/lvm.conf.bak /etc/lvm/lvm.conf
+    fi
+}
+
 set_lvm_conf() {
     # shellcheck disable=SC3043
     local FORCE="$1"
+    # shellcheck disable=SC3043
+    local FORCE_THIN_CHECK_OPTIONS="$2"
     LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
 
     if [ ! -e /etc/lvm/lvm.conf ]; then
@@ -20,6 +54,15 @@ set_lvm_conf() {
 
     # keep user changes afterwards provided marker is still there..
     if grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf && test -z "$FORCE"; then
+        if test -n "$FORCE_THIN_CHECK_OPTIONS"; then
+            export LVM_SUPPRESS_FD_WARNINGS=1
+            if ! check_lvm_thin_check_options; then
+                echo "Backing up lvm.conf before setting pve-manager specific settings.."
+                cp -vb /etc/lvm/lvm.conf /etc/lvm/lvm.conf.bak
+                set_lvm_thin_check_options
+                validate_lvm_conf
+            fi
+        fi
         return 0 # only do these changes once
     fi
 
@@ -48,6 +91,10 @@ set_lvm_conf() {
         SET_SCAN_LVS=1
         BACKUP=1
     fi
+    if ! check_lvm_thin_check_options; then
+        SET_THIN_CHECK_OPTIONS=1
+        BACKUP=1
+    fi
     if test -n "$BACKUP"; then
         echo "Backing up lvm.conf before setting pve-manager specific settings.."
         cp -vb /etc/lvm/lvm.conf /etc/lvm/lvm.conf.bak
@@ -85,10 +132,11 @@ devices {
 EOF
     fi
 
-    if ! lvmconfig --validate; then
-        echo "Invalid LVM config detected - restoring from /etc/lvm/lvm.conf.bak"
-        mv /etc/lvm/lvm.conf.bak /etc/lvm/lvm.conf
+    if test -n "$SET_THIN_CHECK_OPTIONS"; then
+        set_lvm_thin_check_options
     fi
+
+    validate_lvm_conf
 }
 
 update_ceph_conf() {
@@ -204,9 +252,15 @@ case "$1" in
     if test ! -e /proxmox_install_mode && test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.4~'; then
         # TODO: remove with PVE 10
         # pass FORCE as we want to ensure the filter for RBDs gets added to our existing one.
-        set_lvm_conf 1
+        set_lvm_conf 1 1
     else
-        set_lvm_conf
+        if test ! -e /proxmox_install_mode && test -n "$2" && dpkg --compare-versions "$2" 'lt' '9.0.5'; then
+            # TODO: remove the added thin_check_options again once the issue has been fixed in the
+            # LVM packages
+            set_lvm_conf '' 1
+        else
+            set_lvm_conf
+        fi
     fi
 
     if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.11'; then
@@ -239,13 +293,6 @@ case "$1" in
             echo "migration failed, see output above for errors and try to migrate existing data manually by running '/usr/libexec/proxmox/proxmox-rrd-migration-tool --migrate'"
     fi
 
-    if test -n "$2" && dpkg --compare-versions "$2" 'lt' '9.0.4'; then
-        if test -e /etc/lvm/lvm.conf && grep "^\s*thin_check_options" /etc/lvm/lvm.conf | grep -qv -- "--clear-needs-check-flag"; then
-            printf '\nNOTE: Detected override for 'thin_check_options' without '--clear-needs-check-flag' option in /etc/lvm/lvm.conf\n'
-            printf 'Add the option to the override or thin pools with minor issues might not automatically activate anymore!\n\n'
-        fi
-    fi
-
     ;;
 
   abort-upgrade|abort-remove|abort-deconfigure)
-- 
2.47.2