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

["Dominik Rusovac" <d.rusovac@proxmox.com>]

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]