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

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

[Fabian Grünbichler]

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
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',
 	    ;
     }
 
-- 
2.30.2

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

[Stoiko Ivanov]

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',
>  	    ;
>      }
>  

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

[Fabian Grünbichler]

On January 17, 2023 3:07 pm, Stoiko Ivanov wrote:
> 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.

we did some more testing, and it's actually a tad bit more complicated..

so I'd adapt the commit message to say

else in the face of snapdir=visible on a ZFS-backed mountpoint/rootfs, creating
stop or suspend mode backups will fail under certain circumstances (because
automounting on access of .zfs/snapshot/XXX fails), and restoring a
(successful) backup made in suspend mode onto a ZFS 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 exact failure modes are as follows:
- suspend mode backups work, as long as the container is not restarted between
backups and only suspend mode is used
- suspend mode backups fail if snapshots have been auto-mounted and the
auto-mount was triggered outside the currently running container instance (i.e.,
by a stop mode backup, directly on the host, before a container reboot, ..)
- first stop mode backup is fine, if no snapshots have been auto-mounted yet
- stop mode backup fails if auto-mounting has happened in any fashion before the
backup was started

> 
> 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',
>>  	    ;
>>      }
>>  
> 
>