Re: [PATCH proxmox v2 04/40] resource-scheduling: introduce generic scheduler implementation

Dominik Rusovac
2026-03-26T10:19:24Z

lgtm

pls find my comments inline, mostly nits.

On Tue Mar 24, 2026 at 7:29 PM CET, Daniel Kral wrote:
> The existing score_nodes_to_start_resource(...) function is dependent on
> the StaticNodeUsage and StaticServiceUsage structs.
>
> To use this function for other usage stats structs as well, declare
> generic NodeStats and ResourceStats structs, that the users can convert
> into. These are used to make score_nodes_to_start_resource(...) and its
> documentation generic.
>
> The pve_static::score_nodes_to_start_service(...) is marked as
> deprecated accordingly. The usage-related structs are marked as
> deprecated as well as the specific usage implementations - including
> their serialization and deserialization - should be handled by the
> caller now.
>
> This is best viewed with the git option --ignore-all-space.
>
> No functional changes intended.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> This patch was "[RFC proxmox 2/5] resource-scheduling: introduce generic
> cluster usage implementation" in the RFC v1. Sorry for the ambigious
> naming with the next patch!
>
> changes v1 -> v2:
> - add more information to the patch message
> - split out `NodeStats` and `ResourceStats` to their own modules, which
> will also be used in an upcoming patch for the `Usage` implementation
> - add deprecation note to pve_static::score_nodes_to_start_service()
> - add deprecation attribute to other major pve_static items as well
> - impl `Add` and `Sum` for ResourceStats, which will be used for the
> resource bundling in pve-rs later
> - eagerly implement common traits (especially Clone and Debug)
> - add test cases for the
> scheduler::Scheduler::score_nodes_to_start_resource()

[snip]

good to have:

#[derive(Clone, Debug)]

> +pub struct Scheduler {
> + nodes: Vec<NodeUsage>,
> +}
> +

nit: The implementation of `Scheduler` is totally fine as-is. This is
just my two cents, as this was mentioned off-list.

I believe that for the implementation of the scheduler working with
enum variants and a trait and then exploiting static dispatch is more
convenient and easier to maintain, e.g.:

pub enum Schedulerr<Nodes> {
Topsis(Nodes),
BruteForce(Nodes),
}

pub trait Decide {
fn node_imbalance(&self) -> f64;

fn node_imbalance_with_migration_candidate(&self, candidate: &MigrationCandidate) -> f64;

fn score_best_balancing_migration_candidates(
&self,
candidates: &[MigrationCandidate],
limit: usize,
) -> Result<Vec<ScoredMigration>, Error>;
}

impl Decide for Schedulerr<Vec<NodeUsage>> {
fn node_imbalance(&self) -> f64 {
match self {
Self::Topsis(nodes) | Self::BruteForce(nodes) => {
calculate_node_imbalance(nodes, |node| node.stats.load())
}
}
}

fn node_imbalance_with_migration_candidate(&self, candidate: &MigrationCandidate) -> f64 {
match self {
Self::Topsis(nodes) | Self::BruteForce(nodes) => {
calculate_node_imbalance(nodes, |node| {
let mut new_stats = node.stats;

if node.name == candidate.migration.source_node {
new_stats.remove_running_resource(&candidate.stats);
} else if node.name == candidate.migration.target_node {
new_stats.add_running_resource(&candidate.stats);
}

new_stats.load()
})
}
}
}

fn score_best_balancing_migration_candidates(
&self,
candidates: &[MigrationCandidate],
limit: usize,
) -> Result<Vec<ScoredMigration>, Error> {
match self {
Self::Topsis(nodes) => {
let len = nodes.len();

let matrix = candidates
.iter()
.map(|candidate| {
let resource_stats = &candidate.stats;
let source_node = &candidate.migration.source_node;
let target_node = &candidate.migration.target_node;

let mut highest_cpu = 0.0;
let mut squares_cpu = 0.0;
let mut highest_mem = 0.0;
let mut squares_mem = 0.0;

for node in nodes.iter() {
let new_stats = {
let mut new_stats = node.stats;

if &node.name == source_node {
new_stats.remove_running_resource(resource_stats);
} else if &node.name == target_node {
new_stats.add_running_resource(resource_stats);
}

new_stats
};

let new_cpu = new_stats.cpu_load();
highest_cpu = f64::max(highest_cpu, new_cpu);
squares_cpu += new_cpu.powi(2);

let new_mem = new_stats.mem_load();
highest_mem = f64::max(highest_mem, new_mem);
squares_mem += new_mem.powi(2);
}

// Add 1.0 to avoid boosting tiny differences: e.g. 0.004 is twice as much as 0.002, but
// 1.004 is only slightly more than 1.002.
PveTopsisAlternative {
average_cpu: 1.0 + (squares_cpu / len as f64).sqrt(),
highest_cpu: 1.0 + highest_cpu,
average_memory: 1.0 + (squares_mem / len as f64).sqrt(),
highest_memory: 1.0 + highest_mem,
}
.into()
})
.collect::<Vec<_>>();

let best_alternatives = topsis::rank_alternatives(
&topsis::Matrix::new(matrix)?,
&PVE_HA_TOPSIS_CRITERIA,
)?;

Ok(best_alternatives
.into_iter()
.take(limit)
.map(|i| {
let imbalance =
self.node_imbalance_with_migration_candidate(&candidates[i]);

ScoredMigration::new(candidates[i].clone(), imbalance)
})
.collect())
}
Self::BruteForce(_) => {
let mut scored_migrations = candidates
.iter()
.map(|candidate| {
let imbalance = self.node_imbalance_with_migration_candidate(candidate);

// NOTE: could avoid clone if Migration had additional score field
Reverse(ScoredMigration::new(candidate.clone(), imbalance))
})
.collect::<BinaryHeap<_>>();

let mut best_migrations = Vec::with_capacity(limit);

// BinaryHeap::into_iter_sorted() is still in nightly unfortunately
while best_migrations.len() < limit {
match scored_migrations.pop() {
Some(Reverse(alternative)) => best_migrations.push(alternative),
None => break,
}
}

Ok(best_migrations)
}
}
}
}

pub fn score_best_balancing_migration_candidates(
scheduler: &impl Decide,
candidates: &[MigrationCandidate],
limit: usize,
) -> Result<Vec<ScoredMigration>, Error> {
scheduler.score_best_balancing_migration_candidates(candidates, limit)
}

In a nutshell, this declares what a scheduler ought to be able to do to be used
for scoring (that is, implementing the `Decide` trait); and implements
all the functionality for all the variants in one place.

Nice side-effects of this design:
* one scoring function implements all the variants in one place, which is nice, I think
* adding/removing a scheduler variant would become more systematic
* modifying scheduler variants in terms of how they score or, for example, how they measure
imbalance, would also be more straightforward

Again, just my two cents.

> +impl Scheduler {
> + /// Instantiate scheduler instance from node usages.
> + pub fn from_nodes(nodes: I) -> Self
> + where
> + I: IntoIterator<Item: Into<NodeUsage>>,
> + {
> + Self {
> + nodes: nodes.into_iter().map(|node| node.into()).collect(),
> + }
> + }
> +
> + /// Scores nodes to start a resource with the usage statistics `resource_stats` on.
> + ///
> + /// The scoring is done as if the resource is already started on each node. This assumes that
> + /// the already started resource consumes the maximum amount of each stat according to its
> + /// `resource_stats`.
> + ///
> + /// Returns a vector of (nodename, score) pairs. Scores are between 0.0 and 1.0 and a higher
> + /// score is better.
> + pub fn score_nodes_to_start_resource<T: Into<ResourceStats>>(
> + &self,
> + resource_stats: T,
> + ) -> Result<Vec<(String, f64)>, Error> {
> + let len = self.nodes.len();
> + let resource_stats = resource_stats.into();
> +
> + let matrix = self
> + .nodes
> + .iter()
> + .enumerate()
> + .map(|(target_index, _)| {
> + // Base values on percentages to allow comparing nodes with different stats.
> + let mut highest_cpu = 0.0;
> + let mut squares_cpu = 0.0;
> + let mut highest_mem = 0.0;
> + let mut squares_mem = 0.0;
> +
> + for (index, node) in self.nodes.iter().enumerate() {
> + let mut new_stats = node.stats;
> +
> + if index == target_index {
> + new_stats.add_started_resource(&resource_stats)
> + };
> +
> + let new_cpu = new_stats.cpu_load();
> + highest_cpu = f64::max(highest_cpu, new_cpu);
> + squares_cpu += new_cpu.powi(2);
> +
> + let new_mem = new_stats.mem_load();
> + highest_mem = f64::max(highest_mem, new_mem);
> + squares_mem += new_mem.powi(2);
> + }
> +
> + // Add 1.0 to avoid boosting tiny differences: e.g. 0.004 is twice as much as 0.002, but
> + // 1.004 is only slightly more than 1.002.
> + PveTopsisAlternative {
> + average_cpu: 1.0 + (squares_cpu / len as f64).sqrt(),
> + highest_cpu: 1.0 + highest_cpu,
> + average_memory: 1.0 + (squares_mem / len as f64).sqrt(),
> + highest_memory: 1.0 + highest_mem,
> + }
> + .into()
> + })
> + .collect::<Vec<_>>();
> +
> + let scores =
> + topsis::score_alternatives(&topsis::Matrix::new(matrix)?, &PVE_HA_TOPSIS_CRITERIA)?;
> +
> + Ok(scores
> + .into_iter()
> + .enumerate()
> + .map(|(n, score)| (self.nodes[n].name.to_string(), score))
> + .collect())
> + }
> }

[snip]

in the future, the proptest crate could come in handy for such helper
functions.

in general, I would propose to add proptests in the future.

for some examples and inspiration, see [0].

[0] https://lore.proxmox.com/all/20260306082046.34311-1-d.rusovac@proxmox.com/T/

> +fn new_homogeneous_cluster_scheduler() -> Scheduler {
> + let (maxcpu, maxmem) = (16, 64 * (1 << 30));
> +
> + let node1 = NodeUsage {
> + name: String::from("node1"),
> + stats: NodeStats {
> + cpu: 1.7,
> + maxcpu,
> + mem: 12334 << 20,
> + maxmem,
> + },
> + };
> +
> + let node2 = NodeUsage {
> + name: String::from("node2"),
> + stats: NodeStats {
> + cpu: 15.184,
> + maxcpu,
> + mem: 529 << 20,
> + maxmem,
> + },
> + };
> +
> + let node3 = NodeUsage {
> + name: String::from("node3"),
> + stats: NodeStats {
> + cpu: 5.2,
> + maxcpu,
> + mem: 9381 << 20,
> + maxmem,
> + },
> + };
> +
> + Scheduler::from_nodes(vec![node1, node2, node3])
> +}
> +
> +fn new_heterogeneous_cluster_scheduler() -> Scheduler {
> + let node1 = NodeUsage {
> + name: String::from("node1"),
> + stats: NodeStats {
> + cpu: 1.7,
> + maxcpu: 16,
> + mem: 12334 << 20,
> + maxmem: 128 << 30,
> + },
> + };
> +
> + let node2 = NodeUsage {
> + name: String::from("node2"),
> + stats: NodeStats {
> + cpu: 15.184,
> + maxcpu: 32,
> + mem: 529 << 20,
> + maxmem: 96 << 30,
> + },
> + };
> +
> + let node3 = NodeUsage {
> + name: String::from("node3"),
> + stats: NodeStats {
> + cpu: 5.2,
> + maxcpu: 24,
> + mem: 9381 << 20,
> + maxmem: 64 << 30,
> + },
> + };
> +
> + Scheduler::from_nodes(vec![node1, node2, node3])
> +}

[snip]

> +#[test]
> +fn test_score_homogeneous_nodes_to_start_resource() -> Result<(), Error> {
> + let scheduler = new_homogeneous_cluster_scheduler();
> +
> + let heavy_memory_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 1.0,
> + mem: 0,
> + maxmem: 12 << 30,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, heavy_memory_resource_stats)?,
> + vec!["node2", "node3", "node1"]
> + );
> +
> + let heavy_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 12.0,
> + mem: 0,
> + maxmem: 0,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, heavy_cpu_resource_stats)?,
> + vec!["node1", "node3", "node2"]
> + );
> +
> + let unlimited_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 0.0,
> + mem: 0,
> + maxmem: 0,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, unlimited_cpu_resource_stats)?,
> + vec!["node1", "node3", "node2"]
> + );
> +

nit: confusing variable name

> + let unlimited_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 12.0,
> + mem: 0,
> + maxmem: 12 << 30,
> + };
> +
> + assert_eq!(

nit: confusing variable name

> + rank_nodes_to_start_resource(&scheduler, unlimited_cpu_resource_stats)?,
> + vec!["node2", "node3", "node1"]
> + );
> +
> + Ok(())
> +}
> +
> +#[test]
> +fn test_score_heterogeneous_nodes_to_start_resource() -> Result<(), Error> {
> + let scheduler = new_heterogeneous_cluster_scheduler();
> +
> + let heavy_memory_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 1.0,
> + mem: 0,
> + maxmem: 12 << 30,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, heavy_memory_resource_stats)?,
> + vec!["node2", "node1", "node3"]
> + );
> +
> + let heavy_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 12.0,
> + mem: 0,
> + maxmem: 0,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, heavy_cpu_resource_stats)?,
> + vec!["node3", "node2", "node1"]
> + );
> +
> + let unlimited_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 0.0,
> + mem: 0,
> + maxmem: 0,
> + };
> +
> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, unlimited_cpu_resource_stats)?,
> + vec!["node1", "node3", "node2"]
> + );
> +

nit: confusing variable name

> + let unlimited_cpu_resource_stats = ResourceStats {
> + cpu: 0.0,
> + maxcpu: 12.0,
> + mem: 0,
> + maxmem: 12 << 30,
> + };
> +

nit: confusing variable name

> + assert_eq!(
> + rank_nodes_to_start_resource(&scheduler, unlimited_cpu_resource_stats)?,
> + vec!["node2", "node1", "node3"]
> + );
> +
> + Ok(())
> +}