[pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

[DERUMIER, Alexandre]

> +        snapext => { optional => 1 },

>>needs to be "fixed", as the code doesn't handle mixing internal
>>and external snapshots on a single storage..

indeed, I'll fix it

> 
>  
> +my sub alloc_backed_image {
> +    my ($class, $storeid, $scfg, $volname, $backing_snap) = @_;
> +
> +    my $path = $class->path($scfg, $volname, $storeid);
> +    my $backing_path = $class->path($scfg, $volname, $storeid,
> $backing_snap);

>>should we use a relative path here like we do when doing a linked
>>clone? else
>>it basically means that it is no longer possible to move the storage
>>mountpoint,
>>unless I am mistaken?

I dind't have thinked about mountpoint change, but yes, I'll not work
anymore without related path.

> +        #delete external snapshots
> +        if ($scfg->{snapext}) {
> +            my $snapshots = $class->volume_snapshot_info($scfg,
> $storeid, $volname);
> +            for my $snapid (
> +                sort { $snapshots->{$b}->{order} <=> $snapshots-
> >{$a}->{order} }
> +                keys %$snapshots
> +            ) {
> +                my $snap = $snapshots->{$snapid};
> +                next if $snapid eq 'current';
> +                next if !$snap->{ext};
> +                free_snap_image($class, $storeid, $scfg, $volname,
> $snapid);
> +            }
> +        }
> +

>>this is a bit tricky.. once we've deleted the first snapshot, we've
>>basically invalidated
>>the whole image..
Well, we want to delete it anyway, we don't care about invalidate it ?

>> should we try to continue freeing as much as possible? and maybe
even
>>start with the "current" image, so that a partial removal doesn't
>>look like valid image
>>anymore?

currently the volume_snapshot_info is reading the snapshot chain from
the current image (to do only 1 qemu-img call), that why I'm removing
snapshots in reverse order.
If something hang with partial delete, you can still try again later.
If we want to delete from the current to last snap, I'll need something
like calling qemu-info info on each snap file to find each parent.

or maybe use something else than volume_snapshot_info here, simply glob
all the vm disk && snap files and delete them in random order, as we
want to delete it anyway.







>>the naming scheme here still clashes with regular volids
unfortunately:

>>$ pvesm alloc ext4 12344321 snap-foobar-12344321-disk-foofoobar.qcow2
>>1G
>>Formatting '/mnt/pve/ext4/images/12344321/snap-foobar-12344321-disk-
>>foofoobar.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off
>>preallocation=off compression_type=zlib size=1073741824
>>lazy_refcounts=off refcount_bits=16
>>successfully created 'ext4:12344321/snap-foobar-12344321-disk-
>>foofoobar.qcow2'
>>$ pvesm list ext4 -content images -vmid 12344321 | grep foobar
>>ext4:12344321/snap-foobar-12344321-disk-foofoobar.qcow2 qcow2  
>>images    1073741824 12344321
>>$ qm set 12344321 --scsi0 ext4:12344321/snap-foobar-12344321-disk-
>>foofoobar.qcow2

>>should we maybe move snapshot files into a subdir, since `/` is not
>>allowed in volnames?

what about lvm ? (I think it should be great to have same scheme for
both, but lvm have also reserved characters like @)