Re: [pve-devel] [PATCH ha-manager 09/15] manager: apply colocation rules when selecting service nodes

[Daniel Kral <d.kral@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