Am 13.02.25 um 14:17 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 (not_)allowed_nodes too.
What do you mean by "unavailable_storages"?
Maybe add a quick rationale for why it's okay to always return the node
lists, i.e.: API consumers that did not check the node lists for online
migration will not break (e.g. our UI without patches) and API consumers
that already tried to check the node lists for online migration too will
just get more accurate information.
> to make the code a bit easier to read, reorganize how we construct
> the (not_)allowed_nodes properties.
Yes, it's very nice now :)
> 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>
With the outdated comment removed/adapted and the hunk from the next
patch squashed in (see below):
Reviewed-by: Fiona Ebner <f.eb...@proxmox.com>
> ---
> no changes in v6
> PVE/API2/Qemu.pm | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 1ed0d3dc..6a303337 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4595,6 +4595,8 @@ __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 => {
> @@ -4608,7 +4610,7 @@ __PACKAGE__->register_method({
> description => "An allowed node",
> },
> optional => 1,
> - description => "List nodes allowed for offline migration, only
> passed if VM is offline"
> + description => "List nodes allowed for migration.",
Nit (pre-existing): "List of nodes ..."
> },
> not_allowed_nodes => {
> type => 'object',
> @@ -4624,7 +4626,7 @@ __PACKAGE__->register_method({
> },
> },
> },
> - description => "List not allowed nodes with additional
> information, only passed if VM is offline"
> + description => "List not allowed nodes with additional
> information.",
Nit (pre-existing): "List of not ..."
> },
> local_disks => {
> type => 'array',
> @@ -4702,7 +4704,6 @@ __PACKAGE__->register_method({
>
> my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
> PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
> - delete $missing_mappings_by_node->{$localnode};
>
> my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
> if ($res->{running} && $vga->{'clipboard'} && $vga->{'clipboard'} eq
> 'vnc') {
> @@ -4711,28 +4712,34 @@ __PACKAGE__->register_method({
>
> # if vm is not running, return target nodes where local storage/mapped
> devices are available
> # for offline migration
This comment is outdated.
> - 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->{allowed_nodes} = [];
> + $res->{not_allowed_nodes} = {};
>
> - if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> - push @{$res->{allowed_nodes}}, $node;
> - }
> + my $storage_nodehash =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> +
> + my $nodelist = PVE::Cluster::get_nodelist();
> + for my $node ($nodelist->@*) {
> + next if $node eq $localnode;
> +
> + # extract missing storage info
> + if (my $storage_info = $storage_nodehash->{$node}) {
> + $res->{not_allowed_nodes}->{$node} = $storage_info;
> + }
> +
> + # extract missing mappings info
> + my $missing_mappings = $missing_mappings_by_node->{$node};
> + if (scalar($missing_mappings->@*)) {
> + $res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> + }
>
> + # if nothing came up, add it to the allowed nodes
> + if (!$res->{not_allowed_nodes}->{$node}) {
There's a hunk in the next patch improving the readability here that
could/should be squashed in.
> + 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'} = [ sort keys $mapped_resources->%* ];
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
