[pve-devel] [PATCH v2 pve-storage 1/2] add external snasphot support

[Fabian Grünbichler]

> Fabian Grünbichler <f.gruenbichler at proxmox.com> hat am 23.10.2024 12:12 CEST geschrieben:
> 
>  
> some high level comments:
> 
> I am not sure how much we gain here with the raw support? it's a bit confusing to have a volid ending with raw, with the current volume and all but the first snapshot actually being stored in qcow2 files, with the raw file being the "oldest" snapshot in the chain..
> 
> if possible, I'd be much happier with the snapshot name in the snapshot file being a 1:1 match, see comments inline
> - makes it a lot easier to understand (admin wants to manually remove snapshot "foo", if "foo" was the last snapshot then right now the volume called "foo" is actually the current contents!)
> - means we don't have to do lookups via the full snapshot list all the time (e.g., if I want to do a full clone from a snapshot "foo", I can just pass the snap-foo volume to qemu-img)
> 
> the naming scheme for snapshots needs to be adapted to not clash with regular volumes:
> 
> $ pvesm alloc extsnap 131314 vm-131314-disk-foobar.qcow2 2G
> Formatting '/mnt/pve/ext4/extsnap/images/131314/vm-131314-disk-foobar.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off preallocation=off compression_type=zlib size=2147483648 lazy_refcounts=off refcount_bits=16
> successfully created 'extsnap:131314/vm-131314-disk-foobar.qcow2'
> $ qm rescan --vmid 131314
> rescan volumes...
> can't parse snapname from path at /usr/share/perl5/PVE/Storage/Plugin.pm line 1934.
> 
> storage_migrate needs to handle external snapshots, or at least error out. I haven't tested that part or linked clones or a lot of other advanced related actions at all ;)

I'll add some more high-level comments (the threading seems to be broken for some reason, so I'll use this as "entrypoint"):

- snapext should probably be fixed for dir-type storages as well
- the volume ID should be static for both plugins, snapshots should be encoded on the storage layer in a fashion that doesn't "break through" to the API layers and makes it impossible to confuse the "main" volname with snapshots:
-- alloc_image shouldn't be able to allocate a volume that is then interpreted as snapshot
-- free_image shouldn't be able to free a snapshot volume directly
-- listing images should never return snapshots
-- ..
- for the LVM part, snapshots still require allocation a full-sized volume+some overhead for the qcow2 each. should we attempt to shrink them once they become read-only? in practice, only the LV backing the top image needs to be full-sized.. how do we ensure the underlying storage doesn't waste all that "empty" space?