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

Dominik Rusovac
2026-03-26T10:28:40Z

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 <d.kral@proxmox.com>
> ---
> 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<Item = &str> {
self.resources.iter().map(String::as_str)
}

> + pub fn resources_iter(&self) -> impl Iterator<Item = &String> {
> + self.resources.iter()
> + }

[snip]

> + pub fn moving_to(&mut self, target_node: String) -> Result<(), Error> {
> + match &self.placement {
> + ResourcePlacement::Stationary { current_node } => {
> + self.placement = ResourcePlacement::Moving {
> + current_node: current_node.to_string(),

nit:

current_node: current_node.to_owned(),

represents the intention best, that is, owning rather than converting

[snip]

> + /// Handles the external removal of a node.
> + ///
> + /// Returns whether the resource does not have any node left.

Considering what it does, I find the name of this function a bit confusing.

> + pub fn remove_node(&mut self, nodename: &str) -> bool {
> + match &self.placement {
> + ResourcePlacement::Stationary { current_node } => current_node == nodename,
> + ResourcePlacement::Moving {
> + current_node,
> + target_node,
> + } => {
> + if current_node == nodename {
> + self.placement = ResourcePlacement::Stationary {
> + current_node: target_node.to_string(),

nit: to_owned() represents the intention best

> + };
> + } else if target_node == nodename {
> + self.placement = ResourcePlacement::Stationary {
> + current_node: current_node.to_string(),

nit: to_owned() represents the intention best

> + };
> + }
> +
> + false
> + }
> + }
> + }

[snip]

> + /// Add a node to the cluster usage.
> + ///
> + /// This method fails if a node with the same `nodename` already exists.
> + pub fn add_node(&mut self, nodename: String, stats: NodeStats) -> Result<(), Error> {
> + if self.nodes.contains_key(&nodename) {
> + bail!("node '{}' already exists", nodename);

nit:

bail!("node '{nodename}' already exists");

> + }

[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].

> + fn add_resource_to_nodes(&mut self, sid: &str, nodenames: Vec<&str>) -> Result<(), Error> {
> + if nodenames
> + .iter()
> + .any(|nodename| !self.nodes.contains_key(*nodename))
> + {
> + bail!("resource nodes do not exist");
> + }
> +
> + nodenames.iter().for_each(|nodename| {
> + if let Some(node) = self.nodes.get_mut(*nodename) {
> + node.add_resource(sid);
> + }
> + });
> +
> + Ok(())
> + }

[snip]

> + /// Add `resource` with identifier `sid` to cluster usage.
> + ///
> + /// This method fails if a resource with the same `sid` already exists or the resource's nodes
> + /// do not exist in the cluster usage.
> + pub fn add_resource(&mut self, sid: String, resource: Resource) -> Result<(), Error> {
> + if self.resources.contains_key(&sid) {
> + bail!("resource '{}' already exists", sid);
> + }
> +
> + self.add_resource_to_nodes(&sid, resource.nodenames())?;

[0]:

self.add_resource_to_nodes(&sid, &resource.nodenames())?;

> +
> + self.resources.insert(sid.to_string(), resource);

nit: to_owned() instead of of to_string() represents the intention best

[snip]

> + pub fn add_resource_usage_to_node(
> + &mut self,
> + nodename: &str,
> + sid: &str,
> + stats: ResourceStats,
> + ) -> Result<(), Error> {
> + if let Some(resource) = self.resources.get_mut(sid) {
> + resource.moving_to(nodename.to_string())?;
> +
> + self.add_resource_to_nodes(sid, vec![nodename])

[1]:

self.add_resource_to_nodes(sid, &[nodename])

[snip]

> +#[test]
> +fn test_no_duplicate_nodes() -> Result<(), Error> {
> + let mut usage = Usage::new();
> +
> + usage.add_node("node1".to_string(), NodeStats::default())?;
> +
> + match usage.add_node("node1".to_string(), NodeStats::default()) {
> + Ok(_) => bail!("cluster usage does allow duplicate node entries"),
> + Err(_) => 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"
);

> +}
> +
> +#[test]
> +fn test_no_duplicate_resources() -> Result<(), Error> {
> + let mut usage = Usage::new();
> +
> + usage.add_node("node1".to_string(), NodeStats::default())?;
> +
> + let placement = ResourcePlacement::Stationary {
> + current_node: "node1".to_string(),
> + };
> + let resource = Resource::new(ResourceStats::default(), ResourceState::Stopped, placement);
> +
> + usage.add_resource("vm:101".to_string(), resource.clone())?;
> +
> + match usage.add_resource("vm:101".to_string(), resource) {
> + Ok(_) => bail!("cluster usage does allow duplicate resource entries"),
> + Err(_) => Ok(()),
> + }

assert instead of bail:

assert!(
usage.add_resource("vm:101".to_string(), resource).is_err(),
"cluster usage allows duplicate resource entries"
);

> +}
> +
> +#[test]
> +#[allow(deprecated)]
> +fn test_add_resource_usage_to_node() -> Result<(), Error> {
> + let mut usage = Usage::new();
> +
> + usage.add_node("node1".to_string(), NodeStats::default())?;
> + usage.add_node("node2".to_string(), NodeStats::default())?;
> + usage.add_node("node3".to_string(), NodeStats::default())?;
> +
> + usage.add_resource_usage_to_node("node1", "vm:101", ResourceStats::default())?;
> + usage.add_resource_usage_to_node("node2", "vm:101", ResourceStats::default())?;
> +
> + if usage
> + .add_resource_usage_to_node("node3", "vm:101", ResourceStats::default())
> + .is_ok()
> + {
> + bail!("add_resource_usage_to_node() allows adding resource to more than two nodes");
> + }

assert instead of bail:

assert!(
usage
.add_resource_usage_to_node("node3", "vm:101", ResourceStats::default())
.is_err(),
"add_resource_usage_to_node() allows adding resource to more than two nodes"
);

> +
> + Ok(())
> +}
> +
> +#[test]
> +fn test_add_remove_stationary_resource() -> Result<(), Error> {
> + let mut usage = Usage::new();
> +
> + let (sid, nodename) = ("vm:101", "node1");
> +
> + usage.add_node(nodename.to_string(), NodeStats::default())?;
> +
> + let placement = ResourcePlacement::Stationary {
> + current_node: nodename.to_string(),
> + };
> + let resource = Resource::new(ResourceStats::default(), ResourceState::Stopped, placement);
> +
> + usage.add_resource(sid.to_string(), resource)?;
> +
> + match (usage.get_resource(sid), usage.get_node(nodename)) {
> + (Some(_), Some(node)) => {
> + if !node.contains_resource(sid) {
> + bail!("resource '{sid}' was not added to node '{nodename}'");
> + }
> + }
> + _ => bail!("resource '{sid}' or node '{nodename}' were not added"),
> + }

assert instead of bail:

assert!(
usage.get_resource(sid).is_some(),
"resource '{sid}' was not added"
);
assert!(
usage
.get_node(nodename)
.map(|node| {
assert!(
node.contains_resource(sid),
"resource '{sid}' was not added to node '{nodename}'"
);
})
.is_some(),
"node '{nodename}' was not added"
);

> +
> + usage.remove_resource(sid);
> +
> + match (usage.get_resource(sid), usage.get_node(nodename)) {
> + (None, Some(node)) => {
> + if node.contains_resource(sid) {
> + bail!("resource '{sid}' was not removed from node '{nodename}'");
> + }
> + }
> + _ => bail!("resource '{sid}' was not removed"),
> + }

assert instead of bail:

assert!(
usage.get_resource(sid).is_none(),
"resource '{sid}' was not removed"
);
assert!(
usage
.get_node(nodename)
.map(|node| {
assert!(
!node.contains_resource(sid),
"resource '{sid}' was not removed from node '{nodename}'"
);
})
.is_some(),
"node '{nodename}' was not added"
);

> +
> + Ok(())
> +}
> +
> +#[test]
> +fn test_add_remove_moving_resource() -> Result<(), Error> {
> + let mut usage = Usage::new();
> +
> + let (sid, current_nodename, target_nodename) = ("vm:101", "node1", "node2");
> +
> + usage.add_node(current_nodename.to_string(), NodeStats::default())?;
> + usage.add_node(target_nodename.to_string(), NodeStats::default())?;
> +
> + let placement = ResourcePlacement::Moving {
> + current_node: current_nodename.to_string(),
> + target_node: target_nodename.to_string(),
> + };
> + let resource = Resource::new(ResourceStats::default(), ResourceState::Stopped, placement);
> +
> + usage.add_resource(sid.to_string(), resource)?;
> +

analogously, here I'd find asserting more appropriate than bailing

> + match (
> + usage.get_resource(sid),
> + usage.get_node(current_nodename),
> + usage.get_node(target_nodename),
> + ) {
> + (Some(_), Some(current_node), Some(target_node)) => {
> + if !current_node.contains_resource("vm:101") {
> + bail!("resource '{sid}' was not added to current node '{current_nodename}'");
> + }
> +
> + if !target_node.contains_resource("vm:101") {
> + bail!("resource '{sid}' was not added to target node '{target_nodename}'");
> + }
> + }
> + _ => bail!("resource '{sid}' or nodes were not added"),
> + }
> +
> + usage.remove_resource(sid);

analogously, here I'd find asserting more appropriate than bailing

> +
> + match (
> + usage.get_resource(sid),
> + usage.get_node(current_nodename),
> + usage.get_node(target_nodename),
> + ) {
> + (None, Some(current_node), Some(target_node)) => {
> + if current_node.contains_resource(sid) {
> + bail!("resource '{sid}' was not removed from current node '{current_nodename}'");
> + }
> +
> + if target_node.contains_resource(sid) {
> + bail!("resource '{sid}' was not removed from target node '{target_nodename}'");
> + }
> + }
> + _ => bail!("resource '{sid}' was not removed"),
> + }
> +
> + Ok(())
> +}