Re: [pve-devel] [PATCH ha-manager 5/9] rules: resource affinity: decouple get_resource_affinity helper from Usage class

[Fiona Ebner <f.ebner@proxmox.com>]

Am 30.09.25 um 4:21 PM schrieb Daniel Kral:
> The resource affinity rules need information about the other HA
> resource's used nodes to be enacted correctly, which has been proxied
> through $online_node_usage before.
> 
> The get_used_service_nodes(...) reflects the same logic to retrieve the
> nodes, where a HA resource $sid currently puts load on, as in
> recompute_online_node_usage(...).
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

with some comments:

> ---
> I wanted to put this information in PVE::HA::Usage directly, but figured
> it would make Usage much more dependent on $ss / $sd.. We can still do
> that later on or move the helper elsewhere, e.g. making ServiceStatus
> its own class?

I think having the get_used_service_nodes() helper in PVE::HA::Usage is
better than in PVE::HA::Tools, because to me, "tools" sounds sounds like
things that should not be concerned with concrete HA state logic. You
could adapt the signature to take e.g.
($state, $online_nodes, $node, $target)
rather than $sd to avoid any need to know about its internal structure.

> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 468e41eb..0226e427 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -121,12 +121,12 @@ sub flush_master_status {
>  
>  =head3 select_service_node(...)
>  
> -=head3 select_service_node($rules, $online_node_usage, $sid, $service_conf, $sd, $node_preference)
> +=head3 select_service_node($rules, $online_node_usage, $sid, $service_conf, $ss, $node_preference)
>  

Pre-existing, but is this duplicate heading intentional?

> diff --git a/src/PVE/HA/Tools.pm b/src/PVE/HA/Tools.pm
> index 71eb5d0b..7f718e25 100644
> --- a/src/PVE/HA/Tools.pm
> +++ b/src/PVE/HA/Tools.pm
> @@ -188,6 +188,35 @@ sub count_fenced_services {
>      return $count;
>  }
>  
> +sub get_used_service_nodes {
> +    my ($sd, $online_nodes) = @_;
> +
> +    my $result = {};
> +
> +    my ($state, $node, $target) = $sd->@{qw(state node target)};
> +
> +    return $result if $state eq 'stopped' || $state eq 'request_start';
> +
> +    if (
> +        $state eq 'started'
> +        || $state eq 'request_stop'
> +        || $state eq 'fence'
> +        || $state eq 'freeze'
> +        || $state eq 'error'
> +        || $state eq 'recovery'
> +        || $state eq 'migrate'
> +        || $state eq 'relocate'
> +    ) {
> +        $result->{current} = $node if $online_nodes->{$node};
> +    }
> +
> +    if ($state eq 'migrate' || $state eq 'relocate' || $state eq 'request_start_balance') {
> +        $result->{target} = $target if defined($target) && $online_nodes->{$target};
> +    }
> +
> +    return $result;

Returning ($current, $target) seems to better match what callers need.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel