From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 75F0C1FF16B for ; Tue, 29 Jul 2025 13:43:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 780B9100CF; Tue, 29 Jul 2025 13:44:52 +0200 (CEST) Message-ID: Date: Tue, 29 Jul 2025 13:44:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Daniel Kral References: <20250704182102.467624-1-d.kral@proxmox.com> <20250704182102.467624-5-d.kral@proxmox.com> From: =?UTF-8?Q?Michael_K=C3=B6ppl?= Content-Language: en-US In-Reply-To: <20250704182102.467624-5-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753789448513 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.010 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [resourceaffinity.pm, rules.pm, proxmox.com] Subject: Re: [pve-devel] [PATCH ha-manager v3 04/13] rules: add global checks between node and resource affinity rules X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 7/4/25 20:20, Daniel Kral wrote: > > diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm > index 3121424..892e7aa 100644 > --- a/src/PVE/HA/Rules.pm > +++ b/src/PVE/HA/Rules.pm > @@ -6,6 +6,7 @@ use warnings; > use PVE::JSONSchema qw(get_standard_option); > use PVE::Tools; > > +use PVE::HA::HashTools qw(set_intersect set_union sets_are_disjoint); > use PVE::HA::Tools; > > use base qw(PVE::SectionConfig); > @@ -476,4 +477,197 @@ sub get_next_ordinal : prototype($) { > return $current_order + 1; > } > > +=head1 INTER-PLUGIN RULE CHECKERS > + > +=cut > + > +=head3 check_single_priority_node_affinity_in_resource_affinity_rules(...) > + > +Returns a list of resource affinity rule ids, defined in > +C<$resource_affinity_rules>, where the resources in the resource affinity rule > +are in node affinity rules, defined in C<$node_affinity_rules>, which have > +multiple priority groups defined. > + > +That is, the resource affinity rule cannot be statically checked to be feasible > +as the selection of the priority group is dependent on the currently online > +nodes. > + > +If there are none, the returned list is empty. > + > +=cut > + > +sub check_single_priority_node_affinity_in_resource_affinity_rules { > + my ($resource_affinity_rules, $node_affinity_rules) = @_; > + > + my @errors = (); > + > + while (my ($resource_affinity_id, $resource_affinity_rule) = each %$resource_affinity_rules) { > + my $priority; > + my $resources = $resource_affinity_rule->{resources}; > + > + for my $node_affinity_id (keys %$node_affinity_rules) { > + my $node_affinity_rule = $node_affinity_rules->{$node_affinity_id}; > + > + next if sets_are_disjoint($resources, $node_affinity_rule->{resources}); Just to note this, since we had just discussed it off-list: I noted during my review of v2 that I would expect a resource affinity rule to only fail this rule check if its node affinity rule contained more than 1 node [0]. One option would be to add an explicit check for the length of the list of nodes in the node affinity rule of the given resource. If it is 1, move on to the next iteration. This results in a conflict for the resource affinity rule because of multiple priorities if there is either one rule with > 1 nodes with at least one priority assigned (case A) or if there are multiple node affinity rules for the same resource with a total of > 1 nodes and at least one priority assigned across the rules (case B). However, since case B also means that the node affinity rules are in conflict and therefore not applied, this should probably not result in a conflict for the resource affinity rule. The other option, as Dano suggested, would be to instead move the $priority variable to the inner loop: while (my ($resource_affinity_id, $resource_affinity_rule) = each %$resource_affinity_rules) { my $resources = $resource_affinity_rule->{resources}; for my $node_affinity_id (keys %$node_affinity_rules) { my $node_affinity_rule = $node_affinity_rules->{$node_affinity_id}; my $priority; next if sets_are_disjoint($resources, $node_affinity_rule->{resources}); for my $node (values %{ $node_affinity_rule->{nodes} }) { $priority = $node->{priority} if !defined($priority); if ($priority != $node->{priority}) { push @errors, $resource_affinity_id; last; # early return to check next resource affinity rule } } } } This would result in a conflict for case A, but not for case B, which seems more in line with the general handling of reconciling resource and node affinity rules. [0] https://lore.proxmox.com/pve-devel/f82ae0b8-0dbe-497e-8cbd-c7c2f6a7a9c6@proxmox.com/ > + > + for my $node (values %{ $node_affinity_rule->{nodes} }) { > + $priority = $node->{priority} if !defined($priority); > + > + if ($priority != $node->{priority}) { > + push @errors, $resource_affinity_id; > + last; # early return to check next resource affinity rule > + } > + } > + } > + } > + > + @errors = sort @errors; > + return \@errors; > +} > + > +__PACKAGE__->register_check( > + sub { > + my ($args) = @_; > + > + return check_single_priority_node_affinity_in_resource_affinity_rules( > + $args->{resource_affinity_rules}, > + $args->{node_affinity_rules}, > + ); > + }, > + sub { > + my ($ruleids, $errors) = @_; > + > + for my $ruleid (@$ruleids) { > + push @{ $errors->{$ruleid}->{resources} }, > + "resources are in node affinity rules with multiple priorities"; > + } > + }, > +); > + > +=head3 check_positive_resource_affinity_node_affinity_consistency(...) > + > +Returns a list of positive resource affinity rule ids, defined in > +C<$positive_rules>, where the resources in the positive resource affinity rule > +are restricted to a disjoint set of nodes by their node affinity rules, defined > +in C<$node_affinity_rules>. > + > +That is, the positive resource affinity rule cannot be fullfilled as the > +resources cannot be placed on the same node. > + > +If there are none, the returned list is empty. > + > +=cut > + > +sub check_positive_resource_affinity_node_affinity_consistency { > + my ($positive_rules, $node_affinity_rules) = @_; > + > + my @errors = (); > + > + while (my ($positiveid, $positive_rule) = each %$positive_rules) { > + my $allowed_nodes; > + my $resources = $positive_rule->{resources}; > + > + for my $node_affinity_id (keys %$node_affinity_rules) { > + my ($node_affinity_resources, $node_affinity_nodes) = > + $node_affinity_rules->{$node_affinity_id}->@{qw(resources nodes)}; > + > + next if sets_are_disjoint($resources, $node_affinity_resources); > + > + $allowed_nodes = { $node_affinity_nodes->%* } if !defined($allowed_nodes); > + $allowed_nodes = set_intersect($allowed_nodes, $node_affinity_nodes); > + > + if (keys %$allowed_nodes < 1) { > + push @errors, $positiveid; > + last; # early return to check next positive resource affinity rule > + } > + } > + } > + > + @errors = sort @errors; > + return \@errors; > +} > + > +__PACKAGE__->register_check( > + sub { > + my ($args) = @_; > + > + return check_positive_resource_affinity_node_affinity_consistency( > + $args->{positive_rules}, > + $args->{node_affinity_rules}, > + ); > + }, > + sub { > + my ($ruleids, $errors) = @_; > + > + for my $ruleid (@$ruleids) { > + push @{ $errors->{$ruleid}->{resources} }, > + "two or more resources are restricted to different nodes"; > + } > + }, > +); > + > +=head3 check_negative_resource_affinity_node_affinity_consistency(...) > + > +Returns a list of negative resource affinity rule ids, defined in > +C<$negative_rules>, where the resources in the negative resource affinity rule > +are restricted to less nodes than needed to keep them separate by their node > +affinity rules, defined in C<$node_affinity_rules>. > + > +That is, the negative resource affinity rule cannot be fullfilled as there are > +not enough nodes to spread the resources on. > + > +If there are none, the returned list is empty. > + > +=cut > + > +sub check_negative_resource_affinity_node_affinity_consistency { > + my ($negative_rules, $node_affinity_rules) = @_; > + > + my @errors = (); > + > + while (my ($negativeid, $negative_rule) = each %$negative_rules) { > + my $allowed_nodes = {}; > + my $located_resources; > + my $resources = $negative_rule->{resources}; > + > + for my $node_affinity_id (keys %$node_affinity_rules) { > + my ($node_affinity_resources, $node_affinity_nodes) = > + $node_affinity_rules->{$node_affinity_id}->@{qw(resources nodes)}; > + my $common_resources = set_intersect($resources, $node_affinity_resources); > + > + next if keys %$common_resources < 1; > + > + $located_resources = set_union($located_resources, $common_resources); > + $allowed_nodes = set_union($allowed_nodes, $node_affinity_nodes); > + > + if (keys %$allowed_nodes < keys %$located_resources) { > + push @errors, $negativeid; > + last; # early return to check next negative resource affinity rule > + } > + } > + } > + > + @errors = sort @errors; > + return \@errors; > +} > + > +__PACKAGE__->register_check( > + sub { > + my ($args) = @_; > + > + return check_negative_resource_affinity_node_affinity_consistency( > + $args->{negative_rules}, > + $args->{node_affinity_rules}, > + ); > + }, > + sub { > + my ($ruleids, $errors) = @_; > + > + for my $ruleid (@$ruleids) { > + push @{ $errors->{$ruleid}->{resources} }, > + "two or more resources are restricted to less nodes than available to the resources"; > + } > + }, > +); > + > 1; > diff --git a/src/PVE/HA/Rules/ResourceAffinity.pm b/src/PVE/HA/Rules/ResourceAffinity.pm > index 57ccc09..b024c93 100644 > --- a/src/PVE/HA/Rules/ResourceAffinity.pm > +++ b/src/PVE/HA/Rules/ResourceAffinity.pm > @@ -167,7 +167,8 @@ __PACKAGE__->register_check( > my ($args) = @_; > > return check_negative_resource_affinity_resources_count( > - $args->{negative_rules}, $args->{nodes}, > + $args->{negative_rules}, > + $args->{nodes}, > ); > }, > sub { _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel