fix(spur-sched): allocate CPUs and cap nodes when tasks fewer than nodes

[shiv-tyagi]

What this fixes

CPU-only jobs requesting fewer tasks than nodes (e.g. sbatch -N4 -n1) reserved 0 CPUs per node. The per-node CPU share used floor division num_tasks / num_nodes, which truncates to 0 when tasks are fewer than nodes. The allocation was then invisible to resource tracking, so any number of such jobs stacked on the same nodes, and the job also held nodes it could never place work on. Tracked as SPUR-13.

Approach

• base_node_request computes per-node CPUs with ceiling division and floors at one CPU. The busiest node's share is never under-reserved, and every allocated node consumes tracked capacity.
• submit_job normalizes num_nodes to min(num_nodes, num_tasks) at intake, unless --ntasks-per-node pins the per-node layout. This matches Slurm cons_tres. Because the persisted count equals what is allocated, squeue, scontrol, sacct, and the in-job SLURM_NNODES all stay consistent with the nodelist. The scheduler trusts num_nodes as the single source of truth.

Design choices

• Normalization runs once at controller intake on the leader propose path, not in the scheduler, so reporting and allocation share one value.
• No schema, proto, or config change. num_nodes keeps its type; only its stored value is reduced for affected jobs, so serialized Raft/WAL state stays compatible.
• Pre-upgrade Raft entries replay unchanged (the apply path does not re-normalize). Old jobs may schedule with their original larger count; new submissions get the corrected count. This is non-crashing and acceptable.

Testing

• cargo test --locked: 2521 passed, 0 failed. cargo clippy --workspace --exclude spur-ffi --all-targets --locked: clean.
• New unit tests cover effective_num_nodes, the base_node_request CPU cases, and submit_job normalization.
• Bare-metal (4 nodes, 4 CPUs each) vs Slurm 25.11.7, node-selection cases T1-T7:

Test	Command	Slurm 25.11.7	This PR
T1	-N4	both RUNNING	both RUNNING
T2	-N4 -n1	both RUNNING	both RUNNING
T3	-N4 -n1 -c1	both RUNNING	both RUNNING
T4	-N4 -c4	j2 PENDING	j2 PENDING
T5	-N2 -n1 (x2)	both RUNNING	both RUNNING
T6	-N4 --exclusive	j2 PENDING	j2 PENDING
T7	-N4 --ntasks-per-node=1	both RUNNING	both RUNNING

Reporting: -N4 -n1 now reports NODES=1 / NumNodes=1, matching Slurm.

Follow-ups

• A -N4 -n1 job submitted to a partition with min_nodes>1 now pends as PartitionConfig instead of running. Narrow edge case, arguably correct. Slurm's bump-up-to-min_nodes behavior can be added separately if wanted.

[Copilot]

Pull request overview

This PR fixes a scheduling/accounting bug where CPU-only jobs with fewer tasks than requested nodes (e.g. -N4 -n1) could reserve 0 CPUs per node due to truncating division, making the allocation effectively invisible to resource tracking and allowing oversubscription. It also aligns persisted/reporting node counts with what the scheduler can actually allocate by capping num_nodes at submission time when no explicit per-node layout is provided.

Changes:

• Normalize submitted JobSpec.num_nodes to effective_num_nodes() (caps nodes to tasks unless --ntasks-per-node is set) so persisted specs and reporting reflect allocatable resources.
• Fix per-node CPU reservation to use ceiling division and floor at 1 CPU, preventing “0 CPU” allocations when tasks < nodes.
• Add unit tests covering node normalization and CPU reservation edge cases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
crates/spurctld/src/cluster.rs	Normalizes num_nodes at submit-time and adds tests ensuring persisted specs match allocatable node counts.
crates/spur-sched/src/backfill.rs	Fixes per-node CPU calculation via div_ceil and adds tests for CPU/memory reservation behavior when tasks < nodes.
crates/spur-core/src/job.rs	Introduces JobSpec::effective_num_nodes() and unit tests for node-capping semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.59543% with 27 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
+ Coverage   75.83%   76.19%   +0.36%     
==========================================
  Files         166      166              
  Lines       64696    65235     +539     
