Repository navigation
GTEST/UCT: Add outstanding purge test base - #11891
Conversation
Co-authored-by: Zihao Zhao <zizhao@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| _UCT_INSTANTIATE_TEST_CASE(test_rc_mlx5_token_query, rc_mlx5) | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
blocker: this class has no UCS_TEST* bodies and no _UCT_INSTANTIATE_TEST_CASE, so nothing here runs. The added test_rc_purge_outstanding class defines only helper methods (init, purge_outstanding, send_op, err_handler, purge_cb, query_tx_token, query_rx_token, ensure_undelivered, check_caps_v2) but there are no UCS_TEST*/UCS_TEST_SKIP_COND_P bodies that use it. Looks like the actual purge test is missing — pls add the test case(s) and instantiation, or drop this scaffolding.
| return ucs_test_all_flags(attr.cap.flags, required_flags); | ||
| } | ||
|
|
||
| void send_op(const uct_ep_op_info_t *info, purge_ctx *ctx) |
There was a problem hiding this comment.
send_op: the switch (info->operation) has only a default branch that aborts, so ++ctx->num_replayed is currently unreachable for any real operation; the intended replay/case handling seems unfinished.
|
|
||
| _UCT_INSTANTIATE_TEST_CASE(test_rc_mlx5_token_query, rc_mlx5) | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
check_caps_v2, purge_ctx::comp, and num_replayed are never referenced by any caller.
|
🤖 Starting review — findings will be posted here when done. |
| { | ||
| ASSERT_TRUE(info->field_mask & UCT_EP_OP_INFO_FIELD_OPERATION); | ||
|
|
||
| switch (info->operation) { |
There was a problem hiding this comment.
blocker: the switch only has default: abort, so ++ctx->num_replayed is unreachable and EXPECT_GT(ctx.num_replayed, 0u) (L1721) can never pass — any purge callback invocation aborts the test. Please add the real operation case(s) that replay the op so num_replayed is reachable, or clarify the intended flow.
|
|
||
| _UCT_INSTANTIATE_TEST_CASE(test_rc_mlx5_token_query, rc_mlx5) | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
Class defined but not instantiated in this hunk: where is test_purge_outstanding invoked? I don't see a UCS_TEST_P / _UCT_INSTANTIATE_TEST_CASE for test_rc_purge_outstanding — without it the static helpers are unused (build warnings / dead code) and no test actually runs.
|
|
||
| if (receiver_next_psn == | ||
| uct_ib_mlx5_txwq_get_next_wqe_psn(&rc_ep->tx.wq)) { | ||
| *rx_token = htobe32((receiver_next_psn - 1) & UCS_MASK(24)); |
There was a problem hiding this comment.
minor: PSN wrap in ensure_undelivered — when receiver_next_psn == 0, (receiver_next_psn - 1) & UCS_MASK(24) wraps to 0xFFFFFF. This may be intended (24-bit PSN wraparound); please add a brief comment confirming the wrap is deliberate.
There was a problem hiding this comment.
There's an intentional 24-bit PSN wraparound
| uct_rc_mlx5_tx_token_t tx_token = {}; | ||
| uct_rc_mlx5_rx_token_t rx_token = {}; | ||
|
|
||
| ASSERT_UCS_OK(uct_ep_invalidate(m_e1->ep(0), &invalidate_params)); |
There was a problem hiding this comment.
I think we need to inject ep_invalidate in the tight loop of posts to have both - delivered and undelivered ops
There was a problem hiding this comment.
Yes, it’s associated with the previous comments for progress. Please note that once error CQE is progressed, we won’t be able to post anything.
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Azure job "Coverity coverity devel on coverity_rh7" (build 134183) failed at the Root cause: In the new switch (info->operation) {
default:
UCS_TEST_ABORT("unsupported operation " << info->operation);
}
++ctx->num_replayed; // line 1614The Implicated commit: File: Suggested fix: Make the replay path reachable. Preferred: land the concrete operation cases in the same commit, e.g. switch (info->operation) {
case UCT_EP_OP_AM_SHORT:
/* re-post the AM ... */
break;
/* other supported ops ... */
default:
UCS_TEST_ABORT("unsupported operation " << info->operation);
}
++ctx->num_replayed;If the handler must stay a stub for now, avoid the dead-code pattern instead of relying on a if (info->operation != <supported_op>) {
UCS_TEST_ABORT("unsupported operation " << info->operation);
}
/* replay ... */
++ctx->num_replayed;Do not suppress this with a Coverity annotation; the report is a genuine sign that the incremental commit split left an unfinished code path. Note this build is only failing on this single new defect, so fixing it should make the Coverity stage green (baseline count returns to 0). Related: PR #11891 (branch |
| } while (status == UCS_ERR_NO_RESOURCE); | ||
| } | ||
|
|
||
| void send_op(const uct_ep_op_info_t *info, purge_ctx *ctx) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| EXPECT_EQ(UCS_INPROGRESS, | ||
| uct_ep_flush(m_e1->ep(0), 0, flush_comp)); | ||
|
|
||
| wait_for_flag(&m_err_count); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| _UCT_INSTANTIATE_TEST_CASE(test_rc_mlx5_token_query, rc_mlx5) | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
blocker: this whole class (test_rc_purge_outstanding) and all its helpers (init, post_ops, purge_outstanding, test_purge_outstanding, check_caps_v2, etc.) is never instantiated — there's no UCS_TEST_P/UCS_TEST_SKIP_COND_P calling test_purge_outstanding() and no _UCT_INSTANTIATE_TEST_CASE. A tree-wide search finds no other references. As-is it adds no coverage and the unused static/private helpers may fail to compile with -Werror. Are the test bodies / instantiation missing from this PR?
| } | ||
| } | ||
|
|
||
| void replay_op(const uct_ep_op_info_t *info, purge_ctx *ctx) |
There was a problem hiding this comment.
replay_op always aborts in the default: arm, so ++ctx->num_replayed is unreachable (marked /* coverity[unreachable] */), yet test_purge_outstanding asserts EXPECT_GT(ctx.num_replayed, 0u) — that can never pass. If purge_cb is invoked for any operation the test aborts, and if it's never invoked num_replayed stays 0. This looks like the base is meant to be subclassed with a real replay_op; the derived test and instantiation appear to be missing.
|
🤖 CI Triage Agent — TL;DR: The EFA/valgrind gtest run failed on a single UD test, Full analysisSummary: Root cause: In the failing instance the test expected a private UD endpoint on the passive side to exist for the (reused) Evidence that this is a timing/flakiness issue in the test rather than a regression from this PR:
Implicated commit: Not this PR's commit (f7ee0c1). The test and helper originate from PR #11822 "UCT/UD: Remove stale private UD connections" (exact sha not visible in the history snapshot available to me; nearest related commit on these files is File: test/gtest/uct/ib/test_ud.cc:963 (assertion Suggested fix:
Related: PR #11891 (this PR), PR #11822 (UCT/UD: Remove stale private UD connections — added the failing test), PR #11899 (v1.23.x backport), PR #11800 / commit 0ee470a (UD dest_id generation ID) |
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| _UCT_INSTANTIATE_TEST_CASE(test_rc_mlx5_token_query, rc_mlx5) | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
This whole class has no UCS_TEST_* cases and no _UCT_INSTANTIATE_TEST_CASE, so test_purge_outstanding() and its helpers are never compiled into any running test. Was the intent to also add the test cases and instantiation? As-is the new purge coverage does not run.
| { | ||
| ASSERT_TRUE(info->field_mask & UCT_EP_OP_INFO_FIELD_OPERATION); | ||
|
|
||
| switch (info->operation) { |
There was a problem hiding this comment.
As written the switch (info->operation) only has a default that aborts, so any purged op makes the test fail. Since the test expects ctx.num_purged > 0, the first purge_cb → validate_op() invocation would abort the test. Please handle the operations the test actually posts with explicit case branches (e.g. UCT_EP_OP_AM_SHORT, UCT_EP_OP_AM_BCOPY, UCT_EP_OP_AM_ZCOPY, UCT_EP_OP_PUT_SHORT, UCT_EP_OP_PUT_BCOPY, UCT_EP_OP_PUT_ZCOPY).
|
|
|
🤖 CI Triage Agent — TL;DR: The gtest binary aborted on a debug assertion in the DC send path — Full analysisSummary: Root cause: From the log, the sequence is: the test injects a local failure with Implicated commit: Not this PR. Most likely candidates, all landed days before this run and all touching exactly this path: File: Suggested fix:
Related: PR #11891 (the build under test, gtest-only); #11829, #11840, #11563, #11562 (recent DC error-handling / GET-failover / FT recovery work); #11397 previously disabled a flaky fault-tolerance test; #7711 / #7071 are earlier instances of "don't send on a failed DCI" fixes.
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
Residual coverage note: even once test cases are added, |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
roiedanino
changed the title
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
roiedanino
changed the title
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: ASAN roce gtest shard 1/4 aborted: Root cause: Two-stage failure, both visible in the log:
This looks like a latent/environment-sensitive test bug (aggravated by ASAN timing on the RoCE worker), not something introduced by the purge-test-base change in PR #11891. Implicated commit: unknown — the File: test/gtest/uct/test_p2p_am.cc:726 (skip condition) and test/gtest/uct/uct_p2p_test.cc:93 / test/gtest/uct/uct_test.cc:766 (unbounded cleanup flush) Suggested fix:
Related: PR #11891 (this build); adjacent purge work in #11857 — no existing issue found for |
| {completion_cb, 0, UCS_OK}, | ||
| {completion_cb, 0, UCS_OK}, |
There was a problem hiding this comment.
why need to initialize both with the same value?
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| UCT_INSTANTIATE_TEST_CASE(test_uct_peer_failure) | ||
|
|
||
| class test_uct_purge_outstanding : public uct_test { |
There was a problem hiding this comment.
This adds the whole test_uct_purge_outstanding fixture and its helpers, but there are no UCS_TEST_*P cases and no UCT_INSTANTIATE_TEST_CASE(test_uct_purge_outstanding), so none of it runs. pls add the actual test case(s) and instantiation, or drop the dead fixture.
|
Even with test cases added, |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
What?
Add a reusable UCT endpoint outstanding-purge test fixture.
The fixture centralizes connected endpoint setup, capability checks, deterministic posting around endpoint invalidation, token exchange, purge callback dispatch, operation descriptor validation, and completion handling. This PR intentionally does not instantiate an operation-specific test.
Why?
AM short, AM bcopy, and PUT zcopy purge tests need the same fault-tolerance flow. Sharing the common UCT-level mechanics keeps the operation-specific PRs small and avoids duplicated test logic between #11857 and #11874.
How?
The fixture uses only public UCT APIs and skips transports that do not advertise peer-failure handling and token-query support. It first completes one operation, then posts an operation and an endpoint flush before endpoint invalidation, fills the remaining send resources without progress, and purges using the receiver token. Common validation covers operation identity, completion ownership, flush metadata, purge counts, and flush cancellation. Follow-up PRs add an operation case to validate_op() and a test that supplies its send function.