Re: [PATCH ha-manager v2 31/40] usage: add dynamic usage scheduler

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

On Mon Mar 30, 2026 at 1:17 PM CEST, Dominik Rusovac wrote:
> lgtm, consider modulo nits and check on falsy stats
>
> On Tue Mar 24, 2026 at 7:30 PM CET, Daniel Kral wrote:
>> The dynamic usage scheduler allows the HA Manager to make scheduling
>> decisions based on the current usage of the nodes and cluster resources
>> in addition to the maximum usage stats as reported by the PVE::HA::Env
>> implementation.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes v1 -> v2:
>> - guard PVE::HA::Usage::Dynamic with my $have_dynamic_scheduling as
>>   PVE::RS::ResourceScheduling::Dynamic might not be available (as
>>   suggested by @Thomas)
>> - add add_service() impl
>>
>
> [snip]
>
>> +sub new {
>> +    my ($class, $haenv, $service_stats) = @_;
>> +
>> +    my $node_stats = eval { $haenv->get_dynamic_node_stats() };
>> +    die "did not get dynamic node usage information - $@" if $@;
>
> nit: consider rewording "did not get" to "could not retrieve"
>

Thanks, but NACK this for now to have consistent error messages with
PVE::HA::Usage::Static. Can be done in a single patch in the future
to change both at the same time though!

>> +
>> +    my $scheduler = eval { PVE::RS::ResourceScheduling::Dynamic->new() };
>> +    die "unable to initialize dynamic scheduling - $@" if $@;
>> +
>> +    return bless {
>> +        'node-stats' => $node_stats,
>> +        'service-stats' => $service_stats,
>> +        haenv => $haenv,
>> +        scheduler => $scheduler,
>> +    }, $class;
>> +}
>> +
>> +sub add_node {
>> +    my ($self, $nodename) = @_;
>> +
>> +    my $stats = $self->{'node-stats'}->{$nodename}
>> +        or die "did not get dynamic node usage information for '$nodename'\n";
>
> nit: consider rewording "did not get" to "could not retrieve"
>

Same here

>> +    die "dynamic node usage information for '$nodename' missing cpu count\n" if !$stats->{maxcpu};
>> +    die "dynamic node usage information for '$nodename' missing memory\n" if !$stats->{maxmem};
>
> to allow for falsy 0-valued stats consider:
>
>        die "dynamic node usage information for '$nodename' missing cpu count\n" 
>            if !defined($stats->{maxcpu});
>        die "dynamic node usage information for '$nodename' missing memory\n" 
>            if !defined($stats->{maxmem});
>

Good catch, thanks!

>> +
>> +    eval { $self->{scheduler}->add_node($nodename, $stats); };
>> +    die "initializing dynamic node usage for '$nodename' failed - $@" if $@;
>> +}
>
> [snip]
>
>> +my sub get_service_usage {
>> +    my ($self, $sid) = @_;
>> +
>> +    my $service_stats = $self->{'service-stats'}->{$sid}->{usage}
>> +        or die "did not get dynamic service usage information for '$sid'\n";
>
> nit: consider rewording "did not get" to "could not retrieve"
>

Same here

>> +
>> +    return $service_stats;
>> +}
>> +
>> +sub add_service {
>> +    my ($self, $sid, $current_node, $target_node, $running) = @_;
>> +
>> +    # do not add service which do not put any usage on the nodes
>
> nit:
>
>        # do not add service[s] [...] 
>
> or
>
>        # do not add service, which doe[s] [...] 
>
> [snip]

Will do so!

>
> Reviewed-by: Dominik Rusovac <d.rusovac@proxmox.com>