[pve-devel] [PATCH ha-manager v2 02/26] manager: improve signature of select_service_node

[Thomas Lamprecht]

Am 20.06.25 um 16:31 schrieb Daniel Kral:
> As the signature of select_service_node(...) has become rather long
> already, make it more compact by retrieving service- and
> affinity-related data directly from the service state in $sd and
> introduce a $mode parameter to distinguish the behaviors of $try_next
> and $best_scored, which have already been mutually exclusive before.
> 
> Signed-off-by: Daniel Kral <d.kral at proxmox.com>
> ---
> changes since v1:
>     - NEW!
> 
>  src/PVE/HA/Manager.pm      | 87 +++++++++++++++++++++-----------------
>  src/test/test_failover1.pl | 17 +++-----
>  2 files changed, 53 insertions(+), 51 deletions(-)
> 
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 85f2b1a..85bb114 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -149,18 +149,41 @@ sub get_node_priority_groups {
>      return ($pri_groups, $group_members);
>  }
>  
> +=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).

> +
> +=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'>: ...?


> +
> +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?



> +
> +=back
> +
> +=cut
> +
>  sub select_service_node {
> -    my (
> -        $groups,
> -        $online_node_usage,
> -        $sid,
> -        $service_conf,
> -        $current_node,
> -        $try_next,
> -        $tried_nodes,
> -        $maintenance_fallback,
> -        $best_scored,
> -    ) = @_;
> +    my ($groups, $online_node_usage, $sid, $service_conf, $sd, $mode) = @_;
> +
> +    my ($current_node, $tried_nodes, $maintenance_fallback) =
> +        $sd->@{qw(node failed_nodes maintenance_node)};
>  
>      my $group = get_service_group($groups, $online_node_usage, $service_conf);
>