Repository navigation
Extract shared cgroup locator for quota commands - #37
Merged
Merged
Conversation
2 tasks done
feinburger244
self-requested a review
October 30, 2026 03:08
Quota commands get the job cgroup or the login search list from one call, and cgroupAncestors reads /proc/self/cgroup itself, so the walk can no longer receive an unvalidated path. After the #34 review removed cgroupAncestors' duplicate ".." check, the walk stopped only if callers validated first. For a path above the root, such as "/../x", filepath.Dir reaches "/" without ever equaling the root, and the loop appends until the process runs out of memory. Ruled out: - Restoring the parent == current guard. It can't trigger once the walk validates its own input. - Having findSlurmJobCgroup call cgroupAncestors. Login scope selection also needs the ancestor list, so callers would read /proc/self/cgroup twice. #34 and #35 call currentCgroupScope from selectCPUContext and selectMemoryContext. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A job_<n> cgroup anywhere in the path counted as a Slurm job. A user's user@<uid>.service subtree is delegated to them, so they can create app.slice/job_1, or even slurmstepd.scope/job_1, inside it. Quota would then skip the login cap. Only root can create cgroups under system.slice. slurmd runs as a host systemd service (infra-config ansible/roles/slurmd) with CgroupPlugin=cgroup/v2 (kubernetes/wato-slurm/slurmctld.tf). Slurm 24.05.3, built by slurm-dist v0.0.13 from WATonomous/slurm@417362ec, names the scope system.slice/slurmstepd.scope and job directories job_<id> (src/plugins/cgroup/v2/cgroup_v2.c:64-65,1330). Recheck this when upgrading Slurm or moving slurmd off the host. Upstream later made the slice configurable (CgroupSlice, SchedMD/slurm@3267cde307f8) and named job directories by SLUID (SchedMD/slurm@de2b2dd9189d). If the layout stops matching, quota falls back to the login path: it measures only the current cgroup and reports no allocation. Ruled out: - SLURM_JOB_ID and other env vars. Without LaunchParameters=use_interactive_step, a bare salloc leaves the shell on the login node with SLURM_JOB_ID set. - scontrol pidinfo. slurmd answers from the same cgroup membership, the caller still needs the job's path, and it fails without scontrol or slurmd. - Matching only the parent directory name. Users can create slurmstepd.scope in their own subtree too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
currentCgroupPath reads the file passed as procSelfCgroup, but its "not found" error always said /proc/self/cgroup. The read error is left as is, since os.ReadFile's error already includes the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
llllrui
pushed a commit
that referenced
this pull request
Oct 30, 2026
Integrate PR #37 at db15fe2 without rewriting existing review history. Alvin's shared locator now reads and validates /proc/self/cgroup once and trusts only jobs immediately under /system.slice/slurmstepd.scope. Select CPU context through currentCgroupScope so delegated job-like names cannot bypass the enclosing user's tighter cap. The existing error fixtures used /system.slice/job_42, which is no longer a valid Slurm layout. Update those fixtures to exercise the same invalid cpuset and cpu.max errors under the actual host hierarchy. Add regression coverage for three delegated lookalike job paths under a user manager; each must retain the outer user's 1-CPU limit instead of the child limit. CPU sampling, allocation arithmetic and throttling accounting are unchanged. Validation on Go 1.24.4, macOS arm64: gofmt and git diff --check clean; uncached full tests (quota coverage 60.8%), uncached race tests, go vet, native build and Linux amd64 CLI cross-build all pass. Shared locator files match db15fe2 exactly. The current integration has not been run on live WATcloud hosts; earlier live CPU validation predates this locator change.
llllrui
pushed a commit
that referenced
this pull request
Oct 30, 2026
Integrate the updated CPU dependency at b8ac7b8, including Alvin's PR #37 scope API and trusted host Slurm layout, while preserving existing review history. The shared locator and all CPU files match that base exactly; PR #35 remains limited to memory reporting and its tests. Select the job or bounded login scope through currentCgroupScope. Add three regression cases for job-like paths inside a delegated user manager subtree: a child 4-GiB limit must not mask the outer user's 2-GiB limit. Working-set arithmetic, allocation states and output are unchanged. Validation on Go 1.24.4, macOS arm64: gofmt and git diff --check clean; uncached full tests (quota coverage 78.6%), uncached race tests, go vet, native build and Linux amd64 CLI cross-build all pass. The build includes the complete reviewed locator and CPU dependency chain. This modified memory CLI has not been run or installed on WATcloud hosts; Linux runtime and live multi-node Slurm behavior remain untested.
2 tasks done
feinburger244
force-pushed
the
refactor/shared-cgroup-locator
branch
from
October 30, 2026 17:35
db15fe2 to
38ab020
Compare
Contributor
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
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.
Description
CPU (#34) and memory (#35) quota reporting must read the same cgroup: the process's Slurm job, or its login session up to
user-<uid>.slice. As asked in the #34 review, this extracts that locator so it can be reviewed on its own.currentCgroupScopeis the only entry point. It reads/proc/self/cgrouponce and returns either the job cgroup or the cgroups to search for the login cap. Only/system.slice/slurmstepd.scope/job_<id>counts as a Slurm job, because that's where slurmd's cgroup/v2 plugin creates jobs.SLURM_*variables are not used.No command uses it yet. Merge this first, then retarget #34 to
main.Checklist