* [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
@ 2023-12-13 15:35 Stefan Hanreich
2023-12-13 17:07 ` Stefan Hanreich
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2023-12-13 15:35 UTC (permalink / raw)
To: pve-devel
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 RADOS block
devices.
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.
The following cases can happen where postinst gets executed:
Upgrading from < 8.1.4: We force the new global_filter to be set in
the LVM config
Upgrading from >= 8.1.4: do nothing
New Installation: Run the function as before, just with the new
global_filter value
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
I have tested this for all cases by executing the script manually in a
VM.
Just in the case of upgrading from < 8.1.4 the resulting LVM config is
quite ugly:
```
devices {
# added by pve-manager to avoid scanning ZFS zvols
# global_filter=["r|/dev/zd.*|"]
}
devices {
# added by pve-manager to avoid scanning ZFS zvols and RADOS block devices
global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]
}
```
Trying to find and delete the existing, enclosing devices {} part also
seemed a bit brittle to me, particularly since users could be adding
custom values to this section as well - which we would have to handle
then as well.
Does anyone maybe have a better idea on how to handle this without
generating such an ugly config?
debian/postinst | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/debian/postinst b/debian/postinst
index 4c9a1f250..59d88105c 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -9,21 +9,25 @@ 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
+ FILTER_VALUE='"r|/dev/zd.*|","r|/dev/rbd.*|"'
+
OLD_VALUE="$(lvmconfig --typeconfig full devices/global_filter)"
- NEW_VALUE='global_filter=["r|/dev/zd.*|"]'
+ NEW_VALUE="global_filter=[$FILTER_VALUE]"
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
+ if echo "$OLD_VALUE" | grep -qvF "$FILTER_VALUE"; then
SET_FILTER=1
BACKUP=1
fi
@@ -37,14 +41,14 @@ 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
devices {
- $LVM_CONF_MARKER ZFS zvols
+ $LVM_CONF_MARKER ZFS zvols and RADOS block devices
$NEW_VALUE
}
EOF
@@ -165,6 +169,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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
2023-12-13 15:35 [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices Stefan Hanreich
@ 2023-12-13 17:07 ` Stefan Hanreich
2023-12-14 9:34 ` Friedrich Weber
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2023-12-13 17:07 UTC (permalink / raw)
To: pve-devel
On 12/13/23 16:35, 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 RADOS block
> devices.
>
> 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.
>
> The following cases can happen where postinst gets executed:
>
> Upgrading from < 8.1.4: We force the new global_filter to be set in
> the LVM config
>
> Upgrading from >= 8.1.4: do nothing
>
> New Installation: Run the function as before, just with the new
> global_filter value
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> I have tested this for all cases by executing the script manually in a
> VM.
>
> Just in the case of upgrading from < 8.1.4 the resulting LVM config is
> quite ugly:
>
> ```
> devices {
> # added by pve-manager to avoid scanning ZFS zvols
> # global_filter=["r|/dev/zd.*|"]
> }
> devices {
> # added by pve-manager to avoid scanning ZFS zvols and RADOS block devices
> global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]
> }
> ```
>
> Trying to find and delete the existing, enclosing devices {} part also
> seemed a bit brittle to me, particularly since users could be adding
> custom values to this section as well - which we would have to handle
> then as well.
>
> Does anyone maybe have a better idea on how to handle this without
> generating such an ugly config?
>
>
>
> debian/postinst | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/debian/postinst b/debian/postinst
> index 4c9a1f250..59d88105c 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -9,21 +9,25 @@ 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
>
> + FILTER_VALUE='"r|/dev/zd.*|","r|/dev/rbd.*|"'
> +
> OLD_VALUE="$(lvmconfig --typeconfig full devices/global_filter)"
> - NEW_VALUE='global_filter=["r|/dev/zd.*|"]'
> + NEW_VALUE="global_filter=[$FILTER_VALUE]"
>
> 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
> + if echo "$OLD_VALUE" | grep -qvF "$FILTER_VALUE"; then
This would also replace global_filter directives where
`"r|/dev/zd.*|","r|/dev/rbd.*|"` is only a part of the global_filter. In
order to prevent this I would have to include the square brackets as well.
Friedrich volunteered for testing, so I'll wait for his review with a v2
(and maybe some possible suggestions on how to prevent the resulting
ugly config)
> SET_FILTER=1
> BACKUP=1
> fi
> @@ -37,14 +41,14 @@ 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
> devices {
> - $LVM_CONF_MARKER ZFS zvols
> + $LVM_CONF_MARKER ZFS zvols and RADOS block devices
> $NEW_VALUE
> }
> EOF
> @@ -165,6 +169,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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
2023-12-13 17:07 ` Stefan Hanreich
@ 2023-12-14 9:34 ` Friedrich Weber
2023-12-14 9:55 ` Stefan Hanreich
0 siblings, 1 reply; 6+ messages in thread
From: Friedrich Weber @ 2023-12-14 9:34 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
Already discussed with Stefan off-list yesterday, posting here for the
record:
There is one problem when upgrading from < 8.1.4 with a custom LVM
config where global_filter spans multiple lines, e.g.:
devices {
# added by pve-manager to avoid scanning ZFS zvols
global_filter=["r|/dev/zd.*|",
"r|/dev/foobar|"]
}
With the patch, this is rewritten but only the first line is commented out:
devices {
# added by pve-manager to avoid scanning ZFS zvols
# global_filter=["r|/dev/zd.*|",
"r|/dev/foobar|"]
}
devices {
# added by pve-manager to avoid scanning ZFS zvols and RADOS block
devices
global_filter=["r|/dev/zd.*|","r|/dev/rbd.*|"]
}
And LVM doesn't like this:
# lvs
Parse error at byte 111808 (line 2453): unexpected token
Failed to load config file /etc/lvm/lvm.conf
Not sure how we could extend the global_filter in a reliable way if it
is customized ... worst case, it might be an option to only rewrite the
LVM config if it is unchanged (plus our ZFS zvol addition), and leave
custom configs alone (and add a hint in the upgrade guide)?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
2023-12-14 9:34 ` Friedrich Weber
@ 2023-12-14 9:55 ` Stefan Hanreich
2023-12-14 9:56 ` Stefan Hanreich
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2023-12-14 9:55 UTC (permalink / raw)
To: Friedrich Weber, Proxmox VE development discussion
On 12/14/23 10:34, Friedrich Weber wrote:
> Not sure how we could extend the global_filter in a reliable way if it
> is customized ... worst case, it might be an option to only rewrite the
> LVM config if it is unchanged (plus our ZFS zvol addition), and leave
> custom configs alone (and add a hint in the upgrade guide)?
Yes, at this point I'm also not sure there is a sane way to handle this.
Alternativel we could add a check to pve7to8, add a note to the release
notes and *maybe* check this in the postinst as well.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
2023-12-14 9:55 ` Stefan Hanreich
@ 2023-12-14 9:56 ` Stefan Hanreich
2023-12-14 10:17 ` Friedrich Weber
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2023-12-14 9:56 UTC (permalink / raw)
To: Friedrich Weber, Proxmox VE development discussion
On 12/14/23 10:55, Stefan Hanreich wrote:
> Yes, at this point I'm also not sure there is a sane way to handle this.
doing it for new installations should be possible though
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices
2023-12-14 9:56 ` Stefan Hanreich
@ 2023-12-14 10:17 ` Friedrich Weber
0 siblings, 0 replies; 6+ messages in thread
From: Friedrich Weber @ 2023-12-14 10:17 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
On 14/12/2023 10:56, Stefan Hanreich wrote:> On 12/14/23 10:55, Stefan
Hanreich wrote:
>> Yes, at this point I'm also not sure there is a sane way to handle this.
>
> doing it for new installations should be possible though
Yeah, I'd agree that it's probably the safest to not rewrite existing
global_filters. So we could just update the snippet that is added by the
pve-manager postinst to also include the rbd filter, but keep the
general rewriting logic the same (meaning, we do not touch the file if
it contains LVM_CONF_MARKER). And then add some check to pve7to8 and a
note to the upgrade guide.
I think even then, we would get a kind-of automatic migration path for
free (I haven't actually tested this though): When upgrading from PVE 7
to 8, users are (IIRC always) asked whether they want to keep
/etc/lvm/lvm.conf or install the package maintainers version. If they
choose the package maintainers version (which seems reasonable if they
never edited the LVM config), lvm.conf is replaced with the upstream lvm
default config, and subsequently our pve-manager postinst would add the
global filter again, but this time with the rbd filter?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-14 10:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-13 15:35 [pve-devel] [PATCH pve-manager] postinst: Filter RADOS block devices Stefan Hanreich
2023-12-13 17:07 ` Stefan Hanreich
2023-12-14 9:34 ` Friedrich Weber
2023-12-14 9:55 ` Stefan Hanreich
2023-12-14 9:56 ` Stefan Hanreich
2023-12-14 10:17 ` Friedrich Weber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox