Re: [pve-devel] [PATCH v1 container 8/9] api: move-volume: add move to another container

[Aaron Lauterer <a.lauterer@proxmox.com>]

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 ],
>>