UCT/BASE: revert log level and token cap

[jeynmann]

What?

revert log level and token cap

Why?

Do not change log level for all transports.
Set token cap until token, purge and in progress return code are all supported.

How?

Revert log level changes.
Revert cap flags.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

UCS_INPROGRESS is a documented valid return of uct_error_handler_t ("error handling in progress"), but changing the condition from UCS_STATUS_IS_ERR(err_handler_status) to err_handler_status != UCS_OK now treats it as FATAL. This function is shared by cma, srd, rc_verbs, rc_mlx5, and dc_mlx5 failure paths, so a handler returning UCS_INPROGRESS would now cause a FATAL log (process abort) instead of the normal per-transport failure level. Is escalating UCS_INPROGRESS to FATAL intended? If not, keep UCS_STATUS_IS_ERR(err_handler_status).

[svc-nvidia-pr-review]

Scope: the log-level change in uct_iface.h and the removal of UCT_IFACE_FLAG_V2_QUERY_TOKEN in rc_mlx5_iface.c / the test look unrelated. Per REVIEW.md, bug-fix/feature PRs should stay focused. If they are logically tied, the description should make that clear; otherwise consider splitting. (Minor.)

Coverage: the query_token_support test now only asserts the flag is absent; there is no regression test for the log-level change (no test exercises a UCS_INPROGRESS return from an error handler). If the escalation change is intentional, a focused test would help; if not, it should be reverted rather than tested.

[roiedanino]

Do we still need that test? it just check query_v2 is not returning V2_QUERY_TOKEN

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

err_handler_status != UCS_OK escalates the documented UCS_INPROGRESS return to FATAL. uct_base_iface_failure_log_level() receives the error handler's return value via uct_iface_handle_ep_err(), i.e. the value returned by uct_error_handler_t. That callback's contract in src/uct/api/uct_def.h explicitly allows three outcomes: UCS_OK (handled), UCS_INPROGRESS ("error handling is in progress and the transport should not purge outstanding operations"), and otherwise (not handled). UCS_STATUS_IS_ERR(x) is x < 0, and UCS_INPROGRESS is positive. So the old code escalated to UCS_LOG_LEVEL_FATAL only for real errors and treated UCS_INPROGRESS as a non-fatal, in-progress case. After the change, UCS_INPROGRESS != UCS_OK is true, so a handler that returns the documented UCS_INPROGRESS now produces a FATAL failure log. This looks like an unintended behavior change rather than a fix. No in-tree error handler currently returns UCS_INPROGRESS (ucp_worker_iface_error_handler returns UCS_OK/UCS_ERR_NO_ELEM; all gtest handlers return UCS_OK), so this is not caught by existing coverage and would only bite an external handler using the documented API contract. Is escalating UCS_INPROGRESS to FATAL intended? If so, update the callback documentation in uct_def.h and add a focused test; if not:

