Skip to content

fix(accounting): filter users by name - #447

Merged
yansun1996 merged 2 commits into
ROCm:mainfrom
hnotshe:fix/filter-users-by-name
Jul 21, 2026
Merged

fix(accounting): filter users by name#447
yansun1996 merged 2 commits into
ROCm:mainfrom
hnotshe:fix/filter-users-by-name

Conversation

@hnotshe

@hnotshe hnotshe commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix sacctmgr show user name=X returning every configured user.

The CLI now forwards name= and user= through the accounting RPC. ListUsersRequest gains an additive user field, and the accounting query applies exact user and account filters server-side.

Closes #418.

Testing

  • Full Linux validation: formatting, SPDX headers, cargo-deny, clippy, all-target build, and complete test suite
  • Focused sacctmgr parameter regression test
  • PostgreSQL integration test covering user-only, account-plus-user, and no-match filtering
  • Independent reviewer: PASS

@hnotshe
hnotshe marked this pull request as ready for review July 15, 2026 23:25
@yansun1996

Copy link
Copy Markdown
Member

Clean, correct, and backward-compatible. The new proto field user = 2 is a genuinely free tag (additive, no wire-compat break), the list_users query rewrite preserves the columns, DISTINCT ON, and ORDER BY, empty-string from existing callers maps to None so list-all is preserved, and the filters are parameter-bound. A couple of minor Slurm-parity notes inline.

Comment thread crates/spur-cli/src/sacctmgr.rs
Comment thread crates/spurctld/src/accounting/db.rs
Comment thread crates/spur-cli/src/sacctmgr.rs
yansun1996
yansun1996 previously approved these changes Jul 21, 2026
@yansun1996

Copy link
Copy Markdown
Member

Rebased onto the latest main — there were a few conflicts in the accounting tests where recent changes to add_user (association limits) and list_users overlapped with this PR's signature changes; resolved by keeping both sets of tests and threading the new filter argument through.

Also verified the fix end-to-end on a 2-node cluster with accounting enabled:

  • sacctmgr show user name=<user> returns only that user
  • sacctmgr show user user=<user> (alias form) returns only that user
  • sacctmgr show user name=<user> account=<acct> applies both filters
  • sacctmgr show user account=<acct> returns just that account's users
  • a non-existent name returns an empty result (header only), no error

All green — build, clippy, fmt, and the unit tests pass on the rebased branch.

yansun1996
yansun1996 previously approved these changes Jul 21, 2026
@codecov-commenter

codecov-commenter commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 15.32258% with 105 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
- Coverage   71.70%   71.62%   -0.08%     
==========================================
  Files         153      153              
  Lines       50708    50797      +89     
