DAOS-19326 cart: bulk transfer issued from CORPC root handler fails by frostedcmos · Pull Request #18729 · daos-stack/daos

frostedcmos · 2026-07-27T21:57:53Z

When sending CORPC to self the mercury handle and associated address are not available. Instead save self address in the cart context and use it when mercury handle is not available (sending to self)
Added test_corpc_bulks ftest to test inline corpc bulks as well as explicit bulk transfers from within corpc handlers.

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

- When sending CORPC to self the mercury handle and associated address are not available. Instead save self address in the cart context and use it when mercury handle is not available (sending to self) - Added test_corpc_bulks ftest to test inline corpc bulks as well as explicit bulk transfers from within corpc handlers. Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

github-actions · 2026-07-27T22:03:42Z

Ticket title is 'CART: bulk transfer issued from CORPC root handler fails/crashes'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-19326

daosbuild3 · 2026-07-27T22:10:37Z

Test stage Build on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18729/2/execution/node/521/log

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

frostedcmos · 2026-07-28T17:19:41Z

New tests being ran/passing:
https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-18729/3/artifact/Functional%20on%20EL%209/cart/corpc_one_node.py/test-results/09-._cart_corpc_one_node.py_CartCoRpcOneNodeTest.test_cart_corpc_one_node_run-ENV-env_D_PROVIDER-ofi_tcp-hosts-hosts_1-tests-corpc_bulk_inline-2cf5

https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-18729/3/artifact/Functional%20on%20EL%209/cart/corpc_one_node.py/test-results/10-._cart_corpc_one_node.py_CartCoRpcOneNodeTest.test_cart_corpc_one_node_run-ENV-env_D_PROVIDER-ofi_tcp-hosts-hosts_1-tests-corpc_bulk_explicit-0ade

soumagne · 2026-07-28T17:14:38Z

+	/* cache self address */
+	hg_ret = HG_Addr_self(hg_ctx->chc_hgcla, &hg_ctx->chc_self_addr);
+	if (hg_ret != HG_SUCCESS) {
+		hg_ctx->chc_self_addr = HG_ADDR_NULL;


if it fails, the address should not have been touched so in theory there's no need to reset it...

soumagne · 2026-07-28T17:16:28Z

 	crt_hg_pool_fini(hg_ctx);

+	/* Free the cached self address while the HG class is still valid */
+	if (hg_ctx->chc_self_addr != HG_ADDR_NULL && hg_ctx->chc_hgcla != NULL) {


hg_ctx->chc_hgcla should always be non-null here no ?

changed to assert

soumagne · 2026-07-28T17:26:21Z

+		} else {
+			ctx_idx   = ctx->cc_idx;
+			orig_addr = hg_ctx->chc_self_addr;
+		}


I guess I'm a little scared here that you could fall into a case where it's neither a bind bulk transfer nor a corpc, in which case if for some reason the HG address is not set, you'd transparently use the self address without realizing it. To some extent it would be nice if there were a `else if (corpc == true) { use self address } else {error}. And I'm mostly saying that because the logic is within crt_hg itself which I think should really have nothing to do with corpcs in the first place...

added corpc checks

soumagne · 2026-07-28T17:26:55Z

+#include <unistd.h>
+
+#include <cart/api.h>
+#include "crt_utils.h"


headers are still in the wrong order :)

soumagne · 2026-07-28T17:28:17Z

+	assert(0);
+}
+
+#define error_exit() __error_exit(__LINE__, __func__);


are there not macros already defined for that ? :)

i didnt find one; we should do it in crtu_utils, but probably in separate pr

soumagne · 2026-07-28T17:28:29Z

