Re: [pve-devel] [PATCH ha-manager v5 11/23] manager: apply node affinity rules when selecting service nodes

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

On Thu Jul 31, 2025 at 7:26 AM CEST, Thomas Lamprecht wrote:
> Am 30.07.25 um 20:01 schrieb Daniel Kral:
>> @@ -210,4 +215,82 @@ __PACKAGE__->register_check(
>>      },
>>  );
>>  
>> +=head1 NODE AFFINITY RULE HELPERS
>> +
>> +=cut
>> +
>> +my $get_resource_node_affinity_rule = sub {
>> +    my ($rules, $sid) = @_;
>> +
>> +    # with the current restriction a resource can only be in one node affinity rule
>> +    my $node_affinity_rule;
>> +    PVE::HA::Rules::foreach_rule(
>> +        $rules,
>> +        sub {
>> +            my ($rule) = @_;
>> +
>> +            $node_affinity_rule = dclone($rule) if !$node_affinity_rule;
>> +        },
>> +        {
>> +            sid => $sid,
>> +            type => 'node-affinity',
>> +            exclude_disabled_rules => 1,
>> +        },
>
> meh. this a bit hard to read, passing the opts as hash value not hash
> ref could be nicer?
>
> foreach_rule : prototype($$;%) {
>     my ($rules, $func, %opts) = @_;
>
>     # here avoid the useless intermediate variables
>
>     ....-+
> }
>
>
> PVE::HA::Rules::foreach_rule(
>     $rules,
>     sub {
>         ...
>     },
>     sid => $sid,
>     type => 'node-affinity',
>     ....
> }

ACK, that look much cleaner, I'll send it as a follow-up patch (after
the more important follow-ups of course).

>
>> +    );
>> +
>> +    return $node_affinity_rule;
>> +};
>> +
>> +=head3 get_node_affinity($rules, $sid, $online_node_usage)
>> +
>> +Returns a list of two hashes representing the node affinity of C<$sid>
>> +according to the node affinity rules in C<$rules> and the available nodes in
>> +C<$online_node_usage>.
>> +
>> +The first hash is a hash set of available nodes, i.e. nodes where the
>> +resource C<$sid> is allowed to be assigned to, and the second hash is a hash set
>> +of preferred nodes, i.e. nodes where the resource C<$sid> should be assigned to.
>> +
>> +If there are no available nodes at all, returns C<undef>.
>> +
>> +=cut
>> +
>> +sub get_node_affinity : prototype($$$) {
>> +    my ($rules, $sid, $online_node_usage) = @_;
>> +
>> +    my $node_affinity_rule = $get_resource_node_affinity_rule->($rules, $sid);
>> +
>> +    # default to a node affinity rule with all available nodes
>> +    if (!$node_affinity_rule) {
>
> This seems not so nice, uses auto-vifivication to get the hash going again,
> that's almost always a code smell, let's rather explicitly assign and check,
> e.g. something like:
>
> if (!defined($node_affinity_rule) || !scalar($node_affinity_rule->%*)) {
>      $node_affinity_rule //= {};
>

ACK, will also send a follow-up patch cleaning that up!

>
>
>> +        for my $node ($online_node_usage->list_nodes()) {
>> +            $node_affinity_rule->{nodes}->{$node} = { priority => 0 };
>> +        }
>> +    }
>> +
>> +    # add remaining nodes with low priority for non-strict node affinity rules
>> +    if (!$node_affinity_rule->{strict}) {
>> +        for my $node ($online_node_usage->list_nodes()) {
>> +            next if defined($node_affinity_rule->{nodes}->{$node});
>> +
>> +            $node_affinity_rule->{nodes}->{$node} = { priority => -1 };
>> +        }
>> +    }
>> +
>> +    my $allowed_nodes = {};
>> +    my $prioritized_nodes = {};
>> +
>> +    while (my ($node, $props) = each %{ $node_affinity_rule->{nodes} }) {
>> +        next if !$online_node_usage->contains_node($node); # node is offline
>> +
>> +        $allowed_nodes->{$node} = 1;
>> +        $prioritized_nodes->{ $props->{priority} }->{$node} = 1;
>
> style nit: please avoid using hash for hash key access.

ACK, here too.

>
>> +    }
>> +
>> +    my $preferred_nodes = {};
>> +    my $highest_priority = (sort { $b <=> $a } keys %$prioritized_nodes)[0];
>> +    $preferred_nodes = $prioritized_nodes->{$highest_priority} if defined($highest_priority);
>> +
>> +    return ($allowed_nodes, $preferred_nodes);
>> +}
>> +
>>  1;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel