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 7DEAA1FF0E1 for ; Mon, 13 Jul 2026 14:43:26 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id F2ED021357; Mon, 13 Jul 2026 14:43:25 +0200 (CEST) Message-ID: <03abfed5-06ee-441d-be5b-ca3fefcf0056@proxmox.com> Date: Mon, 13 Jul 2026 14:43:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: David Riley Subject: Re: [PATCH ha-manager v2 03/12] rules: node affinity: implement negative node affinity rules To: pve-devel@lists.proxmox.com, Daniel Kral References: <20260602100226.180071-1-d.kral@proxmox.com> <20260602100226.180071-4-d.kral@proxmox.com> Content-Language: en-US In-Reply-To: <20260602100226.180071-4-d.kral@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783946584308 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.346 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: VUZIEHSKQKHASDGPKQRJUHB3M4H77EGC X-Message-ID-Hash: VUZIEHSKQKHASDGPKQRJUHB3M4H77EGC 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: 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 > --- > 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" : {} > + } > +}