From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id EC9161FF0E1 for ; Mon, 13 Jul 2026 14:41:33 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id BF527213EC; Mon, 13 Jul 2026 14:41:33 +0200 (CEST) Message-ID: <9b5d0cf3-18f5-4012-9922-5420409d2af2@proxmox.com> Date: Mon, 13 Jul 2026 14:41:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: David Riley Subject: Re: [PATCH ha-manager v2 02/12] rules: rename ambiguous argument nodes to cluster nodes To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260602100226.180071-1-d.kral@proxmox.com> <20260602100226.180071-3-d.kral@proxmox.com> Content-Language: en-US In-Reply-To: <20260602100226.180071-3-d.kral@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783946474734 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.371 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_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: S4O7UX4ZADHFIES4YNKBN7ASUQ5YR4LL X-Message-ID-Hash: S4O7UX4ZADHFIES4YNKBN7ASUQ5YR4LL X-MailFrom: d.riley@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. Consider this: Reviewed-by: David Riley On 6/2/26 12:03 PM, Daniel Kral wrote: > Rename any argument instances, which expects a list of currently > configured cluster nodes. This reduces the ambiguity which node list > should be supplied. > > Furthermore, rename the global rule checker property 'nodes' to > 'cluster-nodes' as well for the same reason. > > Signed-off-by: Daniel Kral > --- > changes since v1: > - new! (prep for using 'cluster-nodes' in transform as well) > > src/PVE/HA/Rules.pm | 37 ++++++++++++++-------------- > src/PVE/HA/Rules/NodeAffinity.pm | 4 +-- > src/PVE/HA/Rules/ResourceAffinity.pm | 17 +++++++------ > 3 files changed, 30 insertions(+), 28 deletions(-) > > diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm > index 3a14eeb3..2b48cb60 100644 > --- a/src/PVE/HA/Rules.pm > +++ b/src/PVE/HA/Rules.pm > @@ -385,13 +385,14 @@ sub get_check_arguments { > return $global_args; > } > > -=head3 $class->check_feasibility($rules, $nodes) > +=head3 $class->check_feasibility($rules, $cluster_nodes) > > Checks whether the given C<$rules> are feasible by running all checks, which > were registered with Cregister_check(...) >>>, > and returns a hash map of erroneous rules. > > -C<$nodes> is a list of the configured cluster nodes. > +C<$cluster_nodes> is a list of the configured cluster nodes, which is added as > +an argument for the registered rule feasibility checkers. > > The checks are run in the order in which the rule plugins were registered, > while global checks, i.e. checks between different rule types, are run at the > @@ -400,14 +401,14 @@ very last. > =cut > > sub check_feasibility { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > my $global_errors = {}; > my $removable_ruleids = []; > > my $global_args = $class->get_check_arguments($rules); > > - $global_args->{nodes} = $nodes; > + $global_args->{'cluster-nodes'} = $cluster_nodes; > > for my $type (@$types, 'global') { > for my $entry (@{ $checkdef->{$type} }) { > @@ -421,11 +422,11 @@ sub check_feasibility { > return $global_errors; > } > > -=head3 $class->transform($rules, $nodes) > +=head3 $class->transform($rules, $cluster_nodes) > > Modifies C<$rules> to contain only feasible rules. > > -C<$nodes> is a list of the configured cluster nodes. > +C<$cluster_nodes> is a list of the configured cluster nodes. > > This is done by running all checks, which were registered with > Cregister_check(...) >>> and removing any > @@ -438,10 +439,10 @@ Returns a list of messages with the reasons why rules were removed. > =cut > > sub transform { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > my $messages = []; > - my $global_errors = $class->check_feasibility($rules, $nodes); > + my $global_errors = $class->check_feasibility($rules, $cluster_nodes); > > for my $ruleid (keys %$global_errors) { > delete $rules->{ids}->{$ruleid}; > @@ -469,33 +470,33 @@ sub transform { > > =head3 $class->plugin_compile(...) > > -=head3 $class->plugin_compile($rules, $nodes) > +=head3 $class->plugin_compile($rules, $cluster_nodes) > > B Must be implemented in a I. > > -Called in C<$class->compile($rules, $nodes)> in order to get a more compact > -representation of the rule plugin's rules in C<$rules>, which holds only the > -relevant information for the scheduler and other users. > +Called in C<$class->compile($rules, $cluster_nodes)> in order to get a more > +compact representation of the rule plugin's rules in C<$rules>, which holds only > +the relevant information for the scheduler and other users. > > -C<$nodes> is a list of the configured cluster nodes. > +C<$cluster_nodes> is a list of the configured cluster nodes. > > =cut > > sub plugin_compile { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > die "implement in subclass"; > } > > =head3 $class->compile(...) > > -=head3 $class->compile($rules, $nodes) > +=head3 $class->compile($rules, $cluster_nodes) > > Compiles and returns a hash, where each key-value pair represents a rule > plugin's more compact representation compiled from the more verbose rules > defined in C<$rules>. > > -C<$nodes> is a list of the configured cluster nodes. > +C<$cluster_nodes> is a list of the configured cluster nodes. > > The transformation to the compact representation for each rule plugin is > implemented in Cplugin_compile(...) >>>. > @@ -503,13 +504,13 @@ implemented in Cplugin_compile(...) >>>. > =cut > > sub compile { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > my $compiled_rules = {}; > > for my $type (@$types) { > my $plugin = $class->lookup($type); > - my $compiled_plugin_rules = $plugin->plugin_compile($rules, $nodes); > + my $compiled_plugin_rules = $plugin->plugin_compile($rules, $cluster_nodes); > > die "plugin_compile(...) of type '$type' must return hash reference\n" > if ref($compiled_plugin_rules) ne 'HASH'; > diff --git a/src/PVE/HA/Rules/NodeAffinity.pm b/src/PVE/HA/Rules/NodeAffinity.pm > index 1f15ae2d..5d4401f6 100644 > --- a/src/PVE/HA/Rules/NodeAffinity.pm > +++ b/src/PVE/HA/Rules/NodeAffinity.pm > @@ -165,7 +165,7 @@ sub get_plugin_check_arguments { > } > > sub plugin_compile { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > my $node_affinity = {}; > > @@ -178,7 +178,7 @@ sub plugin_compile { > > # add remaining nodes with low priority for non-strict node affinity > if (!$rule->{strict}) { > - for my $node (@$nodes) { > + for my $node (@$cluster_nodes) { > next if defined($effective_nodes->{$node}); > > $effective_nodes->{$node} = { priority => -1 }; > diff --git a/src/PVE/HA/Rules/ResourceAffinity.pm b/src/PVE/HA/Rules/ResourceAffinity.pm > index 474d3000..6c43d5ff 100644 > --- a/src/PVE/HA/Rules/ResourceAffinity.pm > +++ b/src/PVE/HA/Rules/ResourceAffinity.pm > @@ -102,7 +102,7 @@ sub get_plugin_check_arguments { > } > > sub plugin_compile { > - my ($class, $rules, $nodes) = @_; > + my ($class, $rules, $cluster_nodes) = @_; > > my $positive = {}; > my $negative = {}; > @@ -173,23 +173,24 @@ __PACKAGE__->register_check( > }, > ); > > -=head3 check_negative_resource_affinity_resources_count($negative_rules, $nodes) > +=head3 check_negative_resource_affinity_resources_count($negative_rules, $cluster_nodes) > > Returns a list of negative resource affinity rule ids, defined in > -C<$negative_rules>, which do have more resources defined than available according > -to the node list C<$nodes>, i.e., there are not enough nodes to separate the > -resources on, even if all nodes are available. > +C<$negative_rules>, which do have more resources defined than available > +according to the currently configured cluster node list C<$cluster_nodes>, i.e., > +there are not enough nodes to separate the resources on, even if all nodes are > +available. > > If there are none, the returned list is empty. > > =cut > > sub check_negative_resource_affinity_resources_count { > - my ($negative_rules, $nodes) = @_; > + my ($negative_rules, $cluster_nodes) = @_; > > my @conflicts = (); > > - my $total_node_count = @$nodes; > + my $total_node_count = @$cluster_nodes; > > while (my ($negativeid, $negative_rule) = each %$negative_rules) { > push @conflicts, $negativeid if keys $negative_rule->{resources}->%* > $total_node_count; > @@ -205,7 +206,7 @@ __PACKAGE__->register_check( > > return check_negative_resource_affinity_resources_count( > $args->{negative_rules}, > - $args->{nodes}, > + $args->{'cluster-nodes'}, > ); > }, > sub {