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
