Skip to content

UCP/WORKER: Add device filter for worker address query - #11717

Merged
shasson5 merged 6 commits into
openucx:masterfrom
shasson5:worker-address-device-api
Aug 26, 2026
Merged

shasson5 merged 6 commits into
openucx:masterfrom
shasson5:worker-address-device-api

Conversation

@shasson5

@shasson5 shasson5 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What

Add an optional address_device_name input field to ucp_worker_attr_t and a matching UCP_WORKER_ATTR_FIELD_ADDRESS_DEVICE_NAME field mask.

When ucp_worker_query() is asked for a worker address with this field set, UCP packs only resources whose context device name matches the requested device. Requests for a device with no addressable worker resources return UCS_ERR_NO_DEVICE.

Why

Some users need to exchange a worker address for a specific network device instead of all resources opened by the worker. This lets applications reduce address payload size and control which device is exposed during out-of-band address exchange.

@shasson5
shasson5 force-pushed the worker-address-device-api branch 2 times, most recently from 4c04986 to a4d3a00 Compare August 23, 2026 14:36
@shasson5
shasson5 marked this pull request as ready for review August 23, 2026 16:04
@shasson5 shasson5 added the WIP-DNM Work in progress / Do not review label Aug 23, 2026
@shasson5

Copy link
Copy Markdown
Contributor Author

@svc-nvidia-pr-review

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread test/gtest/ucp/test_ucp_worker.cc Resolved
Comment thread test/gtest/ucp/test_ucp_worker.cc Resolved
@shasson5
shasson5 force-pushed the worker-address-device-api branch from a4d3a00 to e6a34be Compare August 23, 2026 16:39
@shasson5
shasson5 force-pushed the worker-address-device-api branch from e6a34be to ca965ae Compare August 23, 2026 16:42
@svc-ucx

svc-ucx commented Aug 23, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit ca965aee

TL;DR: The Coverity stage failed with 2 new FORWARD_NULL defects in test/gtest/ucp/test_ucp_worker.cc (lines 944 and 993) — Coverity doesn't understand that gtest's ASSERT_NE(nullptr, worker_attr.address) aborts the test, so it thinks the zero-initialized worker_attr.address (NULL) is passed to ucp_address_unpack()/ucp_worker_address_query(). Add an explicit null guard (or a coverity[var_deref_model] annotation) after the ASSERT in the new test_ucp_worker_address_query tests.

Full analysis

Summary: coverity devel on coverity_rh7 (Azure build 132118) ended with ##[error]Coverity found 2 issues: — build/compile and cov-analyze succeeded (736 TUs, 1563 total occurrences), but cov-format-errors reported 2 non-baselined defects, so the script returned 2 and failed the stage.

Root cause: Both defects are Coverity FORWARD_NULL ("Explicit null dereferenced") false positives introduced by the new test code in this PR:

  • test/gtest/ucp/test_ucp_worker.cc:930 / :984 declare ucp_worker_attr_t worker_attr{}; — the brace-init explicitly zeroes worker_attr.address, so Coverity now knows it is NULL.
  • ucp_worker_query() only writes attr->address when UCP_WORKER_ATTR_FIELD_ADDRESS is set and ucp_worker_address_pack() succeeds (src/ucp/core/ucp_worker.c:3172-3181; it can return UCS_ERR_NO_DEVICE at line 3153 without writing). Coverity therefore reports "does overwrite worker_attr.address on some paths, [but] also contains at least one feasible path which does not overwrite it".
  • The test does guard with ASSERT_UCS_OK(status) and ASSERT_NE(nullptr, worker_attr.address) (lines 940-941, 988-989), but gtest's ASSERT_* expands to switch(0) case 0: ... if (AssertionResult) ; else return ..., and Coverity's trace shows it "taking true branch / falling through" without narrowing the pointer to non-NULL. It then flags the NULL being passed to ucp_address_unpack() (line 944 → address.c:1137 deref) and ucp_worker_address_query() (line 993 → ucp_worker.c:3202 → ucp_address_get_uuid → address.c:1137).

The library code itself is correct; only the new gtest triggers the checker.

Implicated commit: ca965aee — "UCP/WORKER: Add device filter for worker address query", shachar hasson (the PR head commit)

File: test/gtest/ucp/test_ucp_worker.cc:941-947 and test/gtest/ucp/test_ucp_worker.cc:989-994

Suggested fix: Make the non-NULL invariant explicit so Coverity can see it, in both places:

    auto status = ucp_worker_query(worker, &worker_attr);
    ASSERT_UCS_OK(status);
    ASSERT_NE(nullptr, worker_attr.address);
    if (worker_attr.address == nullptr) {
        return; /* not reached: silence static analysis */
    }

