From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 665991FF13C for ; Thu, 19 Mar 2026 10:13:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D4A39140E4; Thu, 19 Mar 2026 10:13:24 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 19 Mar 2026 10:12:48 +0100 Message-Id: Subject: Re: [RFC PATCH-SERIES many 00/36] dynamic scheduler + load rebalancer From: "Daniel Kral" To: "Thomas Lamprecht" , X-Mailer: aerc 0.21.0-38-g7088c3642f2c-dirty References: <20260217141437.584852-1-d.kral@proxmox.com> <3fcd4459-e5ff-48ca-8b70-53411a666247@proxmox.com> In-Reply-To: <3fcd4459-e5ff-48ca-8b70-53411a666247@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773911526219 X-SPAM-LEVEL: Spam detection results: 0 AWL -2.522 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.408 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.819 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.903 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLACK 3 Contains an URL listed in the URIBL blacklist [rust-lang.github.io] Message-ID-Hash: SGQVLMOZDVBFT2DAGQR4MPZ7KI3H4VAK X-Message-ID-Hash: SGQVLMOZDVBFT2DAGQR4MPZ7KI3H4VAK X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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=C3=84T 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 stil= l > 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!