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

[Fiona Ebner]

Am 25.03.25 um 16:12 schrieb Daniel Kral:
> Add a mechanism to the node selection subroutine, which enforces the
> colocation rules defined in the rules config.
> 
> The algorithm manipulates the set of nodes directly, which the service
> is allowed to run on, depending on the type and strictness of the
> colocation rules, if there are any.
> 
> This makes it depend on the prior removal of any nodes, which are
> unavailable (i.e. offline, unreachable, or weren't able to start the
> service in previous tries) or are not allowed to be run on otherwise
> (i.e. HA group node restrictions) to function correctly.
> 
> Signed-off-by: Daniel Kral <d.kral at proxmox.com>
> ---
>  src/PVE/HA/Manager.pm      | 203 ++++++++++++++++++++++++++++++++++++-
>  src/test/test_failover1.pl |   4 +-
>  2 files changed, 205 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 8f2ab3d..79b6555 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -157,8 +157,201 @@ sub get_node_priority_groups {
>      return ($pri_groups, $group_members);
>  }
>  

I feel like these helper functions should rather go into the colocation
plugin or some other module to not bloat up Manager.pm more.

> +=head3 get_colocated_services($rules, $sid, $online_node_usage)
> +
> +Returns a hash map of all services, which are specified as being in a positive
> +or negative colocation in C<$rules> with the given service with id C<$sid>.
> +
> +Each service entry consists of the type of colocation, strictness of colocation
> +and the node the service is currently assigned to, if any, according to
> +C<$online_node_usage>.
> +
> +For example, a service C<'vm:101'> being strictly colocated together (positive)
> +with two other services C<'vm:102'> and C<'vm:103'> and loosely colocated
> +separate with another service C<'vm:104'> results in the hash map:
> +
> +    {
> +	'vm:102' => {
> +	    affinity => 'together',
> +	    strict => 1,
> +	    node => 'node2'
> +	},
> +	'vm:103' => {
> +	    affinity => 'together',
> +	    strict => 1,
> +	    node => 'node2'
> +	},
> +	'vm:104' => {
> +	    affinity => 'separate',
> +	    strict => 0,
> +	    node => undef

Why is the node undef here?

> +	}
> +    }
> +
> +=cut
> +
> +sub get_colocated_services {
> +    my ($rules, $sid, $online_node_usage) = @_;
> +
> +    my $services = {};
> +
> +    PVE::HA::Rules::Colocation::foreach_colocation_rule($rules, sub {
> +	my ($rule) = @_;
> +
> +	for my $csid (sort keys %{$rule->{services}}) {
> +	    next if $csid eq $sid;
> +
> +	    $services->{$csid} = {
> +		node => $online_node_usage->get_service_node($csid),
> +		affinity => $rule->{affinity},
> +		strict => $rule->{strict},
> +	    };
> +        }
> +    }, {
> +	sid => $sid,
> +    });
> +
> +    return $services;
> +}
> +
> +=head3 get_colocation_preference($rules, $sid, $online_node_usage)
> +
> +Returns a list of two hashes, where each is a hash map of the colocation
> +preference of C<$sid>, according to the colocation rules in C<$rules> and the
> +service locations in C<$online_node_usage>.
> +
> +The first hash is the positive colocation preference, where each element
> +represents properties for how much C<$sid> prefers to be on the node.
> +Currently, this is a binary C<$strict> field, which means either it should be

s/it/the service/

> +there (C<0>) or must be there (C<1>).
> +
> +The second hash is the negative colocation preference, where each element
> +represents properties for how much C<$sid> prefers not to be on the node.
> +Currently, this is a binary C<$strict> field, which means either it should not

s/it/the service/

