From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 6A7E31FF13F for ; Thu, 26 Mar 2026 15:15:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B9BAF1515C; Thu, 26 Mar 2026 15:15:25 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 26 Mar 2026 15:15:19 +0100 Message-Id: From: "Daniel Kral" To: "Dominik Rusovac" , Subject: Re: [PATCH proxmox v2 05/40] resource-scheduling: implement generic cluster usage implementation X-Mailer: aerc 0.21.0-38-g7088c3642f2c-dirty References: <20260324183029.1274972-1-d.kral@proxmox.com> <20260324183029.1274972-6-d.kral@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774534471301 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.058 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 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: S243FMYQ33DLLBBXKOSTJHL7CSUSL24X X-Message-ID-Hash: S243FMYQ33DLLBBXKOSTJHL7CSUSL24X 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 Thu Mar 26, 2026 at 11:28 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: >> This is a more generic version of the `Usage` implementation from the >> pve_static bindings in the pve_rs repository. >> >> As the upcoming load balancing scheduler actions and dynamic resource >> scheduler will need more information about each resource, this further >> improves on the state tracking of each resource: >> >> In this implementation, a resource is composed of its usage statistics >> and its two essential states: the running state and the node placement. >> The non_exhaustive attribute ensures that usages need to construct the >> a Resource instance through its API. >> >> Users can repeatedly use the current state of Usage to make scheduling >> decisions with the to_scheduler() method. This method takes an >> implementation of UsageAggregator, which dictates how the usage >> information is represented to the Scheduler. >> >> Signed-off-by: Daniel Kral >> --- >> changes v1 -> v2: >> - new! >> >> This patch is added to move the handling of specific usage stats and >> their (de)serialization to the pve-rs bindings and have the general >> functionality in this crate. > > [snip] > > nit: imo, it's more convenient to expose the more ergonomic `&str` type, > using: > > pub fn resources_iter(&self) -> impl Iterator { > self.resources.iter().map(String::as_str) > } > Thanks, will do that! >> + pub fn resources_iter(&self) -> impl Iterator { >> + self.resources.iter() >> + } > > [snip]=20 > >> + pub fn moving_to(&mut self, target_node: String) -> Result<(), Erro= r> { >> + match &self.placement { >> + ResourcePlacement::Stationary { current_node } =3D> { >> + self.placement =3D ResourcePlacement::Moving { >> + current_node: current_node.to_string(), > > nit:=20 > > current_node: current_node.to_owned(),=20 > > represents the intention best, that is, owning rather than converting > > [snip] Thanks, will do so for this and the rest! [...] >> + /// Add a node to the cluster usage. >> + /// >> + /// This method fails if a node with the same `nodename` already ex= ists. >> + pub fn add_node(&mut self, nodename: String, stats: NodeStats) -> R= esult<(), Error> { >> + if self.nodes.contains_key(&nodename) { >> + bail!("node '{}' already exists", nodename); > > nit:=20 > > bail!("node '{nodename}' already exists"); > ACK >> + } > > [snip] > > we are reading only, consider using a slice for `nodenames` here (just > like for `remove_resource_from_nodes`): > > fn add_resource_to_nodes(&mut self, sid: &str, nodenames: &[&str])= -> Result<(), Error> { > > pls find the related changes [0] and [1]. > Right, that makes more sense, will go for that! [...] >> +#[test] >> +fn test_no_duplicate_nodes() -> Result<(), Error> { >> + let mut usage =3D Usage::new(); >> + >> + usage.add_node("node1".to_string(), NodeStats::default())?; >> + >> + match usage.add_node("node1".to_string(), NodeStats::default()) { >> + Ok(_) =3D> bail!("cluster usage does allow duplicate node entri= es"), >> + Err(_) =3D> Ok(()), >> + } > > since this is supposed to be a test case, I would rather assert instead > of bail, using: > > assert!( > usage > .add_node("node1".to_string(), NodeStats::default()) > .is_err(), > "cluster usage allows duplicate node entries" > ); > Right, that's more appropriate, will do so here and for all the following, thanks!