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