From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 725731FF38C for ; Fri, 31 May 2024 15:38:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6A51B362C0; Fri, 31 May 2024 15:38:33 +0200 (CEST) Message-ID: <6a7e20b5-dfb6-4feb-879f-a70e245af221@proxmox.com> Date: Fri, 31 May 2024 15:37:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Dominik Csapak References: <20240419124556.3334691-1-d.csapak@proxmox.com> <20240419124556.3334691-15-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20240419124556.3334691-15-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.059 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemu.pm] Subject: Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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