[pve-devel] [PATCH pve-manager v4] postinst: filter rbds in lvm

[pve-devel] [PATCH pve-manager v4] postinst: filter rbds in lvm

[Stefan Hanreich]

Since LVM 2.03.15 RBD devices are also scanned by default [1]. This
can lead to guest volumes being recognized and displayed on the host
when using KRBD for RBD-backed disks. In order to prevent this we add
an additional filter to the LVM config to avoid scanning rbds.

This also prevents a bug where LVM created a very high amount of
archive entries when there were logical volumes with the same path
available. This could happen when two guests with RBD disks had the
same LVM layout or a guest and host had the same layout.

previous behavior:
If there is no marker in the LVM conf and global_filter does not
contain '/dev/zd.*': replace the global_filter with our version

new behavior:
Replace the global_filter iff:
- There is no marker and global_filter is empty
- The global_filter is exactly the old default

If we don't replace the filter and it is a non-default value: We print
a warning. Addtionally we force this function to run once when
upgrading from older versions.

The previous versions could replace custom global_filters where the
comment had been removed and the zvol directive removed. The new
behavior is slightly more conservative, but works the same in other
cases.

[1] https://gitlab.com/lvmteam/lvm2/-/commit/6a431eb24241caf2277d3e5b4718782d92650a2a

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Changes from v3 -> v4:
- Move LVM_SUPPRESS_FD_WARNINGS=1 in order to prevent fd warnings from
  the lvmconfig invocation

Changes from v2 -> v3:
- Additionally only change empty values if there is no marker
- Print a warning when encountering a non-default value
- Check the LVM config for validity afterwards and restore it from
  backup if it is invalid

Changes from v1 -> v2:
- changed replacement logic:
  - if there is an existing global_filter, we replace the line
  - if there is no existing global_filter we add a whole 'devices' block
- we only rewrite if there is no global_filter set or if it is the value
we set in versions <= 8.1.3

 debian/postinst | 51 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/debian/postinst b/debian/postinst