+static int
+corpc_aggregate(crt_rpc_t *src, crt_rpc_t *result, void *priv)
+{
+	return 0;


not doing anything ?

- co_aggregate is now optional in corpcs - removed unneeded empty corpc_ops from tests - corpc checks added in bulk transfer when handle is null - naming cleanup Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

liw · 2026-07-28T23:53:55Z

 				memset(child_rpc_priv->crp_pub.cr_output, 0,
 				       child_rpc_priv->crp_pub.cr_output_size);
 			} else {
-				D_ASSERT(co_ops->co_aggregate != NULL);


[Question] Why change the co_aggregate assertions?

in few tests, including an earlier version of this test, an empty co_ops with empty co_aggregate had to be declared just to work. all other co_* callbacks are optional; as such after talking with Jerome we decided to change this one as well and make it optional. The original ASSERT here is from 2017

liw · 2026-07-28T23:58:34Z

+			/* regular RPCs must have a valid handle */
+			if (!rpc_priv->crp_coll) {
+				RPC_ERROR(rpc_priv, "Unexpected crp_hg_hdl=NULL\n");
+				D_GOTO(out, rc = -DER_INVAL);
+			}


[Question] Not knowing the answer, I wonder if this should be an assertion?

The main intention here was to dump/log RPC_ERROR with a problematic 'rpc_priv' before failing, however an assert would be better here indeed instead of D_GOTO() with a failure that we cant recover from.

daltonbohning

ftest LGTM

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

daosbuild3 · 2026-07-31T03:30:03Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18729/6/testReport/

frostedcmos · 2026-07-31T17:20:13Z

1 test pool/mem_ratio.py failed due to SSD error on hdr-102: https://daosio.atlassian.net/browse/DAOS-19271

soumagne · 2026-07-31T17:45:54Z

-				if (co_info->co_rc == 0)
-					co_info->co_rc = rc;
+
+			if (co_ops && co_ops->co_aggregate) {


minor but since it's a d_list_for_each loop, you could have done:

if (!co_ops || !co_ops->co_aggregate) continue;

soumagne · 2026-07-31T17:47:35Z

-						child_req->cr_opc, DP_RC(rc));
-					if (co_info->co_rc == 0)
-						co_info->co_rc = rc;
+				if (co_ops && co_ops->co_aggregate) {


also here could have combined else and else if :) to save on braces...

soumagne · 2026-07-31T18:02:20Z

+			D_ERROR("HG_Bulk_(bind)transfer failed, hg_ret: " DF_HG_RC "\n",
+				DP_HG_RC(hg_ret));
+			D_FREE(bulk_cbinfo);
+			D_FREE(bulk_desc_dup);


I don't know if I'm not reading correctly but it seems that the D_FREE here do not apply to the previous HG_Bulk_bind_transfer if it fails

good catch, thanks. fixed

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

frostedcmos requested review from a team as code owners July 27, 2026 21:57

- style

38ad47e

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

frostedcmos requested review from jgmoore-or, liw and soumagne July 27, 2026 22:10

- cleanup

fa72503

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

soumagne reviewed Jul 28, 2026

View reviewed changes

Address review feedback and cleanup:

be1ac95

- co_aggregate is now optional in corpcs - removed unneeded empty corpc_ops from tests - corpc checks added in bulk transfer when handle is null - naming cleanup Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

liw reviewed Jul 28, 2026

View reviewed changes

daltonbohning reviewed Jul 29, 2026

View reviewed changes

frostedcmos added 2 commits July 29, 2026 20:03

- review feedback

5ccaf92

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

Merge branch 'master' into frostedcmos/DAOS-19326

6c79038

mjmac mentioned this pull request Jul 30, 2026

DAOS-623 test: instrument NLT fault-injection sweep hang #18756

Draft

frostedcmos requested review from liw and soumagne July 31, 2026 17:22

frostedcmos added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jul 31, 2026

soumagne requested changes Jul 31, 2026

View reviewed changes

- error cleanup for both branches

245fd5a

Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

soumagne approved these changes Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

frostedcmos commented Jul 27, 2026

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Jul 27, 2026

Uh oh!

frostedcmos commented Jul 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daltonbohning left a comment

Choose a reason for hiding this comment

Uh oh!

daosbuild3 commented Jul 31, 2026

Uh oh!

frostedcmos commented Jul 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Jul 27, 2026 •

edited

Loading