(equivalently, replace the ASSERT with if (worker_attr.address == nullptr) { UCS_TEST_ABORT("NULL worker address"); }, or, if the team prefers annotations as already used elsewhere in the tree — e.g. coverity[assert_side_effect] in src/ucp/core/ucp_worker.c:3213 — put /* coverity[var_deref_model] */ immediately before the ucp_address_unpack() and ucp_worker_address_query() calls). Note the same pattern will recur for every ucp_worker_attr_t x{} + ASSERT_NE(nullptr, x.address) in the new fixture, so apply it consistently in check_address_by_device() and the query test.

Related: PR #11717 (branch worker-address-device-api); no existing issue found for this Coverity signature.

@svc-ucx

svc-ucx commented Aug 24, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit 7959184f

TL;DR: The Coverity devel stage failed the build gate with 1 new FORWARD_NULL defect in the new test code added by this PR: worker_attr.address (zero-initialized to NULL) is passed to ucp_address_unpack() at test/gtest/ucp/test_ucp_worker.cc:919, because Coverity can't prove ucp_worker_query() writes the field and doesn't model gtest's ASSERT_NE as terminating the function. Replace the ASSERT_NE(nullptr, worker_attr.address) with a check Coverity understands (a throwing UCS_TEST_ABORT or an explicit if (... == nullptr) return;).

Full analysis

Summary: coverity devel on coverity_rh7 failed at the end of cov-analyze/cov-format-errors: ##[error]Coverity found 1 issues: /__w/1/s/test/gtest/ucp/test_ucp_worker.cc:919 Type: Explicit null dereferenced (FORWARD_NULL). The compile and analysis themselves succeeded (736/736 units, 22:56 analysis time); only the defect gate (nerrors=1 -> exit 1) failed.

Root cause: In check_address_by_device():

  • line 886: ucp_worker_attr_t worker_attr{}; zero-initializes, so worker_attr.address == NULL (Coverity step 1 assign_zero).
  • line 915: ucp_worker_query() writes address only when UCP_WORKER_ATTR_FIELD_ADDRESS is set and packing succeeds; Coverity reports no_write_call: "although ucp_worker_query does overwrite worker_attr.address on some paths, it also contains at least one feasible path which does not overwrite it."
  • line 917: ASSERT_NE(nullptr, worker_attr.address) — Coverity does not correlate the gtest assertion's internal gtest_ar bool with the pointer value, so it "falls through to end of if statement" and keeps the NULL hypothesis.
  • line 919: ucp_address_unpack(worker, worker_attr.address, ...) → ucp_address_unpack_header() dereferences buffer (src/ucp/wireup/address.c:1135-1137), producing the explicit-null-dereference report.

Reading ucp_worker_address_pack() (src/ucp/core/ucp_worker.c:3114-3158) confirms this is not a real bug: the empty-device-bitmap case returns UCS_ERR_NO_DEVICE, so on a UCS_OK return the address is always allocated. It is a Coverity modeling limitation, but the CI gate treats any new defect as a failure, and this test code is new in this PR.

Implicated commit: 7959184 "UCP/TEST: Simplify worker address device tests" (shachar hasson, 2026-08-24), on top of ca965ae "UCP/WORKER: Add device filter for worker address query" (same author)

File: test/gtest/ucp/test_ucp_worker.cc:886-919 (defect reported at line 919)

Suggested fix: Make the null check explicit and no-return so Coverity prunes the NULL path, e.g. in check_address_by_device():

status = ucp_worker_query(worker, &worker_attr);
ASSERT_UCS_OK(status);
if (worker_attr.address == nullptr) {
    UCS_TEST_ABORT("ucp_worker_query() returned NULL address for device " <<
                   address_device_name);
}

status = ucp_address_unpack(worker, worker_attr.address, ...);

UCS_TEST_ABORT throws, which Coverity treats as not returning (a plain if (worker_attr.address == nullptr) { return; } after the ASSERT_NE also works). Alternatively copy into a local void *address = worker_attr.address; and guard that. Note the same function should also free the address with ucp_worker_release_address() on every exit path — worth confirming while touching this code.

Related: PR #11717 (this PR); precursor commit ca965ae added the address_device_name/address_flags query API. No existing issue found for this Coverity signature.

@shasson5

Copy link
Copy Markdown
Contributor Author

@svc-nvidia-pr-review

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@shasson5 shasson5 removed the WIP-DNM Work in progress / Do not review label Aug 24, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

Comment thread src/ucp/core/ucp_worker.c Resolved
Comment thread src/ucp/core/ucp_worker.c Resolved
Comment thread src/ucp/core/ucp_worker.c Resolved
Comment thread test/gtest/ucp/test_ucp_worker.cc Resolved
Comment thread test/gtest/ucp/test_ucp_worker.cc Resolved
@shasson5
shasson5 requested a review from rakhmets August 24, 2026 16:10
@svc-ucx

