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

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

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

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

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

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