Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
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
Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
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
---
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
Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
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
> ---
> 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.
> },
> 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)?
> + 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
[pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
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
---
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.",
},
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->%* };
+ 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->%* ];
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
