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 DEC496BEE8 for ; 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 ; 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 ; 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 ; Thu, 5 Aug 2021 14:46:00 +0200 (CEST) To: Fabian Ebner , 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 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 >> --- >> 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 ], >>