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->%* ];
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
