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 066A81FF183 for ; Wed, 27 Aug 2025 15:42:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 105D21B508; Wed, 27 Aug 2025 15:42:02 +0200 (CEST) Mime-Version: 1.0 Date: Wed, 27 Aug 2025 15:41:28 +0200 Message-Id: From: =?utf-8?q?Michael_K=C3=B6ppl?= To: "Proxmox VE development discussion" Cc: "pve-devel" X-Mailer: aerc 0.20.1 References: <20250821143705.256562-1-d.kral@proxmox.com> <20250821143705.256562-3-d.kral@proxmox.com> In-Reply-To: <20250821143705.256562-3-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1756302083368 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.023 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 0.001 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 0.001 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 0.001 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [manager.pm, nodestatus.pm] Subject: Re: [pve-devel] [PATCH ha-manager 02/18] manager: retranslate rules if nodes are added or removed X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 2 minor suggestions inline On Thu Aug 21, 2025 at 4:35 PM CEST, Daniel Kral wrote: > Some rule checks depend on the list of cluster nodes, e.g., to check > whether a negative resource affinity rule doesn't specify more HA > resources than cluster nodes. > > The HA Manager retranslate rules only in certain conditions to reduce > unnecessary computations, but lacks a check whether cluster nodes have > been added or removed, which is different from what users are reported > through the rules API endpoints and web interface. > > Fixes: 6c4c0458 ("rules: add haenv node list to the rules' canonicalization stage") > Signed-off-by: Daniel Kral > --- > src/PVE/HA/Manager.pm | 2 ++ > src/PVE/HA/NodeStatus.pm | 14 ++++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm > index ba59f642..92a2c05e 100644 > --- a/src/PVE/HA/Manager.pm > +++ b/src/PVE/HA/Manager.pm > @@ -692,6 +692,7 @@ sub manage { > my ($haenv, $ms, $ns, $ss) = ($self->{haenv}, $self->{ms}, $self->{ns}, $self->{ss}); > > my ($node_info) = $haenv->get_node_info(); > + my $has_changed_nodelist = $ns->check_for_changed_nodelist($node_info); > my ($lrm_results, $lrm_modes) = $self->read_lrm_status(); > > $ns->update($node_info, $lrm_modes); > @@ -745,6 +746,7 @@ sub manage { > > if ( > !$self->{rules} > + || $has_changed_nodelist > || $new_rules->{digest} ne $self->{last_rules_digest} > || $self->{groups}->{digest} ne $self->{last_groups_digest} > || $services_digest && $services_digest ne $self->{last_services_digest} > diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm > index 0d04cd53..e2abfd6e 100644 > --- a/src/PVE/HA/NodeStatus.pm > +++ b/src/PVE/HA/NodeStatus.pm > @@ -92,6 +92,20 @@ sub list_online_nodes { > return $res; > } > > +sub check_for_changed_nodelist { > + my ($self, $node_info) = @_; > + > + for my $node (sort keys %$node_info) { > + return 1 if !$self->{status}->{$node}; > + } > + > + for my $node (sort keys $self->{status}->%*) { > + return 1 if !$node_info->{$node}; > + } > + > + return 0; > +} Is the sort necessary for the loops? It seems to me that the order of keys doesn't really matter for this. Also, could it make sense (not necessarily in this series) to move this to HashTools? It's not used anywhere else now, but I might come in handy for further extensions of the HA rules implementation, whenever you need to check if 2 sets match exactly. > + > my $delete_node = sub { > my ($self, $node) = @_; > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel