Re: [PATCH proxmox v3 05/40] resource-scheduling: implement generic cluster usage implementation

Dominik Rusovac
2026-03-31T07:26:06Z

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 <d.kral@proxmox.com>
> ---
> 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 `nodename` in cluster usage.
> + ///
> + /// For the first call, the resource is assumed to be started and stationary on the given node.
> + /// If there was no intermediate call to remove the resource, the second call will assume that
> + /// the given node is the target node and the resource is being moved there. The second call
> + /// will ignore the value of `stats`.
> + #[deprecated = "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) = self.resources.remove(sid) {
> + match resource.placement() {
> + ResourcePlacement::Stationary { current_node } => {
> + let placement = ResourcePlacement::Moving {
> + current_node: current_node.to_owned(),
> + target_node: nodename.to_owned(),
> + };
> + let new_resource = Resource::new(resource.stats(), resource.state(), placement);
> +

nit: the logic in here is kinda tricky, adding an explanatory
comment would certainly help the understanding

> + if let Err(err) = self.add_resource(sid.to_owned(), new_resource) {
> + self.add_resource(sid.to_owned(), resource)?;
> +
> + bail!(err);
> + }
> +
> + Ok(())
> + }
> + ResourcePlacement::Moving { target_node, .. } => {
> + bail!("resource '{sid}' is already moving to target node '{target_node}'")
> + }
> + }
> + } else {
> + let placement = ResourcePlacement::Stationary {
> + current_node: nodename.to_owned(),
> + };
> + let resource = Resource::new(stats, ResourceState::Started, placement);
> +
> + self.add_resource(sid.to_owned(), resource)
> + }
> + }

[snip]