fix(healthcheck): FI_EFA_IFACE per-device pinning + INSTANCE_TYPE override for in-pod EFA checks - #1221
Draft
KeitaW wants to merge 2 commits into
Draft
fix(healthcheck): FI_EFA_IFACE per-device pinning + INSTANCE_TYPE override for in-pod EFA checks#1221KeitaW wants to merge 2 commits into
KeitaW wants to merge 2 commits into
Conversation
…nexistent FI_EFA_DEVICE_NAME
checks/6-efa-loopback.sh's per-device loop pinned each fi_pingpong
invocation with FI_EFA_DEVICE_NAME="${domain}". That variable does not
exist in libfabric -- confirmed against a running libfabric.so.1
(2.4.0amzn3.0): 0 string occurrences of FI_EFA_DEVICE_NAME/EFA_DEVICE_NAME,
and it is absent from `fi_info -e`'s full FI_EFA_* parameter list (37
entries, FI_EFA_IFACE among them, FI_EFA_DEVICE_NAME not).
libfabric silently ignores unrecognized FI_* env vars -- no warning, no
error, exit 0. Effect measured on 2 idle p6-b200.48xlarge nodes (16 EFA
devices total): every "per-device" iteration actually ran on device 0
(hw-counter attribution: rdmap79s0 ended at 19x a single run's byte count
across 19 total runs; every other device's counters stayed at 0), and a
completely nonexistent device name still returned PASS with real bandwidth
numbers (542 MB/sec) -- proof the per-device claim in a green check 6 was
not backed by anything.
Fix: FI_EFA_IFACE="${domain%-rdm}". FI_EFA_IFACE is a real, honored
libfabric parameter, but it takes the kernel/ibv device name (e.g.
"rdmap80s0"), not the libfabric domain name ("rdmap80s0-rdm") that this
script's own fi_info-based enumeration produces -- so the "-rdm" suffix is
stripped before use.
Also adds a bogus-device negative control before the per-device loop: if a
provably nonexistent device name ever returns PASS again, check 6 now
fails loudly (RESET) instead of silently reporting a green per-device sweep
that isn't one. This is the regression guard for this exact failure class.
Verified (against libfabric 2.4.0amzn3.0, the version in the vllm-uccl-ep
EKS pod image this suite targets -- not re-tested against other EFA
installer versions):
- Counter attribution: FI_EFA_IFACE=<kdev> moves that device's own
tx_bytes/rx_bytes counters and leaves all others at their prior value,
across all 8 devices on 2 nodes (16/16 correctly pinned).
- Negative control: FI_EFA_IFACE=<bogus> -> exit 103,
"util/pingpong.c:571 ctrl/read: no data or remote connection closed".
- bash -n clean; --dry-run path unaffected (dry-run returns before this
code path).
Blast radius: 1 file (checks/6-efa-loopback.sh), 1 function
(run_pingpong_for_domain) plus the enumeration/negative-control block in
run_check(). No infra/state change -- this is a diagnostic shell script.
Behavioral change: check 6 becomes STRICTER, not more lenient -- it now
actually tests the device it claims to test, and can newly FAIL (RESET)
where it previously vacuously PASSed. On real, healthy hardware this is a
no-op (all 16 measured devices on 2 nodes pass under the corrected pinning
too). Rollback: revert this commit; no state or migration involved.
…us in-pod
lib/common.sh unconditionally set INSTANCE_TYPE="" at declare time, which
clobbered any value a caller had pre-exported before sourcing the file.
IMDS is unreachable from a pod running in EKS under default hop-limit
settings (confirmed: IMDSv2 token request returns empty, IMDSv1 fallback
also empty), so detect_instance_type() -> load_instance_profile() ended
with EXPECTED_EFA_COUNT=0. checks/2-efa-enumeration.sh:30 guards its whole
PCI-count comparison on `-n "${EXPECTED_EFA_COUNT}" && -gt 0`, so with
EXPECTED_EFA_COUNT=0 the comparison never runs and check 2 PASSes without
ever comparing anything -- reproduced here by running checks/2 with
INSTANCE_TYPE pre-exported against both the pre-fix and post-fix
lib/common.sh: pre-fix, the pre-exported value is silently dropped and
check 2 [PASS]es with 0 PCI devices detected; post-fix, the same input
correctly reaches the comparison and [FAIL]s (ISOLATE) on the same
0-vs-8 mismatch.
Fix, three parts:
1. lib/common.sh: `INSTANCE_TYPE="${INSTANCE_TYPE:-}"` instead of
`INSTANCE_TYPE=""` -- a pre-exported value now survives being sourced.
load_instance_profile()'s existing `-z` guard before calling
detect_instance_type() already does the right thing once the value
isn't being clobbered first; no changes needed there.
2. lib/common.sh: detect_instance_type() gets a new last-resort branch --
if IMDS/ec2-metadata come up empty and NODE_NAME is set (k8s downward
API `spec.nodeName`) and kubectl is on PATH, fall back to reading the
node's `node.kubernetes.io/instance-type` label. This is the same
fallback kubernetes/agent.sh already implemented locally for its own
use (agent.sh:59-63 prior to this commit) -- centralizing it in
common.sh means every entrypoint that sources common.sh gets it, not
just the DaemonSet agent (e.g. the quarantine Job's direct
gpu-healthcheck.sh --check 2/--check 6 invocations, which previously
had no fallback at all).
3. kubernetes/agent.sh: detect_instance() simplified to just call
detect_instance_type() and no longer reimplements the kubectl
fallback locally (now redundant with lib/common.sh's version).
kubernetes/manifests/05-job-quarantine.yaml: export NODE_NAME in the
Job's inline script so the new common.sh fallback branch has what it
needs. The quarantine Job's ServiceAccount already has `get` on nodes
(02-rbac.yaml:19-20) -- no RBAC change required.
Verified: sourced the patched lib/common.sh directly (bash -n clean) and
called load_instance_profile() with INSTANCE_TYPE pre-exported --
EXPECTED_EFA_COUNT correctly resolves to 8 for p6-b200.48xlarge (from
instance-profiles.conf:18) instead of being reset to 0. Also verified the
unset-INSTANCE_TYPE path still falls through gracefully (no crash) when
IMDS/ec2-metadata/kubectl are all unavailable, matching prior behavior for
that case. Did not test the kubectl-fallback branch itself against a live
cluster (would need actual node access and IMDS-unreachable-in-pod
conditions to observe end-to-end; the branch mirrors agent.sh's
already-working equivalent logic almost verbatim, low risk, but flagging
as not independently re-executed against a real EKS pod in this session).
Blast radius: 3 files. lib/common.sh behavioral change is STRICTER, not
more lenient: callers that inject INSTANCE_TYPE now get a real profile
comparison instead of a silently-skipped one; callers that don't inject
anything and have no IMDS/kubectl path behave identically to before
(graceful "no profile found" default, unchanged). No infra/state change.
Rollback: revert this commit; no state, no migration.
KeitaW
marked this pull request as draft
August 3, 2026 12:58
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
1 participant
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.
Problem 1 — check 6's per-device EFA loopback test is a no-op
checks/6-efa-loopback.shpins each per-devicefi_pingponginvocation withFI_EFA_DEVICE_NAME="${domain}". This environment variable does not exist in libfabric —confirmed against a running
libfabric.so.1(2.4.0amzn3.0): 0 string occurrences, absentfrom
fi_info -e's fullFI_EFA_*parameter list. libfabric silently ignores unrecognizedFI_*variables, so every "per-device" iteration in the loop actually exercises whicheverdevice libfabric selects by default — measured on 2 real p6-b200.48xlarge nodes (16 devices
total): all 8 per-node iterations landed on the same device (hardware-counter attribution:
one device accumulated exactly 19x a single run's byte delta across 19 total runs; every
other device's counters stayed at zero). A nonexistent device name still returns PASS with
real bandwidth numbers — proof the per-device claim in a green check 6 is not backed by
anything.
Fix 1
FI_EFA_DEVICE_NAME="${domain}"→FI_EFA_IFACE="${domain%-rdm}".FI_EFA_IFACEis a real,honored libfabric parameter that takes the kernel/ibv device name, not the libfabric domain
name this script's
fi_infoenumeration produces — hence the suffix strip. Also adds abogus-device negative control before the per-device loop: if a nonexistent device name ever
returns PASS again, the check now fails (RESET) instead of silently reporting a per-device
sweep that isn't one.
Problem 2 — check 2 is vacuous inside an EKS pod
IMDS is unreachable from a pod under EKS default hop-limit settings.
lib/common.shunconditionally reset
INSTANCE_TYPE=""at declare time, which silently dropped any value acaller pre-exported to work around this — so
EXPECTED_EFA_COUNTalways ended up0, andchecks/2-efa-enumeration.sh's PCI-count comparison guard (-n "$EXPECTED_EFA_COUNT" && -gt 0)skipped the comparison entirely. Check 2 PASSed without ever comparing anything, in-pod.
Fix 2
INSTANCE_TYPE="${INSTANCE_TYPE:-}"instead of an unconditional reset, so a pre-exportedvalue survives. Also centralizes
kubernetes/agent.sh's existing kubectl-node-labelIMDS-fallback into
lib/common.sh'sdetect_instance_type()so every caller gets it (notjust the DaemonSet agent) —
kubernetes/agent.shde-duplicated to use the shared version,and
kubernetes/manifests/05-job-quarantine.yamlnow exportsNODE_NAMEso the quarantineJob template's direct check invocations benefit too.
Verification
Fix 1 was validated on 2 idle p6-b200.48xlarge nodes (16 EFA devices, 0 GPU-h) via hardware
counter attribution (pinning moves the targeted device's own counters and no others') and a
negative control (bogus iface name correctly fails, exit 103, vs. the old mechanism's bogus
name incorrectly passing, exit 0). Not re-tested against a different EFA-installer/libfabric
version — version-pinned to 2.4.0amzn3.0.
Fix 2 was reproduced (the vacuous-PASS mechanism) against both pre-fix and post-fix
lib/common.shin a sandbox without IMDS access — pre-fix, a pre-exportedINSTANCE_TYPEissilently dropped and check 2 passes with 0 PCI devices detected against 0 expected; post-fix,
the same input reaches a real comparison. The new kubectl-node-label fallback branch itself
was not exercised against a live cluster in this round — it mirrors
agent.sh'salready-working equivalent logic near-verbatim, so risk is assessed as low, but this is not
the same evidence bar as Fix 1's counter-attribution proof. Please treat Fix 1 and Fix 2 as
having different levels of validation.
Blast radius / rollback
4 files changed, no infra/state changes (sample shell/YAML repo). Both fixes are strictly
stricter, not more lenient — no scenario turns a previously-FAILing case into a PASS; only
previously-invisible failures become visible:
will now be caught instead of silently reported healthy).
mismatch will now be caught instead of silently reported healthy).
Anyone currently relying on these checks staying green in-pod should expect this PR to
surface pre-existing fabric issues that were previously masked, not introduce new ones.
Revert either commit independently to roll back; no migration, no RBAC changes were made
(existing ServiceAccount permissions already cover the new kubectl fallback call — verified
against
kubernetes/manifests/02-rbac.yaml, which already grantsgeton nodes).Follow-through from diagnostic work on 2 idle p6-b200.48xlarge nodes (0 GPU-h) that found both
suite defects while investigating an unrelated hardware hypothesis (ruled out, unrelated to
these suite bugs).