Re: [pve-devel] [RFC manager] vzdump: exclude zfs control dirs by default

[Stoiko Ivanov <s.ivanov@proxmox.com>]

Thanks for tackling this and providing the patch

LGTM code-wise and I think the potential for regression should be pretty
small (plus users who want this can always adapt the vzdump invocation).

small nit on the commit-message:

On Mon, 16 Jan 2023 13:21:20 +0100
Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:

> else in the face of snapdir=visible on a ZFS-backed mountpoint/rootfs, creating
> stop mode backups will fail (because automounting on access of
> .zfs/snapshot/XXX fails), and restoring a suspend mode backup onto a ZFS

While trying to reproduce this for a quick test I was confused - until I
noticed - that the first backup in any mode (suspend/stop) always works,
it's from the second backup where suspend and stop fail

The reason is that the first backup automounts the/all snapshots in the
PVE node, and the second backup again triggers a mount (probably due to
the different mount namespace), which in turn fails (because the snapshot
is already mounted.

w/ w/o a potential adaptation of the commit message:
Tested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Reviewed-by: Stoiko Ivanov <s.ivanov@proxmox.com>

> storage will fail (because an attempt to `mkdir /path/to/target/.zfs/snapshot/XXX`
> fails - or worse, if the "zfs_admin_snapshot" module parameter is enabled, will
> create an XXX snapshot for the newly-restored dataset).
> 
> the two sub directories of .zfs were chosen to decrease the chance of false
> positives, since backing up or restoring the .zfs dir itself is unproblematic.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     see https://forum.proxmox.com/threads/restore-cannot-mkdir-permission-denied.121096
>     
>     alternatively, this could also be handled in pve-container by checking for each
>     mountpoint and explicitly skipping .zfs only if that mountpoint is actually
>     backed by a ZFS storage..
>     
>     if this patch is ACKed, the description of 'stdexcludes' in pve-guest-common should
>     probably also be updated..
> 
>  PVE/VZDump.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index a04837e7..9b9d37a8 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -542,6 +542,8 @@ sub new {
>  	    '/tmp/?*',
>  	    '/var/tmp/?*',
>  	    '/var/run/?*.pid',
> +	    '.zfs/snapshot',
> +	    '.zfs/shares',
>  	    ;
>      }
>