From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 6F71F1FF0DD for ; Thu, 22 Oct 2026 14:05:59 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8EB83215F5; Thu, 22 Oct 2026 14:05:56 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 22 Oct 2026 14:05:50 +0200 Message-Id: Subject: Re: [PATCH pve-ha-manager 12/15] manager: log changes to crs settings From: "Daniel Kral" To: "Dominik Rusovac" , X-Mailer: aerc 0.22.0-10-g6373ac9d2179-dirty References: <20261021085932.1196302-1-d.rusovac@proxmox.com> <20261021085932.1196302-13-d.rusovac@proxmox.com> In-Reply-To: <20261021085932.1196302-13-d.rusovac@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792670750483 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.724 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: R43QH7GRYSP5O3Y5FSVSTXHNG2RVTKI7 X-Message-ID-Hash: R43QH7GRYSP5O3Y5FSVSTXHNG2RVTKI7 X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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; > } > =20 > +sub update_and_log_change { > + my ($self, $curr, $next, $msg) =3D @_; > + > + my $haenv =3D $self->{haenv}; > + > + if (defined($$curr) && $$curr ne $next) { > + $haenv->log('info', "$msg from '$$curr' to '$next'"); > + } > + > + $$curr =3D $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) =3D @_; my $haenv =3D $self->{haenv}; my $old_value =3D $$optionref; my $new_value =3D $value // $default; if (!defined($old_value)) { $haenv->log('info', "using $name '$new_value'") if $new_value ne $d= efault; } elsif ($old_value ne $new_value) { $haenv->log('info', "switching $name from '$old_value' to '$new_val= ue'"); $$optionref =3D $new_value; } } and then sub update_crs_scheduler_mode { my ($self) =3D @_; my $dc_cfg =3D $haenv->get_datacenter_settings(); my $crs_cfg =3D $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) =3D @_; > =20 > @@ -106,37 +120,70 @@ sub update_crs_scheduler_mode { > my $dc_cfg =3D $haenv->get_datacenter_settings(); > my $crs_cfg =3D $dc_cfg->{crs} // {}; > =20 > - $self->{crs}->{rebalance_on_request_start} =3D !!$crs_cfg->{'ha-reba= lance-on-start'}; > - $self->{crs}->{auto_rebalance}->{enable} =3D !!$crs_cfg->{'ha-auto-r= ebalance'}; > + my $log_prefix =3D "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 =3D $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} =3D > - ($crs_cfg->{'ha-auto-rebalance-threshold'} // 30) / 100.0; > - $self->{crs}->{auto_rebalance}->{method} =3D $crs_cfg->{'ha-auto-reb= alance-method'} > - // 'bruteforce'; > - $self->{crs}->{auto_rebalance}->{hold_duration} =3D $crs_cfg->{'ha-a= uto-rebalance-hold-duration'} > - // 3; > - $self->{crs}->{auto_rebalance}->{margin} =3D > - ($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", > + ); > =20 > my $default_weight =3D 1; > - $self->{crs}->{auto_rebalance}->{cpu_weight} =3D $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 =3D 5 if ($self->{crs}->{auto_rebalance}->{method} e= q 'topsis'); > - $self->{crs}->{auto_rebalance}->{mem_weight} =3D $crs_cfg->{'ha-auto= -rebalance-mem-weight'} // $default_weight; > + $default_weight =3D 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", > + ); > =20 > - my $old_mode =3D $self->{crs}->{scheduler}; > + my $old_mode =3D \$self->{crs}->{scheduler}; > my $new_mode =3D $crs_cfg->{ha} || 'basic'; > =20 > - 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' t= o '$new_mode'"); > } > - > - $self->{crs}->{scheduler} =3D $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. > + ); > =20 > return; > }