Re: [PATCH pve-ha-manager 12/15] manager: log changes to crs settings

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

Nice, thanks for adding this!

This is definitely helpful to have in the log so changes in the config
have a timestamp if one wants to triage a bug or follow what happened in
the past.

On Wed Oct 21, 2026 at 10:59 AM CEST, Dominik Rusovac wrote:
> Especially with the addition of weights, it is important to be able to
> trace how the load balancer is configured.
>
> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>  src/PVE/HA/Manager.pm | 89 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index c6818e2..4d0114c 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -99,6 +99,20 @@ sub new {
>      return $self;
>  }
>  
> +sub update_and_log_change {
> +    my ($self, $curr, $next, $msg) = @_;
> +
> +    my $haenv = $self->{haenv};
> +
> +    if (defined($$curr) && $$curr ne $next) {
> +        $haenv->log('info', "$msg from '$$curr' to '$next'");
> +    }
> +
> +    $$curr = $next;
> +
> +    return;
> +}

It would be nice that this also logs what the value is set to at the
start when it is first set, i.e., when defined($$curr) is true and isn't
the default value, just as it's already done for the scheduler mode.

Would be nice to see this first factor out the logic already present for
the scheduler mode and then in a second patch use it on the other
options.

Furthermore, I think we can specialize the helper to crs settings for
now as long as we don't have any other parts to configure in the near
future.

Maybe we could go with a helper like

sub update_crs_option {
    my ($self, $name, $optionref, $value, $default) = @_;

    my $haenv = $self->{haenv};

    my $old_value = $$optionref;
    my $new_value = $value // $default;

    if (!defined($old_value)) {
        $haenv->log('info', "using $name '$new_value'") if $new_value ne $default;
    } elsif ($old_value ne $new_value) {
        $haenv->log('info', "switching $name from '$old_value' to '$new_value'");

        $$optionref = $new_value;
    }
}


and then


sub update_crs_scheduler_mode {
    my ($self) = @_;

    my $dc_cfg = $haenv->get_datacenter_settings();
    my $crs_cfg = $dc_cfg->{crs} // {};

    $self->update_crs_option(
        'scheduler mode',
        \$self->{crs}->{scheduler},
        $crs_cfg->{ha},
        'basic',
    );

    # ... and so on ...
}


s/switching/changing/ can be done in a separate patch if that makes it
more general for all kinds of options.

> +
>  sub update_crs_scheduler_mode {
>      my ($self) = @_;
>  
> @@ -106,37 +120,70 @@ sub update_crs_scheduler_mode {
>      my $dc_cfg = $haenv->get_datacenter_settings();
>      my $crs_cfg = $dc_cfg->{crs} // {};
>  
> -    $self->{crs}->{rebalance_on_request_start} = !!$crs_cfg->{'ha-rebalance-on-start'};
> -    $self->{crs}->{auto_rebalance}->{enable} = !!$crs_cfg->{'ha-auto-rebalance'};
> +    my $log_prefix = "crs settings:";
> +
> +    $self->update_and_log_change(
> +        \$self->{crs}->{rebalance_on_request_start},
> +        !!$crs_cfg->{'ha-rebalance-on-start'},
> +        "$log_prefix 'ha-rebalance-on-start' changed",
> +    );
> +
> +    my $auto_rebalance = $self->{crs}->{auto_rebalance};
> +
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{enable},
> +        !!$crs_cfg->{'ha-auto-rebalance'},
> +        "$log_prefix 'ha-auto-rebalance' changed",
> +    );
> +
>      # configured as percent for UI/config consistency, scaled to [0, 1] to match the
>      # internal imbalance value that the comparison gates against.
> -    $self->{crs}->{auto_rebalance}->{threshold} =
> -        ($crs_cfg->{'ha-auto-rebalance-threshold'} // 30) / 100.0;
> -    $self->{crs}->{auto_rebalance}->{method} = $crs_cfg->{'ha-auto-rebalance-method'}
> -        // 'bruteforce';
> -    $self->{crs}->{auto_rebalance}->{hold_duration} = $crs_cfg->{'ha-auto-rebalance-hold-duration'}
> -        // 3;
> -    $self->{crs}->{auto_rebalance}->{margin} =
> -        ($crs_cfg->{'ha-auto-rebalance-margin'} // 10) / 100.0;
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{threshold},
> +        ($crs_cfg->{'ha-auto-rebalance-threshold'} // 30) / 100.0,
> +        "$log_prefix 'ha-auto-rebalance-threshold' changed",
> +    );
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{method},
> +        $crs_cfg->{'ha-auto-rebalance-method'} // 'bruteforce',
> +        "$log_prefix 'ha-auto-rebalance-method' changed",
> +    );
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{hold_duration},
> +        $crs_cfg->{'ha-auto-rebalance-hold-duration'} // 3,
> +        "$log_prefix 'ha-auto-rebalance-hold-duration' changed",
> +    );
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{margin},
> +        ($crs_cfg->{'ha-auto-rebalance-margin'} // 10) / 100.0,
> +        "$log_prefix 'ha-auto-rebalance-margin' changed",
> +    );
>  
>      my $default_weight = 1;
> -    $self->{crs}->{auto_rebalance}->{cpu_weight} = $crs_cfg->{'ha-auto-rebalance-cpu-weight'} // $default_weight;
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{cpu_weight},
> +        $crs_cfg->{'ha-auto-rebalance-cpu-weight'} // $default_weight,
> +        "$log_prefix 'ha-auto-rebalance-cpu-weight' changed",
> +    );
>      # by default TOPSIS weights mem higher than cpu
> -    $default_weight = 5 if ($self->{crs}->{auto_rebalance}->{method} eq 'topsis');
> -    $self->{crs}->{auto_rebalance}->{mem_weight} = $crs_cfg->{'ha-auto-rebalance-mem-weight'} // $default_weight;
> +    $default_weight = 5 if ($auto_rebalance->{method} eq 'topsis');
> +    $self->update_and_log_change(
> +        \$auto_rebalance->{mem_weight},
> +        $crs_cfg->{'ha-auto-rebalance-mem-weight'} // $default_weight,
> +        "$log_prefix 'ha-auto-rebalance-mem-weight' changed",
> +    );
>  
> -    my $old_mode = $self->{crs}->{scheduler};
> +    my $old_mode = \$self->{crs}->{scheduler};
>      my $new_mode = $crs_cfg->{ha} || 'basic';
>  
> -    if (!defined($old_mode)) {
> +    if (!defined($$old_mode)) {
>          $haenv->log('info', "using scheduler mode '$new_mode'") if $new_mode ne 'basic';
> -    } elsif ($new_mode eq $old_mode) {
> -        return; # nothing to do
> -    } else {
> -        $haenv->log('info', "switching scheduler mode from '$old_mode' to '$new_mode'");
>      }
> -
> -    $self->{crs}->{scheduler} = $new_mode;
> +    $self->update_and_log_change(
> +        $old_mode,
> +        $new_mode,
> +        "$log_prefix scheduler mode changed"

Even though we don't have any test cases for this yet, which would catch
that the log messages for switching the scheduler mode changed, we
should make sure to not change them as long as feasible.

Otherwise, the scheduler mode should probably be logged first before all
of the other options.

> +    );
>  
>      return;
>  }