Conversation
|
Thanks for the contribution!
中文感谢你的贡献!
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches host-level setup that runs with sudo on production Slurm allocations (RDMA/QoS preflight and hugepage reclamation), a human look would still be worthwhile.
What was reviewed: the new RDMA/QoS/DCQCN preflight in check-rdma.sh (checked that nicctl failures now hard-fail instead of the base script's graceful skip); the hugepage reclaim logic in setup.sh (checked it only shrinks nr_hugepages to used+reserved and never restores the prior pool size); the new VRAM-drain gate's default 10% threshold for MI355X native-single-node runs; and the hard dependency of benchmark_lib.sh on sourcing common.sh via return 1 on failure.
Extended reasoning...
The PR refactors srt-slurm cluster setup hooks: extracts wait_for_amd_gpu_clean into a shared common.sh, adds a new sudo-invoked RDMA/QoS/DCQCN preflight and a hugepage-reclaim step for MI355X, and documents the hook convention in AGENTS.md and docs/configuration-procedures.md. It touches no auth/crypto surface but does run privileged host commands (sudo nicctl, writing /proc/sys/vm/nr_hugepages) on shared Slurm nodes, which is the security/operational-sensitive part. No findings were reported by the bug hunt, but the change is host-setup code with sudo and irreversible-looking side effects (hugepage pool shrink with no restore), so a human familiar with the cluster's operational risk should still confirm intent before merge.
This review covers commit 9d89131, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
Folded into #2542. |
Moves the srt-slurm cluster hook convention from #2542 onto #3352.
runners/srt-slurm/hooks/<cluster>/setup.shholds per-allocation host checks and setup; the cluster profile invokes it throughdefault_host_setup.runners/srt-slurm/hooks/common.shholds shared host-check functions.wait_for_amd_gpu_cleanmoves there frombenchmark_lib.sh, which now sources it.launch_mi355x-amds.shpassesGITHUB_WORKSPACEto the profile renderer for the hook path.AGENTS.mdanddocs/configuration-procedures.md.default_bash_preambletohooks/mi300x-amd/setup.sh. The hook fails when firmware is older than 177 instead of settingHSA_NO_SCRATCH_RECLAIMinside every container. The cluster's nodes report 192.