On 6/23/25 18:21, Thomas Lamprecht wrote:
Am 20.06.25 um 16:31 schrieb Daniel Kral:
+=head3 select_service_node(...)
+
+=head3 select_service_node($groups, $online_node_usage, $sid, $service_conf,
$sd, $mode)
+
+Used to select the best fitting node for the service C<$sid>, with the
+configuration C<$service_conf> and state C<$sd>, according to the groups
defined
+in C<$groups>, available node utilization in C<$online_node_usage>, and the
+given C<$mode>.
+
+The C<$mode> can be set to:
+
+=over
+
+=item C<'none'>
+
+Try to stay on the current node as much as possible.
Why use "none" then? "no mode" is not really helping understand
what happens. Maybe it can be resolved with better names/descriptions,
even for the parameter name (like maybe "node_preference"?), but another
option might be to combine the flags into a hash ref $opts parameter, as
that would also avoid the issues of rather opaque "1" or "0" params on
the call sites, not saying it's a must, but we use that pattern a few
times and conflating such flags into a single string param comes with
its own can of worms (bit more about that below).
I thought about putting them in an $opts, but I felt like a enum-like
type would fit here better to reduce the amount of states slightly as
setting $try_next more or less already implied $best_scored (not the
other way around). $mode = 'none' is really no good information here, I
think $node_preference will be a better name for it here.
+
+=item C<'best-score'>
+
+Try to select the best-scored node.
+
+=item C<'try-next'>
Not sure if switching to a free form string that is nowhere checked for
unknown (invalid) values really is an improvement over the status quo.
Rejecting unknown modes upfront should be definitively added.
Also can we please move the item description as short sentence after the
`=item ...`, i.e., on the same line, to reduce bloating up the code too
much with POD.
=item C<'none'>: Try to stay on the current node as much as possible.
=item C<'best-score'>: Try to select the best-scored node.
=item C<'try-next'>: ...?
Will definitely do that!
Speaking of 'try-next' (more below), shouldn't $sd->{failed_nodes}->@*
be cleaned as soon as the service was successfully started on a node? I
feel like $try_next was introduced to separate it from the 'stay on
current node as much ass possible' behavior and might not be needed
anymore now with an explicit $node_preference = 'none'? Then we're only
left with $node_preference = 'none' / 'best-score' (where the latter is
the standard behavior of the helper.
+
+Try to select the best-scored node, which is not in C<< $sd->{failed_nodes} >>,
+while trying to stay on the current node.
might be a bit to long since I did more in the HA stack, but is "while trying
to stay on the current node" correct?
Right, I misread the last part at select_service_node(...)
my $found;
for (my $i = scalar(@nodes) - 1; $i >= 0; $i--) {
my $node = $nodes[$i];
if ($node eq $current_node) {
$found = $i;
}
}
if ($mode eq 'try-next') {
if (defined($found) && ($found < (scalar(@nodes) - 1))) {
return $nodes[$found + 1];
} else {
return $nodes[0];
}
} else {
return $nodes[0];
}
for preferring the $current_node there but it actually is the next-best
node after $current_node after sorting the nodes by the Usage's scores.
If 'try-next' is set, shouldn't $current_node already have been deleted
from $pri_nodes/@nodes here by
if ($mode eq 'try-next') {
foreach my $node (@$tried_nodes) {
delete $pri_nodes->{$node};
}
}
Either way, the last part of the description is wrong and I'll remove it.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
