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

[Daniel Kral]

On 3/25/25 16:12, Daniel Kral wrote:
>   sub select_service_node {
> -    my ($groups, $online_node_usage, $sid, $service_conf, $current_node, $try_next, $tried_nodes, $maintenance_fallback, $best_scored) = @_;
> +    # TODO Cleanup this signature post-RFC
> +    my ($rules, $groups, $online_node_usage, $sid, $service_conf, $current_node, $try_next, $tried_nodes, $maintenance_fallback, $best_scored) = @_;

I'm currently trying to clean up the helper's signature here, but doing 
something like

sub select_service_node {
     my ($service_info, $affinity_info, $try_next, $best_scored) = @_;

     my ($sid, $service_conf, $current_node) = $service_info->@{qw(sid 
config current_node)};
     my ($rules, $groups, $online_node_usage, $tried_nodes, 
$maintenance_fallback) =
	$affinity_info->@{qw(rules groups online_node_usage failed_nodes 
maintenance_node)};

would require us to create helper structures on all four call sites (one 
of them is just the test case ./test_failover1.pl), or introduce another 
helper to just create them for passing it here and immediately 
de-structuring it in select_service_node(...):

sub get_service_affinity_info {
     my ($self, $sid, $cd, $sd) = @_;

     my $service_info = {
	sid => $sid,
	config => $cd,
	current_node => $sd->{node},
     };

     my $affinity_info = {
	rules => $self->{rules},
	groups => $self->{groups},
	failed_nodes => $sd->{failed_nodes},
	maintenance_node => $sd->{maintenance_node},
	online_node_usage => $self->{online_node_usage},
     };

     return ($service_info, $affinity_info);
};

Also the call site in next_state_recovery(...) does not pass 
$sd->{failed_nodes}, $sd->{maintenance_node} and $best_scored to it. 
AFAICS $sd->{failed_nodes} should be undef in next_state_recovery(...) 
anyway, but I feel like I have missed some states it could be in there. 
And $sd->{maintenance_node} could be set anytime.

If there's nothing speaking against that, I'd prefer to elevate 
select_service_node(...) to be a method as it needs quite a lot of state 
anyway, especially as we will need global information about other 
services than just the current one in the future anyway.

So, I'd do something like

sub select_service_node {
     my ($self, $sid, $service_conf, $sd, $mode) = @_;

     my ($rules, $groups, $online_node_usage) = $self->@{qw(rules groups 
online_node_usage)};
     my ($current_node, $tried_nodes, $maintenance_fallback) = 
$self->@{qw(node failed_nodes maintenance_node)};

here. It's not fancy as in there's a well-defined interface one can 
immediately see what this helper needs (as it has access to the whole 
$self) and doesn't have the guarantees of a standalone helper (won't 
touch $self), but I think it could be better than creating helper 
structures which are only pass a message, which is immediately 
destructured anyway. We could also just pass $self slightly differently, 
but I don't see much difference there.

The $mode could then be a enumeration of e.g. whether $try_next (e.g. 
'try_again') or $best_scored (e.g. 'rebalance') is used (and can be 
extended of course). Those are mutually exclusive in the three call 
sites right now. If next_state_recovery(...) really does have states 
where $tried_nodes is set (and $maintenance_node too), then we can also 
introduce a 'recovery' state, which will ignore them.

The names for $service_conf and $sd can also be improved, but I wanted 
to introduce minimal change to select_service_node(...) as well as stay 
to the $sd name for the service data as in other places of the Manager.pm.

That's still just a work in progress and I'd very appreciate some 
feedback if any of the two above are viable options here. If it helps 
any, I'd send the result as a separate series in advance which the HA 
colocation will then be based on, so we don't loose focus in the HA 
colocation patch series.

CC'd @Fiona and @Fabian here, if you have any thoughts here :).