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

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

On Mon Jul 13, 2026 at 2:43 PM CEST, David Riley wrote:
> Thanks for the patch series.
> comments inline.
>

Thanks for taking the time to review this series!

> On 6/2/26 12:04 PM, Daniel Kral wrote:

[ snip ]

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

Good catch, that seems like a remnant of when I introduced this helper
for the HA rules series back then, but the helper went unused in the
end. Will remove that prototype for a newer revision!

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

[ snip ]

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

Yeah, good catch! If I remember correctly I decided on this syntax usage
here for the HA rules in a code review.

As these checkers are meant to iterate through all rules as these should
check all rules. Otherwise, one should use the mentioned for loop which
dereferences the hash entries. So this becomes only troublesome if
someone doesn't know these each-subroutine caveats and the test cases
would quite likely immediately fail if not all items are evaluated.

FWIW as we EOL PVE 8, we might be able to make this use Perl v5.40,
where for loops with multiple iterations variables [0], which AFAIK do
not have these caveats with a hash-global iterator. Then we don't have
to deal with these kind of faults anymore.

[0] https://perldoc.perl.org/perlexperiment#for-loop-with-multiple-iteration-variables

>> +        next if $rule->{affinity} ne 'negative';
>> +
>> +        push @conflicts, $ruleid if keys $rule->{nodes}->%* >= $total_node_count;
>> +    }
>> +
>> +    @conflicts = sort @conflicts;
>> +    return \@conflicts;
>> +}

[ snip ]