Suggested change

	if (err_handler_status != UCS_OK) {
	if (UCS_STATUS_IS_ERR(err_handler_status)) {

[svc-nvidia-pr-review]

Scope — the two changes are unrelated. The uct_iface.h log-level change and the rc_mlx5_iface.c / test removal of UCT_IFACE_FLAG_V2_QUERY_TOKEN reporting have no logical connection. Per REVIEW.md, bug-fix/feature PRs should stay focused; either the description should tie them together or they should be split. (Already raised; support the existing thread.)

Notes on the token-flag removal (change 2): the flag macro UCT_IFACE_FLAG_V2_QUERY_TOKEN remains defined in the public uct_v2.h (still documented as a capability), but rc_mlx5 no longer reports it. If nothing consumes the flag anymore, that's consistent; if any consumer still queries it, they'll now always see it unset. I found no in-tree consumer of the flag other than the deleted test, so removing the report and its test is internally consistent.

Residual coverage gap: no test exercises the log-level path with a UCS_INPROGRESS (or other positive, non-OK) handler return, which is exactly the behavior this diff changes. If the escalation is intentional, add a focused test; otherwise revert the uct_iface.h hunk.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests roce on worker 3) · commit 11bd5280

TL;DR: The only failure in the whole roce on worker 3 gtest run is ib/test_md.alloc/1 (mlx5_1) — an IB memory-domain alloc/registration stress test that is unrelated to this PR's change (a log-level / rate-limit token-cap revert in src/uct/base/uct_iface.h); it is an environment-sensitive flake on the loaded worker, so re-run the job and harden the test's allocation-failure handling rather than changing product code.

Full analysis

Summary: make test in build-test/test/gtest exited with code 1: 8690 of 8691 tests passed, [ FAILED ] ib/test_md.alloc/1, where GetParam() = mlx5_1.

Root cause: From the log, the failing case is test_md.alloc for the ib component on device mlx5_1 (test/gtest/uct/test_md.cc:413). That test performs 300 randomized uct_mem_alloc() + uct_md_mkey_pack_v2() (INVALIDATE_RMA|INVALIDATE_AMO) cycles per allocatable memory type and only tolerates UCS_ERR_NO_MEMORY up to 50% of iterations (EXPECT_LT((double)num_alloc_failures / iterations, 0.5)), while its log filter (ignore_alloc_failure_log_handler, lines 394-411) suppresses only messages containing "failed to allocate" or "exceeds maximal supported size" — any other allocation/registration error text (e.g. an ibv_reg_mr/mmap failure on a memory-pressured host) is treated as an unexpected error and fails the test. Nothing in the PR touches this path: the PR's only source change under src/uct/base is src/uct/base/uct_iface.h (commit 408e6d6 "UCT/BASE: revert log level and token cap"), which affects endpoint error-handler/failover logging, and the alloc test creates no endpoints and no IB MD code (src/uct/ib/base/ib_md.c) was modified. Every other IB/RoCE test on the same devices (mlx5_0:1, mlx5_1:1) passed, including all other test_md cases, which is consistent with a transient host-resource flake rather than a code regression.
Caveat: the Azure log is truncated to the tail, so only the gtest summary line for this test is available — the assertion text itself scrolled out of the retained log (the failure occurred early in the ~3565 s run).

Implicated commit: none in this PR; the PR head commit 11bd528 / 408e6d6 (Zihao Zhao, UCT/BASE: revert log level and token cap) does not touch the failing code path.

File: test/gtest/uct/test_md.cc:413 (test body), test/gtest/uct/test_md.cc:394-411 (log filter), test/gtest/uct/test_md.cc:493 (failure threshold)

Suggested fix: Re-trigger the roce on worker 3 job to confirm the flake; the PR should not be blocked on it. To stop recurrence, make test_md.alloc resilient to host memory pressure: (1) also treat registration/mapping errors as retryable by extending the err_logs ignore list in ignore_alloc_failure_log_handler (e.g. "ibv_reg_mr", "Cannot allocate memory", "mmap"), and (2) map those failures into num_alloc_failures with the existing usleep() backoff instead of failing immediately. If it reproduces deterministically on this worker, capture the untruncated gtest output (run with --gtest_filter='ib/test_md.alloc*' and UCX_LOG_LEVEL=info) to obtain the actual assertion line before making any product-code change.

Related: PR #11851 (this PR, UCT/BASE: revert log level and token cap); PR #10150 "TEST/MD: Add short sleep before retrying memory allocation" — prior flakiness fix for this exact test.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

The condition change is a behavioral regression for a documented, valid handler return value. err_handler_status != UCS_OK now escalates a UCS_INPROGRESS return to FATAL, but uct_error_handler_t documents UCS_INPROGRESS as a valid non-error return ("The error handling is in progress and the transport should not purge outstanding operations."). Because FATAL at/below log_level_trigger calls ucs_fatal_error_message (i.e. abort), this can turn a legitimate in-progress error-handling flow into a crash. The old UCS_STATUS_IS_ERR(err_handler_status) (status < 0) escalated only genuine errors and let UCS_INPROGRESS (positive) fall through to the normal ERROR/failure_level path. Should this stay UCS_STATUS_IS_ERR(err_handler_status), or is escalating positive statuses intentional?

[svc-nvidia-pr-review]

Scope: the two changes (uct_iface.h condition change and the token-flag removal in rc_mlx5_iface.c + test) are unrelated. Already covered in an unresolved thread — +1 rather than repeat.

Token-flag removal internal consistency: UCT_IFACE_FLAG_V2_QUERY_TOKEN remains defined and documented in uct_v2.h, but rc_mlx5 no longer reports it and the only in-tree consumer was the deleted test, so the removal is internally consistent. If this is a deliberate revert, the PR description should say so.

Residual coverage gap: no test exercises the uct_base_iface_failure_log_level path with a positive (UCS_INPROGRESS) handler return, which is exactly the behavior the diff changes.