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.csa...@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
