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 BF1FE1FF389
	for <inbox@lore.proxmox.com>; 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 <f.ebner@proxmox.com>,
 Proxmox VE development discussion <pve-devel@lists.proxmox.com>
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 <d.csapak@proxmox.com>
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 <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-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

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 <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.
> 

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