[pve-devel] [PATCH qemu 3/3] add migration precondition api endpoint

[Thomas Lamprecht]

what I'm missing here in general is the HA ingnorance, i.e., while a
VM / CT _could_ be migrated to Node Foo it may well be that HA group
priority just moves it straight back where it came from (or another
member of the same priority level). That's even an open bug, also I need
such informations for the planned maintenance mode - to decide where,
or if at all, the VM/CT should get migrated to.

We can ignore HA for now, but I can try to get the "is node a possible
target where I can stay" method ready for HA, as I need it anyway and
it'd allow to solve the bug easily, we can then just add it here.

On 5/22/19 3:25 PM, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index fa4ff63..47dd2d4 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3186,6 +3186,102 @@ my $check_vm_disks_local = sub {
>  	    $local_disks->{$volid}->{volid} = $volid;
>  	}
>      });
> +
> +    return $local_disks;
> +};
> +

^^^^ above hunk belongs into 2/3

> +__PACKAGE__->register_method({
> +    name => 'migrate_vm_precondition',
> +    path => '{vmid}/migrate',

semi-unsure about the proposed API path, but that's just details we can
switch before releasing this..

> +    method => 'GET',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Get preconditions for migration.",
> +    permissions => {
> +	check => ['perm', '/vms/{vmid}', [ 'VM.Migrate' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
> +	    target => get_standard_option('pve-node', {
> +		description => "Target node.",
> +		completion =>  \&PVE::Cluster::complete_migration_target,
> +		optional => 1,
> +	    }),
> +	},
> +    },
> +    returns => {
> +	type => "object",
> +	properties => {
> +	    running => { type => 'boolean' },
> +	    allowed_nodes => {
> +		type => 'array',
> +		optional => 1,
> +		description => "List nodes allowed for offline migration with same local storage as source node, only passed if VM is offline"
> +	    },
> +	    local_disks => {
> +		type => 'array',
> +		description => "List local disks including CD-Rom, unsused and not referenced disks"
> +	    },
> +	    local_resources => {
> +		type => 'array',
> +		description => "List local resources e.g. pci, usb"
> +	    }
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +
> +	my $authuser = $rpcenv->get_user();
> +
> +	PVE::Cluster::check_cfs_quorum();
> +
> +	my $res = {};
> +
> +	my $vmid = extract_param($param, 'vmid');
> +	my $target = extract_param($param, 'target');
> +	my $localnode = PVE::INotify::nodename();
> +
> +
> +	# test if VM exists
> +	my $vmconf = PVE::QemuConfig->load_config($vmid);
> +	my $storecfg = PVE::Storage::config();
> +
> +
> +	# try to detect errors early
> +	PVE::QemuConfig->check_lock($vmconf);

this errors, or? As this is a "check if migrations is possible" it could
make sense to not let it error but rather return a 'has_lock => boolean'
(or lockname) in the res?

> +
> +	$res->{'running'} = PVE::QemuServer::check_running($vmid) ? 1:0;

we normally do not use the string syntax for accessing hash keys which
do not require it, very seldom yes, but not here, AFAIS, so please just:
$res->{running} = ...

> +
> +	# if vm is not running, return target nodes where local storage is available
> +	# for offline migration
> +	if (!$res->{'running'}) {
> +	    my $shared_nodes = PVE::QemuServer::shared_nodes($vmconf, $storecfg);
> +
> +	    delete $shared_nodes->{$localnode} if $shared_nodes->{$localnode};
> +
> +	    my @allowed_nodes = keys %$shared_nodes;
> +	    $res->{'allowed_nodes'} = \@allowed_nodes;

the following is the same as above two lines, only a bit more concise:
$res->{allowed_nodes} = [ keys %$shared_nodes ];

> +	}
> +
> +
> +	my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
> +	my @local_disks_array = values %$local_disks;
> +	$res->{'local_disks'} = \@local_disks_array;

as above:
$res->{local_disks} = [ values %$local_disks ];

> +
> +	my $local_resources =  PVE::QemuServer::check_local_resources($vmconf, 1);
> +
> +	$res->{'local_resources'} = $local_resources;

directly save into result hash? but just nit-picking..

> +
> +	return $res;
> +
> +
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'migrate_vm',
>      path => '{vmid}/migrate',
>