Re: [PATCH perl-rs v2 13/40] pve-rs: resource-scheduling: use generic usage implementation

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

On Fri Mar 27, 2026 at 3:13 PM CET, Dominik Rusovac wrote:
> lgtm, consider modulo nits
>
> On Tue Mar 24, 2026 at 7:29 PM CET, Daniel Kral wrote:
>> The proxmox_resource_scheduling crate provides a generic usage
>> implementation, which is backwards compatible with the pve_static
>> bindings. This reduces the static resource scheduling bindings to a
>> slightly thinner wrapper.
>
> good measure, to make proxmox-resource-scheduling handle usage
>
>>
>> This also exposes the new `add_resource(...)` binding, which allows
>> callers to add services with additional state other than the usage
>> stats. It is exposed as `add_service(...)` to be consistent with the
>> naming of the rest of the existing methods.
>>
>> Where it is sensible for the bindings, the documentation is extended
>> with a link to the documentation of the underlying methods.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes v1 -> v2:
>> - add patch message for context
>> - change from only creating the
>>   proxmox_resource_scheduling::scheduler::ClusterUsage (now,
>>   proxmox_resource_scheduling::scheduler::Scheduler), to using the new
>>   but backwards-compatible `Usage` implementation instead
>> - this essentially also squashes the 'store services stats independently
>>   of node' patch in here as this is also tracked by the generic `Usage`
>>   impl
>> - add `usage` and `resource` crate for shared code
>
> [snip]
>
>> +
>> +impl<T: Into<ResourceStats>> TryFrom<PveResource<T>> for Resource {
>> +    type Error = Error;
>> +
>> +    fn try_from(resource: PveResource<T>) -> Result<Self, Error> {
>> +        let state = if resource.running {
>> +            ResourceState::Started
>> +        } else {
>> +            ResourceState::Starting
>> +        };
>> +
>> +        let placement = match (resource.current_node, resource.target_node) {
>
> as it came up off-list, we might not only prohibit equivalence of 
> current_node and target_node in proxmox-resource-scheduling, but 
> also here
>

Thanks! Even though it's redundant, I added here now to make it a little
easier to track down what the constraints are here.

>> +            (Some(current_node), Some(target_node)) => ResourcePlacement::Moving {
>> +                current_node,
>> +                target_node,
>> +            },
>> +            (Some(current_node), None) | (None, Some(current_node)) => {
>
> it would be good to have a comment (// NOTE: ...) explaining as to why
> this arm's code
>

I've decided to move this behavior to where it's relevant in the
pve-ha-manager (PVE::HA::Usage::{Dynamic,Static}::add_service()) as it's
easier to understand there why this is true.

>> +                ResourcePlacement::Stationary { current_node }
>> +            }
>> +            _ => bail!("neither current_node nor target_node are set"),
>> +        };
>> +
>> +        Ok(Resource::new(resource.stats.into(), state, placement))
>> +    }
>> +}
>
> [snip]
>
>> +impl UsageAggregator for StartedResourceAggregator {
>> +    fn aggregate(usage: &Usage) -> Vec<NodeUsage> {
>> +        usage
>> +            .nodes_iter()
>> +            .map(|(nodename, node)| {
>
> nice fold!
>
> nit: by making `node_stats` mutable in the first place, variable
>      shadowing can be avoided, see:
>
>     let stats = node.resources_iter().fold(node.stats(), |mut node_stats, sid| {
>     
>         if let Some(resource) = usage.get_resource(sid) {
>             node_stats.add_started_resource(&resource.stats());
>         }
>     
>         node_stats
>     });
>

Nice, thanks!

>> +                let stats = node.resources_iter().fold(node.stats(), |node_stats, sid| {
>> +                    let mut node_stats = node_stats;
>> +
>> +                    if let Some(resource) = usage.get_resource(sid) {
>> +                        node_stats.add_started_resource(&resource.stats());
>> +                    }
>> +
>> +                    node_stats
>> +                });
>> +
>> +                NodeUsage {
>> +                    name: nodename.to_string(),
>> +                    stats,
>> +                }
>> +            })
>> +            .collect()
>> +    }
>> +}
>
> Reviewed-by: Dominik Rusovac <d.rusovac@proxmox.com>