svc-ucx commented Aug 25, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit 2be90a8e

TL;DR: The gpu worker-3 gtest run failed on a single unrelated test, tcp/test_uct_sockaddr.err_handle/2 </lo>, which is a long-standing known flake (issues #6451, #6968) where the expected "server unavailable" error event never arrives; retrigger the job — nothing in PR #11717 (UCP worker-address device filter) touches UCT CM code.

Full analysis

Summary: make test in the "gpu on worker 3" job exited with Error 1 because 1 of 10212 gtests failed: [ FAILED ] tcp/test_uct_sockaddr.err_handle/2, where GetParam() = /lo (all other tests passed; 20 skipped).

Root cause: From the log, the run completed normally (continuous output, no hang, no crash/sanitizer report) and only this one assertion-level test failed — so it is a test-logic flake, not a build/timeout problem. test_uct_sockaddr.err_handle (test/gtest/uct/test_sockaddr.cc:990-1004) calls connect() to a port on lo that the test never binds — the port comes from ucs::get_port() in init() (test_sockaddr.cc:142-144) and start_listen() is deliberately not called. The test then requires the client CM to report UCS_ERR_UNREACHABLE/NOT_CONNECTED/CONNECTION_RESET so that TEST_STATE_CLIENT_GOT_SERVER_UNAVAILABLE is set (handle_client_connecting_status, test_sockaddr.cc:205-221). If that error event is not delivered within the wait_for_bits deadline — because the unreserved port happened to be occupied by another process on the shared node (swx-rdmz-ucx-gpu-01, which hosts several parallel gtest workers), or because the tcp_sockcm EPOLLERR notification was lost — the EXPECT_TRUE at line 1003 fails. This exact test/param combination is already tracked as open flakiness in openucx/ucx issues #6451 and #6968. PR #11717 ("UCP/WORKER: Add device filter for worker address query") only changes UCP worker-address query and cannot affect the UCT tcp_sockcm connection-manager path, so the failure is not attributable to commit 2be90a8.

Implicated commit: Not the PR commit. Pre-existing flaky test; nearest changes to this area are b82a920 (Yossi Itigin, "UCT/TCP: Bound negative connect test timeout") and 2faf305 (Leonid Genkin, "UCS/ASYNC: Filter stale missed fd events"), which touch test/gtest/uct/test_sockaddr.cc and src/uct/tcp/tcp_sockcm_ep.c.

File: test/gtest/uct/test_sockaddr.cc:990-1004 (assert at :1003); port selection at test/gtest/uct/test_sockaddr.cc:142-144

Suggested fix:

  1. Immediate: re-run the "gpu on worker 3" job — this is a single known-flaky UCT CM test unrelated to the PR's UCP change; do not block the PR on it.
  2. Durable fix for the test: make the "no listener" port deterministic instead of hoping a random port is free — bind/listen on the port via start_listen(), record m_listen_addr.get_port(), then uct_listener_destroy(m_server->revoke_listener()) before connect(), so the port is provably unbound and connect() must be refused. Additionally give wait_for_bits an explicit bounded timeout and print the observed m_state on failure (as conn_to_non_exist_ip does at :1183) so the next occurrence is diagnosable.
  3. If it keeps reproducing on /lo after that, investigate whether the tcp_sockcm EPOLLERR is being dropped by the new stale-missed-fd-event filter in 2faf305 (src/ucs/async/async.c, src/uct/tcp/tcp_sockcm_ep.c).

Related: #6451, #6968, #11415, #11458, PR under test: #11717

Comment thread test/gtest/ucp/test_ucp_worker.cc Outdated
}
}

static bool iface_can_connect(const uct_iface_attr_t &attrs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per f2f discussion, please check whether other place implement the same logic -> eliminate code duplication

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of the existing tests checks the content of the returned worker address.
However, In our test we must check the content in order to verify it was filtered by specific device

Comment thread test/gtest/ucp/test_ucp_worker.cc Outdated
}
}

static bool iface_can_connect(const uct_iface_attr_t &attrs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • const methods, where applicable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all methods are either static or calling sender() (which is non-const)

Comment thread test/gtest/ucp/test_ucp_worker.cc Outdated

class test_ucp_worker_address_query : public ucp_test {
public:
enum class address_device_type {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if we can reuse any existing flags / enums from UCX core code or gtest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using directly the flags (INTER_NODE/INTRA_NODE..) makes the code less readable. IMO better stay with this enum

Comment thread src/ucp/core/ucp_worker.c Resolved
Comment thread test/gtest/ucp/test_ucp_worker.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@shasson5
shasson5 requested a review from gleon99 August 25, 2026 16:33
@shasson5
shasson5 merged commit a4d764e into openucx:master Aug 26, 2026
162 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants