Re: [pve-devel] [RFC PATCH ha-manager 2/2] rules: node affinity: implement negative node affinity rules

[Fiona Ebner <f.ebner@proxmox.com>]

Am 19.12.25 um 2:36 PM schrieb Daniel Kral:
> 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 no change in the apply logic.

Nit: 'allows making no change'

> As node priority groups do only hold semantic value for positive node
> affinity rules, add all resulting nodes to the default priority group.
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>

[I] root@pve9a1 ~# pvesh create /cluster/ha/rules/ --rule ha-rule-new
--resources ct:105 --type node-affinity --affinity negative --nodes
pve9a1:1,pve9a2:2

This currently goes through, but should be rejected, indicating that
node priorities may not be specified with negative affinity.

Two other smaller comments below, otherwise it looks good to me.

> ---
>  src/PVE/HA/HashTools.pm                       | 20 +++++++
>  src/PVE/HA/Rules.pm                           |  2 +
>  src/PVE/HA/Rules/NodeAffinity.pm              | 59 ++++++++++++++++++-
>  .../defaults-for-node-affinity-rules.cfg      | 15 +++++
>  ...efaults-for-node-affinity-rules.cfg.expect | 58 +++++++++++++++++-
>  5 files changed, 151 insertions(+), 3 deletions(-)
> 
> 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($$) {
> +    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 c4a2ccea..7f9f428d 100644
> --- a/src/PVE/HA/Rules.pm
> +++ b/src/PVE/HA/Rules.pm
> @@ -459,6 +459,8 @@ sub transform {
>          for my $transform ($transformdef->{$type}->@*) {
>              my $global_args = $class->get_check_arguments($rules);
>  
> +            $global_args->{nodes} = $nodes;

Maybe 'cluster-nodes' as the key to be more explicit?

> +
>              $transform->($rules, $global_args);
>          }
>      }
> diff --git a/src/PVE/HA/Rules/NodeAffinity.pm b/src/PVE/HA/Rules/NodeAffinity.pm
> index 1f15ae2d..cdf67a55 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'>
> +
> +Positive node affinity rules specify the nodes, which SHOULD NOT/MUST NOT be
> +preferred 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,43 @@ __PACKAGE__->register_check(
>      },
>  );
>  
> +=head1 NODE AFFINITY RULE TRANSFORMATION HELPERS
> +
> +=cut
> +
> +=head3 invert_negative_node_affinity_rules($rules, $node_affinity_rules, $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.
> +
> +=cut
> +
> +sub invert_negative_node_affinity_rules {
> +    my ($rules, $node_affinity_rules, $nodes) = @_;
> +
> +    my $cluster_nodes = { map { $_ => 1 } @$nodes };
> +
> +    while (my ($node_affinity_id, $node_affinity_rule) = each %$node_affinity_rules) {
> +        next if $node_affinity_rule->{affinity} ne 'negative';
> +
> +        my $positive_nodes = { map { $_ => 1 } keys $node_affinity_rule->{nodes}->%* };

I'm confused by the variable name. There is negative affinity towards
these nodes, so why $positive_nodes?

> +        my $new_nodes = set_difference($cluster_nodes, $positive_nodes);
> +        $new_nodes->{$_} = { priority => 0 } for keys %$new_nodes;
> +
> +        $rules->{ids}->{$node_affinity_id}->{affinity} = 'positive';
> +        $rules->{ids}->{$node_affinity_id}->{nodes} = $new_nodes;
> +    }
> +}
> +
> +__PACKAGE__->register_transform(sub {
> +    my ($rules, $args) = @_;
> +
> +    invert_negative_node_affinity_rules(
> +        $rules, $args->{node_affinity_rules}, $args->{nodes},
> +    );
> +});
> +
>  =head1 NODE AFFINITY RULE HELPERS
>  
>  =cut