Repository navigation
docs(skills): [Pre-flight] Add cluster-network-topology skill and scripts - #615
Merged
Merged
Conversation
Complements `mori check`: where env_check answers "is this host configured correctly?" and drives its peer over SSH, this maps the fabric and localizes a failure to a tier from inside a scheduler allocation, where SSH to compute nodes is often unavailable. Bundles the probe/matrix/report scripts referenced by the document. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jhchouuu
reviewed
Sep 3, 2026
jhchouuu
left a comment
jhchouuu
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for putting this together — the methodology in SKILL.md is useful, especially Step 3's tier localization and the discipline of drawing an inferred tier as inferred rather than measured.
I ran the scripts on one of our AMD ionic boxes (8 RoCE rails, MI355X) before commenting. probe_topology.sh works: it correctly paired GPU2→ionic_2 and GPU3→ionic_3 even though the netdev names are transposed on that machine (ionic_2→benic4p1, ionic_3→benic3p1), because it sorts by PCI address rather than by name. That is the right call and it is not obvious. The mgmt NIC was excluded correctly too.
The other two scripts have three problems; details inline.
lcskrishna
marked this pull request as ready for review
September 3, 2026 14:08
Contributor
Author
|
@jhchouuu Thanks for reviewing the PR. I noticed I haven't fully added finished updating this PR. Let me move it to draft mode and we can re-review again. |
lcskrishna
marked this pull request as draft
September 7, 2026 08:07
Rail discovery is now one shared library (rail_detect.sh) sourced by the matrix worker, the fast worker and the probe, so the tools can no longer disagree about a node's rails or GID index. It accepts IPv4-mapped and global IPv6 RoCEv2 GIDs, which is what let the same scripts run unmodified on IPv4 (DigitalOcean MI350X, OCI MI300X) and IPv6-ULA (Crusoe MI355X) fabrics. A zero-rail detection now aborts with a diagnostic instead of an unbound array reference, and the workers fail loudly rather than defaulting the peer to themselves — a silent loopback run reads as a clean pass. xrail_worker.sh writes the result.txt make_report.py consumes; the NxN sweep keeps matrix.txt. make_diagrams.py generates the node and cross-rail diagrams the report previously expected someone to hand-author, and SKILL.md specifies the result.txt grammar the parser requires. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lcskrishna
marked this pull request as ready for review
September 9, 2026 06:31
…kill Three defects found by hardware dry runs on two clusters, each of which produced a wrong answer that looked like a right one: - The target node stopped serving as soon as the RDMA phase ended, so its batch body exited and the scheduler tore down the allocation while the tester was still walking the IP matrix. The partial result.txt still parsed into a confident verdict. The target now waits on an all_done sentinel, and make_report.py flags any result.txt lacking the final DONE marker as INCOMPLETE. - A scheduler that dispatches the batch body twice on one node gave two concurrent appenders writing one report, which parsed as a node with twice as many GPUs. probe_topology.sh now builds in a private temp and renames into place. - The probe ran after the cross-rail test, so on per-node dispatch the peer's probe began exactly at teardown and its report went missing, silently dropping a node from the diagrams. It now runs first. Also: implement probe_topology.sh --peer (was a no-op), emit the address family and GID index in addrs.<node>, fix ping -6 fallback precedence and active_mtu parsing, and document the result.txt grammar in SKILL.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jhchouuu
approved these changes
Sep 9, 2026
jhchouuu
left a comment
jhchouuu
left a comment
Collaborator
There was a problem hiding this comment.
Thanks — all six points look addressed. I ran this on one of our 8× MI355X / 8× ionic node and it works.
LGTM
jhchouuu
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Complements
mori check: where env_check answers "is this host configured correctly?" and drives its peer over SSH, this maps the fabric and localizes a failure to a tier from inside a scheduler allocation, where SSH to compute nodes is often unavailable.Bundles the probe/matrix/report scripts referenced by the document.
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist