[PowerX] verify Slurm completion and normal llm-d exits / 验证 Slurm 完成与 llm-d 正常退出 - #3052
[PowerX] verify Slurm completion and normal llm-d exits / 验证 Slurm 完成与 llm-d 正常退出#3052edwingao28 wants to merge 5 commits into
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
验证 Slurm 完成状态并让 llm-d 工作进程正常退出，先保存诊断证据。
ec4a58f to
85986f4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
按完整字段解析 Slurm 分配退出码，避免误用 DerivedExitCode。
在现有 CPU 工作流中执行 llm-d 生命周期回归，并跟踪相关脚本变更。
合并 Kimi-K3 B200 前置，并保留所有路径在失败返回前的诊断采集。
将已验证的信号就绪屏障移至共享原生采集前置，避免重放 exec 前的竞态。
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks how success/failure is determined for Slurm jobs and llm-d multi-node workers across every Slurm launcher, and the author's own checklist flags launcher qualification and CODEOWNER sign-off as still pending, a human look would still be worthwhile before merge.
What was reviewed: the new verify_slurm_job_completion sacct-with-scontrol-fallback logic and outcome-file recording in runners/slurm_utils.sh; the llm-d job.slurm/server.sh rework that replaces the scancel-on-marker watcher with a real exit-code marker (finish_llmd_node) and a worker polling loop; and exit-code propagation (SRT_JOB_RC/main_rc) through all 8 Slurm launcher scripts plus the new unit tests covering these paths.
Extended reasoning...
Overview
This PR changes how the InferenceX runner stack decides whether a Slurm job (and, for llm-d, its multi-node workers) truly succeeded, rather than inferring success from "left the queue" or "marker file appeared." Core additions are verify_slurm_job_completion() in runners/slurm_utils.sh (reads sacct, falls back to scontrol show job, writes a receipt file, requires exact COMPLETED/0:0), a rewrite of the llm-d job.slurm/server.sh shutdown protocol (real exit-code marker instead of scancel-on-touch, worker polling instead of wait), and propagation of the verified return code through all 8 Slurm launcher scripts. Tests (test_slurm_utils.py, new test_llmd_lifecycle.py) exercise the sacct/scontrol fallback matrix and the llm-d marker publication/consumption boundary with subprocess-level fixtures.
Security risks
No injection, auth, or data-exposure surface is introduced; this is internal CI/benchmark orchestration logic operating on trusted Slurm/CI infrastructure. The main risk class is correctness/availability (a benchmark job silently reported as failed/succeeded incorrectly), not security.
Level of scrutiny
This warrants more than a quick skim: it touches a shared bash library (slurm_utils.sh) used by every Slurm-based launcher, changes exit-code semantics that downstream CI/reporting depends on, and reworks a fragile shell IPC pattern (file-based markers, EXIT/TERM traps, SIGTERM forwarding) across coordinator/worker roles in server.sh. The PR description itself marks "Actual launcher qualification" and "CODEOWNER sign-off" as pending, which is a direct signal from the author that this hasn't been validated end-to-end on real hardware yet.
Other factors
Test coverage for the new logic is genuinely present and reasonably thorough (parametrized sacct/scontrol race scenarios, marker publish/consume ordering via a printf shim). No outstanding third-party review comments are visible in the timeline, and CODEOWNERS only lists the generic @ InferenceX/core for these paths (no specific blocking owner). Given the breadth (8 launchers + shared lib + multi-node protocol rewrite) and the author's own pending-qualification caveat, a human check-in before merge is the safer default even though the automated hunt turned up nothing actionable.
This review covers commit ec4a58f, which is no longer the latest commit on this pull request; later commits are not covered by it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe3a0d4. Configure here.
| # empties. Ask the controller before judging the final outcome. | ||
| record=$(scontrol show job "$job_id" --oneliner 2>/dev/null) || record="" | ||
| state=$(printf '%s\n' "$record" | sed -n 's/.*JobState=\([^ ]*\).*/\1/p') | ||
| exit_code=$(printf '%s\n' "$record" | tr ' ' '\n' | sed -n 's/^ExitCode=//p') |
There was a problem hiding this comment.
scontrol fallback drops job outcome
High Severity
The scontrol fallback runs scontrol show job "$job_id" --oneliner. scontrol takes options before the command (the rest of this repo uses scontrol show job -o <id>), so --oneliner is often treated as another job id. A non-zero status then does record="", discarding any sacct row. Successful jobs whose accounting is missing or still non-terminal are then judged state=unknown and failed.
Reviewed by Cursor Bugbot for commit fe3a0d4. Configure here.
1 participant
Description
Check terminal allocation status using the allocation ExitCode and preserve diagnostics before returning failure. Publish llm-d completion atomically so successful workers exit normally.
Testing: Linux CPU CI: 360 passed; 51 local Slurm/lifecycle/Kimi tests and nine saved-receipt replays passed.
Pending: Actual launcher qualification and CODEOWNER sign-off.
中文
使用分配的 ExitCode 检查最终状态，并在返回失败前保留诊断。原子发布 llm-d 完成状态，使成功工作进程正常退出。
测试： Linux CPU CI 360 项通过；本地 51 项 Slurm/生命周期/Kimi 测试及 9 项既有回执回放通过。
待完成： 实际启动器资格验证和 CODEOWNER 签核。
Related Issue
Split from #3026; independent of native power collection.
拆分自 #3026；独立于原生功耗采集。
Type of Change
Checklist
perf-changelog.yamland have not edited historical entriesOWNER/MEMBER/COLLABORATOR) has commented/reuse-sweep-runon this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.Note
Medium Risk
Touches Slurm job lifecycle and failure propagation across many cluster launchers and llm-d multinode benchmarks; misclassification could fail good runs or mask bad ones, but changes are heavily tested and mostly affect CI orchestration.
Overview
Adds terminal Slurm verification so benchmark launchers distinguish real
COMPLETEDallocations from cancellations or hidden failures.verify_slurm_job_completioninslurm_utils.shreadssacct(withscontrolfallback), writesslurm_job_*_outcome.txt, and is wired intostream_slurm_job_logand multiplelaunch_*.shscripts so logs and power artifacts are staged before the workflow exits with the verified RC.llm-d no longer
scancels the allocation when the coordinator finishes. The decode coordinator atomically publishes exit status in.bench_done.$SLURM_JOB_ID; workers poll that marker and exit with the same code, whilejob.slurmpropagatessrunfailure without cancelling successful runs.CI power audit uploads include the outcome receipts; changelog gate runs new
test_llmd_lifecycle.py/ Slurm utils tests; docs andperf-changelog.yamlrecord the behavior for several fixed-seq recipes.Reviewed by Cursor Bugbot for commit fe3a0d4. Bugbot is set up for automated code reviews on this repo. Configure here.