Skip to content

fix: harden GPU runtime convergence - #1396

Open
dholt wants to merge 1 commit into
masterfrom
dholt/release-26.09-blockers
Open

fix: harden GPU runtime convergence#1396
dholt wants to merge 1 commit into
masterfrom
dholt/release-26.09-blockers

Conversation

@dholt

@dholt dholt commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

Several GPU-cluster paths diverged as newer DGX OS, Slurm, and Ubuntu releases
changed their service, module, package, and security defaults. In combination,
these gaps prevent a complete Slurm deployment with monitoring and Pyxis from
converging and validating reliably.

Changes

  • Select the correct SSH service when applying the Slurm login GPU guard.
  • Transition DGX systems safely from legacy nv_peer_mem to the in-tree
    nvidia_peermem module.
  • Make exclusive-job detection custom-prefix aware and fail closed when Slurm
    queries fail or return malformed data.
  • Install a command-scoped AppArmor profile for Enroot instead of changing the
    host-wide unprivileged-user-namespace policy.
  • Select and verify the DGX OS 7 DCGM 4 package on Ubuntu 24.04 and newer.
  • Put custom-prefix Slurm client binaries on the exporter container's PATH as
    read-only mounts.
  • Avoid repeating a direct-login driver test after Slurm has intentionally
    hidden GPUs from SSH; allocated srun validation remains authoritative.

This incorporates and hardens the useful changes proposed in #1389 through
#1395. The GPU power/clock batching from #1393 remains separate because it is
an optimization rather than part of this correctness fix.

Validation

  • python3 -m unittest discover scripts/validation/tests — 25 tests passed,
    including failed, empty, malformed, exclusive, and last-user Slurm query
    cases.
  • Full Ansible role lint passed with ansible-lint 26.1.1.
  • Syntax checks passed for the Slurm, Kubernetes, monitoring, GPU, and MAAS
    playbooks.
  • Public-content sanitization and the complete non-live release QA pipeline
    passed.
  • Exact commit bc571ff9 passed a fresh Ubuntu 24.04 GPU-backed Slurm
    deployment and its immediate convergence rerun. The final playbook completed
    with failed=0; the validator proved direct-login GPU isolation, allocated
    srun GPU access, a populated custom-prefix Slurm exporter scrape with zero
    collector errors, a GPU container through Pyxis/Enroot, and correct
    exclusive/nonexclusive prolog selection.
  • The live run also loaded the command-scoped AppArmor profile while preserving
    the host-wide unprivileged-user-namespace restriction.

All 30 public CI checks pass at the validated commit.

Resolve the release-blocking SSH guard, peer-memory, DCGM, exporter, and Pyxis contracts while preserving standalone component behavior and fail-closed validation.
@dholt
dholt force-pushed the dholt/release-26.09-blockers branch from adbefea to bc571ff Compare September 4, 2026 22:23
@dholt
dholt marked this pull request as ready for review September 4, 2026 23:08
@dholt
dholt requested a review from michael-balint September 4, 2026 23:08
@dholt dholt changed the title Fix GPU runtime convergence across Slurm deployment paths fix: harden GPU runtime convergence Sep 4, 2026
@100milliongold

Copy link
Copy Markdown
Contributor

Thanks for pulling these together, and for the hardening on top — several of the
changes here fix real gaps in what I proposed. Adding apparmor to
pyxis_ubuntu_deps is one: my version would have failed on a minimal install
where apparmor_parser is absent. set -o pipefail, the fail-closed
num_jobs=1 default, and rejecting a zero node count are all stricter than what
I had.

One request on attribution. The body credits #1389#1395, which I appreciate,
but the single squashed commit lists one author, so the contributions won't show
up in git log, git blame, or the contributor graph. Would you consider adding
Co-authored-by: trailers before this merges? For my part:

Co-authored-by: 100milliongold <je.kim@xiilab.com>

#1389, #1390 and #1394 came from a different contributor (@100-JM) — their
address is in those commits if you'd like to include them too.

Happy to close #1391, #1392 and #1395 once this lands. #1393 stays open as you
noted.

@100-JM

100-JM commented Sep 7, 2026

Copy link
Copy Markdown

Following up on the attribution point above for #1389, #1390 and #1394, which this PR incorporates (the ssh_unit selection, the nvidia_peermem probe with the legacy nv_peer_mem transition, and the DGX OS DCGM package handling).

I appreciate the extra hardening — the /sys/module/nv_peer_mem check before unloading and the post-install DCGM assert are both stricter than what I had. Since the squashed commit carries a single author, could you add a Co-authored-by: trailer so the contribution stays visible in git log and the contributor graph? The address matches the DCO sign-off on those commits:

Co-authored-by: 백지명 <wlaud9805@gmail.com>

I'll close #1389, #1390 and #1394 once this lands. #1397 (running the remaining nvidia-smi tasks outside the ssh cgroup) is newer and not covered here, so it stays open.

@lannex

lannex commented Sep 7, 2026

Copy link
Copy Markdown

@dholt
As a fellow developer, I must express my deep disappointment with how the commits in this PR were handled.

Squashing the hard work and original code of multiple contributors (@100milliongold and @100-JM from PRs #1389 through #1395) into a single commit under one author's name goes against the fundamental principles of open-source collaboration.

While the additional hardening work is appreciated, stripping the original authors of their proper credit in the git history and contributor graph is simply unacceptable.

Please respect your fellow contributors and amend this commit to include the requested Co-authored-by trailers before merging. Proper attribution is a core pillar of the open-source community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants