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

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

On Thu Mar 26, 2026 at 11:34 AM CET, Dominik Rusovac wrote:
> 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>,
>

Might make sense, but then we can't reuse the same structure in
`MigrationCandidate` anymore, so I would let ScoredMigration be it's own
type, what do you think?

>> +}
>
> [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 [...]

Thanks, will change that!

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

Thanks, good catch! I also weren't aware of them being non-deterministic
here, that's good to know.

We briefly also talked about this off-list, I'll adapt the test cases
below to not eq() the ScoredMigration directly as the truncation here
might be non-deterministic (and I assumed otherwise).

More-so, it's more important to verify the order in which the migrations
are scored than their exact imbalance score as you suggested.

>
> [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

Nice, will change it to that!

>
>> +    pub stats: ResourceStats,
>> +}
>
> [snip]