==========================================
+ Hits        49058    49701     +643     
+ Misses      15638    15534     -104     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[biluriuday]

The Raft reasoning in the description checks out against the code: normalization runs before expand_job_specs and propose, the JobSubmit apply arm (crates/spurctld/src/cluster.rs:3450-3468) is a pure insert with no re-derivation, followers forward raw requests, and effective_num_nodes is pure and idempotent — so replicas cannot diverge and pre-upgrade entries replay byte-identically. The two test fixups adding num_tasks = 2 (crates/spurctld/src/cluster.rs:8009, 10446) preserve each test's intent rather than weakening assertions, which is the honest way to handle it.

Few comments on the changes. Please take a look

[C1] Clients that don't send a task count silently lose nodes. The reduction is applied unconditionally in submit_job, but by then "the user didn't pass -n" has already been erased and replaced with the literal 1:

crates/spurctld/src/rest/handlers.rs:91-92 — num_nodes: body.job.nodes.unwrap_or(1), num_tasks: body.job.ntasks.unwrap_or(1)
crates/spurctld/src/server.rs:2163-2164 — num_tasks: spec.num_tasks.max(1), so proto3's absent-field 0 becomes 1
crates/spur-ffi/src/types.rs:34 defaults num_tasks: 1 while crates/spur-ffi/src/lib.rs:78 maps min_nodes to num_nodes
So POST {"job": {"nodes": 4, "script": "..."}} with no ntasks allocated 4 nodes before this PR and allocates 1 after, with no error or warning. Same for a C caller that sets min_nodes = 4 and leaves num_tasks alone — the natural thing to do, since real Slurm's slurm_init_job_desc_msg sets num_tasks = NO_VAL meaning "one per node". sbatch escapes this only because effective_ntasks (crates/spur-cli/src/sbatch.rs:799-812) already defaults tasks to the node count. That asymmetry is why cluster testing through the CLI couldn't catch it: the one path that has the information does the right thing, and the three that don't now silently misbehave. These are surfaces AGENTS.md requires to stay Slurm-compatible.

Upstream Slurm handles this in src/sbatch/opt.c, and the guard is the whole point:

} else if (opt.nodes_set && opt.ntasks_set) {
if (opt.ntasks < opt.max_nodes)
opt.max_nodes = opt.ntasks;
if (opt.ntasks < opt.min_nodes) {
warning("can't run %d processes on %d nodes, setting nnodes to %d",
opt.ntasks, opt.min_nodes, opt.ntasks);
opt.min_nodes = opt.max_nodes = opt.ntasks;
}
}
Your empirical NODES=1 observation is correct — Slurm does reduce. But it gates on nodes_set && ntasks_set, it warns, and it runs in the client because that's the only layer that knows which flags the user typed. effective_num_nodes is this logic with the guard removed.

Preferred fix, which keeps the part of your design that's genuinely good: make num_tasks an optional uint32 in proto/slurm.proto (wire-compatible — field number and type unchanged), map None to num_nodes in proto_to_job_spec, and take body.job.ntasks as an Option in REST. Normalization then stays in submit_job where the determinism property holds, but only fires when a task count was actually supplied — fixing REST, gRPC, FFI, and any future adapter at once.

[C2] --gpus=N totals collapse onto a single node. resolve_gpu_demand derives per-node placement from spec.num_nodes (crates/spur-core/src/gpu_request.rs:203), and the allocator spreads a total across assigned nodes (crates/spur-sched/src/backfill.rs:138-146). So --gpus=8 -N4 -n1, which meant 2 GPUs on each of 4 nodes, now demands 8 GPUs on one node — unschedulable on any cluster with 4-GPU nodes. Related: --gpus-per-node=2 -N4 -n1 drops the job's total from 8 GPUs to 2, and --mem drops 4× because effective_memory_mb multiplies by num_nodes (crates/spur-core/src/job.rs:616-627). For a GPU-first scheduler this is the highest-impact consequence of the change, and there's no test for any of it.