index 4c9a1f250..8028e39ee 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -9,23 +9,33 @@ set -e
 # installed and configured.
 
 set_lvm_conf() {
+    local FORCE="$1"
     LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
 
     # keep user changes afterwards provided marker is still there..
-    if grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf; then
+    if grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf && test -z "$FORCE"; then
         return 0 # only do these changes once
     fi
 
-    OLD_VALUE="$(lvmconfig --typeconfig full devices/global_filter)"
-    NEW_VALUE='global_filter=["r|/dev/zd.*|"]'
-
     export LVM_SUPPRESS_FD_WARNINGS=1
 
-    # check global_filter
-    # keep previous setting from our custom packaging if it is still there
-    if echo "$OLD_VALUE" | grep -qvF 'r|/dev/zd.*|'; then
+    OLD_VALUE="$(lvmconfig --typeconfig diff devices/global_filter || true)"
+    NEW_VALUE='global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]'
+
+    # update global_filter if:
+    # it is empty and there is no marker OR exactly the one we set before 8.1.4
+    if (! grep -qF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf && test -z "$OLD_VALUE")\
+	|| (echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"');
+    then
         SET_FILTER=1
         BACKUP=1
+    # print warning if global_filter is set but not our old/new default
+    elif test -n "$OLD_VALUE"\
+	&& ! echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"'\
+	&& ! echo "$OLD_VALUE" | grep -qF "$NEW_VALUE";
+    then
+	echo "non-default 'global_filter' value '$OLD_VALUE' in /etc/lvm/lvm.conf, not setting '$NEW_VALUE' automatically"
+	echo "consider adapting your 'global_filter' manually."
     fi
     # should be the default since bullseye
     if lvmconfig --typeconfig full devices/scan_lvs | grep -qv 'scan_lvs=0'; then
@@ -37,17 +47,19 @@ set_lvm_conf() {
         cp -vb /etc/lvm/lvm.conf /etc/lvm/lvm.conf.bak
     fi
     if test -n "$SET_FILTER"; then
-        echo "Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols from being scanned:"
+        echo "Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols and rbds from being scanned:"
         echo "$OLD_VALUE => $NEW_VALUE"
-        # comment out existing setting
-        sed -i -e 's/^\([[:space:]]*global_filter[[:space:]]*=\)/#\1/' /etc/lvm/lvm.conf
-        # add new section with our setting
-        cat >> /etc/lvm/lvm.conf <<EOF
+        if test -n "$OLD_VALUE"; then
+            sed -i -e "s/$LVM_CONF_MARKER ZFS zvols/$LVM_CONF_MARKER ZFS zvols and Ceph rbds/" /etc/lvm/lvm.conf
+            sed -i -e "s!^\([[:space:]]*\)\(global_filter[[:space:]]*=.*\)\$!\1# \2\n\1$NEW_VALUE!" /etc/lvm/lvm.conf
+        else
+            cat >> /etc/lvm/lvm.conf <<EOF
 devices {
-     $LVM_CONF_MARKER ZFS zvols
+     $LVM_CONF_MARKER ZFS zvols and Ceph rbds
      $NEW_VALUE
- }
+}
 EOF
+        fi
     fi
     if test -n "$SET_SCAN_LVS"; then
         echo "Adding scan_lvs=0 setting to /etc/lvm/lvm.conf to prevent LVs from being scanned."
@@ -61,6 +73,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
+    fi
 }
 
 migrate_apt_auth_conf() {
@@ -165,6 +182,12 @@ case "$1" in
         rm -v "$BETA_SOURCES" || true
     fi
 
+    if test ! -e /proxmox_install_mode && test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.4~'; then
+        if test -e /etc/lvm/lvm.conf ; then
+            set_lvm_conf 1
+        fi
+    fi
+
     set_lvm_conf
 
     if test ! -e /proxmox_install_mode; then
-- 
2.39.2

Re: [pve-devel] [PATCH pve-manager v4] postinst: filter rbds in lvm

[Friedrich Weber]

I started testing this and will send a complete mail later, just wanted
to mention one thing I've stumbled upon.

Consider this pre-upgrade lvm.conf:

devices {
     # added by pve-manager to avoid scanning ZFS zvols
     global_filter=[
"r|/dev/zd.*|"]
 }

As `lvmconfig` normalizes the linebreak, SET_FILTER is 1 but apparently
the `sed` command produces a malformed config (I think it comments out
only the first line, but I didn't check). The validity check fails so
the pre-upgrade lvm.conf is restored, according to the logs:

'/etc/lvm/lvm.conf' -> '/etc/lvm/lvm.conf.bak' (backup:
'/etc/lvm/lvm.conf.bak~')
Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols and rbds
from being scanned:
global_filter="r|/dev/zd.*|" =>
global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]
Parse error at byte 103604 (line 2307): unexpected token
  Failed to load config file /etc/lvm/lvm.conf
Invalid LVM config detected - restoring from /etc/lvm/lvm.conf.bak
Setting up proxmox-ve (8.1.0) ...

This is quite the edge case. So I'm not sure if it worth the hassle to
change the logic to handle it properly (especially as the validity check
handles it somewhat gracefully)?

Re: [pve-devel] [PATCH pve-manager v4] postinst: filter rbds in lvm

[Fabian Grünbichler]

On December 29, 2023 1:41 pm, Friedrich Weber wrote:
> I started testing this and will send a complete mail later, just wanted
> to mention one thing I've stumbled upon.
> 
> Consider this pre-upgrade lvm.conf:
> 
> devices {
>      # added by pve-manager to avoid scanning ZFS zvols
>      global_filter=[
> "r|/dev/zd.*|"]
>  }
> 
> As `lvmconfig` normalizes the linebreak, SET_FILTER is 1 but apparently
> the `sed` command produces a malformed config (I think it comments out
> only the first line, but I didn't check). The validity check fails so
> the pre-upgrade lvm.conf is restored, according to the logs:
> 
> '/etc/lvm/lvm.conf' -> '/etc/lvm/lvm.conf.bak' (backup:
> '/etc/lvm/lvm.conf.bak~')
> Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols and rbds
> from being scanned:
> global_filter="r|/dev/zd.*|" =>
> global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]
> Parse error at byte 103604 (line 2307): unexpected token
>   Failed to load config file /etc/lvm/lvm.conf
> Invalid LVM config detected - restoring from /etc/lvm/lvm.conf.bak
> Setting up proxmox-ve (8.1.0) ...
> 
> This is quite the edge case. So I'm not sure if it worth the hassle to
> change the logic to handle it properly (especially as the validity check
> handles it somewhat gracefully)?

IMHO this is okay, and the alternative would require basically
re-implementing the LVM config parser and monitoring it for changes
(e.g., quoting, comments, whitespace handling, ...)

the error/warning is also clear enough w.r.t. what the intention is, so
any user with a weirdly formatted config should be able to manually do
the change if desired.

[pve-devel] applied: [PATCH pve-manager v4] postinst: filter rbds in lvm

[Fabian Grünbichler]

thanks to both of you!

On December 22, 2023 10:58 am, Stefan Hanreich wrote:
> Since LVM 2.03.15 RBD devices are also scanned by default [1]. This
> can lead to guest volumes being recognized and displayed on the host
> when using KRBD for RBD-backed disks. In order to prevent this we add
> an additional filter to the LVM config to avoid scanning rbds.
> 
> This also prevents a bug where LVM created a very high amount of
> archive entries when there were logical volumes with the same path
> available. This could happen when two guests with RBD disks had the
> same LVM layout or a guest and host had the same layout.
> 
> previous behavior:
> If there is no marker in the LVM conf and global_filter does not
> contain '/dev/zd.*': replace the global_filter with our version
> 
> new behavior:
> Replace the global_filter iff:
> - There is no marker and global_filter is empty
> - The global_filter is exactly the old default
> 
> If we don't replace the filter and it is a non-default value: We print
> a warning. Addtionally we force this function to run once when
> upgrading from older versions.
> 
> The previous versions could replace custom global_filters where the
> comment had been removed and the zvol directive removed. The new
> behavior is slightly more conservative, but works the same in other
> cases.
> 
> [1] https://gitlab.com/lvmteam/lvm2/-/commit/6a431eb24241caf2277d3e5b4718782d92650a2a
> 
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 
> Changes from v3 -> v4:
> - Move LVM_SUPPRESS_FD_WARNINGS=1 in order to prevent fd warnings from
>   the lvmconfig invocation
> 
> Changes from v2 -> v3:
> - Additionally only change empty values if there is no marker
> - Print a warning when encountering a non-default value
> - Check the LVM config for validity afterwards and restore it from
>   backup if it is invalid
> 
> Changes from v1 -> v2:
> - changed replacement logic:
>   - if there is an existing global_filter, we replace the line
>   - if there is no existing global_filter we add a whole 'devices' block
> - we only rewrite if there is no global_filter set or if it is the value
> we set in versions <= 8.1.3
> 
>  debian/postinst | 51 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 4c9a1f250..8028e39ee 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -9,23 +9,33 @@ set -e
>  # installed and configured.
>  
>  set_lvm_conf() {
> +    local FORCE="$1"
>      LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
>  
>      # keep user changes afterwards provided marker is still there..
> -    if grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf; then
> +    if grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf && test -z "$FORCE"; then
>          return 0 # only do these changes once
>      fi
>  
> -    OLD_VALUE="$(lvmconfig --typeconfig full devices/global_filter)"
> -    NEW_VALUE='global_filter=["r|/dev/zd.*|"]'
> -
>      export LVM_SUPPRESS_FD_WARNINGS=1
>  
> -    # check global_filter
> -    # keep previous setting from our custom packaging if it is still there
> -    if echo "$OLD_VALUE" | grep -qvF 'r|/dev/zd.*|'; then
> +    OLD_VALUE="$(lvmconfig --typeconfig diff devices/global_filter || true)"
> +    NEW_VALUE='global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]'
> +
> +    # update global_filter if:
> +    # it is empty and there is no marker OR exactly the one we set before 8.1.4
> +    if (! grep -qF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf && test -z "$OLD_VALUE")\
> +	|| (echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"');
> +    then
>          SET_FILTER=1
>          BACKUP=1
> +    # print warning if global_filter is set but not our old/new default
> +    elif test -n "$OLD_VALUE"\
> +	&& ! echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"'\
> +	&& ! echo "$OLD_VALUE" | grep -qF "$NEW_VALUE";
> +    then
> +	echo "non-default 'global_filter' value '$OLD_VALUE' in /etc/lvm/lvm.conf, not setting '$NEW_VALUE' automatically"
> +	echo "consider adapting your 'global_filter' manually."
>      fi
>      # should be the default since bullseye
>      if lvmconfig --typeconfig full devices/scan_lvs | grep -qv 'scan_lvs=0'; then
> @@ -37,17 +47,19 @@ set_lvm_conf() {
>          cp -vb /etc/lvm/lvm.conf /etc/lvm/lvm.conf.bak
>      fi
>      if test -n "$SET_FILTER"; then
> -        echo "Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols from being scanned:"
> +        echo "Setting 'global_filter' in /etc/lvm/lvm.conf to prevent zvols and rbds from being scanned:"
>          echo "$OLD_VALUE => $NEW_VALUE"
> -        # comment out existing setting
> -        sed -i -e 's/^\([[:space:]]*global_filter[[:space:]]*=\)/#\1/' /etc/lvm/lvm.conf
> -        # add new section with our setting
> -        cat >> /etc/lvm/lvm.conf <<EOF
> +        if test -n "$OLD_VALUE"; then
> +            sed -i -e "s/$LVM_CONF_MARKER ZFS zvols/$LVM_CONF_MARKER ZFS zvols and Ceph rbds/" /etc/lvm/lvm.conf
> +            sed -i -e "s!^\([[:space:]]*\)\(global_filter[[:space:]]*=.*\)\$!\1# \2\n\1$NEW_VALUE!" /etc/lvm/lvm.conf
> +        else
> +            cat >> /etc/lvm/lvm.conf <<EOF
>  devices {
> -     $LVM_CONF_MARKER ZFS zvols
> +     $LVM_CONF_MARKER ZFS zvols and Ceph rbds
>       $NEW_VALUE
> - }
> +}
>  EOF
> +        fi
>      fi
>      if test -n "$SET_SCAN_LVS"; then
>          echo "Adding scan_lvs=0 setting to /etc/lvm/lvm.conf to prevent LVs from being scanned."
> @@ -61,6 +73,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
> +    fi
>  }
>  
>  migrate_apt_auth_conf() {
> @@ -165,6 +182,12 @@ case "$1" in
>          rm -v "$BETA_SOURCES" || true
>      fi
>  
> +    if test ! -e /proxmox_install_mode && test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.4~'; then
> +        if test -e /etc/lvm/lvm.conf ; then
> +            set_lvm_conf 1
> +        fi
> +    fi
> +
>      set_lvm_conf
>  
>      if test ! -e /proxmox_install_mode; then
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>