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

[Dominik Csapak <d.csapak@proxmox.com>]

On 5/31/24 15:37, Fiona Ebner wrote:
> 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.
> 

mhmm not sure why i did it this way... but yeah
i'd rather not break the api and just not return the that in the online case

>>   	    },
>>   	    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)?

huh?  we need to iterate over all nodes in 'not_allowed_nodes' for the
unavailable storage check

but we also want to include all nodes that are in the 'missing_mappings_by_node'


nonetheless, i currently find the whole code also not very readable
i'll try to come up with something better...
(i.e. iterate over all nodes via get_nodelist and
fill the allowed/not_allowed based on the missing storage/mapping list)

> 
>> +	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