Re: [pve-devel] [PATCH qemu-server 12/14] qemu_img convert : add external snapshot support

["DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 16245 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 12/14] qemu_img convert : add external snapshot support
Date: Tue, 27 May 2025 13:48:51 +0000
Message-ID: <fe6ff7f68a7bd2aae347e6c7630617495b6ae365.camel@groupe-cyllene.com>

Hi,
>  
>      my $cmd = [];
>      push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> -    push @$cmd, '-l', "snapshot.name=$snapname"
> - if $snapname && $src_format && $src_format eq "qcow2";
> +    push @$cmd, '-l', "snapshot.name=$snapname" if $snapname &&
> $snapshot_type == 2;

>>this breaks backwards compatibility with external plugins that
>>override volume_has_features, and
>>thus still return '1' while using internal qcow2 snapshots.. I think
>>we need to detect that and
>>fallback here? or we need to differentiate all four cases:

>>1: plugin not changed, fallback to format-based decision
>>2: storage snapshot, no need to pass snapshot name to qemu-img
>>3: qcow2-internal snapshot, need to pass snapshot name to qemu-img
>>4: qcow2-external snapshot, no need to pass snapshot name to qemu-img
>>
>>?
>>
>>at that point, it might make more sense to actually name those
>>values:
>>
>>1: plugin not changed, 'legacy' snapshot support
>>'storage': same semantics as '1', but plugin was updated/doesn't
>>override volume_has_features
>>'file-internal': same as 3 above
>>'file-external': same as 4 above

>> or if we want to keep volume_has_features the same, we could
>>introduce a new helper that
>>returns the snapshot type?


I was working on this today (I have send a test patch for missing rbd
&& btrfs test).

Maybe it could be simplier indeed.

has_features->{snapshot}->{raw} = 1 :  it's always a storage snapshot
(assuming we don't want to implement external qcow2 snapshot with raw
backing)

has_features->{snapshot}->{qcow2|other_format_than_raw)} = 1 :  qemu
internal snapshot
has_features->{snapshot}->{qcow2} = 2 :  qemu external snapshot


and could be call like

if ($fmt eq 'raw') {
     type = 'storage'
} else {
   if (has_features->{snapshot}->{$fmt} == 2) {
       type = 'file-external'
   } else {
       type = 'file-internal'
   }
} 

what do you think ? Should not break current plugins, and it should
work for any plugin



or we can add a new helper like

PVE::Storage::snapshot_type($storecfg, $volid)

and in default plugin something like

sub snapshot_type {
....
die "missing snapshot feature" if !has_features->{snapshot}->{$fmt};
if ($fmt eq 'raw') {
       type = 'storage'
} elseif ($fmt eq 'qcow2') {
       type = 'internal'
} else {
   die "unsupported format";
}


with add specific 'external' type for dirplug with snapext && lvm with
qcow2















[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel