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