Re: [pve-devel] [PATCH pve-storage 09/10] lvmplugin: add qcow2 snapshot

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

> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 04.07.2025 08:45 CEST geschrieben:
> we format lvm logical volume with qcow2 to handle snapshot chain.
> 
> like for qcow2 file, when a snapshot is taken, the current lvm volume
> is renamed to snap volname, and a new current lvm volume is created
> with the snap volname as backing file

list_images always returns format 'raw', this needs to be fixed..
the size probably should be queried as well for qcow2-formatted volumes

> 
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
>  src/PVE/Storage/LVMPlugin.pm | 493 +++++++++++++++++++++++++++++------
>  1 file changed, 412 insertions(+), 81 deletions(-)
> 
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 3d07260..ef010b8 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -11,6 +11,8 @@ use PVE::JSONSchema qw(get_standard_option);
>  
>  use PVE::Storage::Common;
>  
> +use JSON;
> +
>  use base qw(PVE::Storage::Plugin);
>  
>  # lvm helper functions
> @@ -267,6 +269,74 @@ sub lvm_list_volumes {
>      return $lvs;
>  }
>  
> +sub free_lvm_volumes {

shouldn't be public, and should be further below..

> +    my ($class, $scfg, $storeid, $volnames) = @_;
> +
> +    my $vg = $scfg->{vgname};

this variable here

> +
> +    # we need to zero out LVM data for security reasons
> +    # and to allow thin provisioning
> +    my $zero_out_worker = sub {
> +        # wipe throughput up to 10MB/s by default; may be overwritten with saferemove_throughput
> +        my $throughput = '-10485760';
> +        if ($scfg->{saferemove_throughput}) {
> +            $throughput = $scfg->{saferemove_throughput};
> +        }
> +        for my $name (@$volnames) {
> +            print "zero-out data on image $name (/dev/$vg/del-$name)\n";
> +
> +            my $cmd = [
> +                '/usr/bin/cstream',
> +                '-i',
> +                '/dev/zero',
> +                '-o',
> +                "/dev/$vg/del-$name",
> +                '-T',
> +                '10',
> +                '-v',
> +                '1',
> +                '-b',
> +                '1048576',
> +                '-t',
> +                "$throughput",
> +            ];
> +            eval {
> +                run_command(
> +                    $cmd,
> +                    errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> +                );
> +            };
> +            warn $@ if $@;
> +
> +            $class->cluster_lock_storage(
> +                $storeid,
> +                $scfg->{shared},
> +                undef,
> +                sub {
> +                    my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"];
> +                    run_command($cmd, errmsg => "lvremove '$vg/del-$name' error");
> +                },
> +            );
> +            print "successfully removed volume $name ($vg/del-$name)\n";
> +        }
> +    };
> +
> +    if ($scfg->{saferemove}) {
> +        for my $name (@$volnames) {
> +            # avoid long running task, so we only rename here
> +            my $cmd = ['/sbin/lvrename', $vg, $name, "del-$name"];
> +            run_command($cmd, errmsg => "lvrename '$vg/$name' error");
> +        }
> +        return $zero_out_worker;
> +    } else {
> +        for my $name (@$volnames) {
> +            my $tmpvg = $scfg->{vgname};

could also be used here..

> +            my $cmd = ['/sbin/lvremove', '-f', "$tmpvg/$name"];
> +            run_command($cmd, errmsg => "lvremove '$tmpvg/$name' error");
> +        }
> +    }
> +}
> +
>  # Configuration
>  
>  sub type {
> @@ -276,6 +346,7 @@ sub type {
>  sub plugindata {
>      return {
>          content => [{ images => 1, rootdir => 1 }, { images => 1 }],
> +        format => [{ raw => 1, qcow2 => 1 }, 'raw'],
>          'sensitive-properties' => {},
>      };
>  }
> @@ -354,7 +425,10 @@ sub parse_volname {
>      PVE::Storage::Plugin::parse_lvm_name($volname);
>  
>      if ($volname =~ m/^(vm-(\d+)-\S+)$/) {

here we don't have the same issue as in Plugin.pm, thankfully!

and it's only possible with raw storage access to create an LV
ending in .qcow2 that would confuse us down the line, so I think
this is okay..

> -        return ('images', $1, $2, undef, undef, undef, 'raw');
> +        my $name = $1;
> +        my $vmid = $2;
> +        my $format = $volname =~ m/\.qcow2$/ ? 'qcow2' : 'raw';
> +        return ('images', $name, $vmid, undef, undef, undef, $format);
>      }
>  
>      die "unable to parse lvm volume name '$volname'\n";
> @@ -363,17 +437,29 @@ sub parse_volname {
>  sub filesystem_path {
>      my ($class, $scfg, $volname, $snapname) = @_;
>  
> -    die "lvm snapshot is not implemented" if defined($snapname);
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
>  
> -    my ($vtype, $name, $vmid) = $class->parse_volname($volname);
> +    die "snapshot is working with qcow2 format only" if defined($snapname) && $format ne 'qcow2';
>  
>      my $vg = $scfg->{vgname};
> +    $name = $class->get_snap_name($volname, $snapname) if $snapname;
>  
>      my $path = "/dev/$vg/$name";
>  
>      return wantarray ? ($path, $vmid, $vtype) : $path;
>  }
>  
> +sub qemu_blockdev_options {
> +    my ($class, $scfg, $storeid, $volname, $machine_version, $options) = @_;
> +
> +    my ($path) = $class->path($scfg, $volname, $storeid, $options->{'snapshot-name'});
> +
> +    my $blockdev = { driver => 'host_device', filename => $path };
> +
> +    return $blockdev;
> +}
> +
>  sub create_base {
>      my ($class, $storeid, $scfg, $volname) = @_;
>  
> @@ -395,7 +481,11 @@ sub find_free_diskname {
>  
>      my $disk_list = [keys %{ $lvs->{$vg} }];
>  
> -    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
> +    $add_fmt_suffix = $fmt eq 'qcow2' ? 1 : undef;
> +
> +    return PVE::Storage::Plugin::get_next_vm_diskname(
> +        $disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix,
> +    );
>  }
>  
>  sub lvcreate {
> @@ -423,13 +513,46 @@ sub lvrename {
>      );
>  }
>  
> -sub alloc_image {
> -    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> +my sub lvm_qcow2_format {
> +    my ($class, $storeid, $scfg, $name, $fmt, $backing_snap, $size) = @_;
> +
> +    die "Can't format the volume, the format is not qcow2" if $fmt ne 'qcow2';

either drop this (it's already checked at all call sites)

> +
> +    $class->activate_volume($storeid, $scfg, $name);
> +    my $path = $class->path($scfg, $name, $storeid);
> +    my $backing_path = $class->path($scfg, $name, $storeid, $backing_snap) if $backing_snap;

not allowed (post if + declaration)
also, this should probably encode a relative path so that renaming the VG and
adapting the storage.cfg entry works without breaking the back reference?

> +    $size = undef if $backing_snap;

could be split into

if ($backing_snap) {
    my $backing_path = $class->path($scfg, $name, $storeid, $backing_snap);
    PVE::Storage::Common::qemu_img_create($scfg, 'qcow2', undef, $path, $backing_path);
} else {
    PVE::Storage::Common::qemu_img_create($scfg, 'qcow2', $size, $path);
}

> +    PVE::Storage::Common::qemu_img_create($scfg, 'qcow2', $size, $path, $backing_path);

or if you keep the check above, use $fmt for this/these ;)

> +
> +}
> +
> +my sub calculate_lvm_size {
> +    my ($size, $fmt, $backing_snap) = @_;
> +    #input size = qcow2 image size in kb
> +
> +    return $size if $fmt ne 'qcow2';
>  
> -    die "unsupported format '$fmt'" if $fmt ne 'raw';
> +    my $options = $backing_snap ? ['extended_l2=on', 'cluster_size=128k'] : [];
> +
> +    my $json = PVE::Storage::Common::qemu_img_measure($size, $fmt, 5, $options);
> +    die "failed to query file information with qemu-img measure\n" if !$json;
> +    my $info = eval { decode_json($json) };
> +    if ($@) {
> +        die "Invalid JSON: $@\n";
> +    }
> +
> +    die "Missing fully-allocated value from json" if !$info->{'fully-allocated'};
> +
> +    return $info->{'fully-allocated'} / 1024;
> +}
> +
> +my sub alloc_lvm_image {
> +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size, $backing_snap) = @_;
> +
> +    die "unsupported format '$fmt'" if $fmt !~ m/(raw|qcow2)/;

missing anchors, but this could just be `$fmt ne 'raw' && $fmt ne 'qcow2'`..
but how should we end up here? the whole storage plugin only allows those
two formats anyway..

>  
>      die "illegal name '$name' - should be 'vm-$vmid-*'\n"
> -        if $name && $name !~ m/^vm-$vmid-/;
> +        if $name !~ m/^vm-$vmid-/;

parse_volname should be enough - VMs can reference volumes owned by other VMs..
then we can also verify that the name and the format match, because right now
it's possible to do

$ pvesm alloc lvm 126 vm-126-disk-1 1G --format qcow2
  Rounding up size to full physical extent 1.00 GiB
  Logical volume "vm-126-disk-1" created.
Formatting '/dev/lvm/vm-126-disk-1', fmt=qcow2 cluster_size=65536 extended_l2=off preallocation=metadata compression_type=zlib size=1073741824 lazy_refcounts=off refcount_bits=16
successfully created 'lvm:vm-126-disk-1'
$  pvesm list lvm
Volid             Format  Type            Size VMID
lvm:vm-126-disk-1 raw     images    1077936128 126

(the other way round is not possible, because the API catches it)

>  
>      my $vgs = lvm_vgs();
>  
> @@ -438,86 +561,98 @@ sub alloc_image {
>      die "no such volume group '$vg'\n" if !defined($vgs->{$vg});
>  
>      my $free = int($vgs->{$vg}->{free});
> +    my $lvmsize = calculate_lvm_size($size, $fmt, $backing_snap);
>  
>      die "not enough free space ($free < $size)\n" if $free < $size;
>  
> -    $name = $class->find_free_diskname($storeid, $scfg, $vmid)
> +    my $tags = ["pve-vm-$vmid"];
> +    #tags all snapshots volumes with the main volume tag for easier activation of the whole group
> +    push @$tags, "\@pve-$name" if $fmt eq 'qcow2';
> +    lvcreate($vg, $name, $lvmsize, $tags);
> +
> +    return if $fmt ne 'qcow2';
> +
> +    #format the lvm volume with qcow2 format
> +    eval { lvm_qcow2_format($class, $storeid, $scfg, $name, $fmt, $backing_snap, $size) };
> +    if ($@) {
> +        my $err = $@;
> +        #no need to safe cleanup as the volume is still empty
> +        eval {
> +            my $cmd = ['/sbin/lvremove', '-f', "$vg/$name"];
> +            run_command($cmd, errmsg => "lvremove '$vg/$name' error");
> +        };
> +        die $err;
> +    }
> +
> +}
> +
> +sub alloc_image {
> +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> +
> +    $name = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
>          if !$name;
>  
> -    lvcreate($vg, $name, $size, ["pve-vm-$vmid"]);
> +    alloc_lvm_image($class, $storeid, $scfg, $vmid, $fmt, $name, $size);
>  
>      return $name;
>  }
>  
> -sub free_image {
> -    my ($class, $storeid, $scfg, $volname, $isBase) = @_;
> +sub alloc_snap_image {

does this need to be public?

> +    my ($class, $storeid, $scfg, $volname, $backing_snap) = @_;
>  
> -    my $vg = $scfg->{vgname};
> +    my ($vmid, $format) = ($class->parse_volname($volname))[2, 6];
> +    my $path = $class->path($scfg, $volname, $storeid, $backing_snap);
>  
> -    # we need to zero out LVM data for security reasons
> -    # and to allow thin provisioning
> +    #we need to use same size than the backing image qcow2 virtual-size
> +    my $size = PVE::Storage::Plugin::file_size_info($path, 5, $format);
> +    $size = $size / 1024; #we use kb in lvcreate
>  
> -    my $zero_out_worker = sub {
> -        print "zero-out data on image $volname (/dev/$vg/del-$volname)\n";
> +    alloc_lvm_image($class, $storeid, $scfg, $vmid, $format, $volname, $size, $backing_snap);
> +}
>  
> -        # wipe throughput up to 10MB/s by default; may be overwritten with saferemove_throughput
> -        my $throughput = '-10485760';
> -        if ($scfg->{saferemove_throughput}) {
> -            $throughput = $scfg->{saferemove_throughput};
> -        }
> +sub free_snap_image {

same question here?

> +    my ($class, $storeid, $scfg, $volname, $snap) = @_;
>  
> -        my $cmd = [
> -            '/usr/bin/cstream',
> -            '-i',
> -            '/dev/zero',
> -            '-o',
> -            "/dev/$vg/del-$volname",
> -            '-T',
> -            '10',
> -            '-v',
> -            '1',
> -            '-b',
> -            '1048576',
> -            '-t',
> -            "$throughput",
> -        ];
> -        eval {
> -            run_command(
> -                $cmd,
> -                errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> -            );
> -        };
> -        warn $@ if $@;
> +    #activate only the snapshot volume
> +    my $path = $class->path($scfg, $volname, $storeid, $snap);
> +    my $cmd = ['/sbin/lvchange', '-aly', $path];
> +    run_command($cmd, errmsg => "can't activate LV '$path' to zero-out its data");
> +    $cmd = ['/sbin/lvchange', '--refresh', $path];
> +    run_command($cmd, errmsg => "can't refresh LV '$path' to zero-out its data");
>  
> -        $class->cluster_lock_storage(
> -            $storeid,
> -            $scfg->{shared},
> -            undef,
> -            sub {
> -                my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$volname"];
> -                run_command($cmd, errmsg => "lvremove '$vg/del-$volname' error");
> -            },
> -        );
> -        print "successfully removed volume $volname ($vg/del-$volname)\n";
> -    };
> +    my $snap_volname = $class->get_snap_volname($volname, $snap);
> +    return $class->free_lvm_volumes($scfg, $storeid, [$snap_volname]);
> +}
>  
> -    my $cmd = ['/sbin/lvchange', '-aly', "$vg/$volname"];
> -    run_command($cmd, errmsg => "can't activate LV '$vg/$volname' to zero-out its data");
> -    $cmd = ['/sbin/lvchange', '--refresh', "$vg/$volname"];
> -    run_command($cmd, errmsg => "can't refresh LV '$vg/$volname' to zero-out its data");
> +sub free_image {
> +    my ($class, $storeid, $scfg, $volname, $isBase, $format) = @_;
>  
> -    if ($scfg->{saferemove}) {
> -        # avoid long running task, so we only rename here
> -        $cmd = ['/sbin/lvrename', $vg, $volname, "del-$volname"];
> -        run_command($cmd, errmsg => "lvrename '$vg/$volname' error");
> -        return $zero_out_worker;
> -    } else {
> -        my $tmpvg = $scfg->{vgname};
> -        $cmd = ['/sbin/lvremove', '-f', "$tmpvg/$volname"];
> -        run_command($cmd, errmsg => "lvremove '$tmpvg/$volname' error");
> +    my $name = ($class->parse_volname($volname))[1];
> +
> +    #activate volumes && snapshot volumes

this is only needed for qcow2-formatted volumes or if zeroing is enabled, right?

so we should guard it accordingly..

> +    my $path = $class->path($scfg, $volname, $storeid);
> +    $path = "\@pve-$name" if $format && $format eq 'qcow2';
> +    my $cmd = ['/sbin/lvchange', '-aly', $path];
> +    run_command($cmd, errmsg => "can't activate LV '$path' to zero-out its data");
> +    $cmd = ['/sbin/lvchange', '--refresh', $path];
> +    run_command($cmd, errmsg => "can't refresh LV '$path' to zero-out its data");
> +
> +    my $volnames = [];
> +    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->{volid};
> +        next if !$snap->{ext};
> +        my ($snap_storeid, $snap_volname) = PVE::Storage::parse_volume_id($snap->{volid});
> +        push @$volnames, $snap_volname;
>      }
> +    push @$volnames, $volname;
>  
> -    return undef;
> +    return $class->free_lvm_volumes($scfg, $storeid, $volnames);
>  }
>  
>  my $check_tags = sub {
> @@ -624,6 +759,12 @@ sub activate_volume {
>  
>      my $lvm_activate_mode = 'ey';
>  
> +    #activate volume && all snapshots volumes by tag
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
> +
> +    $path = "\@pve-$name" if $format eq 'qcow2';
> +
>      my $cmd = ['/sbin/lvchange', "-a$lvm_activate_mode", $path];
>      run_command($cmd, errmsg => "can't activate LV '$path'");
>      $cmd = ['/sbin/lvchange', '--refresh', $path];
> @@ -636,6 +777,10 @@ sub deactivate_volume {
>      my $path = $class->path($scfg, $volname, $storeid, $snapname);
>      return if !-b $path;
>  
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
> +    $path = "\@pve-$name" if $format eq 'qcow2';
> +
>      my $cmd = ['/sbin/lvchange', '-aln', $path];
>      run_command($cmd, errmsg => "can't deactivate LV '$path'");
>  }
> @@ -643,10 +788,14 @@ sub deactivate_volume {
>  sub volume_resize {
>      my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
>  
> -    $size = ($size / 1024 / 1024) . "M";
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
> +
> +    my $lvmsize = calculate_lvm_size($size / 1024, $format);
> +    $lvmsize = "${lvmsize}k";
>  
>      my $path = $class->path($scfg, $volname);
> -    my $cmd = ['/sbin/lvextend', '-L', $size, $path];
> +    my $cmd = ['/sbin/lvextend', '-L', $lvmsize, $path];
>  
>      $class->cluster_lock_storage(
>          $storeid,
> @@ -657,12 +806,18 @@ sub volume_resize {
>          },
>      );
>  
> +    if (!$running && $format eq 'qcow2') {
> +        my $prealloc_opt = PVE::Storage::Plugin::preallocation_cmd_option($scfg, $format);

this got moved to Common in your series (but I'd like to keep it were it was, see comments there)

> +        my $cmd = ['/usr/bin/qemu-img', 'resize', "--$prealloc_opt", '-f', $format, $path, $size];
> +        run_command($cmd, timeout => 10);
> +    }
> +
>      return 1;
>  }
>  
>  sub volume_size_info {
> -    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> -    my $path = $class->filesystem_path($scfg, $volname);
> +    my ($class, $scfg, $storeid, $volname, $timeout, $snap) = @_;

this is never called with $snap set?

> +    my $path = $class->filesystem_path($scfg, $volname, $snap);
>  
>      my $cmd = [
>          '/sbin/lvs',
> @@ -693,30 +848,191 @@ sub volume_size_info {
>  sub volume_snapshot {
>      my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
>  
> -    die "lvm snapshot is not implemented";
> +    my ($vmid, $format) = ($class->parse_volname($volname))[2, 6];
> +
> +    die "can't snapshot this image format\n" if $format ne 'qcow2';

die "cannot snapshot '$format' volume\n" if $format ne 'qcow2';

> +
> +    if ($running) {
> +        #rename with blockdev-reopen is done at qemu level when running
> +        $class->alloc_snap_image($storeid, $scfg, $volname, $snap);
> +        if ($@) {
> +            die "can't allocate new volume $volname: $@\n";
> +        }

missing eval?

> +        return;
> +    }
> +
> +    #rename current volume to snap volume
> +    eval { $class->rename_volume($scfg, $storeid, $volname, $vmid, undef, 'current', $snap) };

same comment as with the Plugin/DirPlugin patch with regard to rename_volume vs rename_snapshot

> +    die "error rename $volname to $snap\n" if $@;
> +
> +    eval { $class->alloc_snap_image($storeid, $scfg, $volname, $snap) };
> +    if ($@) {
> +        my $err = $@;
> +        eval { $class->rename_volume($scfg, $storeid, $volname, $vmid, undef, $snap, 'current') };
> +        die $err;
> +    }
> +}
> +
> +sub volume_rollback_is_possible {
> +    my ($class, $scfg, $storeid, $volname, $snap, $blockers) = @_;
> +
> +    my $snap_path = $class->path($scfg, $volname, $storeid, $snap);
> +
> +    $class->activate_volume($storeid, $scfg, $volname, undef, {});
> +    my $snapshots = $class->volume_snapshot_info($scfg, $storeid, $volname);
> +    my $parent_snap = $snapshots->{current}->{parent};
> +
> +    return 1 if $parent_snap eq $snap;
> +    die "can't rollback, '$snap' is not most recent snapshot on '$volname'\n";

should populate $blockers since we have the info already..

> +
> +    return 1;
>  }
>  
>  sub volume_snapshot_rollback {
>      my ($class, $scfg, $storeid, $volname, $snap) = @_;
>  
> -    die "lvm snapshot rollback is not implemented";
> +    my $format = ($class->parse_volname($volname))[6];
> +
> +    die "can't rollback snapshot for this image format\n" if $format ne 'qcow2';

please include the format in the message!

> +
> +    $class->activate_volume($storeid, $scfg, $volname, undef, {});

last two parameters can be dropped

> +
> +    # we can simply reformat the current lvm volume to avoid
> +    # a long safe remove.(not needed here, as the allocated space
> +    # is still the same owner)
> +    eval { lvm_qcow2_format($class, $storeid, $scfg, $volname, $format, $snap) };

what if the volume got resized along the way?

> +    if ($@) {
> +        die "can't rollback. Error reformating current $volname\n";
> +    }
> +    return undef;
>  }
>  
>  sub volume_snapshot_delete {
> -    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> +    my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
> +
> +    die "can't delete snapshot for this image format\n" if $format ne 'qcow2';

please include the format in the message!

> +
> +    if ($running) {
> +        my $cleanup_worker = eval { $class->free_snap_image($storeid, $scfg, $volname, $snap); };
> +        die "error deleting snapshot $snap $@\n" if $@;
> +
> +        if ($cleanup_worker) {
> +            my $rpcenv = PVE::RPCEnvironment::get();
> +            my $authuser = $rpcenv->get_user();
> +            $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
> +        }
> +        return;
> +    }
> +
> +    my $cmd = "";
> +    my $path = $class->filesystem_path($scfg, $volname);
> +
> +    my $snapshots = $class->volume_snapshot_info($scfg, $storeid, $volname);
> +    my $snappath = $snapshots->{$snap}->{file};
> +    my $snapvolname = $snapshots->{$snap}->{volname};
> +    die "volume $snappath is missing" if !-e $snappath;
> +
> +    my $parentsnap = $snapshots->{$snap}->{parent};
> +
> +    my $childsnap = $snapshots->{$snap}->{child};
> +    my $childpath = $snapshots->{$childsnap}->{file};
> +    my $childvolname = $snapshots->{$childsnap}->{volname};
> +
> +    my $cleanup_worker = undef;
> +    my $err = undef;
> +    #if first snapshot,as it should be bigger,  we merge child, and rename the snapshot to child
> +    if (!$parentsnap) {
> +        print "$volname: deleting snapshot '$snap' by commiting snapshot '$childsnap'\n";
> +        print "running 'qemu-img commit $childpath'\n";
> +        #can't use -d here, as it's an lvm volume
> +        $cmd = ['/usr/bin/qemu-img', 'commit', $childpath];
> +        eval { run_command($cmd) };
> +        if ($@) {
> +            warn
> +                "The state of $snap is now invalid. Don't try to clone or rollback it. You can only try to delete it again later\n";
> +            die "error commiting $childsnap to $snap; $@\n";
> +        }
> +        print "delete $childvolname\n";
> +        $cleanup_worker = eval { $class->free_snap_image($storeid, $scfg, $volname, $childsnap) };
> +        if ($@) {
> +            die "error delete old snapshot volume $childvolname: $@\n";
> +        }
> +
> +        print "rename $snapvolname to $childvolname\n";
> +        my $vg = $scfg->{vgname};
> +        eval { lvrename($vg, $snapvolname, $childvolname) };
> +        if ($@) {
> +            warn $@;
> +            $err = "error renaming snapshot: $@\n";
> +        }
>  
> -    die "lvm snapshot delete is not implemented";
> +    } else {
> +        #we rebase the child image on the parent as new backing image
> +        my $parentpath = $snapshots->{$parentsnap}->{file};
> +        print
> +            "$volname: deleting snapshot '$snap' by rebasing '$childsnap' on top of '$parentsnap'\n";
> +        print "running 'qemu-img rebase -b $parentpath -F qcow -f qcow2 $childpath'\n";
> +        $cmd = [
> +            '/usr/bin/qemu-img',
> +            'rebase',
> +            '-b',
> +            $parentpath,
> +            '-F',
> +            'qcow2',
> +            '-f',
> +            'qcow2',
> +            $childpath,
> +        ];
> +        eval { run_command($cmd) };
> +        if ($@) {
> +            #in case of abort, the state of the snap is still clean, just a little bit bigger
> +            die "error rebase $childsnap from $parentsnap; $@\n";
> +        }
> +        #delete the snapshot
> +        eval { $cleanup_worker = $class->free_snap_image($storeid, $scfg, $volname, $snap); };
> +        if ($@) {
> +            die "error deleting old snapshot volume $snapvolname\n";
> +        }
> +    }
> +    if ($cleanup_worker) {

only doing this here is a tiny bit dangerous, since if somebody adds a die before it
the cleanup never happens.. should we maybe add these 5 lines as a $fork_cleanup helper
in this sub, and then just call it above?
 
> +        my $rpcenv = PVE::RPCEnvironment::get();
> +        my $authuser = $rpcenv->get_user();
> +        $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
> +    }
> +
> +    die $err if $err;
>  }
>  
>  sub volume_has_feature {
>      my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
>  
>      my $features = {
> -        copy => { base => 1, current => 1 },
> -        rename => { current => 1 },
> +        copy => {
> +            base => { qcow2 => 1, raw => 1 },
> +            current => { qcow2 => 1, raw => 1 },
> +            snap => { qcow2 => 1 },
> +        },
> +        'rename' => {
> +            current => { qcow2 => 1, raw => 1 },
> +        },
> +        snapshot => {
> +            current => { qcow2 => 1 },
> +            snap => { qcow2 => 1 },
> +        },
> +        #       fixme: add later ? (we need to handle basepath, volume activation,...)
> +        #       template => {
> +        #           current => { raw => 1, qcow2 => 1},
> +        #       },
> +        #       clone => {
> +        #           base => { qcow2 => 1 },
> +        #       },
>      };
>  
> -    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) = $class->parse_volname($volname);
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
>  
>      my $key = undef;
>      if ($snapname) {
> @@ -724,7 +1040,7 @@ sub volume_has_feature {
>      } else {
>          $key = $isBase ? 'base' : 'current';
>      }
> -    return 1 if $features->{$feature}->{$key};
> +    return 1 if defined($features->{$feature}->{$key}->{$format});
>  
>      return undef;
>  }
> @@ -868,4 +1184,19 @@ sub rename_volume {
>      return "${storeid}:${target_volname}";
>  }
>  
> +sub get_snap_name {
> +    my ($class, $volname, $snapname) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> +        $class->parse_volname($volname);
> +    $name = !$snapname || $snapname eq 'current' ? $name : "snap-$snapname-$name";
> +    return $name;
> +}
> +
> +sub get_snap_volname {
> +    my ($class, $volname, $snapname) = @_;
> +
> +    return $class->get_snap_name($volname, $snapname);
> +}
> +
>  1;
> -- 
> 2.39.5


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