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

Daniel Kral
2026-03-30T10:55:10Z

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>