feat(spur-cli): add scontrol show assoc_mgr for limits against live usage - #746
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #746 +/- ##
==========================================
+ Coverage 80.10% 80.24% +0.14%
==========================================
Files 184 184
Lines 87417 88579 +1162
==========================================
+ Hits 70024 71076 +1052
- Misses 17393 17503 +110 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds an operator-facing “limits vs. live usage” view to Spur’s Slurm-compatible surface by introducing a new controller RPC and wiring it through scontrol show assoc_mgr, with docs describing the new output shape and semantics.
Changes:
- Add
GetAssocMgrInfotoSlurmControllerand newAssocMgr*proto messages for reporting scope + per-user usage and caps. - Implement controller-side aggregation across live job table + accounting caches, and render Slurm-shaped
assoc_mgroutput inspur-cli. - Document the new command in the monitoring guide and note compatibility/differences in the Slurm migration guide.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/slurm.proto | Adds new RPC/messages for assoc_mgr live limits/usage snapshot. |
| docs/user-guide/monitoring-jobs.rst | Documents scontrol show assoc_mgr usage, output, and interpretation. |
| docs/migration-from-slurm.rst | Notes compatibility contract and intentional divergences vs Slurm. |
| crates/spurctld/src/server.rs | Implements get_assoc_mgr_info RPC and proto conversion helpers. |
| crates/spurctld/src/limits_cache.rs | Exposes QosCache::all() to enumerate defined QOS for reporting. |
| crates/spurctld/src/cluster.rs | Computes assoc_mgr usage records from live jobs + cached definitions; adds tests. |
| crates/spurctld/src/association_cache.rs | Exposes AssociationCache::all() to enumerate defined associations for reporting. |
| crates/spur-core/src/accounting.rs | Adds cap/usage structs and exceeded-cap comparison logic; adds TresRecord::types() + tests. |
| crates/spur-cli/src/sinfo.rs | Extends mock RPC surface to include get_assoc_mgr_info for CLI tests. |
| crates/spur-cli/src/scontrol.rs | Adds scontrol show assoc_mgr rendering and related unit tests. |
| crates/spur-cli/src/sacctmgr.rs | Makes blank_if_unset reusable for consistent rendering. |
| crates/spur-cli/src/mock_controller.rs | Extends mock controller implementation with get_assoc_mgr_info. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
shiv-tyagi
left a comment
There was a problem hiding this comment.
Posted comments, PTAL
One additional note (no line in this diff): GetAssocMgrInfo should be added to the diagnostic RPC exclusion list in crates/spurctld/src/rpc_middleware.rs alongside GetSchedStats, or operator polling of scontrol show assoc_mgr will skew RPC latency stats.
yansun1996
left a comment
There was a problem hiding this comment.
Nice feature — the operator gap it closes is real, and computing the verdict controller-side next to the admission checks (rather than re-deriving it in the CLI) is the right call. A few things worth a look before merge.
Authorization on the new RPC. GetAssocMgrInfo doesn't consult the caller's identity — the request's user is used purely as a filter, and an empty value means "no filter". The scope lines (Grp*, plus every QOS and account name) aren't narrowed by the filter at all, so any caller can enumerate cluster-wide account/QOS inventory and per-user consumption. get_jobs handles this with Self::authoritative_user(&mut req.user, __identity.as_ref()), and list_tokens gates enumeration on require_admin. Would it be worth following the get_jobs shape here, so a non-admin is scoped to themselves and admins can still pass an arbitrary user?
A couple of display/semantics mismatches — details inline: the scope line's MaxTRESPU= renders empty where its siblings render N; a non-users= selector is silently taken as a literal username; and the docs' "a literal 0 is a real cap" holds for count caps but not for TRES dimensions, where 0 is treated as unset.
Tests. Three gaps: scope_exceeded_caps_covers_the_group_caps_only can't fail as written (the user's caps are all None, so exceeded_caps() is empty for any implementation); the CLI tests all call render_assoc_mgr directly, so the assoc_mgr dispatch arm, the users= strip and the LimitsReadable banner are untested; and limits_readable == true is never asserted.
Also worth considering as follow-ups: GrpWall, MaxTRESPerJob and MaxSubmitJobsPerAccount are enforced but absent from the record, so a job pended by QosGrpWallLimit shows nothing here.
The LimitsReadable=NO wording points already raised below look right to me, and the same overstatement applies when accounting is disabled entirely — the refresh loop only starts on a successful DB connect, so the banner prints permanently and reads as a fault rather than "there are no caps".
|
Following up on the last paragraph of my earlier review — #744 now defines the predicate this banner should be using: |
|
reply to 3876320563-followup / ISSUE-COMMENT (LimitsReadable + #744) Implemented the per-cache predicate now: I did not add the |
yansun1996
left a comment
There was a problem hiding this comment.
Re-reviewed the authorization path end to end: the filter now comes from the verified identity rather than the request field, an empty caller isn't treated as privileged, and scopes the filtered user has no part in are dropped rather than enumerated. The limits_readable predicate and its descriptions agree now. Approving.
870c8ab to
654050d
Compare
…sage Operators had no way to see how much of a QOS or association a user holds against its caps; the figures existed only inside the admission gate, which recomputes them per candidate job and keeps nothing queryable. Counting squeue rows was the only recourse, and it cannot show a cap already being exceeded. Add a GetAssocMgrInfo RPC on the controller, since only the controller sees the live job table, and aggregate per (user, QOS) and (user, account) using the same sum_running_tres the gate uses, so a record reads as the scheduler sees it rather than as a second opinion. The cap comparison lives in spur-core beside the limits themselves, next to the admission checks it deliberately differs from: those project a candidate job onto current usage, this reports what already stands over a cap. Records come from the queue, not from the accounting definitions, so an unused QOS is absent — sacctmgr show qos lists definitions. A cold accounting cache reports LimitsReadable=NO instead of letting an unreadable cap look absent.
Slurm's assoc_mgr prints a cap and its consumption in one Limit(Consumed) field with N for no limit, dumps every association and QOS its cache holds rather than only those with jobs, and nests per-user limits inside the record they qualify. Existing scripts parse that shape, so diverging from it is a cost paid by every tool that already reads Slurm. Render caps as Limit(Consumed) per count and per TRES dimension, listing the union of dimensions capped and in use. Build records from the accounting definitions as well as the queue, so a cap on a QOS nobody is using stays visible, which needs enumerate-all accessors on both caches. Restructure a record as a scope with users nested under it, which also stops a scope's group figures being repeated once per user. A QOS caps every user identically, so those caps are stated once on the scope and remain visible when no one is using it; an association's are per (user, account) and ride on each user instead. Caps and consumption stay separate fields on the wire so a machine consumer never has to take a display string apart.
The GetAssocMgrInfo handler took `user` straight from the request as a filter, and an empty value meant "no filter", so any caller could read every user's live job counts and TRES holdings. scope_usage narrowed only the per-user list, never the Grp* lines or the QOS/account names, so even a filtered call still enumerated the cluster-wide scope inventory. Scope the read the way get_jobs does: a privileged caller (an admin, or an unauthenticated one under permissive/disabled, the same treatment viewer_is_privileged gives) reads whichever user it names, or every user when it names none; a non-admin authenticated caller is pinned to its own identity. scope_usage now drops a scope entirely when the filtered user holds no work and has no defined association there, so a non-admin can neither read another tenant's usage nor enumerate the QOS/account inventory. A blanket require_admin would be wrong: permissive mode has identity == None, which the existing convention deliberately treats as privileged. Correct the read path while it is in hand: limits_readable is now !accounting_enabled() || (qos_loaded && assoc_loaded), evaluated per cache, so an accounting-off cluster reports readable (it has no caps to load) instead of standing at LimitsReadable=NO forever, and a split cache still reads as incomplete. The association records also read each (user, account) limit from the rows association_cache.all() already returned rather than re-locking the cache once per user.
- Render MaxTRES*= as N when the scope sets no per-user TRES cap, matching its count siblings instead of an empty field a parser splitting on = would read as missing. - Reject an unknown <key>= selector (e.g. qos=highprio) rather than taking it as a literal username and printing an empty result; users=<name> and a bare name still work. The parse moved into a small testable helper. - Soften the LimitsReadable=NO banner: it prints only when accounting is enabled but a cache is cold, where some caps below may be missing rather than all, and is suppressed otherwise. - Dedupe the tres_limit_consumed dimension union with sort+dedup rather than O(n^2) Vec::contains. A BTreeSet is not usable here: TresType is not Ord, and ordering it would sort by discriminant instead of by name. - proto: split the drifted comment block so AssocMgrRecord and AssocMgrCaps each carry their own, and correct the TRES-rendering and limits_readable wording (comments only, no field change). - accounting: give types() and format() their own doc comments, and give the group-caps-only test a per-user cap it is within so an empty result proves the group caps were not leaked onto the user. - docs: narrow the Limit(Consumed) and literal-0 claims (0 is a real count cap but an unset TRES dimension), correct the LimitsReadable wording, and note that an unprivileged caller is scoped to its own associations.
654050d to
d07c9c0
Compare
What this adds
scontrol show assoc_mgr: how much of a QOS or association each user is holding right now, next to the caps that govern them, and which caps are already exceeded.Operators had no way to see this. The figures existed only inside the admission gate, which recomputes a user's running count and TRES per candidate job and keeps nothing queryable, so counting
squeuerows was the only recourse — and that cannot show a cap already being exceeded. Usage over a cap is not a contradiction: caps apply when a job is admitted and running jobs are never re-checked, so tightening a cap under running work leaves exactly that state, silently.Approach
A new
GetAssocMgrInfoRPC onSlurmController— the controller is the only side that sees the live job table, while the accounting service owns the definitions. Aggregation runs per(user, QOS)and(user, account)through the samesum_running_tresthe gate uses, so node figures are distinct-node occupancy and CPU follows each job's request. A record reads as the scheduler sees it rather than as a second opinion that could quietly disagree.The cap comparison lives in
spur-corebeside the limits, not in the CLI, so the view does not re-implement enforcement semantics (including that a TRES dimension capped at0is not a cap). It sits next to the admission checks it deliberately differs from: those project a candidate job onto current usage, this reports what already stands over a cap.Following Slurm's shape
The output follows Slurm's
assoc_mgrwhere that shape is a contract other tooling already depends on:Limit(Consumed)in one field,Nfor no limit —node=4(6)is a cap of four with six in use. Scripts in the wild parse exactly this (splitting on the parenthesis), so rendering caps and usage as separate fields would have broken them for no gain. Applied per count and per TRES dimension, listing the union of dimensions capped and in use.User Limits=does. This also stops a scope's group figures from being repeated once per user, which the first draft did.Where it diverges, deliberately: users are listed on their own lines rather than inside a single
User Limits=field (more readable and greppable), an account's group figures are stated once for the account instead of per association row, and Slurm'saccounts=/qos=/flags=selectors and fairshare internals (SharesRaw,UsageRaw,Lft-Rgt,GrpTRESMins) are not included. All of this is in the migration guide.Design choices
(user, account), so they ride on each user's line instead, andscope_capsis absent there.Limit(Consumed)display is the client's job; a machine consumer should not have to take a display string apart again.Capis spelled neutrally and the server picks the wording. A QOS caps withMaxJobsPU/MaxTRESPU, an association with plainMaxJobs— same breach, different name. Deciding it controller-side means every consumer, REST included, reads one verdict.LimitsReadable=NOrather than letting an unreadable cap look like an absent one. Usage stays accurate; only the caps are missing.sacctmgr'sblank_if_unset(promoted topub(crate)) instead of copying it, so a cap renders identically in both commands.Proto
Additive only — a new RPC plus
AssocMgrRecord,AssocMgrUserRecord,AssocMgrCaps,GetAssocMgrInfoRequest,GetAssocMgrInfoResponse. No existing message, field, or tag is touched.Testing
Twenty new tests:
spur-core(7) — the cap comparison andTresRecord::types(): over the job-count cap, exactly at a cap (a ceiling is not a threshold), unset caps ignored, each TRES dimension compared the way the gate compares it, a zero dimension treated as uncapped, group caps reported from the scope and never duplicated onto users, and dimension ordering stable.spurctld(9) — through the real cluster manager with jobs driven to running: per-user records against a QOS cap with the breach named, a QOS nobody is using still reported with its caps, a QOS the cache no longer knows kept while its jobs run, distinct-node counting, one-user filtering that leaves group totals intact, usage reported when caps could not be read, QOS and account records kept apart with each hierarchy's own caps, a defined association with no jobs still reported, and nothing at all for unscoped work.spur-cli(7) — the rendered layout:Limit(Consumed)withN, association labels without the per-user suffix, an idle scope with caps and no users,OverLimitomitted when compliant, a group breach on the scope line, an explicitly marked empty section, a zero cap rendering as0, and TRES dimensions drawn from either side.Full
cargo test --locked(3,318 tests),cargo clippy --workspace --exclude spur-ffi --all-targets --lockedwarning-free,cargo fmt --all -- --checkclean. Docs: a new section in the monitoring guide and a compatibility row in the migration guide.