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

Daniel Kral
2026-03-26T14:11:14Z

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]