Re: [pve-devel] [PATCH ha-manager 18/18] factor out counting of active services into helper

["Michael Köppl" <m.koeppl@proxmox.com>]

During my review I was wondering why 16/18 and 17/18 added similar
checks to both the manager's and the API's active service counting and
why they're not combined into a single helper function, only to then
notice that the final patch does exactly that. Is there a reason it's
done in this order? Other patches in this series do it the other way
around or combine the introduction of a new helper function and its use.

On Thu Aug 21, 2025 at 4:35 PM CEST, Daniel Kral wrote:
> Make the counting its own helper to be used throughout the code base to
> have a single source of truth of what constitutes a active services.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  src/PVE/API2/HA/Status.pm | 25 +++----------------------
>  src/PVE/HA/LRM.pm         | 20 +-------------------
>  src/PVE/HA/Manager.pm     | 14 +-------------
>  src/PVE/HA/Tools.pm       | 25 +++++++++++++++++++++++++
>  4 files changed, 30 insertions(+), 54 deletions(-)
>
> diff --git a/src/PVE/API2/HA/Status.pm b/src/PVE/API2/HA/Status.pm
> index 3d9aabaa..282577cc 100644
> --- a/src/PVE/API2/HA/Status.pm
> +++ b/src/PVE/API2/HA/Status.pm
> @@ -193,28 +193,9 @@ __PACKAGE__->register_method({
>                  };
>          }
>  
> -        # compute active services for all nodes
> -        my $active_count = {};
> -        foreach my $sid (sort keys %{ $status->{service_status} }) {
> -            my $sd = $status->{service_status}->{$sid};
> -            my $target = $sd->{target}; # count as active if we are the target.
> -            next if !$sd->{node};
> -            $active_count->{ $sd->{node} } = 0 if !defined($active_count->{ $sd->{node} });
> -            my $req_state = $sd->{state};
> -            next if !defined($req_state);
> -            next if $req_state eq 'stopped';
> -            next if $req_state eq 'freeze';
> -            next if $req_state eq 'error';
> -            next if $req_state eq 'request_start';
> -            $active_count->{ $sd->{node} }++;
> -
> -            if ($target && $target ne $sd->{node}) {
> -                $active_count->{$target} = 0 if !defined($active_count->{$target});
> -                $active_count->{$target}++;
> -            }
> -        }
> -
>          foreach my $node (sort keys %{ $status->{node_status} }) {
> +            my $active_count =
> +                PVE::HA::Tools::count_active_services($status->{service_status}, $node);
>              my $lrm_status = PVE::HA::Config::read_lrm_status($node);
>              my $id = "lrm:$node";
>              if (!$lrm_status->{timestamp}) {
> @@ -235,7 +216,7 @@ __PACKAGE__->register_method({
>                      if ($lrm_mode ne 'active') {
>                          $status_str = "$lrm_mode mode";
>                      } else {
> -                        if ($lrm_state eq 'wait_for_agent_lock' && !$active_count->{$node}) {
> +                        if ($lrm_state eq 'wait_for_agent_lock' && !$active_count) {
>                              $status_str = 'idle';
>                          } else {
>                              $status_str = $lrm_state;
> diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
> index b645e5b0..1342c8eb 100644
> --- a/src/PVE/HA/LRM.pm
> +++ b/src/PVE/HA/LRM.pm
> @@ -297,25 +297,7 @@ sub active_service_count {
>  
>      my $ss = $self->{service_status};
>  
> -    my $count = 0;
> -    foreach my $sid (keys %$ss) {
> -        my $sd = $ss->{$sid};
> -        my $target = $sd->{target}; # count as active if we are the target.
> -        next if (!$sd->{node} || $sd->{node} ne $nodename) && (!$target || $target ne $nodename);
> -        my $req_state = $sd->{state};
> -        next if !defined($req_state);
> -        next if $req_state eq 'stopped';
> -        # NOTE: 'ignored' ones are already dropped by the manager from service_status
> -        next if $req_state eq 'freeze';
> -        # erroneous services are not managed by HA, don't count them as active
> -        next if $req_state eq 'error';
> -        # request_start is for (optional) better node selection for stop -> started transition
> -        next if $req_state eq 'request_start';
> -
> -        $count++;
> -    }
> -
> -    return $count;
> +    return PVE::HA::Tools::count_active_services($ss, $nodename);
>  }
>  
>  my $wrote_lrm_status_at_startup = 0;
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 0cbeb28d..374c9022 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -540,19 +540,7 @@ my $have_groups_been_migrated = sub {
>  my $is_lrm_active_or_idle = sub {
>      my ($ss, $node, $lrm_state) = @_;
>  
> -    my $active_count = 0;
> -    for my $sid (sort keys %$ss) {
> -        my $sd = $ss->{$sid};
> -        my $target = $sd->{target}; # count as active if we are the target.
> -        next if (!$sd->{node} || $sd->{node} ne $node) && (!$target || $target ne $node);
> -        my $req_state = $sd->{state};
> -        next if !defined($req_state);
> -        next if $req_state eq 'stopped';
> -        next if $req_state eq 'freeze';
> -        next if $req_state eq 'error';
> -        next if $req_state eq 'request_start';
> -        $active_count++;
> -    }
> +    my $active_count = PVE::HA::Tools::count_active_services($ss, $node);
>  
>      return 1 if $lrm_state eq 'active';
>      return 1 if $lrm_state eq 'wait_for_agent_lock' && !$active_count;
> diff --git a/src/PVE/HA/Tools.pm b/src/PVE/HA/Tools.pm
> index 47b4aa03..32f08723 100644
> --- a/src/PVE/HA/Tools.pm
> +++ b/src/PVE/HA/Tools.pm
> @@ -188,6 +188,31 @@ sub count_fenced_services {
>      return $count;
>  }
>  
> +sub count_active_services {
> +    my ($ss, $node) = @_;
> +
> +    my $count = 0;
> +
> +    foreach my $sid (keys %$ss) {
> +        my $sd = $ss->{$sid};
> +        my $target = $sd->{target}; # count as active if we are the target.
> +        next if (!$sd->{node} || $sd->{node} ne $node) && (!$target || $target ne $node);
> +        my $req_state = $sd->{state};
> +        next if !defined($req_state);
> +        next if $req_state eq 'stopped';
> +        # NOTE: 'ignored' ones are already dropped by the manager from service_status
> +        next if $req_state eq 'freeze';
> +        # erroneous services are not managed by HA, don't count them as active
> +        next if $req_state eq 'error';
> +        # request_start is for (optional) better node selection for stop -> started transition
> +        next if $req_state eq 'request_start';
> +
> +        $count++;
> +    }
> +
> +    return $count;
> +}
> +
>  sub get_verbose_service_state {
>      my ($service_state, $service_conf) = @_;
>  



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