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

[Wolfgang Bumiller]

On Wed, Apr 20, 2016 at 11:14:13AM +0200, Thomas Lamprecht wrote:
> 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/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;

Considering the datastructures we're dealing with, this seems a bit
overkill. Simply copying the first level and then replacing the keys we
want replaced should be enough.

See below for the replacements.

> >  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} = '/';

We don't modify values, we only replace them, so it should be enough to
just copy the outer hash:

$mp = {%$mp}; # copy
$mp->{mp} = '/';

Then we don't need to pray that none of the non-eval{} parts of the code
die and leave the caller's object in a modified state.

> 
> 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.

mountpoint_mount() mounts to "$root/$mp" and we're using temporary
directories where the "$root" is empty, so there's no point in creating
the subdirectories

> > +
> > +    my $new_mp = Storable::dclone($mp);
> > +    $new_mp->{volume} = $new_volid;

Same:
my $new_mp = {%$mp}; # copy
$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]);

That \ is not necessary.

> > +
> > +    };
> > +    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).
Valid point, but rmdir() doesn't die() and instead returns a boolean, so
this won't be an issue.

> 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?

-f only affects NFS and fuse, so here the real question is: do we want
to kill the NFS mount or just leave it up to the user/sysadmin to have a
working storage? I'd say -f here hides a different problem which IMO
needs to be addressed first, so not using it might indeed be better.

> > +    rmdir $mount_dir;
> > +
> > +    #set it back to orgin value
> s/orgin/original/
> > +    $mp->{mp} = $tmp_mp;

With the suggested change this line can go.

> > +
> > +    if ($err) {
> > +	PVE::Storage::vdisk_free($storage_cfg, $new_volid);
> > +	die $err;
> > +    }
> > +    return $new_volid;
> > +}
> >  
> >  1;