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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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