Re: [pve-devel] [PATCH ha-manager 7/9] usage: allow granular changes to Usage implementations

[Fiona Ebner <f.ebner@proxmox.com>]

Am 30.09.25 um 4:20 PM schrieb Daniel Kral:
> This makes use of the new signature for add_service_usage_to_node(...)
> from the Proxmox::RS::ResourceScheduling::Static package, which allows
> tracking of which HA resources have been assigned to which nodes.
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

with some style nits below:

(also good to have a note: dependency bump for pve-rs needed)

> @@ -51,10 +51,18 @@ sub add_service_usage_to_node {
>      }
>  }
>  
> +sub remove_service_usage {
> +    my ($self, $sid) = @_;
> +
> +    for my $node ($self->list_nodes()) {
> +        delete $self->{nodes}->{$node}->{$sid};
> +    }
> +}
> +
>  sub score_nodes_to_start_service {
>      my ($self, $sid, $service_node) = @_;
>  
> -    return $self->{nodes};
> +    return { map { $_ => scalar keys $self->{nodes}->{$_}->%* } keys $self->{nodes}->%* };

Style nit: I feel like 'scalar' is more readable when surrounding its
argument with parentheses.

Style nit: Maybe add
my $nodes = $self->{nodes};
up front to defuse the lengthy expression a bit?

> @@ -89,16 +89,27 @@ my sub get_service_usage {
>  sub add_service_usage_to_node {
>      my ($self, $nodename, $sid, $service_node, $migration_target) = @_;
>  
> -    $self->{'service-counts'}->{$nodename}++;
> +    $self->{'node-services'}->{$nodename}->{$sid} = 1;
>  
>      eval {
>          my $service_usage = get_service_usage($self, $sid, $service_node, $migration_target);
> -        $self->{scheduler}->add_service_usage_to_node($nodename, $service_usage);
> +        $self->{scheduler}->add_service_usage_to_node($nodename, $sid, $service_usage);
>      };
>      $self->{haenv}->log('warning', "unable to add service '$sid' usage to node '$nodename' - $@")
>          if $@;
>  }
>  
> +sub remove_service_usage {
> +    my ($self, $sid) = @_;
> +
> +    delete $self->{'node-services'}->{$_}->{$sid} for $self->list_nodes();

Style nit: add parentheses for the delete(), since other code follows it
(and the argument is quite lengthy too):
https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

> +
> +    eval { $self->{scheduler}->remove_service_usage($sid) };
> +    $self->{haenv}->log('warning', "unable to remove service '$sid' usage - $@") if $@;
> +
> +    delete $self->{'service-stats'}->{$sid}; # Invalidate old service stats
> +}
> +
>  sub score_nodes_to_start_service {
>      my ($self, $sid, $service_node) = @_;
>  
> @@ -111,7 +122,10 @@ sub score_nodes_to_start_service {
>              'err',
>              "unable to score nodes according to static usage for service '$sid' - $err",
>          );
> -        return $self->{'service-counts'};
> +        return {
> +            map { $_ => scalar keys $self->{'node-services'}->{$_}->%* }

Style nit: same as above regarding parentheses and using a variable to
improve the readability

> +                keys $self->{'node-services'}->%*
> +        };
>      }
>  
>      # Take minus the value, so that a lower score is better, which our caller(s) expect(s).



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