From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 406F31FF0AF for ; Fri, 23 Oct 2026 12:00:19 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 08E1321652; Fri, 23 Oct 2026 12:00:19 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 23 Oct 2026 12:00:11 +0200 Message-Id: To: "Daniel Kral" , Subject: Re: [PATCH proxmox 03/15] fix #7993: resource-scheduling: lift to weighted load balancing From: "Dominik Rusovac" X-Mailer: aerc 0.20.0 References: <20261021085932.1196302-1-d.rusovac@proxmox.com> <20261021085932.1196302-4-d.rusovac@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792749611303 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.055 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: F77AGYXG5727CQGXPF5F5HSJUXD5B6JF X-Message-ID-Hash: F77AGYXG5727CQGXPF5F5HSJUXD5B6JF X-MailFrom: d.rusovac@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: thx a lot for your valuable feedback! will send a v2 On Thu Oct 22, 2026 at 11:15 AM CEST, Daniel Kral wrote: > Thanks a lot for working on this, nice work! Especially the cover letter > is a very interesting analysis and read! > > Looks generally sound, though there are some comments inline on having > backwards and forwards compatibility and some patch cleanup. thx! see my comments inline > > On Wed Oct 21, 2026 at 10:59 AM CEST, Dominik Rusovac wrote: >> Blend weights into node load computation, effectively replacing [snip] >> >> Refactor existing tests. >> >> [0] https://en.wikipedia.org/wiki/Arithmetic_mean >> [1] https://en.wikipedia.org/wiki/Weighted_arithmetic_mean >> [2] https://lore.proxmox.com/all/20221110143800.98047-3-f.ebner@proxmox.= com/ > > Changes which were applied should be referenced as commit refs rather > than the patch on the mailing list, i.e. here > > commit 2b325aa5 ("add pve_static module") > ack, thx! >> >> Signed-off-by: Dominik Rusovac >> --- >> proxmox-resource-scheduling/src/node.rs | 27 +- >> proxmox-resource-scheduling/src/pve_static.rs | 3 +- >> proxmox-resource-scheduling/src/scheduler.rs | 248 +++++++++++++----- >> .../tests/scheduler.rs | 136 ++++++++-- >> 4 files changed, 327 insertions(+), 87 deletions(-) >> >> diff --git a/proxmox-resource-scheduling/src/node.rs b/proxmox-resource-= scheduling/src/node.rs >> index 3c7ed393..f9dc5b16 100644 >> --- a/proxmox-resource-scheduling/src/node.rs >> +++ b/proxmox-resource-scheduling/src/node.rs >> @@ -51,9 +51,30 @@ impl NodeStats { >> self.mem as f64 / self.maxmem as f64 >> } >> =20 >> - /// Returns a combined node usage as a percentage. >> - pub fn load(&self) -> f64 { >> - (self.cpu_load() + self.mem_load()) / 2.0 >> + /// Returns the node load as the [weighted arithmetic mean] of [`Se= lf::cpu_load`] and >> + /// [`Self::mem_load`]. >> + /// >> + /// `cpu_weight` controls how much [`Self::cpu_load`] contributes t= o the load, analogously, >> + /// `mem_weight` controls how much [`Self::mem_load`] contributes t= o the load; the higher the >> + /// weight, the more that resource dominates the result. > > nit: can be documented in struct NodeStatsWeights, see below for more > >> + /// >> + /// NOTE: >> + /// - the weights have to be positive >> + /// - only the weights' ratio matters, not their absolute magnitude >> + /// - a weight of 0.0 means no contribution to the load >> + /// - [weighted arithmetic mean] is the same as [arithmetic mean] i= f the weights are equal >> + /// >> + /// [weighted arithmetic mean]: https://en.wikipedia.org/wiki/Weigh= ted_arithmetic_mean >> + /// [arithmetic mean]: https://en.wikipedia.org/wiki/Arithmetic_mea= n >> + pub fn load(&self, cpu_weight: f64, mem_weight: f64) -> f64 { >> + let weights_sum =3D cpu_weight + mem_weight; >> + >> + if weights_sum =3D=3D 0.0 { >> + // both, cpu and mem, should not contribute to the load, so= there is no load >> + return 0.0; >> + } >> + >> + (self.cpu_load() * cpu_weight + self.mem_load() * mem_weight) /= weights_sum >> } > > Would be nice to have a separate weighted_load() next to the existing > load() for backwards compatibility. > > And for some type of forwards compatibility, maybe something like > > #[derive(Copy, Clone, Debug)] > pub struct NodeStatsWeights { > pub cpu_weight: f64, > pub mem_weight: f64, > } > > impl Default for NodeStatsWeights { > fn default() -> Self { > Self { > cpu_weight: 1, > mem_weight: 1, > } > } > } > > pub fn weighted_load(&self, weights: &NodeStatsWeights) -> f64 {} > > ? I will likely take the 'weights as part of Scheduler' route=20 > > The NodeStatsWeights struct can then be used in all callers of > weighted_load() and therefore the members and their documentation do not > need to be repeated there. > > Though we'd probably need a default for TOPSIS as well, because there it > should be 1 : 5 with cpu : mem for backwards compatibility... > true, atm I am not entirely sure how to approach the different defaults requirement will see what I can do and hopefully find a convenient solution in v2 [snip]=20 >> =20 >> -criteria_struct! { >> - /// A given alternative. >> - struct PveTopsisAlternative { >> - #[criterion("average CPU", -1.0)] >> - average_cpu: f64, >> - #[criterion("highest CPU", -2.0)] >> - highest_cpu: f64, >> - #[criterion("average memory", -5.0)] >> - average_memory: f64, >> - #[criterion("highest memory", -10.0)] >> - highest_memory: f64, >> - } >> - >> - const N_CRITERIA; >> - static PVE_HA_TOPSIS_CRITERIA; >> -} >> - > > would prefer keeping the struct, so there's still a type giving the > numbers some name. > > struct PveTopsisAlternative { > average_cpu: f64, > highest_cpu: f64, > average_memory: f64, > highest_memory: f64, > } > > const N_CRITERIA =3D 4; > > impl From for [f64; N_CRITERIA] { > fn from(alternative: PveTopsisAlternative) -> Self { > [ > alternative.average_cpu, > alternative.highest_cpu, > alternative.average_memory, > alternative.highest_memory, > ] > } > } > ack >> #[derive(Clone, Debug)] >> pub struct Scheduler { >> nodes: Vec, >> @@ -111,12 +97,20 @@ pub struct ScoredMigration { >> pub migration: Migration, >> /// The expected node imbalance after the migration. >> pub imbalance: f64, >> + /// The score. >> + /// >> + /// - corresponds to imbalance of weighted loads for >> + /// [`Scheduler::score_best_balancing_migration_candidates`] >> + /// - corresponds to distance to ideal best alternatives for >> + /// [`Scheduler::score_best_balancing_migration_candidates_topsis= `] >> + pub score: f64, > > As discussed off-list, I'm not sure whether we should expose the 'score' > as it isn't relevant to the feedback loop now. > you're right, will be removed in v2 >> } >> =20 >> impl Ord for ScoredMigration { >> fn cmp(&self, other: &Self) -> Ordering { >> - self.imbalance >> - .total_cmp(&other.imbalance) >> + self.score >> + .total_cmp(&other.score) >> + .then(self.imbalance.total_cmp(&other.imbalance)) >> .then(self.migration.cmp(&other.migration)) > > AFAICT this could lead to unwanted load balancing decisions at least for > the brute force case: > > If a user configures cpu_weight =3D 0, mem_weight =3D 1, i.e., do not > include CPU usage in load balancing decisions, then if two migration > candidates would have the equal change in memory distribution but > different ones in CPU (i.e. equal imbalance values), it would then still > include CPU usage in the load balancing decision with the unweighted > 'imbalance' value. > > Of course, this wouldn't even be really noticable, so might not be an > issue. valid point, thx for the heads up! afaict the described issue would rarely show up, but it's=20 nevertheless real and the behaviour of the ranking is wrong in principle v2 will remove `score`, so this should no longer cause issues in v2 > > Though I wonder whether calculating both the unweighted and weighted > node imbalance values for all migration candidates at least in the > bruteforce case does increase the computational load for a relatively > small special case (i.e. equal weighted imbalance values). > valid point too, thx! as mentioned earlier, v2 will remove `score`, so this should no longer cause issues in v2 [snip]=20 >> - /// Scores the given migration `candidates` by the best node imbala= nce improvement with >> - /// exhaustive search. >> + /// Uses brute-force search to score how much either of the given m= igration `candidates` >> + /// improves node imbalance and ranks them accordingly. > > Good catch! > > As noted off-list, the "score" name is unfortunately a bit of a > misnomer, because this doesn't actually score them (the imbalance part > is only information for the feedback loop and log output outside), but > ranks them, therefore making the limit parameter reasonable. > > However, as this is pre-existing and isn't relevant to this specific > patch, this should be done in a separate patch to not clutter this one, > which already changes quite a lot on its own. > > This makes understanding past changes much easier in the future and > reviewing them too. ack, will isolate all the changes refining docs in a separate patch [snip] >> pub fn score_best_balancing_migration_candidates_topsis( > > same for this method as above > >> &self, >> candidates: &[MigrationCandidate], >> limit: usize, >> + cpu_weight: usize, >> + mem_weight: usize, >> ) -> Result, Error> { >> + let (w1, w2) =3D (cpu_weight as f64, mem_weight as f64); >> + >> + let average_cpu =3D topsis::Criterion::new("average_cpu".to_own= ed(), -w1); >> + let highest_cpu =3D topsis::Criterion::new( >> + "highest_cpu".to_owned(), >> + -(TOPSIS_AVERAGE_VS_HIGHEST_FACTOR * w1), >> + ); >> + let average_mem =3D topsis::Criterion::new("average_mem".to_own= ed(), -w2); >> + let highest_mem =3D topsis::Criterion::new( >> + "highest_mem".to_owned(), >> + -(TOPSIS_AVERAGE_VS_HIGHEST_FACTOR * w2), >> + ); >> + >> let matrix =3D candidates >> .iter() >> .map(|candidate| { >> self.topsis_alternative_with(|node| node_stats_after_mi= gration(candidate, node)) >> - .into() >> }) >> .collect::>(); >> =20 >> - let best_alternatives =3D >> - topsis::rank_alternatives(&topsis::Matrix::new(matrix)?, &P= VE_HA_TOPSIS_CRITERIA)?; >> + let mut scored_migrations =3D topsis::score_alternatives( >> + &topsis::Matrix::new(matrix)?, >> + &topsis::Criteria::new([average_cpu, highest_cpu, average_m= em, highest_mem])?, >> + )? >> + .iter() >> + .zip(candidates.iter()) >> + .map(|(topsis_score, candidate)| { >> + let imbalance =3D self.node_imbalance_with_migration_candid= ate(candidate, w1, w2); > > So the meaning of the 'imbalance' field in ScoredMigration would also > differ in meaning between brute force and TOPSIS? in v1 yes, in v2 `imbalance` will simply be the weighted load imbalance wit= h the specified weights for both methods > >> + >> + Reverse(ScoredMigration::new( >> + candidate.clone(), >> + imbalance, >> + // invert score for consistent sorting >> + 1.0 - topsis_score, >> + )) >> + }) >> + .collect::>(); >> =20 >> - Ok(best_alternatives >> - .into_iter() >> - .take(limit) >> - .map(|i| { >> - let imbalance =3D self.node_imbalance_with_migration_ca= ndidate(&candidates[i]); >> + let mut best_migrations =3D Vec::with_capacity(limit); >> =20 >> - ScoredMigration::new(candidates[i].clone(), imbalance) >> - }) >> - .collect()) >> + // BinaryHeap::into_iter_sorted() is still in nightly unfortuna= tely >> + while best_migrations.len() < limit { >> + match scored_migrations.pop() { >> + Some(Reverse(alternative)) =3D> best_migrations.push(al= ternative), >> + None =3D> break, >> + } >> + } >> + >> + Ok(best_migrations) >> } > > Hm, I think we should leave > score_best_balancing_migration_candidates_topsis() to only rank by > TOPSIS and not do the same procedure as is done with brute force. > ack [snip]=20 >> pub fn score_nodes_to_start_resource>( >> &self, >> resource_stats: T, >> + cpu_weight: usize, >> + mem_weight: usize, >> ) -> Result, Error> { > > Unrelated to this patch itself, but the parameters are exposed as > 'ha-auto-rebalance-{cpu,mem}-weight', but here they are also used for > other kinds of load balancing, i.e., initial placement balancing. > technically these weights can be set in multiple places across this series, but the series does not make them configurable in general [0] the parameters were added in these places to make potential future changes easier, in case the weights for initial placement balancing were to become configurable at some point in the future [0] https://lore.proxmox.com/all/DMBA12N76K0Z.31DGWX02RMAYD@proxmox.com/#:~= :text=3D152%2C13%20%40%40%20sub%20score%5Fbest%5Fbalancing%5Fmigrations%5Ft= opsis%20%7B%20%3E%20sub%20score%5Fnodes%5Fto%5Fstart%5Fservice%20%7B%20%3E%= 20my%20%28%24self%2C%20%24sid%29%20%3D%20%40%5F%3B%20%3E-,%3E%20%2B%20my%20= %28%24cpu%5Fweight%2C%20%24mem%5Fweight%29%20%3D%20%281%2C%205%29%3B > So maybe we should change the name of those? this is a more general question, which (as you mentioned) is affecting other patches rather than this one. nevertheless... I was torn on this and decided to not make the weights for initial placement balancing configurable atm, but making them configurable right away is definitely possible [snip]