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 [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 725731FF38C
for <inbox@lore.proxmox.com>; 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 <pve-devel@lists.proxmox.com>,
Dominik Csapak <d.csapak@proxmox.com>
References: <20240419124556.3334691-1-d.csapak@proxmox.com>
<20240419124556.3334691-15-d.csapak@proxmox.com>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
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 <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 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.csapak@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.
> },
> 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