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 BF1FE1FF389 for ; Wed, 5 Jun 2024 11:21:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 94B8A1F6FC; Wed, 5 Jun 2024 11:21:51 +0200 (CEST) Message-ID: <7dbe02e9-7f80-48ba-a197-56a1f1bf90c1@proxmox.com> Date: Wed, 5 Jun 2024 11:21:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Fiona Ebner , Proxmox VE development discussion References: <20240419124556.3334691-1-d.csapak@proxmox.com> <20240419124556.3334691-15-d.csapak@proxmox.com> <6a7e20b5-dfb6-4feb-879f-a70e245af221@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <6a7e20b5-dfb6-4feb-879f-a70e245af221@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.021 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 5/31/24 15:37, Fiona Ebner wrote: > 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. > mhmm not sure why i did it this way... but yeah i'd rather not break the api and just not return the that in the online case >> }, >> 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)? huh? we need to iterate over all nodes in 'not_allowed_nodes' for the unavailable storage check but we also want to include all nodes that are in the 'missing_mappings_by_node' nonetheless, i currently find the whole code also not very readable i'll try to come up with something better... (i.e. iterate over all nodes via get_nodelist and fill the allowed/not_allowed based on the missing storage/mapping list) > >> + 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