Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions

[Fiona Ebner <f.ebner@proxmox.com>]

Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> so that we can show a proper warning in the migrate dialog and check it
> in the bulk migrate precondition check
> 
> the unavailable_storages and should be the same as before, but
> we now always return allowed_nodes too.
> 
> also add a note that we want to redesign the return values here, to make
> * the api call simpler
> * return better structured values
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index f95d8d95..94aa9942 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4450,18 +4450,20 @@ __PACKAGE__->register_method({
>  	},
>      },
>      returns => {
> +	# TODO 9.x: rework the api call to return more sensible structures
> +	# e.g. a simple list of nodes with their blockers and/or notices to show
>  	type => "object",
>  	properties => {
>  	    running => { type => 'boolean' },
>  	    allowed_nodes => {
>  		type => 'array',
>  		optional => 1,
> -		description => "List nodes allowed for offline migration, only passed if VM is offline"
> +		description => "List nodes allowed for offline migration.",

It still only returns the actual list of allowed nodes if not running.
My idea was to return the allowed nodes in both cases. If we keep the
parameter specific to offline migration, I'd still keep the return guarded.

>  	    },
>  	    not_allowed_nodes => {
>  		type => 'object',
>  		optional => 1,
> -		description => "List not allowed nodes with additional informations, only passed if VM is offline"
> +		description => "List not allowed nodes with additional information.",
>  	    },
>  	    local_disks => {
>  		type => 'array',
> @@ -4518,28 +4520,31 @@ __PACKAGE__->register_method({
>  
>  	# if vm is not running, return target nodes where local storage/mapped devices are available
>  	# for offline migration
> +	$res->{allowed_nodes} = [];
> +	$res->{not_allowed_nodes} = {};
>  	if (!$res->{running}) {
> -	    $res->{allowed_nodes} = [];
> -	    my $checked_nodes = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> -	    delete $checked_nodes->{$localnode};
> -
> -	    foreach my $node (keys %$checked_nodes) {
> -		my $missing_mappings = $missing_mappings_by_node->{$node};
> -		if (scalar($missing_mappings->@*)) {
> -		    $checked_nodes->{$node}->{'unavailable-resources'} = $missing_mappings;
> -		    next;
> -		}
> +	    $res->{not_allowed_nodes} = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> +	    delete $res->{not_allowed_nodes}->{$localnode};
> +	}
>  
> -		if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> -		    push @{$res->{allowed_nodes}}, $node;
> -		}
> +	my $merged = { $res->{not_allowed_nodes}->%*, $missing_mappings_by_node->%* };
>  

If we'd need this, I'd just get the keys directly:
my @keys = keys { ... }->%*;

But it just reads wrong. Why even consider the keys for the
not_allowed_nodes? Doesn't this just work because
$missing_mappings_by_node already contains all other node keys (and so
we can simply iterate over those keys)?

> +	for my $node (keys $merged->%*) {
> +	    my $missing_mappings = $missing_mappings_by_node->{$node};
> +	    if (scalar($missing_mappings->@*)) {
> +		$res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} = $missing_mappings;
> +		next;
> +	    }
> +
> +	    if (!$res->{running}) {
> +		if (!defined($res->{not_allowed_nodes}->{$node}->{unavailable_storages})) {
> +		    push $res->{allowed_nodes}->@*, $node;
> +		}
>  	    }
> -	    $res->{not_allowed_nodes} = $checked_nodes;
>  	}
>  
>  	my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
> -	$res->{local_disks} = [ values %$local_disks ];;
> +	$res->{local_disks} = [ values %$local_disks ];
>  
>  	$res->{local_resources} = $local_resources;
>  	$res->{'mapped-resources'} = [ keys $mapped_resources->%* ];


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel