Re: [RFC PATCH-SERIES many 00/36] dynamic scheduler + load rebalancer

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

On Wed Mar 18, 2026 at 5:54 PM CET, Thomas Lamprecht wrote:
> thx for this series!

Thank you very much for the review!

>
> On Tue, 17 Feb 2026, Daniel Kral wrote:
>> proxmox:
>>
>> Daniel Kral (5):
>>   resource-scheduling: move score_nodes_to_start_service to scheduler
>>     crate
>>   resource-scheduling: introduce generic cluster usage implementation
>>   resource-scheduling: add dynamic node and service stats
>>   resource-scheduling: implement rebalancing migration selection
>>   resource-scheduling: implement Add and Default for
>>     {Dynamic,Static}ServiceStats
>
> A few more notes on the proxmox and perl-rs patches, besides what
> Dominik already pointed out (thx!). Posting all those in a single
> reply here as I already started out that way, but can to per-patch
> replies if you prefer that. Will do the latter for the ha-manager
> part.
>
> The new scheduler logic seems to have no dedicated unit tests, i.e.
> the crate only seems to test TOPSIS. Would be nice to have at least
> basic tests for the imbalance calculation and migration scoring.

I will do so for the v2!

>
> In proxmox patch 1/5 add_cpu_usage becomes pub here but goes back to
> private in 2/5. Either move the function into scheduler.rs along with
> its callers, or inline the sentinel logic into add_started_service
> right away; the commit message also has no body, a short line on why
> the move is needed wouldnÄT hurt.

Right, the visibility change doesn't make a lot of sense.

I'll go for copying the logic in scheduler.rs, which will then turn into
add_started_service() in the next patch. add_cpu_usage() is still used
by StaticNodeUsage::add_service_usage(), which is needed to be able to
build older pve-rs with the newer crate.

In general, should I add a #[deprecated] notice or just a comment for
these things?

>
> In proxmox patch 2/5 deriving Debug for developer convenience for the
> new public types (e.g. ServiceStats, NodeStats, NodeUsage, ClusterUsage)
> wouldn't hurt.

ACK, took a look in general at the Rust API Guidelines for good measure
[0].

[0] https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits

>
> For proxmox patch 4/5, remove_running_service subtracts usize fields
> directly. If dynamic stats are stale or inconsistent, the mem subtraction
> can panic in debug or wrap-around in release builds - probably better to
> use a saturating_sub.

Thanks, I'll do that!

> Also, load() gives CPU and memory equal weight, but PveTopsisAlternative
> gives memory 5-10x more weight than CPU.
> So the brute-force and TOPSIS paths use different ideas of "balance",
> either fix that or document why it's fine.

Yes, I'll consider allowing at least a weighted load, which essentially
is the same. Though the TOPSIS method doesn't really have the notion of
an imbalance, but optimizes for low peaks and ~RMS values of the stats.
I'll check this once more. CC @Dominik if you have more insight here as
you took a good look at how TOPSIS behaves in specific scenarios.

>
> ScoredMigration's Ord only compares imbalance, so two migrations with
> the same imbalance but different source/target count as Equal, which
> makes the BinaryHeap output order unpredictable. Maybe use the Migration
> field, which is already Ord itself, to break any ties here as a secondary
> key.

Thanks! I forgot to add this as a FIXME there for the RFC series.
I had a

    impl Ord for ScoredMigration {
        fn cmp(&self, other: &Self) -> Ordering {
            self.imbalance
                .total_cmp(&other.imbalance)
                .reverse()
                .then(self.migration.cmp(&other.migration))
        }
    }

before, but while testing it seemed to not sort as expected. I haven't
looked into this yet, though I guess that different calculations might
end up in different exponents, which totalOrder does define as unequal
[1].

I'll briefly test this again, but sorting here in some reasonable way is
still better than letting the order of the input data decide.

[1] https://en.wikipedia.org/wiki/IEEE_754#Total-ordering_predicate

>
> For proxmox patch 5/5, tiny style thing: Add for DynamicServiceStats has
> Self as return type in its signature, while the Add impl for
> StaticServiceUsage has Self::Output there, both return Self though; while
> it doesn't really matter due to resolving to the same thing, it'd be still
> nice to use one variant for consistency.

Right, slipped right past me but changed it now, thx!

>
> For the perl-rs side: pve_dynamic.rs and pve_static.rs are ~90%
> identical. We already talked about this offlist and you mention this
> as low-prio todo, but given that the Usage struct layout, the
> generate_migration_candidates_from, all four score/select methods,
> and every node/service management method are nearly the same, it
> would be IMO still nice and worth it to have this deduplicated from
> the start on.
> E.g. generate_migration_candidates_from and the score/select
> wrappers should be relatively easily shared, since they only differ
> in the service stats type.

Yeah, as this was needed for resolving some other TODOs anyway (like
tracking the running state of resources), I've gone for this already in
the revised series.

>
> For perl-rs patch 4/6, CompactMigrationCandidate is introduced inside
> pve_static, then moved to mod.rs in patch 6/6 when pve_dynamic needs
> it. Same with the serde import. Better to create the module structure
> and put the shared type there from the start, so we avoid the
> back-and-forth.

Yes, that was rather confusing, I've gone for that for the revised
series already so it makes more sense!

>
> In generate_migration_candidates_from (both copies),
> leader.nodes.iter().next().unwrap() panics if the leader has an empty
> nodes set. That probably cannot happen in practice, but IMO still worth
> to avoid such unwraps in general and rather bail with an error instead.

Right, I revised the validation logic in general to be a bit more tight
than before. I've looked out so v2 won't add any unwrap/panics in
general unless the obvious mutex unwraps.

>
> Typo in the CompactMigrationCandidate doc comment: "MigationCandidate"
> is missing an 'r', i.e. s/MigationCandidate/MigrationCandidate/

ACK!

>
> perl-rs patches 1/6, 2/6, and 5/6 have no commit message body. At least
> for 1/6 and 5/6 short line on the motivation would be nice, since they
> restructure the module layout.

Will do so to make things clearer!

>
> That said, overall those two subseries are in good shape for an RFC!