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

["Daniel Kral" <d.kral@proxmox.com>]

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

Thanks, will do that!

>> +    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]

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 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");
>

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 = 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"
>     );
>

Right, that's more appropriate, will do so here and for all the
following, thanks!