Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
1 task done
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Older RCCL runtimes can fail on the unresolved encryption API instead of ignoring the variable as documented.
2 open findings
What changed in this PR
Enables TLS socket encryption in RCCL performance and correctness tests using a configured pre-shared key.
Changes:
- Configures PSK encryption before communicator creation.
- Documents encryption requirements and behavior.
| File | Description |
|---|---|
projects/rccl-tests/src/common.cu |
Enables encryption through NCCL_TESTS_TLS_PSK. |
projects/rccl-tests/README.md |
Documents the new environment variable. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
i-kosarev
marked this pull request as ready for review
October 15, 2026 15:27
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
3 more finding(s), not posted inline
projects/rccl-tests/src/common.cu:2541(Low) With RCCL headers older than 2.32 the TLS block compiles out, so a set NCCL_TESTS_TLS_PSK is ignored without any noticeprojects/rccl-tests/src/util.cu:377(Low) No test exercises the NCCL_TESTS_TLS_PSK redaction in the JSON report env listprojects/rccl-tests/src/common.cu:2540(Nit)tlsPsk && tlsPsk[0]is tested twice; nest the version check under a single outer if
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
amd-arozanov
requested changes
Oct 26, 2026
i-kosarev
requested review from
amd-arozanov
and
a balanced review from Copilot
October 28, 2026 05:19
amd-arozanov
approved these changes
Oct 28, 2026
Contributor
pr-rocm-systems-13369-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-13369-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13369-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13369-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-13369-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13369-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13369-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
mch
requested changes
Nov 6, 2026
| if (setEncryption == NULL) { | ||
| fprintf(stderr, "NCCL_TESTS_TLS_PSK is set, but the RCCL library (%d) has no ncclSetEncryption; 2.32 or newer is required.\n", | ||
| test_ncclVersion); | ||
| return testInvalidUsage; |
Contributor
There was a problem hiding this comment.
This return sits after MPI_Comm_split (common.cu:2494) but before MPI_Bcast(&ncclId, ...) and MPI_Barrier(MPI_COMM_WORLD) (common.cu:2556-2558), so a rank taking it leaves any rank that does not blocked in the broadcast. In practice the condition is driven by the header/library version and so is uniform across ranks, and the pattern matches the pre-existing return testNcclError at common.cu:2504, so this is a question rather than a defect: for these pre-communicator validation failures, would MPI_Abort be preferable to a return, so an asymmetric failure surfaces as an abort rather than a hang?
Contributor
Author
There was a problem hiding this comment.
let's keep return: the condition is uniform across ranks, like the existing check above. Uneven key propagation hangs in bootstrap instead
NCCL_TESTS_TLS_PSK=<key> makes every process call ncclSetEncryption with that pre-shared key before creating the communicator, so existing perf and data-check runs can exercise TLS-encrypted bootstrap, proxy and socket-transport traffic without code changes. Ignored before RCCL 2.32. Co-authored-by: Cursor <cursoragent@cursor.com>
…an 2.32 A binary built against 2.32 headers can run against an older librccl. Check the run-time version before calling ncclSetEncryption, like the other version-gated calls, and state the key minimum in bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
The -J report records every environment variable, so it would carry the TLS pre-shared key in plaintext. Co-authored-by: Cursor <cursoragent@cursor.com>
…aders too Builds against headers older than 2.32 dropped the variable silently. One outer check on the variable now covers both the header and library cases. Co-authored-by: Cursor <cursoragent@cursor.com>
…S is unavailable A direct reference kept binaries built against 2.32 headers from loading an older librccl under -z now or LD_BIND_NOW. Setting NCCL_TESTS_TLS_PSK without TLS support in the headers or library is now an error instead of a silent plaintext run. Co-authored-by: Cursor <cursoragent@cursor.com>
An unset secret expanded to NCCL_TESTS_TLS_PSK= ran the test in plaintext. Only the variable's presence now enables encryption; ncclSetEncryption rejects keys shorter than 32 bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
…nd logs Unsets NCCL_TESTS_TLS_PSK once RCCL holds its own copy, so RCCL's RAS NCCL_* environment report and child processes never see it. The name has one definition shared with the JSON redaction, the banner no longer prints the key length, and the README shows passing the key to every rank, since a rank without it hangs bootstrap instead of failing. Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
force-pushed
the
users/ilkosare/rccl-tests-tls-psk
branch
from
November 6, 2026 19:08
27310c1 to
35ef704
Compare
dlsym(RTLD_DEFAULT) cannot see the symbols of a statically linked librccl, so NCCL_TESTS_TLS_PSK would report the API as missing. CMake now defines RCCL_TESTS_STATIC_RCCL when roc::rccl is a static library, and the call references the function directly. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
pr-rocm-systems-13369-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13369-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. |
Contributor
pr-rocm-systems-13369-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13369-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
This branch has not been deployed
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
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.
Motivation
rccl-tests has no way to turn on RCCL socket encryption (
ncclSetEncryption), so no perf or data-check run exercises it. CI needs it for the TLS job (#13373, ROCm/tRCCL#612).Technical Details
src/common.cu: withNCCL_TESTS_TLS_PSKset, every process callsncclSetEncryption(resolved at run time) before creating the communicator. With RCCL headers or library older than 2.32 the test fails instead of running in plaintext.README.md: documents the variable.Issue Tracking
JIRA ID: AICOMRCCL-2488
Test Plan
all_reduce_perf -c 1with and without the variable, on builds with and without-DTLS_BACKEND=OPENSSL3, and against RCCL 2.30 headers and library.Test Result
NCCL_TESTS_TLS_PSKLD_BIND_NOW=1Submission Checklist