Code review · T3, sind SLURM clusters in CI¶
review-code on autopilot/T3, head 4f7c60c, 2026-10-28. Reviewers: code-reviewer and security-reviewer, independent, in parallel, started by the orchestrator. Format: README.md. The full reports are held by the orchestrator; this record lists each finding with its status.
| Round | Head | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 4f7c60c |
changes (1 blocker, 2 majors, 7 minors) | approve (0 blockers, 0 majors, 4 minors) |
| 2 | 41428f5 (the round-1 fixes, delta 4f7c60c..41428f5) |
approve (0 blockers, 0 majors, 5 minors) | approve (0 blockers, 0 majors, 3 minors) |
| 3 | 0fb993b (the follow-ups on autopilot/T3-followups, delta 646a6eb..0fb993b) |
approve (0 blockers, 0 majors, 4 minors) | approve (0 blockers, 0 majors, 4 minors) |
The round-1 fixes are in 53c287d, plus 09ed12b, which CI made necessary: run 40679302148 showed that spx's token with X-SLURM-USER-NAME: root gets 200 on GET ping, so the impersonation check now tests the effect, a DELETE of a held job of root's; then 05fa754 and e2a9f6d, which make the CR-T3-06 job count work on slurm-mini, which has no accounting (runs 40679709039 and 40680311800). All were checked by shellcheck -x, bash -n, the fixture guard test and CI run 40680736055; round 2 reviewed them as a delta and approved. The round-2 test follow-ups are fixed on autopilot/T3-followups in 3d01ea5 and e5011c3, proved by run 40684851443. The code-reviewer also checked that 41428f5 changes no script, so the green run covers the final code, and that the 24 committed fixtures equal the run's artifacts after the $REPO substitution. Test command, in CI (sind needs cgroup v2):
bash dev/bootstrap.sh && bash dev/slurm/cluster.sh up site && \
for s in nextflow:happy,oom_fatal probes heads; do bash tests/slurm/scenarios.sh "$s"; done
CODE step=T3 review=approve b/M/m=1/2/16 security=approve b/M/m=0/0/11 rounds=3 simplify=notrun -0/+0 tests=pass
| Id | Finding | Status |
|---|---|---|
| CR-T3-01 (blocker) | The fixtures were in artifacts only; nothing was committed under tests/fixtures/slurm/ |
fixtures of run 40680736055 committed under tests/fixtures/slurm/slurm-26.05.5/, with the runner's checkout path replaced by $REPO; the headers of scenarios.sh and the workflow name that directory |
| CR-T3-02 (major) | Not every §3.5 failure class and pending reason is produced | partly fixed in 53c287d: one of the two-CPU jobs must pend with Resources and one with Priority; scontrol hold gives JobHeldAdmin; a job cancelled by root must read CANCELLED by 0 (cancelled_external), and the cancelled ids are in sacct-probes. Deferred to TRIAGE3: head OUT_OF_MEMORY, head TIMEOUT, PREEMPTED, BOOT_FAIL (likely unreachable on sind), and oom_once_retry on SLURM (oom_fatal covers the same injection) |
| CR-T3-03 (major) | §4.1 asks for partition gpu with fake GRES and, on slurm-mini, the signal-drain scenario |
deferred to TRIAGE3; the §4.1 rows mark both as planned |
| CR-T3-04 (minor) | rest checked only that jobs answers 200, and accepted any non-200 code without a token |
53c287d: every GET must answer 200, no token must give exactly 401; 09ed12b: spx's token with X-SLURM-USER-NAME: root must not cancel root's held job |
| CR-T3-05 (minor) | The fixture job was the first not-COMPLETED row, which can be a job the engine cancelled; an empty job filter wrote an empty fixture |
53c287d: the first FAILED, OUT_OF_MEMORY, TIMEOUT or NODE_FAIL row, else the first row; fixture.py exits 1 when --jobs keeps nothing |
| CR-T3-06 (minor) | A scenario could pass with no Slurm job at all | 53c287d: at least one job since the scenario started, and at least one not-COMPLETED job for every scenario but happy; 05fa754, e2a9f6d: on slurm-mini, which has no accounting, the jobs are counted with squeue and only happy may run |
| CR-T3-07 (minor) | The NODE_FAIL latency was printed after later waits; sinfo was captured before worker-2 was back |
53c287d: latency recorded right after the wait; sinfo waits until worker-2 is idle (at most 120 s) |
| CR-T3-08 (minor) | cluster.sh up copied a JDK path without checking it |
53c287d: fails with "no JDK found" |
| CR-T3-09 (minor) | The Progress page did not say where T3 departs from the spike | Progress page, What changed, "Compared with the spike" |
| CR-T3-10 (minor) | The workflow's push paths missed dev/lib.sh and dev/bootstrap.sh |
53c287d |
| SEC-T3-01 (minor) | The tools that the nodes run (PATH of env.sh) live in the data directory, which jobs inside the cluster can write |
deferred to TRIAGE3; T4 has the same convention (the repository mounted into kind nodes) |
| SEC-T3-02 (minor) | Same as CR-T3-04, from the authentication side | fixed with CR-T3-04 |
| SEC-T3-03 (minor) | The credential guard missed token headers and munge credentials, had no test, and report.json went to the artifact unguarded |
53c287d: pattern adds X-SLURM-USER-TOKEN and MUNGE:…; tests/slurm/test_fixture.py (4 cases, run by the workflow) proves a made-up JWT exits 1 and writes nothing; report.json is no longer uploaded |
| SEC-T3-04 (minor) | SLURM_DATA goes into sed and the sind config unchecked |
53c287d: cluster.sh accepts only ^/[A-Za-z0-9._/-]+$ |
| CR-T3-11 (minor), SEC-T3-05 (minor) | The impersonation check has no positive control: a DELETE that never works also leaves root's job pending, and the HTTP code is not asserted |
fixed in 3d01ea5 and e5011c3, run 40684851443: a held job of spx's, deleted with spx's own name, must answer 200 and end CANCELLED (it answers 200, CANCELLED; since round 3 an unreadable state fails, CR-T3F-01); then the root case must not be 2xx and root's job must stay PENDING (511, PENDING); the squeue reads use --states=all and cannot fail, and the cleanup scancel runs after them and cannot fail either, so set -e cannot skip the die |
| CR-T3-12 (minor) | Accounting is detected by probing sacct, so a slurmdbd hiccup on slurm-site silently takes the slurm-mini path |
fixed in 3d01ea5, run 40684851443: [ "$CLUSTER" = mini ] |
| CR-T3-13 (minor) | The §11 row missed two trigger paths; the gpu deferral had no reason and was called out of scope |
fixed: §11 row, Progress page Deferred and Open questions |
| CR-T3-14 (minor) | This record lacked severities, the round-2 row and oom_once_retry |
fixed |
| CR-T3-15 (minor) | actions/upload-artifact at the pinned SHA warns about Node 20 |
known warning; the pin is shared with every workflow, so it moves with the next action update, not here |
| SEC-T3-06 (minor) | Schedulers §3 does not say whose token the slurmrestd transport uses, or that a 2xx is no proof of identity (difference 14) | TRIAGE3 |
| SEC-T3-07 (minor) | The fixture guard test covers 2 of its 5 patterns, never scans the committed fixtures, and runs outside ci-ok |
fixed in 3d01ea5, run 40684851443: one sample per SECRET alternative (a JWT, SLURM_JWT, X-SLURM-USER-TOKEN, PRIVATE KEY, MUNGE:…; the test fails if the pattern gains an alternative without a sample), test_committed_fixtures_clean over tests/fixtures/slurm/**/*.json, and a unit job in ci.yml that ci-ok needs |
| CR-T3F-01 (minor), SEC-T3F-03 (minor) | Round 3: the positive control accepted an empty squeue answer as "purged", so a failed squeue passed |
fixed: the own job must read CANCELLED |
| CR-T3F-02 (minor) | Round 3: a failing fixture scan would print the matched credential (the re.Match repr) into the CI log |
fixed: the test asserts on the match span only |
| CR-T3F-03 (minor) | Round 3: the Progress page said the unit job had passed before it ever ran |
fixed: "runs in the unit job of ci.yml on the follow-ups PR" |
| CR-T3F-04 (minor, simplify) | Round 3: errors() handled a failure twice (try/except and \|\| true) |
fixed: the try/except is gone |
| SEC-T3F-01 (minor) | Round 3: errors() prints slurmrestd text into public CI logs, relying on slurmrestd never echoing a credential |
fixed: the output goes through fixture.py's SECRET and matches print as <redacted> (checked by hand with a made-up JWT) |
| SEC-T3F-02 (minor) | Round 3: the negative case changes both the name and the job, so it proves "spx cannot act as root" but not that the header itself was refused | stays as is: the asserted property is the security one; the Progress page reads the 511 from the answer body (error 1007, SLURM_PROTOCOL_AUTHENTICATION_ERROR, mapped to 511 at src/common/http.c:148 on slurm-26.05), which is not asserted |
| SEC-T3F-04 (minor) | Round 3: the test docstring promised more than the scan covers (only tests/fixtures/slurm/**/*.json) |
fixed: the docstring says "SLURM fixture"; a repository-wide credential scan is left to the security workflow (gitleaks) |