From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D0FE01FF141 for ; Mon, 30 Mar 2026 10:56:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6A5821B49D; Mon, 30 Mar 2026 10:57:17 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 30 Mar 2026 10:57:12 +0200 Message-Id: From: "Dominik Rusovac" To: "Daniel Kral" , Subject: Re: [PATCH ha-manager v2 30/40] usage: use add_service to add service usage to nodes X-Mailer: aerc 0.20.0 References: <20260324183029.1274972-1-d.kral@proxmox.com> <20260324183029.1274972-31-d.kral@proxmox.com> In-Reply-To: <20260324183029.1274972-31-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774860979063 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.130 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: M5VC63QOUPGROFBRRWUT42ARJRYOR2WX X-Message-ID-Hash: M5VC63QOUPGROFBRRWUT42ARJRYOR2WX X-MailFrom: d.rusovac@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: see comment inline, pls On Tue Mar 24, 2026 at 7:30 PM CET, Daniel Kral wrote: > The pve_static (and upcoming pve_dynamic) bindings expose the new > add_resource(...) method, which allow adding resources in a single call > with the additional running flag. > > The running flag is needed to discriminate starting and started HA > resources from each other, which is needed to correctly account for HA > resources for the dynamic load usage implementation in the next patch. > > This is because for the dynamic load usage, any HA resource, which is > scheduled to start by the HA Manager in the same round, will not be > accounted for in the next call to score_nodes_to_start_resource(...). > This is not a problem for the static load usage, because there the > current node usages are derived from the running resources on every > call already. > > Passing only the HA resources' 'state' property is not enough since the > HA Manager will move any HA resource from the 'request_start' (or > through other transient states such as 'request_start_balance' and a > successful 'migrate'/'relocate') into the 'started' state. > > This 'started' state is then picked up by the HA resource's LRM, which > will actually start the HA resource and if successful respond with a > 'SUCCESS' LRM result. Only then will the HA Manager acknowledges this by > adding the running flag to the HA resource's state. > > Signed-off-by: Daniel Kral > --- > changes v1 -> v2: > - new! > > src/PVE/HA/Usage.pm | 12 +++++++----- > src/PVE/HA/Usage/Basic.pm | 9 ++++++++- > src/PVE/HA/Usage/Static.pm | 20 ++++++++++++++------ > 3 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/src/PVE/HA/Usage.pm b/src/PVE/HA/Usage.pm > index 5f1ac226..822b884c 100644 > --- a/src/PVE/HA/Usage.pm > +++ b/src/PVE/HA/Usage.pm > @@ -33,9 +33,8 @@ sub contains_node { > die "implement in subclass"; > } > =20 > -# Logs a warning to $haenv upon failure, but does not die. > -sub add_service_usage_to_node { > - my ($self, $nodename, $sid) =3D @_; > +sub add_service { > + my ($self, $sid, $current_node, $target_node, $running) =3D @_; > =20 > die "implement in subclass"; > } > @@ -47,8 +46,11 @@ sub add_service_usage { > my $online_nodes =3D { map { $_ =3D> 1 } $self->list_nodes() }; > my ($current_node, $target_node) =3D get_used_service_nodes($online_= nodes, $sd); > =20 > - $self->add_service_usage_to_node($current_node, $sid) if $current_no= de; > - $self->add_service_usage_to_node($target_node, $sid) if $target_node= ; > + # some usage implementations need to discern whether a service is tr= uly running > + # a service does only have the 'running' flag in 'started' state > + my $running =3D ($sd->{state} eq 'started' && $sd->{running}) || def= ined($current_node); This is incorrect, since it would be true whenever state is 'started'. This is due to how defined($current_node) comes about. As discussed off-list, the condition should rather be: my $running =3D ($sd->{state} eq 'started' && $sd->{running}) || ($s= d->{state} ne 'started' && defined($current_node)); A harder to comprehend but more concise option could be: my $running =3D ($sd->{state} ne 'started' && defined($current_node)= ) || $sd->{running}; Had the tests not revealed this yet, it is probably due to the fact that the sim patches are agnostic to the running flag, which apparently is not ok. Will fix this. > + > + $self->add_service($sid, $current_node, $target_node, $running); > } > =20 [snip] Reviewed-by: Dominik Rusovac