Revert check for HCA reachability like rc_mlx5 (#11781) - master

[ofirfarjun7]

What?

Revert #11801

Why?

Reachability check of ib can be wrong for gda

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

why not keep uct_ib_iface_is_reachable_v2 here? gdaki packs a real IB device address and its ep_connect_to_ep_v2 builds the AH from that address via uct_ib_iface_fill_ah_attr_from_addr. With the stub (always return 1), gdaki reports itself reachable to any peer, including a different link layer (IB vs RoCE), a different subnet without FLID, or a mismatched pkey — cases the real check would reject. Since rc_gda participates in UCP wireup (in the ib alias, used as "rc,rc_gda"), UCP could select the gdaki lane toward a genuinely unreachable peer. ep_is_connected still does real device matching and gdaki is normally paired with a primary rc lane that does its own reachability check, so this may be acceptable by design — but pls explain in the description why bypassing reachability is safe for wireup lane selection.

[svc-nvidia-pr-review]

Dropping _UCT_INSTANTIATE_TEST_CASE(test_uct_iface, rc_gda) removes the only reachability coverage for gdaki. The corrupted-address branch cannot pass once the stub is used, but the positive branch (EXPECT_TRUE(is_reachable)) is still valid and worth keeping. Consider keeping a reduced gdaki-specific test that only checks the always-reachable contract, so the behavior is asserted rather than silently untested.

[svc-nvidia-pr-review]

The PR description is not shown; per repository review guidance the "Why?" for switching to always-reachable and for deleting the test should be spelled out in the description.

test_device still instantiates rc_gda and exercises connect/ep_connect_to_ep, so basic connection setup remains covered; only the explicit reachability assertions are lost.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Codestyle commit title) · commit fcc7bc46

TL;DR: The "commit title" codestyle job failed because the PR's commit subject is "UCT/GDAKI: Revert check for HCA reachability like rc_mlx5 (#11781)" — it starts with a literal double-quote (leftover from a git revert-generated Revert "..." subject), so it doesn't match the required MODULE: Description pattern; amend the commit title to remove the surrounding quotes.

Full analysis

Summary: Azure job UCX PR (Codestyle / commit title) build 132774 failed with Bad commit title: '"UCT/GDAKI: Revert check for HCA reachability like rc_mlx5 (#11781)"' and Bash exited with code '1'.

Root cause: codestyle_check_commit_title() validates each non-merge commit subject in base..head against the regex ^[0-9A-Z/_-]+:\ [[:alnum:]_], i.e. the title must begin with an uppercase/digit module prefix followed by : and an alphanumeric character. The commit under review (fcc7bc4, PR #11838, branch topic/revert-gdaki-reachability-master) has a subject whose first character is " — the quotes come from the auto-generated revert subject Revert "UCT/GDAKI: ..." with the Revert word removed but the quotes left in place. The regex therefore fails at offset 0 and the function returns 1. This is a genuine style violation in the PR, not CI infrastructure flakiness — there is no timeout, hang, or environment error anywhere in the log (checkout completed normally at 15:41:31 and the bash step failed 0.3 s later).

Secondary (cosmetic) CI issue visible in the same log: the step body in buildlib/pr/codestyle.yml runs with set -eE, so the non-zero return from codestyle_check_commit_title aborts the script immediately and the if [[ $? -ne 0 ]] branch that would emit the helpful ##vso[task.logissue] message with the contributor-guidelines URL is never reached. The developer only sees Bash exited with code '1'.

Implicated commit: [REDACTED:Hex High Entropy String] (the PR head commit itself; its title is malformed). The checker logic is from buildlib/tools/codestyle.sh / buildlib/pr/codestyle.yml, last touched by cfd9cd1 "BUILD/CI: Limit PR commit range checks" (Yossi Itigin) — not the cause.

File: buildlib/tools/codestyle.sh:14 (regex check), reported from buildlib/pr/codestyle.yml:17-30

Suggested fix:

1. Fix the PR: amend the commit subject to satisfy the pattern, dropping the stray quotes, e.g.

   git commit --amend -m 'UCT/GDAKI: Revert check for HCA reachability like rc_mlx5 (#11781)'
   git push --force-with-lease
   

   (Any MODULE: Text form works; if you want to keep revert semantics explicit, use UCT/GDAKI: Revert HCA reachability check (#11781).)
2. Optional CI improvement so future failures are self-explanatory: in buildlib/pr/codestyle.yml, don't let set -e swallow the diagnostic — e.g.

   if ! codestyle_check_commit_title "$base_revision..$head_revision"; then
       azure_complete_with_issues "Bad commit title(s), see $url for more info."
   fi

   and consider whitelisting Revert "MODULE: ..." subjects in codestyle.sh if auto-generated reverts should be accepted as-is.

Related: PR #11838 (the failing PR); PR #11781 (the commit being reverted); checker introduced/last modified in #11527.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 937db4b1-3a9a-4899-a976-6e933cc219ce in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Making reachability unconditionally return 1 skips valid peer filtering during wireup. The change replaces uct_ib_iface_is_reachable_v2 with ucs_empty_function_return_one_int, so uct_iface_is_reachable_v2() for the gda transport now always reports "reachable". The gda device address is still a standard IB address (iface_get_device_address = uct_ib_iface_get_device_address), so the base IB check works correctly here and every other IB mlx5 transport (rc_mlx5, dc_mlx5, ud_mlx5, gga_mlx5) still uses it. ucp_wireup_select_transport() / ucp_wireup_is_reachable() rely on this to filter out peers with a mismatched pkey, a different IB subnet, or an incompatible/unroutable RoCE address. Unconditionally returning reachable means a gda lane can now be selected toward a peer that the base check would reject, which can defer the failure to connect/QP-transition time instead of lane selection. This warrants an explanation of the "Why?" plus confirmation this is intentional and safe rather than papering over a symptom.

Suggested inline comment: gda device address is a normal IB address and every other IB mlx5 TL keeps uct_ib_iface_is_reachable_v2. why disable the reachability check entirely instead of keeping it? unconditionally returning reachable lets wireup pick a gda lane toward a peer with mismatched pkey / different subnet / unroutable RoCE, moving the failure to connect time.

[svc-nvidia-pr-review]

test/gtest/uct/test_uct_iface.cc

Test removal is a direct consequence, not independently justified. Removing _UCT_INSTANTIATE_TEST_CASE(test_uct_iface, rc_gda) is required because test_is_reachable asserts found_unreachable after feeding corrupted addresses; with the stub always returning reachable that assertion can never be satisfied for gda. This confirms the behavior change is observable. The PR should state why the reachability contract for gda no longer needs coverage (test_device still exercises connect but not reachability).

Suggested inline comment: this removal is only needed because the stub now always returns reachable, so the corrupted-address loop can never see found_unreachable. pls note in the description why gda no longer needs reachability coverage.

Residual coverage gap: no test verifies that gda still rejects genuinely unreachable peers (mismatched pkey/subnet/RoCE); if the always-reachable behavior is intentional, that gap is acceptable, otherwise it should be covered.