From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <a.lauterer@proxmox.com>
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 DEC496BEE8
for <pve-devel@lists.proxmox.com>; Thu, 5 Aug 2021 14:46:32 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CB6952AE6C
for <pve-devel@lists.proxmox.com>; Thu, 5 Aug 2021 14:46:02 +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))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 843D62AE5F
for <pve-devel@lists.proxmox.com>; Thu, 5 Aug 2021 14:46:00 +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 4838D42E20
for <pve-devel@lists.proxmox.com>; Thu, 5 Aug 2021 14:46:00 +0200 (CEST)
To: Fabian Ebner <f.ebner@proxmox.com>, pve-devel@lists.proxmox.com
References: <20210719145254.2269142-1-a.lauterer@proxmox.com>
<20210719145254.2269142-9-a.lauterer@proxmox.com>
<85ad0946-a72e-9804-b497-65db22bc1809@proxmox.com>
From: Aaron Lauterer <a.lauterer@proxmox.com>
Message-ID: <9c969d4b-a3c0-8d6a-1cb3-3fd2b2a9fa1c@proxmox.com>
Date: Thu, 5 Aug 2021 14:45:57 +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: <85ad0946-a72e-9804-b497-65db22bc1809@proxmox.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.442 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.132 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 v1 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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 05 Aug 2021 12:46:32 -0000
On 8/3/21 10:14 AM, Fabian Ebner wrote:
> Am 19.07.21 um 16:52 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 <a.lauterer@proxmox.com>
>> ---
>> 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'.
>>
>> 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 | 270 ++++++++++++++++++++++++++++++++++++++++----
>> src/PVE/CLI/pct.pm | 2 +-
>> 2 files changed, 250 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index b929481..0af22c1 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -27,6 +27,8 @@ use PVE::API2::LXC::Snapshot;
>> use PVE::JSONSchema qw(get_standard_option);
>> use base qw(PVE::RESTHandler);
>> +use Data::Dumper;
>> +
>
> Left-over from debugging?
Yep...
>
>> BEGIN {
>> if (!$ENV{PVE_GENERATING_DOCS}) {
>> require PVE::HA::Env::PVE2;
>> @@ -1784,10 +1786,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 +1803,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 +1837,20 @@ __PACKAGE__->register_method({
>> minimum => '0',
>> default => 'clone limit from datacenter or storage config',
>> },
>> + 'target-mp' => {
>
> Using 'target-volume' would be more consistent.
Good point, I'll change the parameter
>
>> + type => 'string',
>> + description => "The config key the mp will be moved to.",
>> + 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 => {
>> @@ -1841,32 +1865,49 @@ __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_mp = extract_param($param, 'target-mp');
>> +
>> + 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;
>
> Same as for qemu: either require target_vmid *and* target_mp or have it default to the one from the source.
As in qemu, let's fall back to the source key if not explicitly specified.
>
>> - die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
>> + die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
>
> This check was previously inside lock_config, but now it isn't anymore.
Yeah, wanted to have it in one spot for both situations (move to other storage & move to other container) but that will be racy... will need to place it in both lock_config sections.
>
>> - $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
>> - $old_volid = $mpdata->{volume};
>> + my $storecfg = PVE::Storage::config();
>> + my $source_volid;
>> - 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 $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);
>> - PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest});
>> - PVE::LXC::Config->set_lock($vmid, $lockname);
>> - });
>> + $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
>> + $old_volid = $mpdata->{volume};
>> - my $realcmd = sub {
>> + 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);
>> +
>> + 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 +1977,202 @@ __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_mp}' is already in use for container '$target_vmid'\n"
>> + if exists $target_conf->{$target_mp};
>
> Same as for qemu: any reason for using exists?
As in qemu: if the target key exists, I like to err on the safe side and abort, even if it has nothing configured.
>
>> +
>> + 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,
>> + );
>> +
>> + die "Cannot move a bindmound or device mount to another container\n"
>> + if PVE::LXC::Config->classify_mountpoint($source_volid) ne "volume";
>
> The result from classify_mountpoint should be in $drive->{type} already.
>
>> + 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();
>> +
>> + my $drive_param = PVE::LXC::Config->parse_volume(
>> + $target_mp,
>> + $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);
>> +
>> + # The format in find_free_diskname handles two cases for containers.
>> + # If it is set to 'subvol', the prefix will be set to it instead of 'vm',
>> + # this is needed for example for ZFS based storages.
>> + # The other thing to check for, in case of directory/file based storages,
>> + # is .raw files as we need to pass this format in that case.
>> + my $fmt = undef;
>> + if ($source_volname =~ m/(subvol|vm)-\d+-disk-\S+$/) {
>> + $fmt = $1 if $1 eq "subvol";
>> + } else {
>> + die "could not detect source volname prefix\n";
>> + }
>> + if ($source_volname =~ m/vm-\d+-disk-\S+\.(raw)/) {
>> + $fmt = $1;
>> + }
>
> Using parse_volname should be easier and future-proof.
Yep, same as in qemu, now with the behavior of find_free_volname to decide itself if the fmt suffix is needed, this can be done with parse_volname. Previously it would have been problematic as it also returns "raw" for other storages like ZFS or LVM.
>
>> +
>> + my $target_volname = PVE::Storage::find_free_diskname(
>> + $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_mp, $drive_param);
>> + my $running = PVE::LXC::check_running($target_vmid);
>> + my $param = { $target_mp => $drive_string };
>> +
>> + my $err = Dumper(PVE::LXC::Config->update_pct_config(
>> + $target_vmid,
>> + $target_conf,
>> + $running,
>> + $param
>> + ));
>
> $err and Dumper left-over from debugging?
This particular implementation yes. But since "update_pct_config" would return an error hash is some hotplugging would fail, I am thinking of iterating over the hash and print the errors as warnings, should there be any.
I don't want to die / raise some exception since at this point, we removed the config from the source vmid and need to continue in order to write it to the new vmid.
>
>> +
>> + 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_mp'\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 = $@) {
>> + print "Failed to remove replication snapshots on volume ".
>> + "'$target_mp'. Manual cleanup could be necessary.\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_mp}",
>> + $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({
>> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
>> index 7ac5a55..5f2bf04 100755
>> --- a/src/PVE/CLI/pct.pm
>> +++ b/src/PVE/CLI/pct.pm
>> @@ -849,7 +849,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 ],
>> + 'move-volume' => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 'storage', 'target-vmid', 'target-mp'], { node => $nodename }, $upid_exit ],
>> move_volume => { alias => 'move-disk' },
>> snapshot => [ "PVE::API2::LXC::Snapshot", 'snapshot', ['vmid', 'snapname'], { node => $nodename } , $upid_exit ],
>>