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

["Daniel Kral" <d.kral@proxmox.com>]

On Fri Oct 17, 2025 at 1:14 PM CEST, Fiona Ebner wrote:
> Am 30.09.25 um 4:21 PM schrieb Daniel Kral:
>> 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.

Sounds great, I'll do that!

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

Yes, I picked it up from the Perl documentation RFC. This makes it
easier to reference it in perlpod when one can use only the short
heading instead of having to write out the heading with all of the
parameters, while the latter will be used for the LSP AFAICT (which I
actually don't use myself..).

L<< selects the service's node|/select_service_node(...) >>>

This one could be removed because currently it isn't used anywhere.

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

Right, will do!


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