[pve-devel] [PATCH v3 container 5/5] add move_volume api call

Fabian Gr├╝nbichler f.gruenbichler at proxmox.com
Thu Oct 5 14:42:58 CEST 2017


On Tue, Sep 26, 2017 at 02:43:04PM +0200, Wolfgang Bumiller wrote:
> ---
> Changes:
>  * Instead of refusing moves when any snapshots exist, use the new
>    'is_volume_in_use' variant to check whether the volume is actually
>    used in a snapshot, in which case we simply don't allow the --delete
>    parameter.
>  * Don't use both 'disk' and 'volume' in the parameter descriptions.
>  * - Let the config key be referred to as $mpkey instead of $volume,
>    - its parsed data s $mpdata
>    - the actual storage volumes only as $old_volid and $new_volid
>    instead of mixing $mp, $mp->{volume}, $volume, etc. in confusing
>    ways.
> 
>  src/PVE/API2/LXC.pm | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/PVE/CLI/pct.pm  |   1 +
>  2 files changed, 154 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index ac3eefa..b2685ce 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1494,4 +1494,157 @@ __PACKAGE__->register_method({
>  	return PVE::LXC::Config->lock_config($vmid, $code);;
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'move_volume',
> +    path => '{vmid}/move_volume',
> +    method => 'POST',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Move a rootfs-/mp-volume to a different storage",
> +    permissions => {
> +	description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
> +	    "and 'Datastore.AllocateSpace' permissions on the storage.",
> +	check =>
> +	[ 'and',
> +	  ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
> +	  ['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
> +	],

indentation seems wrong here. not sure whether I prefer the original one
in qemu-server.. maybe

check => [
    'and',
    ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
    ['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
],

?

> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
> +	    volume => {
> +		type => 'string',
> +		enum => [ PVE::LXC::Config->mountpoint_names() ],
> +		description => "Volume which will move.",

volumes hopefully don't move on their own ;)

"Moved volume"
"Volume which will be moved"
"Source volume"

> +	    },
> +	    storage => get_standard_option('pve-storage-id', {
> +		description => "Target Storage.",
> +		completion => \&PVE::Storage::complete_storage_enabled,
> +	    }),
> +	    delete => {
> +		type => 'boolean',
> +		description => "Delete the original volume after successful copy. By default the original is kept as an unused volume entry.",
> +		optional => 1,
> +		default => 0,
> +	    },
> +	    digest => {
> +		type => 'string',
> +		description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
> +		maxLength => 40,
> +		optional => 1,
> +	    }
> +	},
> +    },
> +    returns => {
> +	type => 'string',
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $node = extract_param($param, 'node');
> +
> +	my $vmid = extract_param($param, 'vmid');
> +
> +	my $storage = extract_param($param, 'storage');
> +
> +	my $mpkey = extract_param($param, 'volume');
> +
> +	my $delete = extract_param($param, 'delete');
> +
> +	my $digest = extract_param($param, 'digest');

why? you don't pass $param to anything (so no need to clean it up), and
you don't even use all of the above variables (e.g., $node).

I understand $mpkey, and $vmid and $storage can be argued because they
are used often and benefit from a shorter name, but the rest seems a bit
much.

> +
> +	my $lockname = "move-disk";
> +
> +	my ($mpdata, $old_volid);
> +
> +	PVE::LXC::Config->lock_config($vmid, sub {
> +	    my $conf = PVE::LXC::Config->load_config($vmid);
> +	    PVE::LXC::Config->check_lock($conf);
> +
> +	    die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
> +
> +	    if ($mpkey eq 'rootfs') {
> +		$mpdata = PVE::LXC::Config->parse_ct_rootfs($conf->{$mpkey});
> +	    } elsif ($mpkey =~ m/mp\d+/) {
> +		$mpdata = PVE::LXC::Config->parse_ct_mountpoint($conf->{$mpkey});
> +	    } else {
> +		die "Can't parse $mpkey\n";
> +	    }
> +	    $old_volid = $mpdata->{volume};
> +	    my ($old_storage, undef) = PVE::Storage::parse_volume_id($old_volid);
> +	    die "you can't move on the same storage\n" # ...with same format - but we only have raw currently

but this is not true - we do have the old-style subvol mps as well..
also, moving to a new volume on the same storage can make sense (think ZFS
with changed compression/copies/deduplication/recordsize/.. properties)

> +		if $old_storage eq $storage;
> +
> +	    die "you can't move a volume with snapshots and delete the source\n"
> +		if $delete && PVE::LXC::Config->is_volume_in_use($conf, $old_volid, 1, 1);
> +
> +	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
> +
> +	    PVE::LXC::Config->set_lock($vmid, $lockname);
> +	});
> +
> +	my $realcmd = sub {
> +	    eval {
> +		PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
> +
> +		my $conf = PVE::LXC::Config->load_config($vmid);
> +		my $storage_cfg = PVE::Storage::config();
> +
> +		my $new_volid;
> +
> +		eval {
> +		    PVE::Storage::activate_volumes($storage_cfg, [ $old_volid ]);
> +		    $new_volid = PVE::LXC::copy_volume($mpdata, $vmid, $storage, $storage_cfg, $conf);
> +		    $mpdata->{volume} = $new_volid;
> +
> +		    $conf->{$mpkey} = PVE::LXC::Config->print_ct_mountpoint($mpdata, $mpkey eq 'rootfs');
> +
> +		    PVE::LXC::Config->add_unused_volume($conf, $old_volid) if !$delete;
> +
> +		    PVE::LXC::Config->write_config($vmid, $conf);

IMHO we want to re-flock and re-check the (updated) digest or config
lock before writing out the config here.. (copy_volume only reads the
uid/guid maps from the config, so that should be okay).

> +
> +		    eval {
> +			# try to deactivate volumes - avoid lvm LVs to be active on several nodes
> +			PVE::Storage::deactivate_volumes($storage_cfg, [ $new_volid ])
> +		    };
> +		    warn $@ if $@;
> +		};
> +		if (my $err = $@) {
> +		    eval {
> +			PVE::Storage::vdisk_free($storage_cfg, $new_volid)
> +			    if defined($new_volid);
> +		    };
> +		    warn $@ if $@;

shouldn't we remove the lock here if the cleanup was successful?

> +		    die $err;
> +		}
> +
> +		if ($delete) {
> +		    eval {
> +			PVE::Storage::deactivate_volumes($storage_cfg, [ $old_volid ]);
> +			PVE::Storage::vdisk_free($storage_cfg, $old_volid);
> +		    };
> +		    warn $@ if $@;
> +		}
> +	    };
> +	    my $err = $@;
> +	    PVE::LXC::Config->remove_lock($vmid, $lockname);

needs to be guarded by an eval, otherwise the actual error is masked by
a potential error of remove_lock

> +	    die $err if $err;
> +	};
> +	my $task = eval {
> +	    $rpcenv->fork_worker('move_volume', $vmid, $authuser, $realcmd);
> +	};
> +	if (my $err = $@) {
> +	    PVE::LXC::Config->remove_lock($vmid, $lockname);

same as above!

> +	    die $err;
> +	}
> +	return $task;
> +  }});
> +
>  1;
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index 3253906..9bdefc6 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -759,6 +759,7 @@ our $cmddef = {
>      
>      clone => [ "PVE::API2::LXC", 'clone_vm', ['vmid', 'newid'], { node => $nodename }, $upid_exit ],
>      migrate => [ "PVE::API2::LXC", 'migrate_vm', ['vmid', 'target'], { node => $nodename }, $upid_exit],
> +    move_volume => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 'storage'], { node => $nodename }, $upid_exit ],
>      
>      status => [ __PACKAGE__, 'status', ['vmid']],
>      console => [ __PACKAGE__, 'console', ['vmid']],
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



More information about the pve-devel mailing list