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

["Daniel Kral" <d.kral@proxmox.com>]

Thanks for the review and testing! Will send a v2 with the changes and
the ui patches as well.

On Tue Feb 24, 2026 at 1:22 PM CET, Fiona Ebner wrote:
> 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'

Ack!

>
>> 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.

Right, those should be rejected entirely, ack!

>

[ snip ]

>> 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?

Good idea, I'll do that for the v2!

>
>> +
>>              $transform->($rules, $global_args);
>>          }
>>      }
>> diff --git a/src/PVE/HA/Rules/NodeAffinity.pm b/src/PVE/HA/Rules/NodeAffinity.pm
>> index 1f15ae2d..cdf67a55 100644

[ snip ]

>> @@ -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?

Right, I'm sorry, this should be $negative_nodes instead, this slipped
right past me. Will change the name in a v2!

If it makes things more clearer, $new_nodes could then take the name of
$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