Re: [PATCH ha-manager v2 02/12] rules: rename ambiguous argument nodes to cluster nodes

[David Riley <d.riley@proxmox.com>]

Looks good to me.
Consider this:

Reviewed-by: David Riley <d.riley@proxmox.com>

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<d.kral@proxmox.com>
> ---
> 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 C<L<< register_check()|/$class->register_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
>   C<L<< register_check()|/$class->register_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<MANDATORY:> Must be implemented in a I<rule plugin>.
>   
> -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 C<L<< plugin_compile()|/$class->plugin_compile(...) >>>.
> @@ -503,13 +504,13 @@ implemented in C<L<< plugin_compile()|/$class->plugin_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 {