From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E3BDE1FF146 for ; Tue, 12 May 2026 11:06:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C06C0AD0E; Tue, 12 May 2026 11:06:51 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 12 May 2026 11:06:14 +0200 Message-Id: From: "Daniel Kral" To: "Dominik Rusovac" , Subject: Re: [PATCH pve-ha-manager 2/3] manager: set service config value in self X-Mailer: aerc 0.21.0-136-gdb9fe9896a79-dirty References: <20260511155734.149101-1-d.rusovac@proxmox.com> <20260511155734.149101-3-d.rusovac@proxmox.com> In-Reply-To: <20260511155734.149101-3-d.rusovac@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778576662804 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.076 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: ZZ3NO7QCBE7IXKXUK4WYXJRQXCGV3IAX X-Message-ID-Hash: ZZ3NO7QCBE7IXKXUK4WYXJRQXCGV3IAX 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: Looks good to me, left a few nits inline, with those resolved consider this patch as: Reviewed-by: Daniel Kral 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 > --- > 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(); > =20 > my ($sc, $services_digest) =3D $haenv->read_service_config(); > + $self->{sc} =3D $sc; nit: could be ($self->{sc}, my $services_digest) =3D $haenv->read_service_config(); would need a few more s/$sc/$self->{sc}/ > =20 > $self->{groups} =3D $haenv->read_group_config(); # update > =20 > @@ -1011,9 +1012,9 @@ sub manage { > # skip service add/remove when disarmed - handle_disarm manages serv= ice 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 =3D $sc->{$sid}; > + my $cd =3D $self->{sc}->{$sid}; > next if $cd->{state} eq 'ignored'; > =20 > $haenv->log('info', "adding new service '$sid' on node '$cd-= >{node}'"); > @@ -1028,9 +1029,9 @@ sub manage { > =20 > # remove stale or ignored services from manager state > foreach my $sid (keys %$ss) { nit: could also have a my $cd =3D $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'; > =20 > - my $reason =3D defined($sc->{$sid}) ? 'ignored state request= ed' : 'no config'; > + my $reason =3D defined($self->{sc}->{$sid}) ? 'ignored state= requested' : 'no config'; > $haenv->log('info', "removing stale service '$sid' ($reason)= "); > =20 > # 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 =3D $ss->{$sid}; > - my $cd =3D $sc->{$sid} || { state =3D> 'disabled' }; > + my $cd =3D $self->{sc}->{$sid} || { state =3D> 'disabled' }; > =20 > my $lrm_res =3D $sd->{uid} ? $lrm_results->{ $sd->{uid} } : = undef; > =20 The migrate_groups_to_{resources,rules}() calls later in manage() should use $self->{sc} too. nit: Also to be safe, add $self->{sc} =3D {}; 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().