[pve-devel] [PATCH v3 container 3/5] use copy_volume for full clones

[Fabian Grünbichler]

meta, and not related to this patch series (same probably applies to
qemu-server as well)
- shouldn't we lock the source config as well, and drop the flock before
  forking the worker? (note: if we ever extend our locking code, we
  could even support nested config locking for parallel clones of the
  same source container ;))
- we don't actually hold an flock on the new config between checking it
  does not exist and first creating it with file_set_contents

also see the comment inline

On Tue, Sep 26, 2017 at 02:43:02PM +0200, Wolfgang Bumiller wrote:
> ---
> No changes.
> 
>  src/PVE/API2/LXC.pm | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 0397224..ac3eefa 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1191,6 +1191,9 @@ __PACKAGE__->register_method({
>  
>  	my $storage = extract_param($param, 'storage');
>  
> +	die "Full clone requires a target storage.\n"
> +	    if $param->{full} && !$storage;
> +
>          my $localnode = PVE::INotify::nodename();
>  
>  	my $storecfg = PVE::Storage::config();
> @@ -1244,10 +1247,8 @@ __PACKAGE__->register_method({
>  		    if ($mp->{type} eq 'volume') {
>  			my $volid = $mp->{volume};
>  			if ($param->{full}) {
> -			    die "fixme: full clone not implemented";
> -
> -			    die "Full clone feature for '$volid' is not available\n"
> -				if !PVE::Storage::volume_has_feature($storecfg, 'copy', $volid, $snapname, $running);
> +			    die "Cannot do full clones on a running container without snapshots\n"
> +				if $running && !defined($snapname);
>  			    $fullclone->{$opt} = 1;
>  			} else {
>  			    # not full means clone instead of copy
> @@ -1298,17 +1299,20 @@ __PACKAGE__->register_method({
>  			my $mp = $mountpoints->{$opt};
>  			my $volid = $mp->{volume};
>  
> +			my $newvolid;
>  			if ($fullclone->{$opt}) {
> -			    die "fixme: full clone not implemented\n";
> +			    print "create full clone of mountpoint $opt ($volid)\n";
> +			    $newvolid = PVE::LXC::copy_volume($mp, $newid, $storage, $storecfg, $conf, $snapname);
>  			} else {
>  			    print "create linked clone of mount point $opt ($volid)\n";
> -			    my $newvolid = PVE::Storage::vdisk_clone($storecfg, $volid, $newid, $snapname);
> -			    push @$newvollist, $newvolid;
> -			    $mp->{volume} = $newvolid;
> -
> -			    $newconf->{$opt} = PVE::LXC::Config->print_ct_mountpoint($mp, $opt eq 'rootfs');
> -			    PVE::LXC::Config->write_config($newid, $newconf);
> +			    $newvolid = PVE::Storage::vdisk_clone($storecfg, $volid, $newid, $snapname);
>  			}
> +
> +			push @$newvollist, $newvolid;
> +			$mp->{volume} = $newvolid;
> +
> +			$newconf->{$opt} = PVE::LXC::Config->print_ct_mountpoint($mp, $opt eq 'rootfs');
> +			PVE::LXC::Config->write_config($newid, $newconf);

IMHO needs an flock, could even be extended with a reload of config,
lock value and/or digest check ;)

>  		    }
>  
>  		    delete $newconf->{lock};

this one and the write_config after it could be rewritten as remove_lock
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel