Re: [PATCH proxmox v2 09/40] resource-scheduling: implement rebalancing migration selection

Dominik Rusovac
2026-03-26T10:35:10Z

lgtm

pls find my comments inline, mostly relating to nits or tiny things

On Tue Mar 24, 2026 at 7:29 PM CET, Daniel Kral wrote:
> Assuming that a resource will hold the same dynamic resource usage on a
> new node as on the previous node, score possible migrations, where:
>
> - the cluster node imbalance is minimal (bruteforce), or
> - the shifted root mean square and maximum resource usages of the cpu
> and memory is minimal across the cluster nodes (TOPSIS).
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes v1 -> v2:
> - add saturating_sub() in remove_running_resource(...) (as suggested by
> @Thomas)
> - slightly move declarations and impls around so that reading from
> top-to-bottom is a little easier
> - pass NodeUsage vec instead of NodeStats vec to
> calculate_node_imbalance(...)
> - pass a closure to calculate_node_imbalance(...) (as suggested by
> @Dominik)
> - also use `migration` for `Ord` impl of `ScoredMigration`, s.t. the
> struct is now ordered first by the imbalance and then the strings in
> the `Migration` struct
> - fix floating-point issue for the imbalance ordering for
> ScoredMigration
> - correctly implement `Ord` (essentially removing the reverse() and
> moving these Reverse() wrappers to the usages for the BinaryHeap)
> - use the `Migration` struct in `MigrationCandidate` as well
> - drop Scheduler::node_stats() as it's unused now
> - use Vec::with_capacity(...) where possible
> - eagerly implement common traits (especially Clone and Debug)
> - add test cases for the ScoredMigration ordering, node imbalance
> calculation and the two rebalancing migration scoring methods
> - s/score_best_balancing_migrations
> /score_best_balancing_migration_candidates
> to possibly allow the Scheduler/Usage impls handling the migration
> candidate generation in the future instead of the callers

[snip]

> +/// Returns the load imbalance among the nodes.
> +///
> +/// The load balance is measured as the statistical dispersion of the individual node loads.
> +///
> +/// The current implementation uses the dimensionless coefficient of variation, which expresses the
> +/// standard deviation in relation to the average mean of the node loads.
> +///
> +/// The coefficient of variation is not robust, which is a desired property here, because outliers
> +/// should be detected as much as possible.
> +fn calculate_node_imbalance(nodes: &[NodeUsage], to_load: impl Fn(&NodeUsage) -> f64) -> f64 {

very nice docs!

[snip]

> +/// A possible migration.
> +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Serialize, Deserialize)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct Migration {
> + /// The identifier of a leading resource.
> + pub sid: String,
> + /// The current node of the leading resource.
> + pub source_node: String,
> + /// The possible migration target node for the resource.
> + pub target_node: String,

nit: on the long run, instead of having `ScoredMigration`,
it could be more convenient to have a field:

pub imbalance: Option<f64>,

> +}

[snip]

> +impl ScoredMigration {
> + pub fn new<T: Into<Migration>>(migration: T, imbalance: f64) -> Self {
> + // Depending how the imbalance is calculated, it can contain minor approximation errors. As

// Depending [on] how [...]

> + // this struct implements the Ord trait, users of the struct's cmp() can run into cases,
> + // where the imbalance is the same up to the significant digits in base 10, but treated as
> + // different values.
> + //
> + // Therefore, truncate any non-significant digits to prevent these cases.
> + let factor = 10_f64.powf(f64::DIGITS as f64);
> + let truncated_imbalance = f64::trunc(factor * imbalance) / factor;

Nice solution, this appears to be a clean approach to achieve deterministic `Ord`
for `f64`.

One small thing, tho: `f64::DIGITS` is technically not a floating number, but `15_u32`.

let factor = 10_f64.powi(f64::DIGITS as i32);

thus, seems to be the better choice here. `powi` is also generally faster than `powf` [0].

[0] https://doc.rust-lang.org/std/primitive.f64.html#method.powi:~:text=Using%20this%20function%20is%20generally%20faster%20than%20using%20powf

[snip]

> +/// A possible migration candidate with the migrated usage stats.
> +#[derive(Clone, Debug)]
> +pub struct MigrationCandidate {
> + /// The possible migration.
> + pub migration: Migration,
> + /// The to-be-migrated resource usage stats.

imo, easier to comprehend:

/// Usage stats of the resource to be migrated

> + pub stats: ResourceStats,
> +}

[snip]