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

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

one tiny nit inline

lgtm, consider this

On Mon Mar 30, 2026 at 4:30 PM CEST, 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 v2 -> v3:
> - inline bail! formatting variables
> - s/to_string/to_owned/ where reasonable
> - make Node::resources_iter(&self) return &str Iterator impl
> - drop add_resource_to_nodes() and remove_resource_from_nodes()
> - drop ResourcePlacement::nodenames() and Resource::nodenames()
> - drop Resource::moving_to()
> - fix behavior of add_resource_usage_to_node() for already added
>   resources: if the next nodename is non-existing, the resource would
>   still be put into moving but then not add the resource to the nodes;
>   this is fixed now by improving the handling
> - inline behavior of add_resource() to be more consise how both
>   placement strategies are handled
> - no change in Resource::remove_node() documentation as I did not find a
>   better description in the meantime, but as it's internal it can be
>   improved later on as well
>
> test changes v2 -> v3:
> - use assertions whether nodes were added correctly in test cases
> - use assertions whether resource were added correctly in test cases
> - additionally assert whether resource cannot be added to non-existing
>   node with add_resource_usage_to_node() and does not alter state of the
>   Resource for that resource in the mean time as it was in v2
> - use assert!() instead of bail!() in test cases as much as appropriate
>

[snip]

> +    /// Add `stats` from resource with identifier `sid` to node `nodename` in cluster usage.
> +    ///
> +    /// For the first call, the resource is assumed to be started and stationary on the given node.
> +    /// If there was no intermediate call to remove the resource, the second call will assume that
> +    /// the given node is the target node and the resource is being moved there. The second call
> +    /// will ignore the value of `stats`.
> +    #[deprecated = "only for backwards compatibility, use add_resource(...) instead"]
> +    pub fn add_resource_usage_to_node(
> +        &mut self,
> +        nodename: &str,
> +        sid: &str,
> +        stats: ResourceStats,
> +    ) -> Result<(), Error> {
> +        if let Some(resource) = self.resources.remove(sid) {
> +            match resource.placement() {
> +                ResourcePlacement::Stationary { current_node } => {
> +                    let placement = ResourcePlacement::Moving {
> +                        current_node: current_node.to_owned(),
> +                        target_node: nodename.to_owned(),
> +                    };
> +                    let new_resource = Resource::new(resource.stats(), resource.state(), placement);
> +

nit: the logic in here is kinda tricky, adding an explanatory
comment would certainly help the understanding 

> +                    if let Err(err) = self.add_resource(sid.to_owned(), new_resource) {
> +                        self.add_resource(sid.to_owned(), resource)?;
> +
> +                        bail!(err);
> +                    }
> +
> +                    Ok(())
> +                }
> +                ResourcePlacement::Moving { target_node, .. } => {
> +                    bail!("resource '{sid}' is already moving to target node '{target_node}'")
> +                }
> +            }
> +        } else {
> +            let placement = ResourcePlacement::Stationary {
> +                current_node: nodename.to_owned(),
> +            };
> +            let resource = Resource::new(stats, ResourceState::Started, placement);
> +
> +            self.add_resource(sid.to_owned(), resource)
> +        }
> +    }

[snip]

Reviewed-by: Dominik Rusovac <d.rusovac@proxmox.com>