Re: [PATCH ha-manager v2 03/12] rules: node affinity: implement negative node affinity rules

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

Thanks for the patch series.
comments inline.

On 6/2/26 12:04 PM, Daniel Kral wrote:
> Extend the existing node affinity rules plugin to allow users to specify
> negative node affinity constraints, which specify the nodes where HA
> resources SHOULD NOT/MUST NOT be placed.
>
> Negative node affinity rules are internally represented as positive node
> affinity rules, where the positive node affinity rules' nodes set is the
> set complement of the negative node affinity rules' node set. As this is
> semantically equivalent, this allows making no change in the apply logic.
>
> As node priority groups do only hold semantic value for positive node
> affinity rules, reject negative node affinity rules with non-zero
> priorities and add all resulting nodes to the default priority group.
>
> Furthermore, negative node affinity rules, which specify all cluster
> nodes, are dropped as well as their strict variant is unsatisfiable,
> because they effectively restrict HA resource to an empty node set, and
> the non-strict variant is equivalent to no rule at all.
>
> Signed-off-by: Daniel Kral<d.kral@proxmox.com>
> ---
> changes since v1:
> - add rule checker to disallow specifying node priorities for negative
>    node affinity rules (thanks @Fiona!)
> - add rule checker to disallow specifying all cluster nodes
> - rename s/nodes/cluster-nodes/ arguments (thanks @Fiona!)
> - rename wrongly named 'positive_nodes' to 'negative_nodes'
> - add more test cases
>
>   src/PVE/HA/HashTools.pm                       |  20 +++
>   src/PVE/HA/Rules.pm                           |   2 +
>   src/PVE/HA/Rules/NodeAffinity.pm              | 162 +++++++++++++++++-
>   .../defaults-for-node-affinity-rules.cfg      |  15 ++
>   ...efaults-for-node-affinity-rules.cfg.expect |  58 ++++++-
>   ...effective-negative-node-affinity-rules.cfg |  12 ++
>   ...ve-negative-node-affinity-rules.cfg.expect |  17 ++
>   ...iority-in-negative-node-affinity-rules.cfg |  11 ++
>   ...in-negative-node-affinity-rules.cfg.expect |  45 +++++
>   9 files changed, 339 insertions(+), 3 deletions(-)
>   create mode 100644 src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg
>   create mode 100644 src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg.expect
>   create mode 100644 src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg
>   create mode 100644 src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg.expect
>
> diff --git a/src/PVE/HA/HashTools.pm b/src/PVE/HA/HashTools.pm
> index ebe47e38..b6e2136b 100644
> --- a/src/PVE/HA/HashTools.pm
> +++ b/src/PVE/HA/HashTools.pm
> @@ -6,6 +6,7 @@ use warnings;
>   use base qw(Exporter);
>   
>   our @EXPORT_OK = qw(
> +    set_difference
>       set_intersect
>       set_union
>       sets_are_disjoint
> @@ -29,6 +30,25 @@ more verbose implementation.
>   
>   =cut
>   
> +=head3 set_difference($hash1, $hash2)
> +
> +Returns a hash set of the set difference between the hash sets C<$hash1> and
> +C<$hash2>, i.e. the elements that are in C<$hash1> without the elements that
> +are in C<$hash2>.
> +
> +The hashes C<$hash1> and C<$hash2> are expected to be hash sets, i.e.
> +key-value pairs are always set to C<1> or another truthy value.
> +
> +=cut
> +
> +sub set_difference : prototype($$) {


Why did you opt for prototype($$) here?
`set_intersect` and `set_union` don't seem to make use of this.

> +    my ($hash1, $hash2) = @_;
> +
> +    my $result = { map { $hash2->{$_} ? () : ($_ => 1) } keys %$hash1 };
> +
> +    return $result;
> +}
> +
>   =head3 set_intersect($hash1, $hash2)
>   
>   Returns a hash set of the intersection of the hash sets C<$hash1> and
> diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm
> index 2b48cb60..bc67de20 100644
> --- a/src/PVE/HA/Rules.pm
> +++ b/src/PVE/HA/Rules.pm
> @@ -461,6 +461,8 @@ sub transform {
>           for my $transform ($transformdef->{$type}->@*) {
>               my $global_args = $class->get_check_arguments($rules);
>   
> +            $global_args->{'cluster-nodes'} = $cluster_nodes;
> +
>               $transform->($rules, $global_args);
>           }
>       }
> diff --git a/src/PVE/HA/Rules/NodeAffinity.pm b/src/PVE/HA/Rules/NodeAffinity.pm
> index 5d4401f6..bbe76720 100644
> --- a/src/PVE/HA/Rules/NodeAffinity.pm
> +++ b/src/PVE/HA/Rules/NodeAffinity.pm
> @@ -9,6 +9,7 @@ use PVE::Cluster;
>   use PVE::JSONSchema qw(get_standard_option);
>   use PVE::Tools;
>   
> +use PVE::HA::HashTools qw(set_difference);
>   use PVE::HA::Rules;
>   use PVE::HA::Tools;
>   
> @@ -28,6 +29,22 @@ PVE::HA::Rules::NodeAffinity
>   This package provides the capability to specify and apply rules, which put
>   affinity constraints between a set of HA resources and a set of nodes.
>   
> +HA Node Affinity rules can be one of two types:
> +
> +=over
> +
> +=item C<'positive'>
> +
> +Positive node affinity rules specify the nodes, which SHOULD/MUST be preferred
> +by the given HA resources.
> +
> +=item C<'negative'>
> +
> +Negative node affinity rules specify the nodes, which SHOULD/MUST be avoided by
> +the given HA resources.
> +
> +=back
> +
>   HA Node Affinity rules can be either C<'non-strict'> or C<'strict'>:
>   
>   =over
> @@ -66,9 +83,10 @@ sub properties {
>           ),
>           affinity => {
>               description => "Describes whether the HA resources are supposed to"
> -                . " be placed on the given nodes ('positive').",
> +                . " be placed on the given nodes ('positive'), or are supposed"
> +                . " to be placed on any but the given nodes ('negative').",
>               type => 'string',
> -            enum => ['positive'],
> +            enum => ['positive', 'negative'],
>               default => 'positive',
>               optional => 1,
>           },
> @@ -256,6 +274,146 @@ __PACKAGE__->register_check(
>       },
>   );
>   
> +=head3 check_nonempty_negative_nodes_complement($node_affinity_rules, $cluster_nodes)
> +
> +Returns a list of negative node affinity rule ids in C<$node_affinity_rules>,
> +where the complement of the negative node set is an empty node set according to
> +the currently configured cluster node list C<$cluster_nodes>, i.e., the
> +negative node set specifies all cluster nodes.
> +
> +Even though this is only relevant for strict negative node affinity rules, this
> +check also includes non-strict negative node affinity rules as their effective
> +node set would be equivalent to setting no rule at all.
> +
> +If there are none, the returned list is empty.
> +
> +=cut
> +
> +sub check_nonempty_negative_nodes_complement {
> +    my ($node_affinity_rules, $cluster_nodes) = @_;
> +
> +    my @conflicts = ();
> +
> +    my $total_node_count = @$cluster_nodes;
> +
> +    while (my ($ruleid, $rule) = each %$node_affinity_rules) {


I'm not a perl expert by any means, but couldn't this be considered problematic.

Consider this scenario:
You have some kind of function that runs before this is being called which,
iterates over the elements exactly like you do:

while (my ($ruleid, $rule) = each %$node_affinity_rules) {
     if ($rule->{affinity} eq 'negative') {
         return $ruleid;
     }
}

but it has this early return statement. So if you have for example rule_a, rule_b
and rule_c and it returns early on rule_a here, the internal iterator could
still point at rule_a and never actually reset.

It might not be the case at the moment, but further down the line this could
be a wild bug to hunt down or am I misinterpreting the perldocs [0].

Might be safer to just use a regular for loop here instead, but after looking
at other parts of the code this seems to be used a lot in this file.
And I also get why it was used here, as it makes it easier to grab the key-value
pairs directly instead of looking them up manually.
Nothing to worry about right away just something to look out for.

[0] https://perldoc.perl.org/functions/each

> +        next if $rule->{affinity} ne 'negative';
> +
> +        push @conflicts, $ruleid if keys $rule->{nodes}->%* >= $total_node_count;
> +    }
> +
> +    @conflicts = sort @conflicts;
> +    return \@conflicts;
> +}
> +
> +__PACKAGE__->register_check(
> +    sub {
> +        my ($args) = @_;
> +
> +        return check_nonempty_negative_nodes_complement(
> +            $args->{node_affinity_rules},
> +            $args->{'cluster-nodes'},
> +        );
> +    },
> +    sub {
> +        my ($ruleids, $errors) = @_;
> +
> +        for my $ruleid (@$ruleids) {
> +            push $errors->{$ruleid}->{nodes}->@*,
> +                "negative node affinity rule must not specify all cluster nodes";
> +        }
> +    },
> +);
> +
> +=head3 check_unprioritized_negative_nodes($node_affinity_rules)
> +
> +Returns a list of negative node affinity rule ids in C<$node_affinity_rules>,
> +where at least one node has a priority set. A node priority does not have any
> +meaningful semantic value for negative node affinity rules.
> +
> +If there are none, the returned list is empty.
> +
> +=cut
> +
> +sub check_unprioritized_negative_nodes {
> +    my ($node_affinity_rules) = @_;
> +
> +    my @conflicts = ();
> +
> +    while (my ($ruleid, $rule) = each %$node_affinity_rules) {
> +        next if $rule->{affinity} ne 'negative';
> +
> +        for my $node (keys $rule->{nodes}->%*) {
> +            if ($rule->{nodes}->{$node}->{priority}) {
> +                push @conflicts, $ruleid;
> +                last; # one non-zero priority is enough to invalidate rule
> +            }
> +        }
> +    }
> +
> +    @conflicts = sort @conflicts;
> +    return \@conflicts;
> +}
> +
> +__PACKAGE__->register_check(
> +    sub {
> +        my ($args) = @_;
> +
> +        return check_unprioritized_negative_nodes($args->{node_affinity_rules});
> +    },
> +    sub {
> +        my ($ruleids, $errors) = @_;
> +
> +        for my $ruleid (@$ruleids) {
> +            push $errors->{$ruleid}->{nodes}->@*,
> +                "negative node affinity rule must not specify node priorities";
> +        }
> +    },
> +);
> +
> +=head1 NODE AFFINITY RULE TRANSFORMATION HELPERS
> +
> +=cut
> +
> +=head3 invert_negative_node_affinity_rules($rules, $node_affinity_rules, $cluster_nodes)
> +
> +Modifies C<$rules> such that all negative node affinity rules, defined in
> +C<$node_affinity_rules>, are transformed to positive node affinity rules, where
> +the nodes set is the complement of the negative node affinity rules' nodes set.
> +
> +C<$cluster_nodes> is a list of the configured cluster nodes, which is used as
> +the universal set to build the complement node set.
> +
> +=cut
> +
> +sub invert_negative_node_affinity_rules {
> +    my ($rules, $node_affinity_rules, $cluster_nodes) = @_;
> +
> +    # set_difference(...) requires a hash set instead of a list
> +    my $cluster_nodes_hash = { map { $_ => 1 } @$cluster_nodes };
> +
> +    while (my ($node_affinity_id, $node_affinity_rule) = each %$node_affinity_rules) {
> +        next if $node_affinity_rule->{affinity} ne 'negative';
> +
> +        my $negative_nodes = { map { $_ => 1 } keys $node_affinity_rule->{nodes}->%* };
> +        my $positive_nodes = set_difference($cluster_nodes_hash, $negative_nodes);
> +        $positive_nodes->{$_} = { priority => 0 } for keys %$positive_nodes;
> +
> +        $rules->{ids}->{$node_affinity_id}->{affinity} = 'positive';
> +        $rules->{ids}->{$node_affinity_id}->{nodes} = $positive_nodes;
> +    }
> +}
> +
> +__PACKAGE__->register_transform(sub {
> +    my ($rules, $args) = @_;
> +
> +    invert_negative_node_affinity_rules(
> +        $rules,
> +        $args->{node_affinity_rules},
> +        $args->{'cluster-nodes'},
> +    );
> +});
> +
>   =head1 NODE AFFINITY RULE HELPERS
>   
>   =cut


nit: might be nice to split the tests into a separate commit but no hard feelings.

> diff --git a/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg b/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg
> index 27658562..b89d725d 100644
> --- a/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg
> +++ b/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg
> @@ -20,3 +20,18 @@ node-affinity: node-affinity-strict
>   	resources vm:104
>   	nodes node3
>   	strict 1
> +
> +# Case 5: Non-strict negative node affinity rule is transformed to positive node affinity rule with complement node set
> +#         and specified nodes as fallback internally.
> +node-affinity: negative-node-affinity-defaults
> +	resources vm:105
> +	nodes node2
> +	affinity negative
> +
> +# Case 6: Strict negative node affinity rule is transformed to positive node affinity rule with complement node set
> +#         internally.
> +node-affinity: negative-node-affinity-strict
> +	resources vm:106
> +	nodes node3
> +	affinity negative
> +	strict 1
> diff --git a/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg.expect b/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg.expect
> index 0d6e5605..877f1967 100644
> --- a/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg.expect
> +++ b/src/test/rules_cfgs/defaults-for-node-affinity-rules.cfg.expect
> @@ -1,8 +1,39 @@
>   --- Log ---
>   --- Config ---
>   {
> -   "digest" : "fcad82def12abc4422061b79cfd0399967053d93",
> +   "digest" : "308a8b704d8b4dc3fb83d8c97c86133caf735755",
>      "ids" : {
> +      "negative-node-affinity-defaults" : {
> +         "affinity" : "positive",
> +         "nodes" : {
> +            "node1" : {
> +               "priority" : 0
> +            },
> +            "node3" : {
> +               "priority" : 0
> +            }
> +         },
> +         "resources" : {
> +            "vm:105" : 1
> +         },
> +         "type" : "node-affinity"
> +      },
> +      "negative-node-affinity-strict" : {
> +         "affinity" : "positive",
> +         "nodes" : {
> +            "node1" : {
> +               "priority" : 0
> +            },
> +            "node2" : {
> +               "priority" : 0
> +            }
> +         },
> +         "resources" : {
> +            "vm:106" : 1
> +         },
> +         "strict" : 1,
> +         "type" : "node-affinity"
> +      },
>         "node-affinity-defaults" : {
>            "affinity" : "positive",
>            "nodes" : {
> @@ -56,6 +87,8 @@
>         }
>      },
>      "order" : {
> +      "negative-node-affinity-defaults" : 5,
> +      "negative-node-affinity-strict" : 6,
>         "node-affinity-defaults" : 1,
>         "node-affinity-disabled" : 2,
>         "node-affinity-disabled-explicit" : 3,
> @@ -84,6 +117,29 @@
>                  "priority" : 0
>               }
>            }
> +      },
> +      "vm:105" : {
> +         "nodes" : {
> +            "node1" : {
> +               "priority" : 0
> +            },
> +            "node2" : {
> +               "priority" : -1
> +            },
> +            "node3" : {
> +               "priority" : 0
> +            }
> +         }
> +      },
> +      "vm:106" : {
> +         "nodes" : {
> +            "node1" : {
> +               "priority" : 0
> +            },
> +            "node2" : {
> +               "priority" : 0
> +            }
> +         }
>         }
>      },
>      "resource-affinity" : {
> diff --git a/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg b/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg
> new file mode 100644
> index 00000000..a238799a
> --- /dev/null
> +++ b/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg
> @@ -0,0 +1,12 @@
> +# Case 1: Remove non-strict negative node affinity rule, which specifies all cluster nodes.
> +node-affinity: nonstrict-negative-node-affinity-with-all-cluster-nodes
> +    resources vm:101
> +    nodes node1,node2,node3
> +    affinity negative
> +
> +# Case 2: Remove strict negative node affinity rule, which specifies all cluster nodes.
> +node-affinity: strict-negative-node-affinity-with-all-cluster-nodes
> +    resources vm:102
> +    nodes node1,node2,node3
> +    affinity negative
> +    strict
> diff --git a/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg.expect b/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg.expect
> new file mode 100644
> index 00000000..dec03663
> --- /dev/null
> +++ b/src/test/rules_cfgs/ineffective-negative-node-affinity-rules.cfg.expect
> @@ -0,0 +1,17 @@
> +--- Log ---
> +Drop rule 'nonstrict-negative-node-affinity-with-all-cluster-nodes', because negative node affinity rule must not specify all cluster nodes.
> +Drop rule 'strict-negative-node-affinity-with-all-cluster-nodes', because negative node affinity rule must not specify all cluster nodes.
> +--- Config ---
> +{
> +   "digest" : "7a68821f23f0b5b110bfac64a66fd7a01564dd74",
> +   "ids" : {},
> +   "order" : {}
> +}
> +--- Compiled Config ---
> +{
> +   "node-affinity" : {},
> +   "resource-affinity" : {
> +      "negative" : {},
> +      "positive" : {}
> +   }
> +}
> diff --git a/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg b/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg
> new file mode 100644
> index 00000000..64a2fd63
> --- /dev/null
> +++ b/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg
> @@ -0,0 +1,11 @@
> +# Case 1: Do not remove negative node affinity rules with node priorities set to zero as it is equivalent to no node priorities set at all.
> +node-affinity: negative-node-affinity-with-default-node-priorities
> +    resources vm:101
> +    nodes node1:0,node2:0
> +    affinity negative
> +
> +# Case 2: Remove negative node affinity rules with non-zero node priorities.
> +node-affinity: negative-node-affinity-with-non-zero-node-priorities
> +    resources vm:102
> +    nodes node1:2,node2:1
> +    affinity negative
> diff --git a/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg.expect b/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg.expect
> new file mode 100644
> index 00000000..ef50b25e
> --- /dev/null
> +++ b/src/test/rules_cfgs/node-priority-in-negative-node-affinity-rules.cfg.expect
> @@ -0,0 +1,45 @@
> +--- Log ---
> +Drop rule 'negative-node-affinity-with-non-zero-node-priorities', because negative node affinity rule must not specify node priorities.
> +--- Config ---
> +{
> +   "digest" : "d95cf03b7f6c8078854506cc79127fd942b868bd",
> +   "ids" : {
> +      "negative-node-affinity-with-default-node-priorities" : {
> +         "affinity" : "positive",
> +         "nodes" : {
> +            "node3" : {
> +               "priority" : 0
> +            }
> +         },
> +         "resources" : {
> +            "vm:101" : 1
> +         },
> +         "type" : "node-affinity"
> +      }
> +   },
> +   "order" : {
> +      "negative-node-affinity-with-default-node-priorities" : 1
> +   }
> +}
> +--- Compiled Config ---
> +{
> +   "node-affinity" : {
> +      "vm:101" : {
> +         "nodes" : {
> +            "node1" : {
> +               "priority" : -1
> +            },
> +            "node2" : {
> +               "priority" : -1
> +            },
> +            "node3" : {
> +               "priority" : 0
> +            }
> +         }
> +      }
> +   },
> +   "resource-affinity" : {
> +      "negative" : {},
> +      "positive" : {}
> +   }
> +}