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

Dominik Rusovac
2026-03-27T14:13:56Z

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>