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 1F8A31FF0E9 for ; Tue, 14 Jul 2026 09:37:04 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DCD0E213A2; Tue, 14 Jul 2026 09:37:03 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 14 Jul 2026 09:36:57 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH ha-manager v2 03/12] rules: node affinity: implement negative node affinity rules X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260602100226.180071-1-d.kral@proxmox.com> <20260602100226.180071-4-d.kral@proxmox.com> <03abfed5-06ee-441d-be5b-ca3fefcf0056@proxmox.com> In-Reply-To: <03abfed5-06ee-441d-be5b-ca3fefcf0056@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784014601581 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.360 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: CAZ6LLUZQ3MJOPSE4HVKD3Z6AG6MLXNP X-Message-ID-Hash: CAZ6LLUZQ3MJOPSE4HVKD3Z6AG6MLXNP X-MailFrom: d.kral@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: 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. >> =20 >> =3Dcut >> =20 >> +=3Dhead3 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. >> + >> +=3Dcut >> + >> +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) =3D @_; >> + >> + my $result =3D { map { $hash2->{$_} ? () : ($_ =3D> 1) } keys %$has= h1 }; >> + >> + return $result; >> +} >> + >> =3Dhead3 set_intersect($hash1, $hash2) >> =20 >> Returns a hash set of the intersection of the hash sets C<$hash1> and [ snip ] >> @@ -256,6 +274,146 @@ __PACKAGE__->register_check( >> }, >> ); >> =20 >> +=3Dhead3 check_nonempty_negative_nodes_complement($node_affinity_rules,= $cluster_nodes) >> + >> +Returns a list of negative node affinity rule ids in C<$node_affinity_r= ules>, >> +where the complement of the negative node set is an empty node set acco= rding 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 rul= es, this >> +check also includes non-strict negative node affinity rules as their ef= fective >> +node set would be equivalent to setting no rule at all. >> + >> +If there are none, the returned list is empty. >> + >> +=3Dcut >> + >> +sub check_nonempty_negative_nodes_complement { >> + my ($node_affinity_rules, $cluster_nodes) =3D @_; >> + >> + my @conflicts =3D (); >> + >> + my $total_node_count =3D @$cluster_nodes; >> + >> + while (my ($ruleid, $rule) =3D each %$node_affinity_rules) { > > > I'm not a perl expert by any means, but couldn't this be considered probl= ematic. > > Consider this scenario: > You have some kind of function that runs before this is being called whic= h, > iterates over the elements exactly like you do: > > while (my ($ruleid, $rule) =3D each %$node_affinity_rules) { > =C2=A0 =C2=A0 if ($rule->{affinity} eq 'negative') { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return $ruleid; > =C2=A0 =C2=A0 } > } > > 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 cou= ld > still point at rule_a and never actually reset. > > It might not be the case at the moment, but further down the line this co= uld > 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 loo= king > 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 ke= y-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-iteratio= n-variables >> + next if $rule->{affinity} ne 'negative'; >> + >> + push @conflicts, $ruleid if keys $rule->{nodes}->%* >=3D $total= _node_count; >> + } >> + >> + @conflicts =3D sort @conflicts; >> + return \@conflicts; >> +} [ snip ]