From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B0FF91FF19A for <inbox@lore.proxmox.com>; Fri, 7 Mar 2025 13:21:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E7C91A6D5; Fri, 7 Mar 2025 13:21:07 +0100 (CET) Message-ID: <34edeb5b-0639-4204-9b31-619d6ea00457@proxmox.com> Date: Fri, 7 Mar 2025 13:20:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Dominik Csapak <d.csapak@proxmox.com> References: <20250213131716.3062383-1-d.csapak@proxmox.com> <20250213131716.3062383-12-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20250213131716.3062383-12-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.043 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record 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 v6 08/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 <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> 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.csapak@proxmox.com> With the outdated comment removed/adapted and the hunk from the next patch squashed in (see below): Reviewed-by: Fiona Ebner <f.ebner@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