Repository navigation
UCP/GTEST: Add test for multi-rail load-balance - #11842
Conversation
|
🤖 Starting review — findings will be posted here when done. |
c836193 to
a57c61b
Compare
|
🤖 Starting review — findings will be posted here when done. |
| size_t expected_bytes, | ||
| const std::map<std::string, double> &expected_rates) | ||
| { | ||
| constexpr double byte_tolerance = 1.0; |
There was a problem hiding this comment.
the per-lane split is computed with round-up fixed-point weights (ucp_proto_multi_scaled_length() uses div_round_up), so the actual bytes on each device are not the exact expected_bytes * rate / total_rate value used here. is a 1-byte tolerance robust across the possible weight rounding, or could it be flaky? if it depends on the specific 52K/38K message sizes, maybe add a short comment noting why these sizes were chosen.
a57c61b to
fbfab73
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The wire-compat job failed in the Full analysisSummary: Root cause: The script compares Implicated commit: unknown (no code commit implicated; environment/build-config difference between the File: buildlib/tools/check_tls_perf_caps.sh:29-56 (comparison loop) with Suggested fix:
Related: PR #11842 (the innocent PR under test); #11720 / #11654 (recent CMA probe changes — worth ruling out with |
fbfab73 to
8c1cbf4
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| ASSERT_GT(total_rate, 0.0); | ||
| for (ucp_lane_index_t lane = 0; lane < num_lanes; ++lane) { | ||
| const uint64_t lane_bytes = get_zcopy_bytes_sent(ep, lane) - |
There was a problem hiding this comment.
After a speed change the EP is reassigned to a freshly created ep config, and expect_byte_distribution reads ucp_ep_num_lanes(ep) from that new config while bytes_before was sized from the config in effect at capture time. If a future variant of this test (or a config where a rail is dropped/added on speed change) makes the new config have more lanes than bytes_before.size(), bytes_before[lane] reads past the end of the vector. In this specific mock the rail count stays fixed at 2, so it's safe today — worth a short comment noting the assumption, or sizing/reading bytes_before defensively.
8c1cbf4 to
a769f50
Compare
|
🤖 Starting review — findings will be posted here when done. |
Signed-off-by: Zhenlong Ma <zhenlongm@nvidia.com>
a769f50 to
3c5fc94
Compare
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
33140c8 to
be3fc10
Compare
|
🤖 Starting review — findings will be posted here when done. |
| std::vector<uint64_t> bytes; | ||
|
|
||
| bytes.reserve(num_lanes); | ||
| for (ucp_lane_index_t lane = 0; lane < num_lanes; ++lane) { |
There was a problem hiding this comment.
Inconsistent null-lane handling / potential crash: expect_byte_distribution() skips lanes whose rsc_index == UCP_NULL_RESOURCE (L2491), but get_zcopy_bytes_each_lane() (and get_zcopy_bytes_sent()) iterate over every lane 0..num_lanes and unconditionally dereference ucp_ep_get_lane(ep, lane)->stats. If the ep config contains a lane with UCP_NULL_RESOURCE (e.g. a wireup/CM lane — existing code such as test_ucp_rma.cc:408 guards for exactly this case), the collection helper reads a null/invalid uct_ep and can crash before the guard in expect_byte_distribution is ever reached. Suggest applying the same null-resource guard here so a wireup/CM lane doesn't crash the collection. Non-blocking: in this specific rc_x-only mock config (no CM lane, IB_NUM_PATHS=1, MAX_RMA_RAILS=2) the RMA lanes likely all have valid rsc_index, so the crash may not trigger in practice, but the inconsistency with the guard the author already added warrants alignment.
be3fc10 to
ce85e4a
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
ce85e4a to
ea749fd
Compare
|
🤖 Starting review — findings will be posted here when done. |
ea749fd to
62ac4d7
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: From the log evidence: the whole 60-minute gtest run ( Caveat: Azure only returns the tail of this job's log, so the exact gtest assertion text/line for Implicated commit: unknown — not attributable to File: test/gtest/uct/test_md.cc:413-496 (failure point is either Suggested fix:
Related: PR #10150 (TEST/MD: Add short sleep before retrying memory allocation — prior fix for this same flaky test), PR #11842 (the PR under test)
|
62ac4d7 to
f0968f7
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The ASAN/roce shard-4 gtest run produced no output for 8m26s right after starting its first test ( Full analysisSummary: Job "roce on worker 3" (AddressSanitizer, gtest shard 4 of 4) was cancelled by Root cause: Evidence from the task log:
The largest inter-line gap is 8m26s (≈95 % of the gtest runtime), so the process was not making progress: Implicated commit: unknown — no product-code commit implicated; PR head is f0968f7 (Zhenlong Ma), whose changes are gtest-only and do not touch the stalled fixture. File: test/gtest/ucp/test_ucp_rma.cc:748 ( Suggested fix:
Related: PR #11842 (#11842) — the PR under test; no existing issue found for this hang/agent-shutdown signature. |
Reuse configured rates in m_port_speed. Check resource index in advance of reading the lane's byte count. Replace the manual resource lookup with ucp_ep_get_tl_rsc. Signed-off-by: Zhenlong Ma <zhenlongm@nvidia.com>
f0968f7 to
594351f
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
What?
Add a stats-enabled unit test for multi-rail load-balance after a device speed-change event. The test verifies that PUT and GET operations can distribute and reallocate transmission data across two rails based on their actual rates.
Why?
The existing unit test for speed-change only verify the configuration without checking the actual data on each rail. As a result, we need additional tests to guarantee that UCX adopt the latest and correct multi-rail speed distribution.
How?
The test records the byte counters for every lane before and after the data transmission and compares them with the expected distribution.