Spare localusers from the epilog's killall - #1404
Conversation
41-lastuserjob-ssh checks /etc/slurm/localusers.backup before it touches a user, so operator accounts keep their access when a job ends. 40-lastuserjob-processes does not, yet it is the broader of the two: `killall -9 -u` reaps every process the user owns on the node, including an interactive login shell and its sshd session. An operator who submits from a login shell on a compute node is therefore disconnected the instant the epilog runs, even though that account is listed in localusers.backup precisely so it will not be cut off. Observed on a two-node cluster where the controller is also a compute node. Apply the same guard 41 already uses. Job cleanup is unchanged for every user not in that file, and with ProctrackType=proctrack/cgroup the job's own processes are already reaped by Slurm, so this script is a sweep for strays rather than the primary teardown. 42-lastuserjob-cleanup has the same asymmetry -- it deletes the user's files under /tmp and /dev/shm with no exemption check -- but its blast radius is much smaller, so it is left alone here.
dholt
left a comment
There was a problem hiding this comment.
Protecting local operator sessions is a useful improvement. The new guard needs two corrections:
- The backup is created under
slurm_config_dir, but this check hardcodes/etc/slurm/localusers.backup. Use the configured path. - A missing/unreadable file or other grep error currently falls through to
killall, just like a definite nonmatch. Treat lookup errors as unknown and do not authorize destructive cleanup. Use exact literal account-line matching rather than word/regex matching.
Please cover the default/custom path, match, definite nonmatch and lookup-error cases. The broader job-ownership cleanup issue is tracked separately in #1407; this is not a request to expand this PR into that redesign.
…okup
Three corrections to the localusers guard in the lastuserjob epilog.
The backup is installed at {{ slurm_config_dir }}/localusers.backup
(roles/slurm/tasks/compute.yml), but the guard read the hardcoded
/etc/slurm path, so any site that moves slurm_config_dir consulted a file
that does not exist -- and, with the old control flow, that missing file
was indistinguishable from "user is not listed".
grep exits 1 for a definite nonmatch and >1 when the lookup itself fails.
Both landed in the same else branch, so a missing or unreadable list
authorised `killall -9 -u` for every account. The lookup now records its
status, and only a definite nonmatch reaches the cleanup; a failed lookup
logs and exits without killing anything. `|| lookup_rc=$?` keeps the
non-zero status from tripping the script's `set -e`.
The list holds one account per line and real deployments use accounts
containing dots (`je.kim`), which `grep -w` treats as a regex wildcard: a
job user `je.kim` matched a listed `jeXkim` and was wrongly exempted.
Matching whole lines literally with `-x -F` removes that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both corrections are in 1. Configured pathThe backup is installed at localusers_backup='{{ slurm_config_dir }}/localusers.backup'2. Lookup error is no longer a licence to kill
lookup_rc=0
grep -qxF -- "$SLURM_JOB_USER" "$localusers_backup" || lookup_rc=$?
case "$lookup_rc" in
0) exit 0 ;; # listed
1) : ;; # definitely not listed -> cleanup
*) logger -s -t slurm-epilog \
"localusers lookup failed (grep rc=${lookup_rc}, file=${localusers_backup}); skipping process cleanup"
exit 0 ;;
esac
3. Literal whole-line matching
Decision matrixProduced by rendering the actual template (
Same five cases against the pre-fix script, for contrast:
Regex wildcardList contains
The custom-path row also exercises the first correction: under the old hardcoded path that case read a nonexistent |
dholt
left a comment
There was a problem hiding this comment.
The revision addresses the configured backup path, exact account matching, and the distinction between a definite nonmatch and a lookup error. The reported default/custom-path and error cases cover the requested behavior, and current CI is green.
Approved for this scoped fix. The broader cleanup-ownership follow-up remains in #1407. The original commits will be retained with a merge commit.
What happens
epilog.d/41-lastuserjob-sshchecks/etc/slurm/localusers.backupbefore it touches a user:epilog.d/40-lastuserjob-processesdoes not, even though it is the broader of the two:killall -9 -ureaps every process the user owns on that node. That includes an interactive login shell and thesshdsession carrying it.So an operator listed in
localusers.backup— listed there precisely so their access is not cut off — is disconnected the moment their job's epilog runs.How we hit it
Two-node cluster where the controller is also a compute node, so the account submitting jobs also has a login shell on a node that runs the epilog.
and on the operator's terminal, in that same second:
That
sshdpid is a long-lived login session, not one of the short-lived connections the job itself opened.There is a second symptom nearby. Node Health Check runs
check_ps_service -u root -d sshd sshdand fails on these nodes:It fails every interval, not only after a job, so it is a separate problem and is not addressed here. We have not established why yet: on these nodes both
ssh.socketandssh.servicereport active, so the simple "socket activation leaves no daemon" explanation does not hold. Mentioned only so it is not mistaken for a consequence of this change.The change
Apply the guard
41already uses, unchanged in form.Cleanup behaviour is identical for every user not in
localusers.backup. And withProctrackType=proctrack/cgroupthe job's own processes are already reaped by Slurm, so this script is a sweep for strays that escaped the cgroup rather than the primary teardown — skipping it for a handful of operator accounts does not leave the node dirty.Verified
Applied to both nodes of the cluster above:
Not changed
42-lastuserjob-cleanuphas the same asymmetry — it removes the user's files under/tmpand/dev/shmwith no exemption check. Its blast radius is much smaller thankillall -9, so it is left alone here rather than widened into this PR. Happy to follow up if you would rather the three scripts agree.