From 0a5454aacc3cc4c69ccb9c9d7be60034e8d80a28 Mon Sep 17 00:00:00 2001 From: Keita Watanabe Date: Fri, 31 Jul 2026 14:38:34 +0000 Subject: [PATCH 1/2] fix(healthcheck): pin EFA loopback test with FI_EFA_IFACE, not the nonexistent 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= 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= -> 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. --- .../checks/6-efa-loopback.sh | 48 +++++++++++++++---- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/4.validation_and_observability/2.gpu-cluster-healthcheck/checks/6-efa-loopback.sh b/4.validation_and_observability/2.gpu-cluster-healthcheck/checks/6-efa-loopback.sh index 46c8b5e88..53afada41 100755 --- a/4.validation_and_observability/2.gpu-cluster-healthcheck/checks/6-efa-loopback.sh +++ b/4.validation_and_observability/2.gpu-cluster-healthcheck/checks/6-efa-loopback.sh @@ -21,13 +21,26 @@ EFA_INSTALLER_TEST="${EFA_INSTALLER_TEST:-/opt/amazon/efa/test/efa_test.sh}" # - FI_EFA_ENABLE_SHM_TRANSFER=0: force the real EFA hardware path; otherwise # libfabric routes same-host traffic through SHM and the test does not # exercise EFA at all. -# - FI_EFA_DEVICE_NAME=: pin libfabric to the specific EFA domain. +# - FI_EFA_IFACE=: pin libfabric to the specific EFA +# device. NOTE: this takes the kernel/ibv device name (e.g. "rdmap80s0"), +# not the libfabric domain name (e.g. "rdmap80s0-rdm") -- the "-rdm" suffix +# must be stripped from the domain string discovered below. +# (FI_EFA_DEVICE_NAME is NOT a real libfabric env var -- it does not +# appear in `fi_info -e`'s FI_EFA_* list and is silently ignored, which +# previously made every "per-device" iteration below run on whichever +# device libfabric picks by default. Confirmed against a running +# libfabric.so.1 2.4.0amzn3.0: 0 string occurrences of FI_EFA_DEVICE_NAME, +# vs. FI_EFA_IFACE present and honored.) # - explicit -B server_port / -B client_port -P server_port: avoid port # collisions when called per-device in a loop. # Returns 0 on success, non-zero on failure. Writes server+client logs to stdout # on failure for triage. run_pingpong_for_domain() { local domain="$1" + # FI_EFA_IFACE takes the kernel/ibv device name, not the libfabric domain + # name -- strip the "-rdm" suffix (see comment above run_pingpong_for_domain + # invocation site / the header comment block for why). + local iface="${domain%-rdm}" local server_port client_port server_port=$(shuf -n 1 -i 49152-57342) client_port=$(shuf -n 1 -i 57343-65535) @@ -36,7 +49,7 @@ run_pingpong_for_domain() { server_log=$(mktemp) client_log=$(mktemp) - FI_LOG_LEVEL=warn FI_EFA_ENABLE_SHM_TRANSFER=0 FI_EFA_DEVICE_NAME="${domain}" \ + FI_LOG_LEVEL=warn FI_EFA_ENABLE_SHM_TRANSFER=0 FI_EFA_IFACE="${iface}" \ fi_pingpong -e rdm -p efa -B "${server_port}" > "${server_log}" 2>&1 & local server_pid=$! sleep 3 @@ -50,7 +63,7 @@ run_pingpong_for_domain() { fi local ret=0 - FI_LOG_LEVEL=warn FI_EFA_ENABLE_SHM_TRANSFER=0 FI_EFA_DEVICE_NAME="${domain}" \ + FI_LOG_LEVEL=warn FI_EFA_ENABLE_SHM_TRANSFER=0 FI_EFA_IFACE="${iface}" \ timeout "${EFA_TEST_TIMEOUT}" \ fi_pingpong -e rdm -p efa -B "${client_port}" -P "${server_port}" localhost \ > "${client_log}" 2>&1 || ret=$? @@ -99,11 +112,11 @@ run_check() { # Discover EFA libfabric DOMAINS, not kernel ibv device names. The two # naming spaces differ: ibv_devices returns names like 'rdmap86s0', but - # libfabric's -d/FI_EFA_DEVICE_NAME expects domains like 'rdmap86s0-rdm' - # (with the '-rdm' suffix added by the EFA provider). Passing kernel names - # to fi_pingpong yields fi_getinfo -61 (No data available) and the test - # fails on every device. Enumerating via fi_info gets us the correct names - # and also naturally excludes back-side Ethernet NICs that show up under + # libfabric domains (as reported by `fi_info -p efa -t FI_EP_RDM`) are + # named like 'rdmap86s0-rdm'. FI_EFA_IFACE (used to pin below) takes the + # kernel/ibv name, so the '-rdm' suffix gets stripped per-device. + # Enumerating via fi_info gets us the correct domain names and also + # naturally excludes back-side Ethernet NICs that show up under # ibv_devices but are not EFA endpoints. local domains domains=$(fi_info -p efa -t FI_EP_RDM 2>/dev/null \ @@ -119,6 +132,25 @@ run_check() { device_count=$(echo "${domains}" | wc -l | tr -d ' ') log_info "Testing ${device_count} EFA domain(s)" + # Regression guard: a bogus, definitely-nonexistent device name MUST fail. + # This is the exact failure signature of the FI_EFA_DEVICE_NAME defect + # (a per-device pinning env var that libfabric silently ignores, so every + # "per-device" iteration -- including one given a nonexistent name -- ran + # on whichever device libfabric picked by default and reported PASS). If + # this negative control ever passes, per-device pinning is broken again + # and the results below cannot be trusted as per-device -- fail loudly + # instead of reporting a green per-device sweep that isn't one. + log_verbose "Running negative-control check: bogus device name must fail loopback" + local negctrl_exit=0 + run_pingpong_for_domain "definitely-nonexistent-device-rdm" > /dev/null 2>&1 || negctrl_exit=$? + if [[ "${negctrl_exit}" -eq 0 ]]; then + check_fail "${CHECK_NAME}" \ + "Negative control failed: a nonexistent device name (definitely-nonexistent-device-rdm) returned PASS. Per-device pinning is not working -- results below cannot be trusted as per-device. (This is the FI_EFA_DEVICE_NAME silent-no-op failure mode; see fix commit.)" \ + "RESET" + return 1 + fi + log_verbose "Negative control OK: bogus device name correctly failed (exit ${negctrl_exit})" + local failures=0 local results_json="[" From 985c1f827a4382ec647b63b7eec968b519fe8630 Mon Sep 17 00:00:00 2001 From: Keita Watanabe Date: Fri, 31 Jul 2026 14:39:05 +0000 Subject: [PATCH 2/2] fix(healthcheck): allow INSTANCE_TYPE override so check 2 isn't vacuous 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. --- .../kubernetes/agent.sh | 16 +++++------ .../manifests/05-job-quarantine.yaml | 6 +++++ .../2.gpu-cluster-healthcheck/lib/common.sh | 27 +++++++++++++++++-- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/agent.sh b/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/agent.sh index c9991e579..60348c809 100755 --- a/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/agent.sh +++ b/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/agent.sh @@ -51,17 +51,17 @@ require_env() { fi } -# Detect instance type: IMDS first, then K8s node label fallback +# Detect instance type: IMDS first, then K8s node label fallback. +# NOTE: detect_instance_type() in lib/common.sh already implements this exact +# fallback chain (IMDS -> ec2-metadata -> kubectl node-label, gated on +# NODE_NAME being set) as of the common.sh fix for the check-2 vacuous-PASS +# defect -- this wrapper now just adds agent.sh's own "unknown" default +# instead of reinventing the kubectl fallback here. NODE_NAME is required by +# main()'s require_env call before this runs, so the k8s-label branch inside +# detect_instance_type() is live for this caller. detect_instance() { - # Try IMDS (works with hostNetwork: true) INSTANCE_TYPE=$(detect_instance_type 2>/dev/null || true) - if [[ -z "${INSTANCE_TYPE}" ]]; then - log_info "IMDS unavailable, falling back to node label" - INSTANCE_TYPE=$(kubectl get node "${NODE_NAME}" \ - -o jsonpath='{.metadata.labels.node\.kubernetes\.io/instance-type}' 2>/dev/null || true) - fi - if [[ -z "${INSTANCE_TYPE}" ]]; then log_warn "Unable to detect instance type -- using defaults" INSTANCE_TYPE="unknown" diff --git a/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/manifests/05-job-quarantine.yaml b/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/manifests/05-job-quarantine.yaml index 38881b9d5..40d883ed6 100644 --- a/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/manifests/05-job-quarantine.yaml +++ b/4.validation_and_observability/2.gpu-cluster-healthcheck/kubernetes/manifests/05-job-quarantine.yaml @@ -60,6 +60,12 @@ spec: LABEL_PREFIX="gpu-healthcheck.aws-samples.io" RESULTS_DIR="/tmp/gpu-healthcheck-quarantine" export RESULTS_DIR + # NODE_NAME is lib/common.sh's detect_instance_type() kubectl + # fallback key (used when IMDS is unreachable, which it is by + # default from an EKS pod). This Job's ServiceAccount already + # has `get` on nodes (02-rbac.yaml), so exporting this here is + # sufficient -- no RBAC or volume changes needed. + export NODE_NAME="${NODE}" echo "=== GPU Quarantine Check: ${NODE} ===" diff --git a/4.validation_and_observability/2.gpu-cluster-healthcheck/lib/common.sh b/4.validation_and_observability/2.gpu-cluster-healthcheck/lib/common.sh index 805a28f6d..d67cc3434 100644 --- a/4.validation_and_observability/2.gpu-cluster-healthcheck/lib/common.sh +++ b/4.validation_and_observability/2.gpu-cluster-healthcheck/lib/common.sh @@ -26,7 +26,11 @@ DRY_RUN="${DRY_RUN:-0}" JSON_OUTPUT="${JSON_OUTPUT:-0}" # Instance profile variables (populated by load_instance_profile) -INSTANCE_TYPE="" +# INSTANCE_TYPE may be pre-exported by the caller (e.g. a k8s manifest +# threading the node's `node.kubernetes.io/instance-type` label down as an +# env var) to bypass IMDS detection entirely -- see detect_instance_type() +# below for why IMDS alone is not sufficient inside an EKS pod. +INSTANCE_TYPE="${INSTANCE_TYPE:-}" EXPECTED_GPU_COUNT="" EXPECTED_EFA_COUNT="" NVLINK_EXPECTED="" @@ -54,6 +58,13 @@ log_verbose() { # ─── Instance Detection ───────────────────────────────────────────────────── +# IMDS is unreachable from a pod running inside EKS under default hop-limit +# settings (confirmed: IMDSv2 token request returns empty, IMDSv1 fallback +# also empty). load_instance_profile() below only calls this when +# INSTANCE_TYPE is not already set -- callers that know their instance type +# some other way (e.g. a k8s Downward API / node-label lookup) should export +# INSTANCE_TYPE before sourcing this file, or before calling +# load_instance_profile(), to skip IMDS entirely. detect_instance_type() { # Try IMDSv2 first, fall back to IMDSv1, then ec2-metadata CLI local token @@ -73,8 +84,20 @@ detect_instance_type() { INSTANCE_TYPE=$(ec2-metadata --instance-type 2>/dev/null | awk '{print $2}' || true) fi + # Last resort: k8s node label, when running as a pod with NODE_NAME set + # (downward API `spec.nodeName`) and `kubectl` reachable. This mirrors + # kubernetes/agent.sh's own IMDS-unavailable fallback (agent.sh:59-63) -- + # centralized here so any caller of load_instance_profile() gets it, not + # just the DaemonSet agent. Requires the ServiceAccount to have `get` on + # `nodes` (already granted: kubernetes/manifests/02-rbac.yaml:19-20). + if [[ -z "${INSTANCE_TYPE}" && -n "${NODE_NAME:-}" ]] && command -v kubectl > /dev/null 2>&1; then + log_warn "IMDS unavailable -- falling back to k8s node label for instance type" + INSTANCE_TYPE=$(kubectl get node "${NODE_NAME}" \ + -o jsonpath='{.metadata.labels.node\.kubernetes\.io/instance-type}' 2>/dev/null || true) + fi + if [[ -z "${INSTANCE_TYPE}" ]]; then - log_error "Unable to detect instance type via IMDS or ec2-metadata" + log_error "Unable to detect instance type via IMDS, ec2-metadata, or k8s node label" return 1 fi