Re: [PATCH pve-ha-manager 2/3] manager: set service config value in self

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

Looks good to me, left a few nits inline, with those resolved consider
this patch as:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>

On Mon May 11, 2026 at 5:57 PM CEST, Dominik Rusovac wrote:
> This is in preparation for the follow-up patch.
>
> Reading the value of 'auto-rebalance'-flag in the service config of an
> HA resource is required to perform proper resource bundling.
>
> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>  src/PVE/HA/Manager.pm | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index b69a6bb..2a4b31e 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -1003,6 +1003,7 @@ sub manage {
>      $self->try_persistent_group_migration();
>  
>      my ($sc, $services_digest) = $haenv->read_service_config();
> +    $self->{sc} = $sc;

nit: could be

    ($self->{sc}, my $services_digest) = $haenv->read_service_config();

would need a few more s/$sc/$self->{sc}/

>  
>      $self->{groups} = $haenv->read_group_config(); # update
>  
> @@ -1011,9 +1012,9 @@ sub manage {
>      # skip service add/remove when disarmed - handle_disarm manages service status
>      if (!$ms->{disarm}) {
>          # add new service
> -        foreach my $sid (sort keys %$sc) {
> +        foreach my $sid (sort keys $self->{sc}->%*) {

nit: pre-existing, but foreach is a synonym for for and we prefer the
latter according to our Perl Style Guide.

We don't change all at once to cause unnecessary merge conflicts for
already existing patch series, but usually change these when we touch
the relevant line/context, so it can be changed here and for other
foreach's you touch (at least I do so ^^).

>              next if $ss->{$sid}; # already there
> -            my $cd = $sc->{$sid};
> +            my $cd = $self->{sc}->{$sid};
>              next if $cd->{state} eq 'ignored';
>  
>              $haenv->log('info', "adding new service '$sid' on node '$cd->{node}'");
> @@ -1028,9 +1029,9 @@ sub manage {
>  
>          # remove stale or ignored services from manager state
>          foreach my $sid (keys %$ss) {

nit: could also have a

    my $cd = $self->{sc}->{$sid};

so we don't need to repeat the $self->{sc}->{$sid}

> -            next if $sc->{$sid} && $sc->{$sid}->{state} ne 'ignored';
> +            next if $self->{sc}->{$sid} && $self->{sc}->{$sid}->{state} ne 'ignored';
>  
> -            my $reason = defined($sc->{$sid}) ? 'ignored state requested' : 'no config';
> +            my $reason = defined($self->{sc}->{$sid}) ? 'ignored state requested' : 'no config';
>              $haenv->log('info', "removing stale service '$sid' ($reason)");
>  
>              # remove all service related state information
> @@ -1088,7 +1089,7 @@ sub manage {
>          foreach my $sid (sort keys %$ss) {
>              next if $deferred_sids && !$deferred_sids->{$sid};
>              my $sd = $ss->{$sid};
> -            my $cd = $sc->{$sid} || { state => 'disabled' };
> +            my $cd = $self->{sc}->{$sid} || { state => 'disabled' };
>  
>              my $lrm_res = $sd->{uid} ? $lrm_results->{ $sd->{uid} } : undef;
>  

The migrate_groups_to_{resources,rules}() calls later in manage() should
use $self->{sc} too.

nit: Also to be safe, add

    $self->{sc} = {};

to PVE::HA::Manager::new(), or maybe even initialize it with
$haenv->read_service_config(), though we don't have any need to read it
before the assignment in PVE::HA::Manager::manage().