> +be there (C<0>) or must not be there (C<1>).
> +
> +=cut
> +
> +sub get_colocation_preference {
> +    my ($rules, $sid, $online_node_usage) = @_;
> +
> +    my $services = get_colocated_services($rules, $sid, $online_node_usage);

The name $services is a bit too generic, maybe $colocation_per_service
or something?

Maybe it would be better to just merge this one and the helper above
into a single one? I.e. just handle the info while iterating the rules
directly instead of creating a novel temporary per-service
data-structure and iterate twice.

> +
> +    my $together = {};
> +    my $separate = {};
> +
> +    for my $service (values %$services) {
> +	my $node = $service->{node};
> +
> +	next if !$node;
> +
> +	my $node_set = $service->{affinity} eq 'together' ? $together : $separate;
> +	$node_set->{$node}->{strict} = $node_set->{$node}->{strict} || $service->{strict};
> +    }
> +
> +    return ($together, $separate);
> +}
> +
> +=head3 apply_positive_colocation_rules($together, $allowed_nodes)
> +
> +Applies the positive colocation preference C<$together> on the allowed node
> +hash set C<$allowed_nodes> directly.
> +
> +Positive colocation means keeping services together on a single node, and
> +therefore minimizing the separation of services.
> +
> +The allowed node hash set C<$allowed_nodes> is expected to contain any node,
> +which is available to the service, i.e. each node is currently online, is
> +available according to other location constraints, and the service has not
> +failed running there yet.
> +
> +=cut
> +
> +sub apply_positive_colocation_rules {
> +    my ($together, $allowed_nodes) = @_;
> +
> +    return if scalar(keys %$together) < 1;
> +
> +    my $mandatory_nodes = {};
> +    my $possible_nodes = PVE::HA::Tools::intersect($allowed_nodes, $together);
> +
> +    for my $node (sort keys %$together) {
> +	$mandatory_nodes->{$node} = 1 if $together->{$node}->{strict};
> +    }
> +
> +    if (scalar keys %$mandatory_nodes) {
> +	# limit to only the nodes the service must be on.
> +	for my $node (keys %$allowed_nodes) {
> +	    next if exists($mandatory_nodes->{$node});

Style nit: I'd avoid using exists() if you explicitly expect a set
value. Otherwise, it can break because of accidental auto-vivification
in the future.

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

> +	}
> +    }
> +}
> +
> +=head3 apply_negative_colocation_rules($separate, $allowed_nodes)
> +
> +Applies the negative colocation preference C<$separate> on the allowed node
> +hash set C<$allowed_nodes> directly.
> +
> +Negative colocation means keeping services separate on multiple nodes, and
> +therefore maximizing the separation of services.
> +
> +The allowed node hash set C<$allowed_nodes> is expected to contain any node,
> +which is available to the service, i.e. each node is currently online, is
> +available according to other location constraints, and the service has not
> +failed running there yet.
> +
> +=cut
> +
> +sub apply_negative_colocation_rules {
> +    my ($separate, $allowed_nodes) = @_;
> +
> +    return if scalar(keys %$separate) < 1;
> +
> +    my $mandatory_nodes = {};
> +    my $possible_nodes = PVE::HA::Tools::set_difference($allowed_nodes, $separate);
> +
> +    for my $node (sort keys %$separate) {
> +	$mandatory_nodes->{$node} = 1 if $separate->{$node}->{strict};
> +    }
> +
> +    if (scalar keys %$mandatory_nodes) {
> +	# limit to the nodes the service must not be on.
> +	for my $node (keys %$allowed_nodes) {
> +	    next if !exists($mandatory_nodes->{$node});
> +
> +	    delete $allowed_nodes->{$node};
> +	}
> +    } elsif (scalar keys %$possible_nodes) {
> +	# limit to the nodes the service should not be on, if any.
> +	for my $node (keys %$allowed_nodes) {
> +	    next if exists($possible_nodes->{$node});
> +
> +	    delete $allowed_nodes->{$node};
> +	}
> +    }
> +}
> +
> +sub apply_colocation_rules {
> +    my ($rules, $sid, $allowed_nodes, $online_node_usage) = @_;
> +
> +    my ($together, $separate) = get_colocation_preference($rules, $sid, $online_node_usage);
> +
> +    apply_positive_colocation_rules($together, $allowed_nodes);
> +    apply_negative_colocation_rules($separate, $allowed_nodes);

I think there could be a problematic scenario with
* no strict positive rules, but loose strict positive rules
* strict negative rules
where apply_positive_colocation_rules() will limit $allowed_nodes in
such a way that the strict negative rules cannot be satisfied anymore
afterwards.

I feel like what we actually want from non-strict rules is not to limit
the allowed nodes at all, but only express preferences. After scoring,
we could:
1. always take a colocation preference node if present no matter what
the usage score is
2. have a threshold to not follow through, if there is a non-colocation
preference node with a much better usage score relatively
3. somehow massage it into the score itself. E.g. every node that would
be preferred by colocation gets a 0.5 multiplier score adjustment while
other scores are unchanged - remember that lower score is better.
4. [insert your suggestion here]

So to me it seems like there should be a helper that gives us:
1. list of nodes that satisfy strict rules - these we can then intersect
with the $pri_nodes
2. list of nodes that are preferred by non-strict rules - these we can
consider after scoring

> +}
> +
>  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) = @_;
>  
>      my $group = get_service_group($groups, $online_node_usage, $service_conf);
>  
> @@ -189,6 +382,8 @@ sub select_service_node {
>  
>      return $current_node if (!$try_next && !$best_scored) && $pri_nodes->{$current_node};
>  
> +    apply_colocation_rules($rules, $sid, $pri_nodes, $online_node_usage);
> +
>      my $scores = $online_node_usage->score_nodes_to_start_service($sid, $current_node);
>      my @nodes = sort {
>  	$scores->{$a} <=> $scores->{$b} || $a cmp $b