[pve-devel] [PATCH] [FEATURE] Enable snapshot rollback and delete on a thinpool volume.

[Dietmar Maurer]

Some questions inline:

> On January 10, 2016 at 10:32 AM Gerrit Venema <gmoniker at gmail.com> wrote:
> 
> 
> Also little tweaks to ensure activation and bypass free check for size.
> 
> Signed-off-by: Gerrit Venema <gmoniker at gmail.com>
> ---
>  PVE/Storage/LVMPlugin.pm     |  8 +++++---
>  PVE/Storage/LvmThinPlugin.pm | 26 ++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index d776e43..d658fea 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -272,9 +272,11 @@ sub alloc_image {
>  
>      die "no such volume group '$vg'\n" if !defined ($vgs->{$vg});
>  
> -    my $free = int($vgs->{$vg}->{free});
> +    if ($scfg->{type} ne 'lvmthin') {

lvmthin overwrites alloc_image, and never calls this function. So what is
the purpose of this check?

> +        my $free = int($vgs->{$vg}->{free});
>  
> -    die "not enough free space ($free < $size)\n" if $free < $size;
> +        die "not enough free space ($free < $size)\n" if $free < $size;
> +    }
>  
>      $name = lvm_find_free_diskname(lvm_list_volumes($vg), $vg, $storeid,
> $vmid)
>  	if !$name;
> @@ -283,7 +285,7 @@ sub alloc_image {
>  
>      run_command($cmd, errmsg => "lvcreate '$vg/pve-vm-$vmid' error");
>  
> -    return $name;
> +    return $vmid/$name;
>  }

why?

>  sub free_image {
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index 5e404df..48ba4c9 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -189,14 +189,19 @@ sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
>      my $vg = $scfg->{vgname};
> -
> +    # For safety, first activate thinpool
> +    my $tcmd = ['/sbin/lvchange', '-ay', '-K', "$vg/$scfg->{thinpool}"];
> +    run_command($tcmd, errmsg => "activate_volume '$vg/$scfg->{thinpool}'
> thinpool error");
>      # only snapshot volumes needs activation
>      if ($snapname) {
>  	my $snapvol = "snap_${volname}_$snapname";
>  	my $cmd = ['/sbin/lvchange', '-ay', '-K', "$vg/$snapvol"];
>  	run_command($cmd, errmsg => "activate_volume '$vg/$snapvol' error");
> -    } else {
> +    } elsif ($volname) {
>  	# other volumes are active by default
> +	# but it doesn't hurt to make sure
> +	my $cmd = ['/sbin/lvchange', '-ay', '-K', "$vg/$volname"];
> +	run_command($cmd, errmsg => "activate_volume '$vg/$volname' error");

Do you run into problems? Or is this just to be sure?

>      }
>  }
>  
> @@ -293,8 +298,14 @@ sub volume_snapshot_rollback {
>      my $cmd = ['/sbin/lvremove', '-f', "$vg/$volname"];
>      run_command($cmd, errmsg => "lvremove '$vg/$volname' error");
>  
> -    $cmd = ['/sbin/lvcreate', '-kn', '-n', $volname, '-s', "$vg/$snapvol"];
> -    run_command($cmd, errmsg => "lvm rollback '$vg/$snapvol' error");
> +    $cmd = ['/sbin/lvrename', $vg, "$vg/$snapvol", $volname];
> +    run_command($cmd, errmsg => "lvrename '$vg/$snapvol to $volname' error");
> +
> +    $cmd = ['/sbin/lvchange', "$vg/$volname", '-kn', '-ay', '-prw'];
> +    run_command($cmd, errmsg => "lvchange '$vg/$volname' error");
> +
> +    #Make a new snapshot to replace the previous one
> +    volume_snapshot($class, $scfg, $storeid, $volname, $snap);
>  }

Snapshot rollback already work. So what is the purpose of this change?
Can you provide a test case (example code) where the original code fails?

>  sub volume_snapshot_delete {
> @@ -303,8 +314,11 @@ sub volume_snapshot_delete {
>      my $vg = $scfg->{vgname};
>      my $snapvol = "snap_${volname}_$snap";
>  
> -    my $cmd = ['/sbin/lvremove', '-f', "$vg/$snapvol"];
> -    run_command($cmd, errmsg => "lvremove snapshot '$vg/$snapvol' error");
> +    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> +    if ($lvs->{$snapvol}) {
> +	my $cmd = ['/sbin/lvremove', '-f', "$vg/$snapvol"];
> +	run_command($cmd, errmsg => "lvremove snapshot '$vg/$snapvol' error");
> +    }

What is wrong with the original code? Other plugins also throw an error if the
snapshot does not exists, so I do not really see a need for this change?