perf(spurctld): aggregate assoc_mgr scope usage in a single pass - #797
Draft
nikhilsk wants to merge 6 commits into
Draft
perf(spurctld): aggregate assoc_mgr scope usage in a single pass#797nikhilsk wants to merge 6 commits into
nikhilsk wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #797 +/- ##
==========================================
+ Coverage 80.10% 80.26% +0.15%
==========================================
Files 184 184
Lines 87417 88685 +1268
==========================================
+ Hits 70024 71176 +1152
- Misses 17393 17509 +116 🚀 New features to boost your workflow:
|
…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.
scope_usage computed grp_running_jobs, grp_submitted_jobs, grp_running_tres and every per-user figure with its own walk over the whole jobs map, so assoc_mgr_info cost grew as O(scopes x users x jobs). Fold all figures for a scope into one pass over its in-scope jobs, keyed by user, and render from the aggregate — the cost is now O(scopes x jobs). The node dimension is deduped per aggregate (group union and each user's union kept separately) via a shared RunningTresAccumulator, which sum_running_tres now also uses, so the distinct-node rule and the placement-missing fallback the admission gate relies on are counted identically everywhere.
The excluded-scope check went through a node-count proxy and its message claimed the user should not appear, when alice legitimately appears via her in-scope jobs. Assert the user set and her in-scope running count instead, which is what the case is actually about.
nikhilsk
force-pushed
the
perf/assoc-mgr-single-pass
branch
from
September 2, 2026 08:03
8d6314b to
eec6aca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
scontrol show assoc_mgrrecomputed each scope's usage statistics by walking thejob table once per statistic, per user. Cost grew as scopes x users x jobs, so on
a cluster with many users under one QOS an operator-facing read did far more work
than the data required. This aggregates each scope in a single pass over its
in-scope jobs, bucketing per user as it goes, so cost is scopes x jobs.
Approach
The subtlety is that the Node TRES dimension counts distinct occupied nodes, not
the sum of per-job node requests, and it falls back to the job's requested count
when placement is missing. Summing that per statistic is easy to get wrong once the
walks are merged, so the node-union logic is extracted into a
RunningTresAccumulatorthat owns the distinct-node semantics and theplacement-missing fallback.
sum_running_treson the admission path now uses thesame accumulator, so the operator view and the scheduler cannot drift apart.
Each user gets their own accumulator, so two of a user's jobs sharing a node still
count as one node for that user while the group figure unions across everyone.
Testing
cargo clippy --workspace --exclude spur-ffi --all-targets --lockediswarning-free and
cargo test --lockedpasses.Added
scope_usage_single_pass_matches_per_statistic_computation, which builds afixture with several users, two jobs sharing a node, an unplaced job, a
pending-only user, and a job in an excluded scope, then compares the new
implementation against the old per-statistic walks kept in the test as a reference.
Literal assertions pin the distinct-node semantics independently of that reference,
so the test still fails if both implementations drift the same way.
Note for reviewers
This is stacked on #746 and opened as a draft against
main, so the diffcurrently includes that PR's commits. Once #746 merges I will rebase and only the
two commits here will remain.