==========================================
+ Hits        36359    36380      +21     
- Misses      14349    14417      +68     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yansun1996
yansun1996 merged commit b3e16a1 into ROCm:main Jul 21, 2026
14 checks passed
yansun1996 added a commit that referenced this pull request Jul 21, 2026
A test call site added in #447 passed 7 args to upsert_account while #477
concurrently added the grp_tres parameter. Both PRs were green in isolation
but collide on main, breaking compilation of the spurctld test target.
yansun1996 added a commit to yansun1996/spur that referenced this pull request Jul 23, 2026
upstream/main added a `user` filter field to ListUsersRequest (ROCm#447) after
this branch diverged; the reconciler's all-users lookup needs to pass an
empty filter to keep its "empty account/user = all" behavior.
yansun1996 added a commit to yansun1996/spur that referenced this pull request Jul 24, 2026
upstream/main added a `user` filter field to ListUsersRequest (ROCm#447) after
this branch diverged; the reconciler's all-users lookup needs to pass an
empty filter to keep its "empty account/user = all" behavior.
yansun1996 added a commit that referenced this pull request Jul 28, 2026
… + scoped kubeconfig) (#475)

* wip(quota): M1 account->k8s projection module (Namespace/ResourceQuota/LimitRange/RBAC)

Pure, unit-tested mapping from a SPUR account allocation (grp_tres) to the native
k8s objects that enforce it. 6 tests green. Foundation for the quota policy
reconciler; DB/proto persistence + controller + `kubeconfig --user` follow.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(quota): quota policy reconciler in the operator (M1 increment 3)

Makes the quota layer live: an opt-in reconcile loop that projects every SPUR
account into its native k8s objects and drift-corrects them.

- quota_controller::run: connects a SlurmAccountingClient to spurctld, then every
  30s lists accounts (with grp_tres) + members and server-side-applies (force, so
  an admin hand-edit is reverted) the Namespace + ResourceQuota + LimitRange +
  Role + RoleBinding from crate::quota.
- build_account_quota: pure AccountInfo + members -> AccountQuota (unit-tested).
- apply<K>: generic SSA helper (injects apiVersion/kind, which k8s-openapi types
  don't serialize but server-side apply requires).
- Wired into main.rs behind --enable-quota (default off; opt-in policy plane),
  spawned with the same run_with_retry backoff as the other controllers.

Verified: build + spur-k8s tests (146) + fmt + clippy -D warnings. The apply/RPC
path is integration-level (needs a cluster); the mapping + build logic are unit-tested.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(quota): spur k8s kubeconfig --user — scoped ServiceAccount kubeconfig (M1 increment 4)

A SPUR user gets a namespace-scoped credential into their account's namespace,
without an admin ever handing out the cluster-admin kubeconfig.

- spur-core::quota_names: the per-account namespace + per-user ServiceAccount
  naming, shared so the operator (which creates them) and spurctld (which mints
  the kubeconfig) agree. Moved out of spur-k8s::quota.
- proto: ClusterKubeconfigRequest.user; GetAdminKubeconfigRequest gains
  user/namespace/service_account (reuses the existing RPC — no new trait method).
- spurd: K0sAgent::user_kubeconfig ensures the SA exists + mints a bound token via
  `k0s kubectl create token`, then templates a scoped kubeconfig from the admin
  cluster CA/server (pure parse + template helpers, unit-tested).
- spurctld: cluster_kubeconfig resolves the user's account (association cache) ->
  namespace + SA and forwards to the control-plane agent.
- cli: `spur k8s kubeconfig --user <u>`.

Verified (no cluster run, per request): build + tests + fmt + clippy -D warnings
across all crates incl. spurd on Linux (shark-a). The SA/token minting is
integration-level; the naming, CA/server parse, and kubeconfig template are unit-tested.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(quota): correct mem units, DNS name cap, and fail-closed paths from review

- quota_hard: TRES mem is base-10 MB; emit `M` not `Mi` (Mi over-allocated ~4.86%).
- quota_names: include the `spur-acct-`/`spur-user-` prefix in the 63-char DNS cap
  so long account/user names produce a valid namespace/ServiceAccount name.
- limit_range: drop the default *limit* (keep the default request); a forced small
  limit would reject ordinary pods that omit limits — the ResourceQuota bounds usage.
- build_account_quota: fail closed on a non-empty-but-unparseable grp_tres instead of
  silently uncapping the namespace; the reconciler skips such an account.
- cluster_kubeconfig: fail closed when associations aren't loaded (the cache resolves
  fail-open, which could mint an unscoped token).
- tests: mem `M`, prefixed-name length, sanitizer-collision, malformed-grp_tres.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(quota): fail closed on empty inputs in user_kubeconfig

* fix(quota): cap only requests.*, validate account grp_tres, and batch user lookup in reconciler

* test(spurctld): cover scoped-kubeconfig user resolution; align LimitRange memory units

Extract the scoped-kubeconfig resolve-or-reject step into a pure
resolve_user_namespace_sa helper and unit-test its fail-closed paths
(cold association cache -> Unavailable, unassociated user -> NotFound)
and the namespace/ServiceAccount derivation.

Use base-10 128M for the LimitRange default memory request so it matches
the base-10 units the ResourceQuota already emits.

* refactor(proto): rename GetAdminKubeconfig RPC to GetKubeconfig

The RPC now serves both the cluster-admin kubeconfig (empty user) and a
scoped per-user token (user set), so the "admin" name is misleading and
easy to misread as an admin-only credential path. Rename the RPC and its
request/response messages while the surface is still internal and pre-1.0,
avoiding a wire-breaking change after it ships.

* style(spur-k8s): rustfmt after rebase

Formatting drift introduced by auto-merging quota.rs during the rebase
onto latest upstream/main; no functional change.

* fix(quota): pass user filter to ListUsersRequest after rebase

upstream/main added a `user` filter field to ListUsersRequest (#447) after
this branch diverged; the reconciler's all-users lookup needs to pass an
empty filter to keep its "empty account/user = all" behavior.

* fix(spur-k8s): avoid clippy doc-lazy-continuation false positive

Leading "+ " on a wrapped doc-comment line was parsed as a markdown list
item, tripping clippy::doc_lazy_continuation under -D warnings in CI.

---------

Co-authored-by: powderluv <anush@nod-labs.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sacctmgr show user name=X silently ignores the name filter

3 participants