Re: [pve-devel] [PATCH ha-manager 10/18] compile ha rules to a more compact representation

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

2 comments inline

On Thu Aug 21, 2025 at 4:35 PM CEST, Daniel Kral wrote:
>  sub write_rules_config {
> @@ -391,8 +391,9 @@ sub get_resource_motion_info {
>          my $ss = $manager_status->{service_status};
>          my $ns = $manager_status->{node_status};
>  
> -        my $rules = read_and_check_effective_rules_config();
> -        my ($together, $separate) = get_affinitive_resources($rules, $sid);
> +        my $affinity = read_and_compile_rules_config();

nit: maybe something like $compiled_rules? Calling a variable for this
structure $affinity didn't make it quite clear to me what this is,
especially when later on it's used as $affinity->{'resource-affinity'}.
Maybe I'm just misinterpreting this, though.

Or could you maybe explain why this compiled rule structure is then
called affinity?

> +        my ($together, $separate) =
> +            get_affinitive_resources($affinity->{'resource-affinity'}, $sid);
>  
>          for my $csid (sort keys %$together) {
>              next if !defined($ss->{$csid});
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 3013d369..748667e8 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -121,11 +121,11 @@ sub flush_master_status {
>  
>  =head3 select_service_node(...)
>  
> -=head3 select_service_node($rules, $online_node_usage, $sid, $service_conf, $sd, $node_preference)
> +=head3 select_service_node($affinity, $online_node_usage, $sid, $service_conf, $sd, $node_preference)
>  
>  Used to select the best fitting node for the service C<$sid>, with the
> -configuration C<$service_conf> and state C<$sd>, according to the rules defined
> -in C<$rules>, available node utilization in C<$online_node_usage>, and the
> +configuration C<$service_conf> and state C<$sd>, according to the affinity
> +in C<$affinity>, available node utilization in C<$online_node_usage>, and the
>  given C<$node_preference>.
>  
>  The C<$node_preference> can be set to:
> @@ -143,20 +143,21 @@ The C<$node_preference> can be set to:
>  =cut
>  
>  sub select_service_node {
> -    my ($rules, $online_node_usage, $sid, $service_conf, $sd, $node_preference) = @_;
> +    my ($affinity, $online_node_usage, $sid, $service_conf, $sd, $node_preference) = @_;
>  
>      die "'$node_preference' is not a valid node_preference for select_service_node\n"
>          if $node_preference !~ m/(none|best-score|try-next)/;
>  
>      my ($current_node, $tried_nodes, $maintenance_fallback) =
>          $sd->@{qw(node failed_nodes maintenance_node)};
> +    my ($node_affinity, $resource_affinity) = $affinity->@{qw(node-affinity resource-affinity)};
>  
>      my $online_nodes = { map { $_ => 1 } $online_node_usage->list_nodes() };
> -    my ($allowed_nodes, $pri_nodes) = get_node_affinity($rules, $sid, $online_nodes);
> +    my ($allowed_nodes, $pri_nodes) = get_node_affinity($node_affinity, $sid, $online_nodes);
>  
>      return undef if !%$pri_nodes;
>  
> -    my ($together, $separate) = get_resource_affinity($rules, $sid, $online_node_usage);
> +    my ($together, $separate) = get_resource_affinity($resource_affinity, $sid, $online_node_usage);
>  
>      # stay on current node if possible (avoids random migrations)
>      if (
> @@ -418,7 +419,8 @@ sub execute_migration {
>  
>      my ($haenv, $ss) = $self->@{qw(haenv ss)};
>  
> -    my ($together, $separate) = get_affinitive_resources($self->{rules}, $sid);
> +    my $resource_affinity = $self->{affinity}->{'resource-affinity'};
> +    my ($together, $separate) = get_affinitive_resources($resource_affinity, $sid);
>  
>      for my $csid (sort keys %$separate) {
>          next if !defined($ss->{$csid});
> @@ -746,7 +748,7 @@ sub manage {
>      PVE::HA::Groups::migrate_groups_to_resources($self->{groups}, $sc);
>  
>      if (
> -        !$self->{rules}
> +        !$self->{affinity}
>          || $has_changed_nodelist
>          || $new_rules->{digest} ne $self->{last_rules_digest}
>          || $self->{groups}->{digest} ne $self->{last_groups_digest}
> @@ -756,11 +758,12 @@ sub manage {
>  
>          my $nodes = $self->{ns}->list_nodes();
>          my $messages = PVE::HA::Rules->transform($new_rules, $nodes);
> +        my $new_affinity = PVE::HA::Rules->compile($new_rules, $nodes);
>          $haenv->log('info', $_) for @$messages;
>  
> -        $self->{rules} = $new_rules;
> +        $self->{affinity} = $new_affinity;
>  
> -        $self->{last_rules_digest} = $self->{rules}->{digest};
> +        $self->{last_rules_digest} = $new_rules->{digest};
>          $self->{last_groups_digest} = $self->{groups}->{digest};
>          $self->{last_services_digest} = $services_digest;
>      }
> @@ -1020,7 +1023,7 @@ sub next_state_request_start {
>  
>      if ($self->{crs}->{rebalance_on_request_start}) {
>          my $selected_node = select_service_node(
> -            $self->{rules},
> +            $self->{affinity},
>              $self->{online_node_usage},
>              $sid,
>              $cd,
> @@ -1187,7 +1190,7 @@ sub next_state_started {
>              }
>  
>              my $node = select_service_node(
> -                $self->{rules},
> +                $self->{affinity},
>                  $self->{online_node_usage},
>                  $sid,
>                  $cd,
> @@ -1306,7 +1309,7 @@ sub next_state_recovery {
>      $self->recompute_online_node_usage(); # we want the most current node state
>  
>      my $recovery_node = select_service_node(
> -        $self->{rules},
> +        $self->{affinity},
>          $self->{online_node_usage},
>          $sid,
>          $cd,
> diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm
> index a075feac..d7593532 100644
> --- a/src/PVE/HA/Rules.pm
> +++ b/src/PVE/HA/Rules.pm
> @@ -47,6 +47,12 @@ Each I<rule plugin> is required to implement the methods C<L<type()>>,
>  C<L<properties()>>, and C<L<options>> from the C<L<PVE::SectionConfig>> to
>  extend the properties of this I<base plugin> with plugin-specific properties.
>  
> +Each I<rule plugin> is required to implement the method
> +C<L<< plugin_compile()|/$class->plugin_compile(...) >>> to distill a compiled
> +representation of the more verbose C<$rules> from the config file, which is
> +returned by C<L<< compile()|/$class->compile(...) >>> to be more appropriate
> +and efficient for the scheduler and other users.

Is this supposed to say "other uses" or do you actually mean "other
users"? Do you mean that this more efficient representation can (and
probably should) be used for other uses cases when e.g. iterating over
only resource affinity rules?



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