[pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
Dominik Csapak
d.csapak at proxmox.com
Wed Jun 5 11:21:17 CEST 2024
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 at 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->%* ];
More information about the pve-devel
mailing list