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

["Dominik Rusovac" <d.rusovac@proxmox.com>]

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(())
> +}