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