[C3] Validation and persistence disagree about the node count. GPU validation runs before the mutation, inside proto_to_job_spec (crates/spurctld/src/server.rs:2353) and at crates/spurctld/src/rest/handlers.rs:105. So -N4 -n1 --gpus=2 is rejected as TotalLessThanNodes { total: 2, nodes: 4 }, even though the spec that would be persisted (1 node, 2 GPUs) is valid and would run. The scheduler then re-resolves the same demand against the normalized count, so two different node counts are in play for one job. The error message is actively misleading.

[I1] The min_nodes case is a silent permanent stall, not a minor follow-up. validate_partition (crates/spurctld/src/cluster.rs:387-414) checks only partition existence and account ACLs — it never looks at node bounds. Those live in partition_limits_allow (crates/spurctld/src/cluster.rs:4665-4673), consulted only by pending-reason tagging. So -N4 -n1 on a MinNodes=4 partition is accepted, shrunk to 1, tagged PartitionConfig, and dropped from scheduling entirely — the behavior your own existing test asserts at crates/spurctld/src/cluster.rs:8062 ("min_nodes-blocked job must be dropped from scheduling"). It never runs and nothing cancels it. I don't think this is acceptable as a follow-up: Slurm rejects at submit with a clear error, and this PR introduces the condition — before it, the job had num_nodes=4 and passed. Accepting a submission you've already determined is unschedulable is a defect. Either clamp with .max(part.min_nodes) or move the bounds check into validate_partition, run it after normalization, and return SubmitError::invalid.

[I2] The user is never told their request was shrunk. crates/spurctld/src/cluster.rs:333 rewrites a user-facing field with no client-visible warning, no distinct log line, and nothing in SubmitJobResponse; the only log is the unconditional info!(job_id, "job submitted"). Slurm prints sbatch: Warning: can't run 1 processes on 4 nodes, setting nnodes to 1 precisely because this surprises people. At minimum emit a warn! with both counts; better, return a warning in SubmitJobResponse and have sbatch/srun/salloc print it.

[I3] div_ceil changes packing for every uneven job, and the PR doesn't mention it. -N2 -n7 -c1 goes from 3 to 4 CPUs per node — 8 CPUs reserved for 7 tasks. Because the per-node figure drives feasibility filtering, a job that genuinely fits can now be rejected: two nodes with 4 and 3 free CPUs can host -N2 -n7, but every candidate must clear the 4-CPU bar. It also desynchronizes accounting, since job_tres charges num_tasks * cpus_per_task = 7 (crates/spurctld/src/cluster.rs:4698-4701) against a reservation of 8. Rounding up is still the right direction versus the old truncation, so this is a net improvement — but it's the wrong model, and the machinery for the right one already exists: tasks_per_node_counts (crates/spur-core/src/gpu_request.rs:153-170) computes exact per-node counts honoring the distribution policy, and GpuDemand::PerNode already threads a per-node vector through the scheduler. CPUs should use the same representation instead of a scalar.

[I4] The Slurm-compat comments name the wrong layer. crates/spurctld/src/cluster.rs:332 says "matching Slurm's submission-time reduction" and crates/spur-core/src/job.rs:601 says "like Slurm's cons_tres". The reduction lives in the sbatch/srun/salloc option verifier, not cons_tres and not slurmctld, and only fires when both flags are explicit.

[I5] PMIx validation silently flips from reject to accept. validate_single_node_pmix is called immediately after the mutation (crates/spurctld/src/cluster.rs:335-337), so -N4 -n1 --mpi=pmix used to error at submit and now passes. Probably the desired outcome, but it's an unstated and untested change to a guardrail.

