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

Daniel Kral
2026-03-30T13:06:09Z

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>