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

[pve-devel] [PATCH pve-manager v2] 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:
If there is no marker in the LVM conf or we upgrade from
8.1.4: Replace the global_filter with our version if the global_filter
is empty or *exactly* '/dev/zd.*'

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>
---
 debian/postinst | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/debian/postinst b/debian/postinst
index 4c9a1f250..1d2f815e8 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -9,21 +9,22 @@ 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.*|"]'
+    OLD_VALUE="$(lvmconfig --typeconfig diff devices/global_filter || echo '')"
+    NEW_VALUE='global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]'
 
     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
+    # update setting if it is empty or exactly the one we set before 8.1.4
+    if test -z "$OLD_VALUE" || (echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"'); then
         SET_FILTER=1
         BACKUP=1
     fi
@@ -37,17 +38,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."
@@ -165,6 +168,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 v2] postinst: filter rbds in lvm

[Stefan Hanreich]

Forgot to include changes - so here they are:

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

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

[Fabian Grünbichler]

On December 15, 2023 2:51 pm, 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:
> If there is no marker in the LVM conf or we upgrade from
> 8.1.4: Replace the global_filter with our version if the global_filter
> is empty or *exactly* '/dev/zd.*'
> 
> 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>
> ---
>  debian/postinst | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 4c9a1f250..1d2f815e8 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -9,21 +9,22 @@ 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.*|"]'
> +    OLD_VALUE="$(lvmconfig --typeconfig diff devices/global_filter || echo '')"
> +    NEW_VALUE='global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]'
>  
>      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
> +    # update setting if it is empty or exactly the one we set before 8.1.4
> +    if test -z "$OLD_VALUE" || (echo "$OLD_VALUE" | grep -qF '="r|/dev/zd.*|"'); then
>          SET_FILTER=1
>          BACKUP=1
>      fi

this part is now a lot stricter then before (e.g., if the user has
added multipath devices or something else to the filter for whatever
reason, the filter won't be extended).

should we at least print a warning in that case?

iff
- the config is not default (OLD_VALUE is set)
- the old value is neither our expected old value nor our new value

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."

or something along those lines?

also, the combination of marker found, but no $OLD_VALUE would indicate
that the user explicitly disabled/commented our previously set value -
maybe in that case we also should just print a warning instead of
overriding that choice?

> @@ -37,17 +38,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."
> @@ -165,6 +168,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
> 
> 
> 

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

[Friedrich Weber]

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

Tried a couple of upgrades from PVE 7 to PVE 8 (including pve-manager
with this patch). When upgrading, dpkg asks (in most cases) whether to
keep local /etc/lvm/lvm.conf or install package maintainer version, so I
tried both answers. Results were as I'd expect. I'm showing the pre- and
post-upgrade contents of the `devices {}` block below.

* default PVE 7 lvm.conf (with zvol filter):

** keeping local version
   => post-upgrade lvm.conf:
     # added by pve-manager to avoid scanning ZFS zvols and Ceph rbds
     # global_filter=["r|/dev/zd.*|"]
     global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]

** installing package maintainer version
   => post-upgrade lvm.conf:
     # added by pve-manager to avoid scanning ZFS zvols and Ceph rbds
     global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]


* custom lvm.conf 7 with "# added" marker:
     # added by pve-manager to avoid scanning ZFS zvols
     global_filter=["r|/dev/zd.*|",
		    "r|/dev/foo|"]

** keeping local version
   => custom config is kept and untouched

** installing package maintainer version
    => post-upgrade lvm.conf:
     # added by pve-manager to avoid scanning ZFS zvols and Ceph rbds
     global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]

* custom lvm.conf on PVE 7 without "# added" marker:
     global_filter=["r|/dev/zd.*|", "r|/dev/foo|"]

** keeping local version:
   => custom config is kept and untouched

** install package maintainer version
    => post-upgrade lvm.conf:
     # added by pve-manager to avoid scanning ZFS zvols and Ceph rbds
     global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]

* upstream unmodified lvm.conf without any global_filter
    LVM does not ask which version to take
    => post-upgrade lvm.conf:
     # added by pve-manager to avoid scanning ZFS zvols and Ceph rbds
     global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]

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

[Stefan Hanreich]

Although already shortly discussed off-list, here the summary of the
discussion. v3 coming soon.

On 12/19/23 12:54, Fabian Grünbichler wrote:
> this part is now a lot stricter then before (e.g., if the user has
> added multipath devices or something else to the filter for whatever
> reason, the filter won't be extended).

Yes indeed, although if I stuck with the previous logic the filter would
have just gotten replaced which is arguably worse (imo). That's what I
wanted to prevent with the change.

> should we at least print a warning in that case?

> iff
> - the config is not default (OLD_VALUE is set)
> - the old value is neither our expected old value nor our new value
> 
> 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."

Yes, that sounds sensible - I'll add printing a warning as else

> also, the combination of marker found, but no $OLD_VALUE would indicate
> that the user explicitly disabled/commented our previously set value -
> maybe in that case we also should just print a warning instead of
> overriding that choice?


Yes indeed I hadn't thought of that - so we need to change the logic to
this:

$MARKER_FOUND = grep -qLF "$LVM_CONF_MARKER" /etc/lvm/lvm.conf

(!$MARKER_FOUND && test -z "$OLD_VALUE")
|| (echo "$OLD_VALUE" | grep -qF '"r|/dev/zd.*|"')