[I6] --nodelist flips from additive to restrictive. nodelist_is_additive compares the node count to the list length (crates/spur-sched/src/node_match.rs:88-93). -N4 -n1 -w node001,node002 was additive (4 > 2, a floor the scheduler could grow) and is now a hard restriction (1 > 2 is false). The end result is arguably more sensible, but it's an unanalyzed semantic change to a flag the description doesn't mention. Worth a test and a line in the description.

[shiv-tyagi]

Thanks, all addressed in 42268d0; verified with clippy/tests and the bare-metal matrix vs Slurm 25.11.

• C1: Accepted, unset ntasks now defaults to one-per-node on gRPC/REST/FFI (via num_tasks=0 sentinel, not optional, to avoid rippling Option into ~10 internal consumers).
• C2: Accepted, resolved transitively by C1 so GPU/memory compute on the full node count.
• C3: Accepted, GPU validation moved into submit_job after normalization so -N4 -n1 --gpus=2 is no longer wrongly rejected.
• I1: Accepted, node counts outside a partition's Min/MaxNodes are now rejected at submit instead of pending forever.
• I2: Accepted, submit_job returns warnings surfaced over gRPC/REST/CLI plus a warn! log.
• I3: Accepted, replaced the scalar reservation with a per-node CPU vector (-N2 -n7 -> [4,3], verified on hardware) so no over-reservation or wrongful rejection.
• I4: Accepted, comments now name Slurm's sbatch/srun/salloc option verifier as the reducing layer.
• I5: Accepted, added tests for the PMIx flip (-n1 accepted, -n4 rejected).
• I6: Accepted, added a test for the restrictive nodelist after reduction and documented the additive to restrictive shift.

No wire/Raft/config break; one operator note: partition-bound violations that previously pended are now rejected at submit.

[yansun1996]

Thanks @shiv-tyagi — the earlier round looks well handled, and the cross-surface ntasks defaulting (gRPC/REST/FFI all treating an absent ntasks as one-per-node) reads consistently now. No correctness concerns from me.

Two asks before merge:

• The branch conflicts with main now (overlaps the recent submit_job change that landed there) — could you rebase and resolve? The conflict sits right in the intake path, so worth a quick re-run of the tests after.
• A few of the new doc-comments run long — kindly keep comment blocks to a line or two: effective_num_nodes (job.rs), and plan_per_node_alloc / base_node_request / cpu_per_node_counts (backfill.rs), plus the submit_job block (cluster.rs). The fuller Slurm-parity rationale can move to the commit message; just keep the load-bearing why inline.

Minor, non-blocking: the reduction-warning test asserts contains('4') && contains('1'), which would still pass if the two counts were swapped in the message — an exact substring like contains("requested 4 nodes but only 1") would pin it down.

[shiv-tyagi]

Thanks @yansun1996. All three addressed in b491486.

• Rebased onto main and resolved the submit_job conflict (merged the QoS-before-partition ordering that landed there with the normalization pipeline). Re-ran the gate: clippy clean, full test suite green.
• Trimmed the long doc-comments to a line or two each: effective_num_nodes (job.rs), plan_per_node_alloc / base_node_request / cpu_per_node_counts (backfill.rs), and the submit_job block (cluster.rs). The fuller Slurm-parity rationale now lives in the commit message.
• Tightened the reduction-warning test to contains("requested 4 nodes but only 1"), so a swapped-count message fails.

Re-verified the full intake/allocation matrix on the 4-node bare-metal cluster through the real CLI and REST: C1 (CLI and REST both keep 4 nodes when ntasks is absent), the reduction warning surfaces to the client, C3 (-N4 -n1 --gpus=2 accepted and runs on one node), I1 (node count outside a partition's Min/MaxNodes rejected at submit), I3 (-N2 -n7 schedules [4,3] across 2 nodes), I5 (pmix single-node accepted, multi-node rejected), I6 (restrictive nodelist after reduction).

[yansun1996]

LGTM