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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 541547BC91 for ; Tue, 2 Nov 2021 17:45:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3E9B0B718 for ; Tue, 2 Nov 2021 17:44:50 +0100 (CET) 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 3D6EAB708 for ; Tue, 2 Nov 2021 17:44:48 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0F84C45EAC for ; Tue, 2 Nov 2021 17:44:48 +0100 (CET) Message-ID: Date: Tue, 2 Nov 2021 17:44:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Content-Language: en-US To: pve-devel@lists.proxmox.com References: <20211102150335.2371545-1-a.lauterer@proxmox.com> <20211102150335.2371545-9-a.lauterer@proxmox.com> From: Aaron Lauterer In-Reply-To: <20211102150335.2371545-9-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.688 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 -1.14 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 Subject: Re: [pve-devel] [PATCH v4 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: Tue, 02 Nov 2021 16:45:20 -0000 Please wait a bit reviewing this patch. I did send it a bit too early as there are a few things not yet fully done. Especially being able to move unused volume or to move a volume from mpX to an unusedX volume on the target container. Additionally we probably should not allow the rootfs to be moved as that will lead to som unexpected behaviors because AFAICT we never deal with a container missing a rootfs anywhere in our code. On 11/2/21 16:03, Aaron Lauterer wrote: > 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'. > > v3 -> v4: > * add check to avoid potential undefined warning in $vmlist->{$vmid} > * add check if volume is part of a snapshot > * removed line bloat > > v2 -> v3: > * mention default volume key in description > * use $rpcenv->warn should replication snapshot removal fail > * also print the original error > * code style things > > 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 | 253 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 232 insertions(+), 21 deletions(-) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index 69df366..afc16d7 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -1813,10 +1813,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' ]], > @@ -1828,14 +1830,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', > @@ -1856,6 +1864,21 @@ __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. Default is the " . > + "source volume key.", > + enum => [PVE::LXC::Config->valid_volume_keys()], > + optional => 1, > + }, > + '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 => { > @@ -1870,32 +1893,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"); > > @@ -1965,15 +2004,187 @@ __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 "could not find CT ${vmid}\n" if !exists($vmlist->{$vmid}); > + > + 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}) }; > + die "Container ${vmid}: $@" if $@; > + } > + > + if ($target_digest) { > + eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) }; > + die "Container ${target_vmid}: $@" if $@; > + } > + > + 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 "Cannot move volume used by a snapshot to another container\n" > + if PVE::LXC::Config->is_volume_in_use_by_snapshots($source_conf, $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); > + 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); > }; > - if (my $err = $@) { > - eval { PVE::LXC::Config->remove_lock($vmid, $lockname) }; > - warn $@ if $@; > - die $err; > + > + my $logfunc = sub { > + my ($msg) = @_; > + print STDERR "$msg\n"; > + }; > + > + 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 $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6]; > + > + 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 > + ); > + > + foreach my $key (keys %$errors) { > + $rpcenv->warn($errors->{$key}); > + } > + > + 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)) { > + eval { > + PVE::Replication::prepare( > + $storecfg, > + [$new_volid], > + undef, > + 1, > + undef, > + $logfunc, > + ) > + }; > + if (my $err = $@) { > + $rpcenv->warn("Failed to remove replication snapshots on volume ". > + "'${target_mpkey}'. Manual cleanup could be necessary. " . > + "Error: ${err}\n"); > + } > + } > + }); > + }); > + }; > + > + 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({ >