Repository navigation
gds_mt: decode cuFile errors correctly in IO failure logs - #2143
Conversation
|
👋 Hi changhyeonnam! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
Hi, @brminich ! Could you take a look when you get a chance? 😄 |
|
LGTM |
|
@changhyeonnam Please can you rebase since there has been a re-org for GDS plugins? |
cuFileRead and cuFileWrite report filesystem errors as -1 with errno set, and cuFile-level errors either as a negative CUfileOpError return value (as documented) or, as observed in practice, as -1 with the CUfileOpError code stored in errno. runCuFileOp logged strerror(errno) for every failure, so cuFile-level errors produced garbage such as "cuFileRead failed: Unknown error 5019". Decode the return value accordingly: strerror only when errno holds a plain errno value, CUFILE_ERRSTR with the numeric code otherwise, matching the error format used by the GDS batch engine. Verified on a B200 system with cuFile 1.15.1.6 by reading through a file opened write-only; the log now reads "unsupported file open flags (err=5019)" instead of "Unknown error 5019". Signed-off-by: Changhyeon Nam <changhyeon03@sk.com>
2a5e078 to
a201990
Compare
|
Hi @vvenkates27, I rebased onto main. The change is now in gds_mt_engine.cpp, same content. Could you take another look? Thanks! |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/nixl/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughcuFile read and write failures now use a helper that distinguishes system errors from cuFile errors. Both paths retain their existing backend error status and early return. ChangescuFile Error Logging
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change improves cuFile failure messages without a supported change to I/O behavior. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test a201990 |
|
/ok to test b3e0212 |
|
/build |
|
🤖 CI Triage Agent — TL;DR: The build failed in the Full analysisSummary: Stage 588 Root cause: Two independent infrastructure problems, neither in nixl code:
All actual build work succeeded: three Implicated commit: none — commit b3e0212 (branch File: Suggested fix:
Related: none found — issue/PR search for this error signature failed to return results. |
|
🤖 CI Triage Agent — TL;DR: The build failed in the "Allocate Environment" stage because Full analysisSummary: Stage 164 "Allocate Environment" failed: Root cause: The pipeline calls The ~7-minute gap between The commit under test (b3e0212, Implicated commit: none — infrastructure failure, unrelated to [REDACTED:Hex High Entropy String] File: Jenkins pipeline stage "Allocate Environment" ( Suggested fix:
Related: none
|
|
🤖 CI Triage Agent — TL;DR: The build didn't fail on code — the "Allocate Environment" stage spent the full hour of its Full analysisSummary: Root cause: All build work succeeded — "Compiling NIXL Docker Image" finished, meson/ninja built nixl and nixlbench cleanly, and the PR test image
The single 60-minute gap between those two lines is exactly the Implicated commit: No code commit — infrastructure/scheduling. Contributing CI config changes: File: Jenkins pipeline config for Suggested fix: Re-trigger the build — this is retryable and unrelated to the PR. To stop it recurring: (1) have the allocation step distinguish "no resources available" from a real failure and either retry with backoff or mark the GPU-VR branch unstable/skipped instead of failing the whole PR build, since an hour-long queue wait on a scarce Vera Rubin partition is expected load behaviour; (2) ask the cluster admins to re-check the four nodes excluded in #2339 — with those back in the pool the Related: #2339 (node exclusions), #2352 (2-GPU request instead of whole node), #2182 (added the nixl-ci-gpu-vr pipeline)
|
|
/ok to test 42fd117 |
|
/build |
This branch was previously deployed
6 participants
What?
Fix error decoding in
runCuFileOp: usestrerror(errno)only for plain filesystem errors, and decode cuFile-level errors viaCUFILE_ERRSTRwith the numeric code included. Follow-up to #2069, which did the equivalent for the cuda_gds plugin, using the same message format.Why?
cuFileRead/cuFileWrite report filesystem errors as -1 with errno set, and cuFile-level errors either as a negative
CUfileOpErrorreturn value (as documented in cufile.h) or, as observed in practice, as -1 with theCUfileOpErrorcode stored in errno. The current code printsstrerror(errno)for every failure, which produces garbage for cuFile-level errors. Reproduced on a B200 system by forcing a cuFile-level read failure:How?
Added a small file-local helper that branches using the
IS_CUFILE_ERRmacro from cufile.h, used by both the read and write paths, following the helper pattern suggested in the #2069 review. Plain filesystem error messages are unchanged. Verified the changed lines pass clang-format-19 and the plugin builds cleanly.Summary by CodeRabbit