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

[Fiona Ebner]

Am 05.06.24 um 11:21 schrieb Dominik Csapak:
> On 5/31/24 15:37, Fiona Ebner wrote:
>> Am 19.04.24 um 14:45 schrieb Dominik Csapak:
>>> @@ -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

No, you want to iterate over all nodes (except localhost). The loop
below also constructs the list of allowed nodes after all.

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

Which are all nodes (except localhost), no matter if there are missing
mappings or not, because check_local_resources() has:

my $missing_mappings_by_node = { map { $_ => [] } @$nodelist };

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

Yes, that's more straight-forward. All nodes except localhost ;)

>>
>>> +    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->%* ];
> 
>