Re: [pve-devel] [PATCH ha-manager v3 04/13] rules: add global checks between node and resource affinity rules

["Michael Köppl" <m.koeppl@proxmox.com>]

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