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

Dominik Rusovac
2026-03-30T11:17:38Z

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"

> +
> + 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"

> + 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});

> +
> + 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"

> +
> + 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]

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