[pve-devel] [[PATCH_2 pve-container]] Add the functionality of full clone

[Thomas Lamprecht]

some general comments inline:

On 04/20/2016 08:06 AM, Wolfgang Link wrote:
> ---
>  src/PVE/API2/LXC.pm | 21 +++++++++----------
>  src/PVE/LXC.pm      | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 90cdba6..537ab69 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1073,6 +1073,7 @@ __PACKAGE__->register_method({
>  
>  	my $storage = extract_param($param, 'storage');
>  
> +	die "Full clone require a storage.\n"if $param->{full} && !$storage;

space behind the if would give some better readability, also
s/require/requires/

>          my $localnode = PVE::INotify::nodename();
>  
>  	my $storecfg = PVE::Storage::config();
> @@ -1126,10 +1127,6 @@ __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);
>  			    $fullclone->{$opt} = 1;
>  			} else {
>  			    # not full means clone instead of copy
> @@ -1176,18 +1173,20 @@ __PACKAGE__->register_method({
>  		    foreach my $opt (keys %$mountpoints) {
>  			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, $vmid, $newid, $storage, $storecfg, $conf);
>  			} else {
>  			    print "create linked clone of mountpoint $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);
>  		    }
>  
>  		    delete $newconf->{lock};
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index fe97d31..7c773bf 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -22,6 +22,7 @@ use PVE::AccessControl;
>  use PVE::ProcFSTools;
>  use PVE::LXC::Config;
>  use Time::HiRes qw (gettimeofday);
> +use Storable;
>  
>  use Data::Dumper;
>  
> @@ -1348,5 +1349,62 @@ sub userns_command {
>      return [];
>  }
>  
> +sub copy_volume {
> +    my ($mp, $vmid, $newid, $storage, $storage_cfg, $conf) = @_;
> +    my $mount_dir = "/tmp/$vmid";
> +    my $dest = "$mount_dir\/dest";
> +    my $src =  "$mount_dir\/src";
> +
> +    my $new_format =  $storage_cfg->{ids}->{$storage}->{type} eq "zfspool" ? 'subvol' : 'raw';
> +
> +    my $new_volid = PVE::Storage::vdisk_alloc($storage_cfg, $storage, $newid, $new_format, undef, $mp->{size}/1024);
> +
> +    #simplify mount struct  
> +    my $tmp_mp = $mp->{mp};
> +    $mp->{mp} = '/';

as I'm not that involved with mount points this may be a really stupid
question, but what is that for?
seems a little weird and the comment isn't helping me.
A comment explaining why and not you're doing would be helpful, e.g. for
an not mp involved Dev who needs to touch this.

> +
> +    my $new_mp = Storable::dclone($mp);
> +    $new_mp->{volume} = $new_volid;
> +
> +    #get id's for unprivileged container
> +    my (undef, $rootuid, $rootgid) = parse_id_maps($conf);
> +
> +    my $path = PVE::Storage::path($storage_cfg, $new_volid, undef);
> +
> +    eval {
> +	mkfs($path, $rootuid, $rootgid) if $new_format ne "subvol";
> +
> +	mkdir $mount_dir;
> +	mkdir $dest;
> +	mkdir $src;
> +
> +	#mount for copy mp 
> +	mountpoint_mount($mp, $src, $storage_cfg);
> +	mountpoint_mount($new_mp, $dest, $storage_cfg);
> +
> +	PVE::Tools::run_command(['/usr/bin/rsync', '--stats', '-X', '-A', '--numeric-ids',
> +				 '--whole-file', '--inplace', '--one-file-system', '-aH',
> +				 "$src\/.", $dest]);
> +
> +    };
> +    my $err = $@;
> +    eval { PVE::Tools::run_command(['/bin/umount', '-f', $src], errfunc => sub{})};
> +    warn "Can't umount $src\n" if $@;
> +    eval { PVE::Tools::run_command(['/bin/umount', '-f', $dest], errfunc => sub{})};
> +    warn "Can't umount $src\n" if $@;
> +
> +    rmdir $dest;
> +    rmdir $src;

if the previous umount fails those will also fail (rmdir works on empty
directory only).
Also is a force umount a good way, shouldn't be checked if the caches
are all written (i.e. executing a fsync previously),
or am I wrong?
> +    rmdir $mount_dir;
> +
> +    #set it back to orgin value
s/orgin/original/
> +    $mp->{mp} = $tmp_mp;
> +
> +    if ($err) {
> +	PVE::Storage::vdisk_free($storage_cfg, $new_volid);
> +	die $err;
> +    }
> +    return $new_volid;
> +}
>  
>  1;