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

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

> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 22.04.2025 13:51 CEST geschrieben:
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
>  PVE/QemuConfig.pm       |   4 +-
>  PVE/QemuServer.pm       | 237 +++++++++++++++++++++++++++++++++++-----
>  PVE/QemuServer/Drive.pm |  39 ++++---
>  3 files changed, 239 insertions(+), 41 deletions(-)
> 

> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5cce7094..aff430df 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm

[..]

> @@ -4543,13 +4716,36 @@ sub qemu_volume_snapshot_delete {
>  	});
>      }
>  
> +    my $do_snapshots_with_qemu = do_snapshots_with_qemu($storecfg, $volid, $attached_deviceid);
> +
>      if ($attached_deviceid && do_snapshots_with_qemu($storecfg, $volid, $attached_deviceid)) {
> -	mon_cmd(
> -	    $vmid,
> -	    'blockdev-snapshot-delete-internal-sync',
> -	    device => $attached_deviceid,
> -	    name => $snap,
> -	);
> +	if ($do_snapshots_with_qemu == 2) {
> +	    mon_cmd(
> +		$vmid,
> +		'blockdev-snapshot-delete-internal-sync',
> +		device => $attached_deviceid,
> +		name => $snap,
> +	    );
> +	} elsif ($do_snapshots_with_qemu == 3) {
> +	    print "delete qemu external snapshot\n";
> +
> +	    my $path = PVE::Storage::path($storecfg, $volid);
> +	    my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
> +	    my $parentsnap = $snapshots->{$snap}->{parent};
> +	    my $childsnap = $snapshots->{$snap}->{child};
> +
> +	    # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
> +	    # improve-me: if firstsnap > child : commit, if firstsnap < child do a stream.
> +	    if(!$parentsnap) {
> +		print"delete first snapshot $snap\n";
> +		blockdev_commit($storecfg, $vmid, $attached_deviceid, $drive, $childsnap, $snap);
> +		blockdev_rename($storecfg, $vmid, $attached_deviceid, $drive, $snap, $childsnap, $snapshots->{$childsnap}->{child});
> +	    } else {
> +		#intermediate snapshot, we always stream the snapshot to child snapshot
> +		print"stream intermediate snapshot $snap to $childsnap\n";
> +		blockdev_stream($storecfg, $vmid, $attached_deviceid, $drive, $snap, $parentsnap, $childsnap);
> +	    }
> +	}

something here seems also broken, I did the following:

start VM
add disk on extsnap storage
create snapshot test
create snapshot test2

output of "qm listsnapshot 106":

qm listsnapshot 106
`-> test                        2025-05-13 11:59:11     no-description
 `-> test2                      2025-05-13 12:09:35     no-description
  `-> current                                           You are here!

outout of "lvs extsnap"

  LV                             VG      Attr       LSize  Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  snap-test-vm-106-disk-0.qcow2  extsnap -wi-a----- 36.00g
  snap-test2-vm-106-disk-0.qcow2 extsnap -wi-a----- 36.00g
  vm-106-disk-0.qcow2            extsnap -wi-a----- 36.00g

output of perl -e 'use PVE::Storage; use Data::Dumper; $Data::Dumper::Sortkeys = 1; print Dumper(PVE::Storage::volume_snapshot_info(PVE::Storage::config(), "extsnap:vm-106-disk-0.qcow2"));'

$VAR1 = {
          'current' => {
                         'ext' => 1,
                         'file' => '/dev/extsnap/vm-106-disk-0.qcow2',
                         'order' => 0,
                         'parent' => 'test2',
                         'volid' => 'extsnap:vm-106-disk-0.qcow2',
                         'volname' => 'vm-106-disk-0.qcow2'
                       },
          'test' => {
                      'child' => 'test2',
                      'ext' => 1,
                      'file' => '/dev/extsnap/snap-test-vm-106-disk-0.qcow2',
                      'order' => 2,
                      'volid' => 'extsnap:snap-test-vm-106-disk-0.qcow2',
                      'volname' => 'snap-test-vm-106-disk-0.qcow2'
                    },
          'test2' => {
                       'child' => 'current',
                       'ext' => 1,
                       'file' => '/dev/extsnap/snap-test2-vm-106-disk-0.qcow2',
                       'order' => 1,
                       'parent' => 'test',
                       'volid' => 'extsnap:snap-test2-vm-106-disk-0.qcow2',
                       'volname' => 'snap-test2-vm-106-disk-0.qcow2'
                     }
        };

removed snapshot test2 while VM is running:

delete qemu external snapshot
stream intermediate snapshot test2 to current
stream-drive-scsi1: transferred 309.0 MiB of 32.0 GiB (0.94%) in 0s
stream-drive-scsi1: stream-job finished
delete old /dev/extsnap/snap-test2-vm-106-disk-0.qcow2
TASK ERROR: error deleting snapshot test2

from journal:

May 13 12:36:29 pve74test01 pvedaemon[68741]: <root@pam> starting task UPID:pve74test01:00013D6E:0010F721:682320AD:qmdelsnapshot:106:root@pam:
May 13 12:36:29 pve74test01 pvedaemon[81262]: <root@pam> delete snapshot VM 106: test2
May 13 12:36:31 pve74test01 pvedaemon[81262]: VM 106 qmp command failed - VM 106 qmp command 'blockdev-del' failed - Failed to find node with node-name='e-VgFpR5u0cSSgGcquQm4semoCyWK'
May 13 12:36:31 pve74test01 pvedaemon[81262]: VM 106 qmp command failed - VM 106 qmp command 'blockdev-del' failed - Failed to find node with node-name='f-VgFpR5u0cSSgGcquQm4semoCyWK'
May 13 12:36:31 pve74test01 pvedaemon[81262]: error deleting snapshot test2
May 13 12:36:31 pve74test01 pvedaemon[68741]: <root@pam> end task UPID:pve74test01:00013D6E:0010F721:682320AD:qmdelsnapshot:106:root@pam: error deleting snapshot test2

starting over, VM not running, all traces of snapshots and old volumes removed.

add new disk on extsnap storage
create snapshot test
create snapshot test2

remove snapshot test:
commit: merge content of /dev/extsnap/snap-test2-vm-106-disk-0.qcow2 into /dev/extsnap/snap-test-vm-106-disk-0.qcow2
Image committed.
delete snap-test2-vm-106-disk-0.qcow2
TASK ERROR: error delete old snapshot volume snap-test2-vm-106-disk-0.qcow2: unable to parse lvm volume name 'snap-test2-vm-106-disk-0.qcow2' 

starting over, VM not running, all traces of snapshots and old volumes removed.

add new disk on extsnap storage
create snapshot test
create snapshot test2

remove snapshot test2:

rebase: merge diff content between /dev/extsnap/snap-test-vm-106-disk-0.qcow2 and /dev/extsnap/vm-106-disk-0.qcow2 into /dev/extsnap/vm-106-disk-0.qcow2
error delete old snapshot volume snap-test2-vm-106-disk-0.qcow2

what??

it seems to me you didn't really test the version you sent w.r.t. basic snapshot actions?

>      } else {
>  	PVE::Storage::volume_snapshot_delete(
>  	    $storecfg, $volid, $snap, $attached_deviceid ? 1 : undef);
> @@ -7778,27 +7974,16 @@ sub foreach_storage_used_by_vm {
>      }
>  }
>  
> -my $qemu_snap_storage = {
> -    rbd => 1,
> -};
>  sub do_snapshots_with_qemu {
>      my ($storecfg, $volid, $deviceid) = @_;
>  
> -    return if $deviceid =~ m/tpmstate0/;
> +    return if $deviceid && $deviceid =~ m/tpmstate0/;
>  
> -    my $storage_name = PVE::Storage::parse_volume_id($volid);
> -    my $scfg = $storecfg->{ids}->{$storage_name};
> -    die "could not find storage '$storage_name'\n" if !defined($scfg);
> +    my $snapshot_type = PVE::Storage::volume_has_feature($storecfg, 'snapshot', $volid);
>  
> -    if ($qemu_snap_storage->{$scfg->{type}} && !$scfg->{krbd}){
> -	return 1;
> -    }
> +    return $snapshot_type if $snapshot_type == 2 || $snapshot_type == 3;
>  
> -    if ($volid =~ m/\.(qcow2|qed)$/){
> -	return 1;
> -    }
> -
> -    return;
> +    return undef;
>  }
>  
>  sub qga_check_running {
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 0737034d..93903a59 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -27,6 +27,9 @@ print_drive
>  print_drive_throttle_group
>  generate_drive_blockdev
>  generate_throttle_group
> +generate_blockdev_throttle
> +generate_format_blockdev
> +generate_file_blockdev
>  );
>  
>  our $QEMU_FORMAT_RE = qr/raw|qcow|qcow2|qed|vmdk|cloop/;
> @@ -1074,6 +1077,8 @@ sub print_drive_throttle_group {
>  sub generate_file_blockdev {
>      my ($storecfg, $drive, $snap, $nodename) = @_;
>  
> +    $snap = undef if $snap && $snap eq 'current';
> +
>      my $volid = $drive->{file};
>      my $driveid = get_drive_id($drive);
>  
> @@ -1209,6 +1214,8 @@ sub generate_file_blockdev {
>  sub generate_format_blockdev {
>      my ($storecfg, $drive, $file, $snap, $nodename) = @_;
>  
> +    $snap = undef if $snap && $snap eq 'current';
> +
>      my $volid = $drive->{file};
>      #nbd don't support format blockdev, return the fileblockdev
>      return $file if $volid =~ /^nbd:/;
> @@ -1280,6 +1287,15 @@ sub generate_backing_chain_blockdev {
>      return $chain_blockdev;
>  }
>  
> +sub generate_blockdev_throttle {
> +    my ($drive, $blockdev_file) = @_;
> +
> +    my $drive_id = get_drive_id($drive);
> +    #this is the topfilter entry point, use $drive-drive_id as nodename
> +    my $blockdev_throttle = { driver => "throttle", 'node-name' => "drive-$drive_id", 'throttle-group' => "throttle-drive-$drive_id", 'file' => $blockdev_file };
> +    return $blockdev_throttle;
> +}
> +
>  sub generate_drive_blockdev {
>      my ($storecfg, $drive, $live_restore_name) = @_;
>  
> @@ -1303,22 +1319,19 @@ sub generate_drive_blockdev {
>      #pflash0 don't support throttle-filter
>      return $blockdev_format if $drive_id eq 'pflash0';
>  
> -    my $blockdev_live_restore = undef;
> -    if ($live_restore_name) {
> -        die "$drive_id: Proxmox Backup Server backed drive cannot auto-detect the format\n"
> -            if !$drive->{format};
> +    return generate_blockdev_throttle($drive, $blockdev_format) if !$live_restore_name;
>  
> -        $blockdev_live_restore = { 'node-name' => "liverestore-drive-$drive_id",
> -				    backing => $live_restore_name,
> -				    'auto-remove' => 'on', format => "alloc-track",
> -				    file => $blockdev_format };
> -    }
> +    die "$drive_id: Proxmox Backup Server backed drive cannot auto-detect the format\n"
> +        if !$drive->{format};
>  
> -    #this is the topfilter entry point, use $drive-drive_id as nodename
> -    my $blockdev_throttle = { driver => "throttle", 'node-name' => "drive-$drive_id", 'throttle-group' => "throttle-drive-$drive_id" };
>      #put liverestore filter between throttle && format filter
> -    $blockdev_throttle->{file} = $live_restore_name ? $blockdev_live_restore : $blockdev_format;
> -    return $blockdev_throttle,
> +    my $blockdev_live_restore = { 'node-name' => "liverestore-drive-$drive_id",
> +                                  backing => $live_restore_name,
> +                                  'auto-remove' => 'on', format => "alloc-track",
> +                                  file => $blockdev_format };
> +
> +    return generate_blockdev_throttle($drive, $blockdev_live_restore);
> +
>  }
>  
>  sub encode_base62 {
> -- 
> 2.39.5


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