[pve-devel] [PATCH] [FIX] Fix snapshot logic and enable lvmthin storage type.

[Dietmar Maurer]

comments inline

> On January 10, 2016 at 10:45 AM Gerrit Venema <gmoniker at gmail.com> wrote:
> 
> 
> This enables the container to use the lvmthin storage type if one is set up.
> It also fixes some logic in the handling of the freeze, errors on rollback
> and the handling of the config sections to keep a correct tree of snapshots.
> 
> Signed-off-by: Gerrit Venema <gmoniker at gmail.com>
> ---
>  src/PVE/LXC.pm | 54 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 6023334..6bd0d5d 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1795,11 +1795,12 @@ sub snapshot_create {
>  	my $rootinfo = parse_ct_mountpoint($conf->{rootfs});
>  	my $volid = $rootinfo->{volume};
>  
> +	PVE::Storage::volume_snapshot($storecfg, $volid, $snapname);
> +
>  	if ($running) {
>  	    PVE::Tools::run_command(['/usr/bin/lxc-unfreeze', '-n', $vmid]);
>  	};
>  
> -	PVE::Storage::volume_snapshot($storecfg, $volid, $snapname);
>  	&$snapshot_commit($vmid, $snapname);
>      };
>      if(my $err = $@) {

I applied a similar patch, which additionally makes sure that 
we unfreeze in case of errors.


> @@ -1840,17 +1841,17 @@ sub snapshot_delete {
>      my $del_snap =  sub {
>  
>  	check_lock($conf);
> -
> -	if ($conf->{parent} eq $snapname) {
> -	    if ($conf->{snapshots}->{$snapname}->{snapname}) {
> -		$conf->{parent} = $conf->{snapshots}->{$snapname}->{parent};
> -	    } else {
> -		delete $conf->{parent};
> +	my $grand_parent = $conf->{snapshots}->{$snapname}->{parent};
> +	foreach my $snapkey (keys %{$conf->{snapshots}}) {
> +	    if ($conf->{snapshots}->{$snapkey}->{parent} eq $snapname) {
> +		if ($grand_parent) {
> +		    $conf->{snapshots}->{$snapkey}->{parent} = $grand_parent;
> +		} else {
> +	            delete $conf->{snapshots}->{$snapkey}->{parent};
> +		}
>  	    }
>  	}

It is really hard to see the purpose of this code. I thought the current code
works? If not, would you mind to send a test case (example code) which triggers
the bug you want to fix?

> -
>  	delete $conf->{snapshots}->{$snapname};
> -
>  	write_config($vmid, $conf);
>      };
>  
> @@ -1904,30 +1905,25 @@ sub snapshot_rollback {
>  
>  	$conf->{lock} = 'rollback';
>  
> -	my $forcemachine;
> -
> -	# copy snapshot config to current config
> -
> -	my $tmp_conf = $conf;
> -	&$snapshot_copy_config($tmp_conf->{snapshots}->{$snapname}, $conf);
> -	$conf->{snapshots} = $tmp_conf->{snapshots};
> -	delete $conf->{snaptime};
> -	delete $conf->{snapname};
> -	$conf->{parent} = $snapname;
> -
>  	write_config($vmid, $conf);
> -    };
>  
> -    my $unlockfn = sub {
> -	delete $conf->{lock};
> -	write_config($vmid, $conf);
>      };
>  
>      lock_container($vmid, 10, $updatefn);
>  
> -    PVE::Storage::volume_snapshot_rollback($storecfg, $volid, $snapname);
> +    eval {
> +        PVE::Storage::volume_snapshot_rollback($storecfg, $volid, $snapname);
> +    };
> +
> +    die "Container stopped but the snapshot could not be rolled back.\n" . $@
> if $@;
> +
> +    # copy snapshot config to current config
>  
> -    lock_container($vmid, 5, $unlockfn);
> +    delete $conf->{parent};
> +    $conf->{parent} = $snapname;
> +    &$snapshot_copy_config($conf->{snapshots}->{$snapname}, $conf);
> +    delete $conf->{lock};
> +    write_config($vmid, $conf);
>  }

same here - what bug does it fix?

>  
>  sub template_create {
> @@ -2189,7 +2185,7 @@ sub mountpoint_mount {
>  	    if ($scfg->{path}) {
>  		push @extra_opts, '-o', 'loop';
>  		$use_loopdev = 1;
> -	    } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' ||
> $scfg->{type} eq 'rbd') {
> +	    } elsif ($scfg->{type} eq 'lvmthin' || $scfg->{type} eq 'drbd' ||
> $scfg->{type} eq 'lvm' || $scfg->{type} eq 'rbd') {
>  		# do nothing
>  	    } else {
>  		die "unsupported storage type '$scfg->{type}'\n";
> @@ -2321,6 +2317,10 @@ sub create_disks {
>  							   undef, 0);
>  			push @$chown_vollist, $volid;
>  		    }
> +		} elsif ($scfg->{type} eq 'lvmthin') {
> +			$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
> +							   undef, $size_kb);
> +			format_disk($storecfg, $volid, $rootuid, $rootgid);

This code duplication is not really necessary, so I applied a modified version.

>  		} elsif ($scfg->{type} eq 'zfspool') {
>  
>  		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid,
> 'subvol',
> -- 

I think this patch contains 3 different things:

1.) snapshot create bug fix
2.) snapshot config bug fix?
3.) enable lvmthin storage

It is usually better to split such patches.