Re: [pve-devel] [PATCH perl-rs 1/1] pve-rs: resource_scheduling: allow granular usage changes

[Fiona Ebner <f.ebner@proxmox.com>]

Am 30.09.25 um 4:21 PM schrieb Daniel Kral:
> Implements a simple bidirectional map to track which service usages have
> been added to nodes, so that these can be removed later individually.
> 
> The StaticServiceUsage is added to the HashMap<> in StaticNodeInfo to
> reduce unnecessary indirection when summing these values in
> score_nodes_to_start_service(...).
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
> I started out adding and removing the service_usage in StaticNodeUsage
> on each add_service_usage_to_node(...) and remove_service_usage(...)
> call individually, but I think summing those up every time is better for
> numerical stability..

This can go into the commit message too if worded a bit less informally ;)

> 
>  .../bindings/resource_scheduling_static.rs    | 98 ++++++++++++++++---
>  pve-rs/test/resource_scheduling.pl            | 84 ++++++++++++++--
>  2 files changed, 160 insertions(+), 22 deletions(-)
> 
> diff --git a/pve-rs/src/bindings/resource_scheduling_static.rs b/pve-rs/src/bindings/resource_scheduling_static.rs
> index 7cf2f35..535dfe3 100644
> --- a/pve-rs/src/bindings/resource_scheduling_static.rs
> +++ b/pve-rs/src/bindings/resource_scheduling_static.rs
> @@ -16,8 +16,16 @@ pub mod pve_rs_resource_scheduling_static {
>  
>      perlmod::declare_magic!(Box<Scheduler> : &Scheduler as "PVE::RS::ResourceScheduling::Static");
>  
> +    struct StaticNodeInfo {
> +        name: String,
> +        maxcpu: usize,
> +        maxmem: usize,
> +        services: HashMap<String, StaticServiceUsage>,
> +    }
> +
>      struct Usage {
> -        nodes: HashMap<String, StaticNodeUsage>,
> +        nodes: HashMap<String, StaticNodeInfo>,
> +        service_nodes: HashMap<String, Vec<String>>,

Rather than Vec it could be a HashSet, but I'm fine with either way.

>      }
>  
>      /// A scheduler instance contains the resource usage by node.

---snip---

> @@ -67,10 +75,25 @@ pub mod pve_rs_resource_scheduling_static {
>  
>      /// Method: Remove a node from the scheduler.
>      #[export]
> -    pub fn remove_node(#[try_from_ref] this: &Scheduler, nodename: &str) {
> +    pub fn remove_node(#[try_from_ref] this: &Scheduler, nodename: &str) -> Result<(), Error> {
>          let mut usage = this.inner.lock().unwrap();
>  
> -        usage.nodes.remove(nodename);
> +        if let Some(node) = usage.nodes.remove(nodename) {
> +            for (sid, _) in node.services.iter() {
> +                match usage.service_nodes.get_mut(sid) {
> +                    Some(nodes) => {
> +                        nodes.retain(|node| !node.eq(nodename));
> +                    }
> +                    None => bail!(
> +                        "service '{}' not present while removing node '{}'",

Nit: for a user (or developer in 6 months time :P), this error message
is rather unspecific. I'd suggest:
"internal error: ... not present in service_nodes hash map while ..."

> +                        sid,
> +                        nodename
> +                    ),
> +                }
> +            }
> +        }
> +
> +        Ok(())
>      }
>  
>      /// Method: Get a list of all the nodes in the scheduler.
> @@ -93,22 +116,55 @@ pub mod pve_rs_resource_scheduling_static {
>          usage.nodes.contains_key(nodename)
>      }
>  
> -    /// Method: Add usage of `service` to the node's usage.
> +    /// Method: Add service `sid` and its `service_usage` to the node.
>      #[export]
>      pub fn add_service_usage_to_node(
>          #[try_from_ref] this: &Scheduler,
>          nodename: &str,
> -        service: StaticServiceUsage,
> +        sid: &str,
> +        service_usage: StaticServiceUsage,

So this requires a versioned Breaks in d/control. Please mention this in
the commit notes and in the cover letter.

>      ) -> Result<(), Error> {
>          let mut usage = this.inner.lock().unwrap();
>  
>          match usage.nodes.get_mut(nodename) {
>              Some(node) => {
> -                node.add_service_usage(&service);
> -                Ok(())
> +                if node.services.contains_key(sid) {
> +                    bail!("service '{}' already added to node '{}'", sid, nodename);
> +                }
> +
> +                node.services.insert(sid.to_string(), service_usage);
>              }
>              None => bail!("node '{}' not present in usage hashmap", nodename),
>          }
> +
> +        if let Some(nodes) = usage.service_nodes.get_mut(sid) {
> +            nodes.push(nodename.to_string());
> +        } else {
> +            usage
> +                .service_nodes
> +                .insert(sid.to_string(), vec![nodename.to_string()]);
> +        }

Nit: just to be sure, we could check whether the node would be duplicate
in service_nodes too (is more straightforward with using HashSet). Maybe
not really worth it though, because if we keep the mappings node ->
services and service -> nodes consistent with each other (which we of
course should) then the check above already protects us.

---snip---

> @@ -50,7 +65,54 @@ sub test_balance {
>  	    is($nodes[1], "A", 'second should be A');
>  	}
>  
> -	$static->add_service_usage_to_node($nodes[0], $service);
> +	$static->add_service_usage_to_node($nodes[0], $sid_names[$i], $service);

Nit: could also just use something like "vm:$i" or "vm:" . 100 + $i
rather than letters. If it's a variable-controlled enumeration that
seems more natural to me.

> +    }
> +}
> +

---snip---

> @@ -96,9 +158,9 @@ sub test_balance_small_memory_difference {
>      $static->add_node("C", 4, 8_000_000_000);
>  
>      if ($with_start_load) {
> -	$static->add_service_usage_to_node("A", { maxcpu => 4, maxmem => 1_000_000_000 });
> -	$static->add_service_usage_to_node("B", { maxcpu => 2, maxmem => 1_000_000_000 });
> -	$static->add_service_usage_to_node("C", { maxcpu => 2, maxmem => 1_000_000_000 });
> +	$static->add_service_usage_to_node("A", "a", { maxcpu => 4, maxmem => 1_000_000_000 });
> +	$static->add_service_usage_to_node("B", "b", { maxcpu => 2, maxmem => 1_000_000_000 });
> +	$static->add_service_usage_to_node("C", "c", { maxcpu => 2, maxmem => 1_000_000_000 });
>      }
>  
>      my $service = {
> @@ -106,6 +168,7 @@ sub test_balance_small_memory_difference {
>  	maxmem => 16_000_000,
>      };
>  
> +    my @sid_names = ("d" .. "z");
>      for (my $i = 0; $i < 20; $i++) {
>  	my $score_list = $static->score_nodes_to_start_service($service);
>  
> @@ -131,12 +194,13 @@ sub test_balance_small_memory_difference {
>  	    die "internal error, got $i % 4 == " . ($i % 4) . "\n";
>  	}
>  
> -	$static->add_service_usage_to_node($nodes[0], $service);
> +	$static->add_service_usage_to_node($nodes[0], $sid_names[$i], $service);

Nit: same as above regarding service names, we're already very close to
the edge of the alphabet in this case ;P

>      }
>  }
>  
>  test_basic();
>  test_balance();
> +test_balance_removal();
>  test_overcommitted();
>  test_balance_small_memory_difference(1);
>  test_balance_small_memory_difference(0);



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel