From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D7E4D6D2D2 for ; Fri, 13 Aug 2021 10:21:15 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C54892AB77 for ; Fri, 13 Aug 2021 10:21:15 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 95EB82AB68 for ; Fri, 13 Aug 2021 10:21:11 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 65E9341029 for ; Fri, 13 Aug 2021 10:21:11 +0200 (CEST) To: pve-devel@lists.proxmox.com, Aaron Lauterer References: <20210806134647.632054-1-a.lauterer@proxmox.com> <20210806134647.632054-9-a.lauterer@proxmox.com> From: Fabian Ebner Message-ID: Date: Fri, 13 Aug 2021 10:21:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20210806134647.632054-9-a.lauterer@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.402 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lxc.pm] Subject: Re: [pve-devel] [PATCH v2 container 8/9] api: move-volume: add move to another container X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Aug 2021 08:21:15 -0000 Am 06.08.21 um 15:46 schrieb Aaron Lauterer: > The goal of this is to expand the move-volume API endpoint to make it > possible to move a container volume / mountpoint to another container. > > Currently it works for regular mountpoints though it would be nice to be > able to do it for unused mounpoints as well. > > Signed-off-by: Aaron Lauterer > --- > This is mostly the code from qemu-server with some adaptions. Mainly > error messages and some checks. > > Previous checks have been moved to '$move_to_storage_checks'. > > v1 -> v2: > * change --target-mp to --target-volume > * make --target-volume optional and fallback to source mount point > * use parse_volname instead of custom regex > * adapt to find_free_volname > * print warnings from update_pct_config > * move running check back in both lock_config sections > * fixed a few style issues > > rfc -> v1: > * add check if target guest is replicated and fail if the moved volume > does not support it > * check if source volume has a format suffix and pass it to > 'find_free_disk' or if the prefix is vm/subvol as those also have > their own meaning, see the comment in the code > * fixed some style nits > src/PVE/API2/LXC.pm | 268 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 247 insertions(+), 21 deletions(-) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index afef7ec..89f9de8 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -1784,10 +1784,12 @@ __PACKAGE__->register_method({ > method => 'POST', > protected => 1, > proxyto => 'node', > - description => "Move a rootfs-/mp-volume to a different storage", > + description => "Move a rootfs-/mp-volume to a different storage or to a different container.", > permissions => { > description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " . > - "and 'Datastore.AllocateSpace' permissions on the storage.", > + "and 'Datastore.AllocateSpace' permissions on the storage. To move ". > + "a volume to another container, you need the permissions on the ". > + "target container as well.", > check => > [ 'and', > ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]], > @@ -1799,14 +1801,20 @@ __PACKAGE__->register_method({ > properties => { > node => get_standard_option('pve-node'), > vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }), > + 'target-vmid' => get_standard_option('pve-vmid', { > + completion => \&PVE::LXC::complete_ctid, > + optional => 1, > + }), > volume => { > type => 'string', > + #TODO: check how to handle unused mount points as the mp parameter is not configured > enum => [ PVE::LXC::Config->valid_volume_keys() ], > description => "Volume which will be moved.", > }, > storage => get_standard_option('pve-storage-id', { > description => "Target Storage.", > completion => \&PVE::Storage::complete_storage_enabled, > + optional => 1, > }), > delete => { > type => 'boolean', > @@ -1827,6 +1835,20 @@ __PACKAGE__->register_method({ > minimum => '0', > default => 'clone limit from datacenter or storage config', > }, > + 'target-volume' => { > + type => 'string', > + description => "The config key the volume will be moved to.", > + enum => [PVE::LXC::Config->valid_volume_keys()], > + optional => 1, The default could be mentioned here. > + }, > + 'target-digest' => { > + type => 'string', > + description => 'Prevent changes if current configuration file of the target " . > + "container has a different SHA1 digest. This can be used to prevent " . > + "concurrent modifications.', > + maxLength => 40, > + optional => 1, > + }, > }, > }, > returns => { > @@ -1841,32 +1863,48 @@ __PACKAGE__->register_method({ > > my $vmid = extract_param($param, 'vmid'); > > + my $target_vmid = extract_param($param, 'target-vmid'); > + > my $storage = extract_param($param, 'storage'); > > my $mpkey = extract_param($param, 'volume'); > > + my $target_mpkey = extract_param($param, 'target-volume') // $mpkey; > + > + my $digest = extract_param($param, 'digest'); > + > + my $target_digest = extract_param($param, 'target-digest'); > + > my $lockname = '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 "either set storage or target-vmid, but not both\n" > + if $storage && $target_vmid; > > - die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid); > + my $storecfg = PVE::Storage::config(); > + my $source_volid; > > - $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey}); > - $old_volid = $mpdata->{volume}; > + my $move_to_storage_checks = sub { > + PVE::LXC::Config->lock_config($vmid, sub { > + my $conf = PVE::LXC::Config->load_config($vmid); > + PVE::LXC::Config->check_lock($conf); > > - die "you can't move a volume with snapshots and delete the source\n" > - if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid); > + die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid); > > - PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest}); > + $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey}); > + $old_volid = $mpdata->{volume}; > > - PVE::LXC::Config->set_lock($vmid, $lockname); > - }); > + die "you can't move a volume with snapshots and delete the source\n" > + if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid); > > - my $realcmd = sub { > + PVE::Tools::assert_if_modified($digest, $conf->{digest}); > + > + PVE::LXC::Config->set_lock($vmid, $lockname); > + }); > + }; > + > + my $storage_realcmd = sub { > eval { > PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage"); > > @@ -1936,15 +1974,203 @@ __PACKAGE__->register_method({ > warn $@ if $@; > die $err if $err; > }; > - my $task = eval { > - $rpcenv->fork_worker('move_volume', $vmid, $authuser, $realcmd); > + > + my $load_and_check_reassign_configs = sub { > + my $vmlist = PVE::Cluster::get_vmlist()->{ids}; > + die "Both containers need to be on the same node ($vmlist->{$vmid}->{node}) ". > + "but target continer is on $vmlist->{$target_vmid}->{node}.\n" > + if $vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node}; > + > + my $source_conf = PVE::LXC::Config->load_config($vmid); > + PVE::LXC::Config->check_lock($source_conf); > + my $target_conf = PVE::LXC::Config->load_config($target_vmid); > + PVE::LXC::Config->check_lock($target_conf); > + > + die "Can't move volumes from or to template VMs\n" > + if ($source_conf->{template} || $target_conf->{template}); > + > + if ($digest) { > + eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) }; > + if (my $err = $@) { > + die "Container ${vmid}: ${err}"; > + } > + } > + > + if ($target_digest) { > + eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) }; > + if (my $err = $@) { > + die "Container ${target_vmid}: ${err}"; > + } > + } > + > + die "volume '${mpkey}' for container '$vmid' does not exist\n" > + if !defined($source_conf->{$mpkey}); > + > + die "Target volume key '${target_mpkey}' is already in use for container '$target_vmid'\n" > + if exists $target_conf->{$target_mpkey}; > + > + my $drive = PVE::LXC::Config->parse_volume( > + $mpkey, > + $source_conf->{$mpkey}, > + ); > + > + $source_volid = $drive->{volume}; > + > + die "disk '${mpkey}' has no associated volume\n" > + if !$source_volid; > + > + die "Storage does not support moving of this disk to another container\n" > + if !PVE::Storage::volume_has_feature( > + $storecfg, > + 'rename', > + $source_volid, > + ); style nit: post-if condition should be one line > + > + die "Cannot move a bindmound or device mount to another container\n" > + if $drive->{type} ne "volume"; > + die "Cannot move volume to another container while the source container is running\n" > + if PVE::LXC::check_running($vmid) && $mpkey !~ m/^unused\d+$/; > + > + my $repl_conf = PVE::ReplicationConfig->new(); > + my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1); > + my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid); > + my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6]; > + if ( > + $is_replicated > + && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format) > + ) { > + die "Cannot move volume to a replicated container. Storage " . > + "does not support replication!\n"; > + } > + return ($source_conf, $target_conf); > + }; > + > + my $logfunc = sub { > + my ($msg) = @_; > + print STDERR "$msg\n"; > }; > - if (my $err = $@) { > - eval { PVE::LXC::Config->remove_lock($vmid, $lockname) }; > - warn $@ if $@; > - die $err; > + > + my $volume_reassignfn = sub { > + return PVE::LXC::Config->lock_config($vmid, sub { > + return PVE::LXC::Config->lock_config($target_vmid, sub { > + my ($source_conf, $target_conf) = &$load_and_check_reassign_configs(); > + die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid); > + > + my $drive_param = PVE::LXC::Config->parse_volume( > + $target_mpkey, > + $source_conf->{$mpkey}, > + ); > + > + print "moving volume '$mpkey' from container '$vmid' to '$target_vmid'\n"; > + my ($storage, $source_volname) = PVE::Storage::parse_volume_id($source_volid); > + > + my ( > + undef, > + undef, > + undef, > + undef, > + undef, > + undef, > + $fmt > + ) = PVE::Storage::parse_volname($storecfg, $source_volid); Can be shorter like above. > + > + my $target_volname = PVE::Storage::find_free_volname( > + $storecfg, > + $storage, > + $target_vmid, > + $fmt > + ); > + > + my $new_volid = PVE::Storage::rename_volume( > + $storecfg, > + $source_volid, > + $target_volname, > + ); > + > + $drive_param->{volume} = $new_volid; > + > + delete $source_conf->{$mpkey}; > + print "removing volume '${mpkey}' from container '${vmid}' config\n"; > + PVE::LXC::Config->write_config($vmid, $source_conf); > + > + my $drive_string = PVE::LXC::Config->print_volume($target_mpkey, $drive_param); > + my $running = PVE::LXC::check_running($target_vmid); > + my $param = { $target_mpkey => $drive_string }; > + > + my $errors = PVE::LXC::Config->update_pct_config( > + $target_vmid, > + $target_conf, > + $running, > + $param > + ); > + > + my $rpcenv = PVE::RPCEnvironment::get(); The $rpcenv variable already exists (from the beginning of the call). > + foreach my $key (keys %$errors) { > + $rpcenv->warn( $errors->{$key}); style nit: extra space > + } > + > + PVE::LXC::Config->write_config($target_vmid, $target_conf); > + $target_conf = PVE::LXC::Config->load_config($target_vmid); > + > + PVE::LXC::update_lxc_config($target_vmid, $target_conf); > + print "target container '$target_vmid' updated with '$target_mpkey'\n"; > + > + # remove possible replication snapshots > + if (PVE::Storage::volume_has_feature( > + $storecfg, > + 'replicate', > + $source_volid), style nit: parenthesis from function call should be on its own line, comma shouldn't be after parenthesis > + ) { > + eval { > + PVE::Replication::prepare( > + $storecfg, > + [$new_volid], > + undef, > + 1, > + undef, > + $logfunc, > + ) > + }; > + if (my $err = $@) { > + print "Failed to remove replication snapshots on volume ". > + "'$target_mpkey'. Manual cleanup could be necessary.\n"; Could use $rpcenv->warn here as well. Maybe include original error? > + } > + } > + }); > + }); > + }; > + > + if ($target_vmid) { > + $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk']) > + if $authuser ne 'root@pam'; > + > + die "Moving a volume to the same container is not possible. Did you ". > + "mean to move the volume to a different storage?\n" > + if $vmid eq $target_vmid; > + > + &$load_and_check_reassign_configs(); > + return $rpcenv->fork_worker( > + 'move_volume', > + "${vmid}-${mpkey}>${target_vmid}-${target_mpkey}", > + $authuser, > + $volume_reassignfn > + ); > + } elsif ($storage) { > + &$move_to_storage_checks(); > + my $task = eval { > + $rpcenv->fork_worker('move_volume', $vmid, $authuser, $storage_realcmd); > + }; > + if (my $err = $@) { > + eval { PVE::LXC::Config->remove_lock($vmid, $lockname) }; > + warn $@ if $@; > + die $err; > + } > + return $task; > + } else { > + die "Provide either a 'storage' to move the mount point to a ". > + "different storage or 'target-vmid' and 'target-mp' to move ". > + "the mount point to another container\n"; > } > - return $task; > }}); > > __PACKAGE__->register_method({ >