Repository navigation
Conversation
UCP prefers the higher priority when transport scores are equal, but TCP set priority 1 on the interfaces without the default route and 0 on the default one, the opposite of UCX_TCP_PREFER_DEFAULT. The inversion has been effective since v1.10.0, when the default route detection was fixed.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
| UCS_TEST_MESSAGE << m_tcp_iface->if_name << ": is_default " << is_default; | ||
|
|
||
| // UCP prefers the higher priority when transport scores are equal | ||
| EXPECT_EQ(is_default ? 1 : 0, m_ent->iface_attr().priority); |
There was a problem hiding this comment.
minor: uct_tcp_netif_is_default() always sets 0 or 1, so the ternary is redundant.
| EXPECT_EQ(is_default ? 1 : 0, m_ent->iface_attr().priority); | |
| EXPECT_EQ(is_default, m_ent->iface_attr().priority); |
|
|
||
| UCS_TEST_P(test_uct_tcp, prefer_default_disabled, "TCP_PREFER_DEFAULT=n") | ||
| { | ||
| EXPECT_EQ(0, m_ent->iface_attr().priority); |
There was a problem hiding this comment.
this check is vacuous for non-default netdevs (e.g. tcp/lo), where priority is 0 either way. maybe query uct_tcp_netif_is_default() here too and skip if it is not the default netdev, so the test really covers the disabled path?
|
Residual coverage gap worth noting: |
|
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
guy-ealey-morag
left a comment
guy-ealey-morag
left a comment
There was a problem hiding this comment.
I would also update the docs in src/uct/api/uct.h:1180 to say that higher value is preferred.
|
🤖 Starting review — findings will be posted here when done. |
Done |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest shard aborted because Full analysisSummary: Root cause: Evidence from the log:
Implicated commit: none from this PR ( File: Suggested fix:
Related: #11920 (UCT/GTEST: Hardening of UD stale private EP tests) — prior work hardening UD gtests against timeout-induced fatals; no existing issue found for
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Two layers, both unrelated to the PR under test (
Log evidence ordering is unambiguous: Implicated commit: None in this PR. The uninitialized- File: Suggested fix:
Related: PR #12241 (the build under test, unrelated to the failure); no existing issue found for the MPS error signature — worth filing one for the uninitialized-pointer hardening in
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Timeline from the log proves a hang, not slow progress:
Two compounding bugs in the test, both pre-existing:
So the EINTR is the trigger and the misuse of This is unrelated to the PR under test: branch Implicated commit: Not a regression from a recent commit. The latent defect dates to File: Suggested fix:
var nevents int
for {
nevents, err = syscall.EpollWait(epfdLocal, events[:], -1)
if err != syscall.EINTR {
break
}
}
if err != nil {
t.Errorf("Failed to call epoll_wait %v", err)
close(eventsChan) // unblock the main goroutine
return
}
select {
case nevents, ok := <-eventsChan:
if !ok || nevents <= 0 {
t.Fatalf("No events in epoll")
}
case <-time.After(5 * time.Second):
t.Fatal("Timed out waiting for epoll event")
}
Related: PR #12241 (the PR under test, not the cause). No existing issue found tracking this flake — worth opening one against |
What?
Give the TCP interface with the default route priority 1 and the other interfaces priority 0, as
UCX_TCP_PREFER_DEFAULTdescribes ("Give higher priority to the default network interface on the host"). Until now it was the other way around.Why?
UCP prefers the higher
uct_iface_attr_t::prioritywhen transport scores are equal:ucp_score_prio_cmp()with the sum of the local and remote priorities inucp_wireup_select_transport()and in the AM lane tiebreak. With the defaultUCX_TCP_MAX_BW=2200MBs, TCP interfaces of 25 Gb/s and faster report the same bandwidth and the same packed latency, so such ties are common on hosts with several NICs. Because of the inverted priority, the AM and keepalive lanes went to an interface without the default route. For example, with the VRF slave devices from #12182 enabled on a cluster, the AM lane moved from eth0 to an RDMA rail.The mapping has been inverted since the TCP default interface code was added, but it has an effect only since v1.10.0, when the default route detection in
uct_tcp_netif_is_default()was fixed (a8345ce). Before that, every TCP interface had priority 1.Peers with different UCX versions still compute the same local + remote priority sum, so lane selection stays symmetric between them.
How?
Use
netif_caps.is_defaultas the priority inuct_tcp_iface_query();uct_tcp_iface_netif_caps_init()already leaves it 0 whenUCX_TCP_PREFER_DEFAULT=n. The new gtests check that the priority matchesuct_tcp_netif_is_default(), and that it is 0 withUCX_TCP_PREFER_DEFAULT=n.