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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
