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

Daniel Kral
2026-03-26T14:15:09Z

On Thu Mar 26, 2026 at 11:28 AM CET, Dominik Rusovac wrote:
> lgtm
>
> pls find my comments inline, mostly relating to nits or tiny things
>
> On Tue Mar 24, 2026 at 7:29 PM CET, 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 v1 -> v2:
>> - new!
>>
>> This patch is added to move the handling of specific usage stats and
>> their (de)serialization to the pve-rs bindings and have the general
>> functionality in this crate.
>
> [snip]
>
> nit: imo, it's more convenient to expose the more ergonomic `&str` type,
> using:
>
> pub fn resources_iter(&self) -> impl Iterator<Item = &str> {
> self.resources.iter().map(String::as_str)
> }
>

Thanks, will do that!

>> + pub fn resources_iter(&self) -> impl Iterator<Item = &String> {
>> + self.resources.iter()
>> + }
>
> [snip]
>
>> + pub fn moving_to(&mut self, target_node: String) -> Result<(), Error> {
>> + match &self.placement {
>> + ResourcePlacement::Stationary { current_node } => {
>> + self.placement = ResourcePlacement::Moving {
>> + current_node: current_node.to_string(),
>
> nit:
>
> current_node: current_node.to_owned(),
>
> represents the intention best, that is, owning rather than converting
>
> [snip]

Thanks, will do so for this and the rest!

[...]

>> + /// Add a node to the cluster usage.
>> + ///
>> + /// This method fails if a node with the same `nodename` already exists.
>> + pub fn add_node(&mut self, nodename: String, stats: NodeStats) -> Result<(), Error> {
>> + if self.nodes.contains_key(&nodename) {
>> + bail!("node '{}' already exists", nodename);
>
> nit:
>
> bail!("node '{nodename}' already exists");
>

ACK

>> + }
>
> [snip]
>
> we are reading only, consider using a slice for `nodenames` here (just
> like for `remove_resource_from_nodes`):
>
> fn add_resource_to_nodes(&mut self, sid: &str, nodenames: &[&str]) -> Result<(), Error> {
>
> pls find the related changes [0] and [1].
>

Right, that makes more sense, will go for that!

[...]

>> +#[test]
>> +fn test_no_duplicate_nodes() -> Result<(), Error> {
>> + let mut usage = Usage::new();
>> +
>> + usage.add_node("node1".to_string(), NodeStats::default())?;
>> +
>> + match usage.add_node("node1".to_string(), NodeStats::default()) {
>> + Ok(_) => bail!("cluster usage does allow duplicate node entries"),
>> + Err(_) => Ok(()),
>> + }
>
> since this is supposed to be a test case, I would rather assert instead
> of bail, using:
>
> assert!(
> usage
> .add_node("node1".to_string(), NodeStats::default())
> .is_err(),
> "cluster usage allows duplicate node entries"
> );
>

Right, that's more appropriate, will do so here and for all the
following, thanks!