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 33C3A1FF16F
	for <inbox@lore.proxmox.com>; Tue, 29 Apr 2025 11:39:04 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id A79898136;
	Tue, 29 Apr 2025 11:39:13 +0200 (CEST)
Message-ID: <325a5da0-a89c-4ab8-9a04-a69868d09052@proxmox.com>
Date: Tue, 29 Apr 2025 11:39:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250325151254.193177-1-d.kral@proxmox.com>
 <20250325151254.193177-11-d.kral@proxmox.com>
 <67d201c9-64ca-4392-b166-aa14cb2bae48@proxmox.com>
 <e95e915b-3153-428f-9d8c-64e60d0c9689@proxmox.com>
Content-Language: en-US
From: Daniel Kral <d.kral@proxmox.com>
In-Reply-To: <e95e915b-3153-428f-9d8c-64e60d0c9689@proxmox.com>
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.012 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
Subject: Re: [pve-devel] [PATCH ha-manager 09/15] manager: apply colocation
 rules when selecting service nodes
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 4/28/25 16:33, Fiona Ebner wrote:
> Am 28.04.25 um 14:26 schrieb Fiona Ebner:
>> Am 25.03.25 um 16:12 schrieb Daniel Kral:
>>> +
>>> +	    delete $allowed_nodes->{$node};
>>> +	}
>>> +    } elsif (scalar keys %$possible_nodes) {
>>> +	# limit to the possible nodes the service should be on, if there are any.
>>> +	for my $node (keys %$allowed_nodes) {
>>> +	    next if exists($possible_nodes->{$node});
>>> +
>>> +	    delete $allowed_nodes->{$node};
>>
>> This seems wrong. Non-strict rules should not limit the allowed nodes.
>> See below for more on this.
> 
> Ah, if there are no possible nodes at all, then the allowed nodes are
> not modified at all. This is what makes the loose tests work. This
> "secret" here really needs to be properly documented ;)

Yes definitely, I'm working on making the logic clearer here.

I think another improvement we could make here is along with "merging" 
get_colocated_services(...) and get_colocation_preference(...), they 
could already structure together in a strict and non-strict part, so the 
apply_*_colocation_rules(...) helpers don't have to handle that anymore 
and just could focus on applying them in the end.

If we go for that route, we could reduce 
apply_positive_colocation_rules(...) to something like this:

sub apply_positive_colocation_rules {
     my ($together, $allowed_nodes) = @_;

     my $possible_nodes = { $together->{strict}->%* };

     # Consider loose nodes if there are no strict nodes
     $possible_nodes = PVE::HA::Tools::set_intersect($allowed_nodes, 
$together->{loose})
	if !%$possible_nodes;

     # If there are no strict nodes or the loose nodes would result in 
an empty $allowed_nodes, apply nothing
     return if !%$possible_nodes;

     for my $node (keys %$allowed_nodes) {
	next if exists($possible_nodes->{$node});

	delete $allowed_nodes->{$node};
     }
}

> 
> It still would be nice to think about which kind of interaction with
> scoring we want exactly. Currently it's the number 1 I mentioned, i.e.
> "prefer loose colocation over scoring no matter what". Can be fine to
> start out too, just means we'd need to introduce an option/tunable if we
> ever want to change it.

I'll see if I find a good indicator that is understandable for users and 
results in something deterministic from our side to be testable.

But AFAICS the current config interface would need an extra tunable 
anyway to express some kind of factor which controls how much the loose 
colocation rule anyway, so I think that would fit better in a follow-up 
series - especially if users request it :).


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel