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 0E6D81FF137 for ; Tue, 31 Mar 2026 09:26:06 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A11ED111D9; Tue, 31 Mar 2026 09:26:32 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Tue, 31 Mar 2026 09:26:27 +0200 Message-Id: Subject: Re: [PATCH proxmox v3 05/40] resource-scheduling: implement generic cluster usage implementation From: "Dominik Rusovac" To: "Daniel Kral" , Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.20.0 References: <20260330144101.668747-1-d.kral@proxmox.com> <20260330144101.668747-6-d.kral@proxmox.com> In-Reply-To: <20260330144101.668747-6-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774941932685 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.066 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 1 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 1 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 1 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 Message-ID-Hash: 6AJW7IGMMM2FSM7PRKCH54FYYFLATEO3 X-Message-ID-Hash: 6AJW7IGMMM2FSM7PRKCH54FYYFLATEO3 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: one tiny nit inline lgtm, consider this On Mon Mar 30, 2026 at 4:30 PM CEST, 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 v2 -> v3: > - inline bail! formatting variables > - s/to_string/to_owned/ where reasonable > - make Node::resources_iter(&self) return &str Iterator impl > - drop add_resource_to_nodes() and remove_resource_from_nodes() > - drop ResourcePlacement::nodenames() and Resource::nodenames() > - drop Resource::moving_to() > - fix behavior of add_resource_usage_to_node() for already added > resources: if the next nodename is non-existing, the resource would > still be put into moving but then not add the resource to the nodes; > this is fixed now by improving the handling > - inline behavior of add_resource() to be more consise how both > placement strategies are handled > - no change in Resource::remove_node() documentation as I did not find a > better description in the meantime, but as it's internal it can be > improved later on as well > > test changes v2 -> v3: > - use assertions whether nodes were added correctly in test cases > - use assertions whether resource were added correctly in test cases > - additionally assert whether resource cannot be added to non-existing > node with add_resource_usage_to_node() and does not alter state of the > Resource for that resource in the mean time as it was in v2 > - use assert!() instead of bail!() in test cases as much as appropriate > [snip] > + /// Add `stats` from resource with identifier `sid` to node `nodenam= e` in cluster usage. > + /// > + /// For the first call, the resource is assumed to be started and st= ationary on the given node. > + /// If there was no intermediate call to remove the resource, the se= cond call will assume that > + /// the given node is the target node and the resource is being move= d there. The second call > + /// will ignore the value of `stats`. > + #[deprecated =3D "only for backwards compatibility, use add_resource= (...) instead"] > + pub fn add_resource_usage_to_node( > + &mut self, > + nodename: &str, > + sid: &str, > + stats: ResourceStats, > + ) -> Result<(), Error> { > + if let Some(resource) =3D self.resources.remove(sid) { > + match resource.placement() { > + ResourcePlacement::Stationary { current_node } =3D> { > + let placement =3D ResourcePlacement::Moving { > + current_node: current_node.to_owned(), > + target_node: nodename.to_owned(), > + }; > + let new_resource =3D Resource::new(resource.stats(),= resource.state(), placement); > + nit: the logic in here is kinda tricky, adding an explanatory comment would certainly help the understanding=20 > + if let Err(err) =3D self.add_resource(sid.to_owned()= , new_resource) { > + self.add_resource(sid.to_owned(), resource)?; > + > + bail!(err); > + } > + > + Ok(()) > + } > + ResourcePlacement::Moving { target_node, .. } =3D> { > + bail!("resource '{sid}' is already moving to target = node '{target_node}'") > + } > + } > + } else { > + let placement =3D ResourcePlacement::Stationary { > + current_node: nodename.to_owned(), > + }; > + let resource =3D Resource::new(stats, ResourceState::Started= , placement); > + > + self.add_resource(sid.to_owned(), resource) > + } > + } [snip] Reviewed-by: Dominik Rusovac