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

[Fiona Ebner]

Am 13.02.25 um 14:17 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 (not_)allowed_nodes too.

What do you mean by "unavailable_storages"?

Maybe add a quick rationale for why it's okay to always return the node
lists, i.e.: API consumers that did not check the node lists for online
migration will not break (e.g. our UI without patches) and API consumers
that already tried to check the node lists for online migration too will
just get more accurate information.

> to make the code a bit easier to read, reorganize how we construct
> the (not_)allowed_nodes properties.

Yes, it's very nice now :)

> 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 at proxmox.com>

With the outdated comment removed/adapted and the hunk from the next
patch squashed in (see below):

Reviewed-by: Fiona Ebner <f.ebner at proxmox.com>

> ---
> no changes in v6
>  PVE/API2/Qemu.pm | 45 ++++++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 1ed0d3dc..6a303337 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4595,6 +4595,8 @@ __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 => {
> @@ -4608,7 +4610,7 @@ __PACKAGE__->register_method({
>  		    description => "An allowed node",
>  		},
>  		optional => 1,
> -		description => "List nodes allowed for offline migration, only passed if VM is offline"
> +		description => "List nodes allowed for migration.",

Nit (pre-existing): "List of nodes ..."

>  	    },
>  	    not_allowed_nodes => {
>  		type => 'object',
> @@ -4624,7 +4626,7 @@ __PACKAGE__->register_method({
>  			},
>  		    },
>  		},
> -		description => "List not allowed nodes with additional information, only passed if VM is offline"
> +		description => "List not allowed nodes with additional information.",

Nit (pre-existing): "List of not ..."

>  	    },
>  	    local_disks => {
>  		type => 'array',
> @@ -4702,7 +4704,6 @@ __PACKAGE__->register_method({
>  
>  	my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
>  	    PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
> -	delete $missing_mappings_by_node->{$localnode};
>  
>  	my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
>  	if ($res->{running} && $vga->{'clipboard'} && $vga->{'clipboard'} eq 'vnc') {
> @@ -4711,28 +4712,34 @@ __PACKAGE__->register_method({
>  
>  	# if vm is not running, return target nodes where local storage/mapped devices are available
>  	# for offline migration

This comment is outdated.

> -	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->{allowed_nodes} = [];
> +	$res->{not_allowed_nodes} = {};
>  
> -		if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> -		    push @{$res->{allowed_nodes}}, $node;
> -		}
> +	my $storage_nodehash = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> +
> +	my $nodelist = PVE::Cluster::get_nodelist();
> +	for my $node ($nodelist->@*) {
> +	    next if $node eq $localnode;
> +
> +	    # extract missing storage info
> +	    if (my $storage_info = $storage_nodehash->{$node}) {
> +		$res->{not_allowed_nodes}->{$node} = $storage_info;
> +	    }
> +
> +	    # extract missing mappings info
> +	    my $missing_mappings = $missing_mappings_by_node->{$node};
> +	    if (scalar($missing_mappings->@*)) {
> +		$res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} = $missing_mappings;
> +	    }
>  
> +	    # if nothing came up, add it to the allowed nodes
> +	    if (!$res->{not_allowed_nodes}->{$node}) {

There's a hunk in the next patch improving the readability here that
could/should be squashed in.

> +		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'} = [ sort keys $mapped_resources->%* ];