DAOS-16971 cart: add RPC origin address #15820

wangdi1 · 2025-01-30T23:02:37Z

Add cart RPC origin address to error handling.

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

github-actions · 2025-01-30T23:02:53Z

Ticket title is 'bulk cancel causing libfabric progress failure.'
Status is 'Awaiting Verification'
Labels: 'GCP,google-cloud-daos'
https://daosio.atlassian.net/browse/DAOS-16971

frostedcmos

Few comments inline. Perhaps for the error cases we can include 'origin info' as part of RPC_ERROR and RPC_WARN as well - while there is some overhead it might be ok for error cases and help with debug.

frostedcmos · 2025-01-30T23:47:14Z

src/object/srv_obj.c

+		uint32_t addr_size = 48;
+
+		crt_rpc_get_origin_addr(rpc, addr, &addr_size);
+		D_ERROR("rpc %p opc %d send reply, pmv %d, epoch " DF_X64 ", status %d from %s\n",


For a case like this it would be better to add a public variant of RPC_ERROR() macro and use it to print additional info, as it will print decoded opcode for example along with rpcid and other fields that are helpful in debug.

frostedcmos · 2025-01-30T23:47:49Z

src/object/srv_obj.c

+		D_ERROR("rpc %p opc %d send reply, pmv %d, epoch " DF_X64 ", status %d from %s\n",
+			rpc, opc_get(rpc->cr_opc), ioc->ioc_map_ver, orwo->orw_epoch, status, addr);
+	} else {
+		D_DEBUG(DB_IO, "rpc %p opc %d send reply, pmv %d, epoch " DF_X64 ", status %d\n",


same here but a variant of RPC_DEBUG

frostedcmos · 2025-01-30T23:48:56Z

src/include/cart/api.h

@@ -2375,6 +2376,18 @@ int crt_context_quota_limit_get(crt_context_t crt_ctx, crt_quota_type_t quota, i
 int
 crt_req_get_proto_ver(crt_rpc_t *req);

+/**
+ * Get the rpc original address.


s/original/origin/ ?

frostedcmos · 2025-01-30T23:51:04Z

src/include/cart/api.h

+ *
+ * \param[in] rpc		pointer to RPC request
+ * \param[out] addr		pointer to the converted buffer.
+ * \param[in/out] addr_size 	the size of the converted buffer.


nit: would be more clear for me for this to be 'addr_len' instead.

frostedcmos · 2025-01-30T23:55:39Z

src/object/srv_obj.c

+		uint32_t addr_size = 48;
+
+		crt_rpc_get_origin_addr(cb_info->bci_bulk_desc->bd_rpc, addr, &addr_size);
+		D_ERROR("bulk transfer failed: %d from %s\n", cb_info->bci_rc, addr);


maybe we should then include origin_addr info for any transfer fail inside of crt_bulk_transfer/cb so that it covers other places too?

ah, good suggestion.

frostedcmos · 2025-01-30T23:56:55Z

src/object/srv_obj.c

+		uint32_t addr_size = 48;
+
+		crt_rpc_get_origin_addr(rpc, addr, &addr_size);
+		D_ERROR(


same here, a public variant of RPC_ERROR (e.g. D_RPC_ERROR) would be handy here to add and use

github-actions · 2025-01-31T01:15:44Z

Functional on EL 8.8 Test Results

131 tests 127 ✅ 1h 30m 18s ⏱️
41 suites 4 💤
41 files 0 ❌

Results for commit ee2ba3f.

♻️ This comment has been updated with latest results.

johannlombardi

I am personally not a big fan of spreading calls to crt_rpc_get_origin_addr() everywhere in the source code. We cannot use the TLS or even put it in a field of the cart RPC structure?

frostedcmos · 2025-01-31T08:58:11Z

I am personally not a big fan of spreading calls to crt_rpc_get_origin_addr() everywhere in the source code. We cannot use the TLS or even put it in a field of the cart RPC structure?

yeah, we've been further discussing this and jeff also suggested storing resolved addr on first lookup in rpc_priv

github-actions · 2025-01-31T09:04:57Z

Functional Hardware Large Test Results

64 tests 64 ✅ 29m 32s ⏱️
14 suites 0 💤
14 files 0 ❌

Results for commit 24f7c0c.

github-actions · 2025-01-31T11:22:43Z

Functional Hardware Medium Test Results

154 tests 127 ✅ 2h 4m 42s ⏱️
38 suites 27 💤
38 files 0 ❌

Results for commit 24f7c0c.

github-actions · 2025-01-31T14:39:25Z

Functional Hardware Medium Verbs Provider Test Results

55 tests 54 ✅ 4h 36m 37s ⏱️
7 suites 1 💤
7 files 0 ❌

Results for commit 24f7c0c.

Add cart RPC origin address to daos engine log. Run-GHA: true Allow-unstable-test: true Signed-off-by: Di Wang <[email protected]>

wangdi1 · 2025-01-31T19:22:34Z

I am personally not a big fan of spreading calls to crt_rpc_get_origin_addr() everywhere in the source code. We cannot use the TLS or even put it in a field of the cart RPC structure?

yeah, I put it to rpc_private structure, though still get it by this API in D_ERROR or D_DEBUG

frostedcmos

lgtm; one nitpick inline and you also might want to merge with the latest master as i've modified RPC_* macros few days ago there as well

frostedcmos · 2025-01-31T19:40:45Z

src/include/cart/api.h

@@ -2375,6 +2376,15 @@ int crt_context_quota_limit_get(crt_context_t crt_ctx, crt_quota_type_t quota, i
 int
 crt_req_get_proto_ver(crt_rpc_t *req);

+/**
+ * Get the rpc original address.


still a typo:)

daosbuild1 · 2025-01-31T20:05:36Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15820/3/testReport/

fix style issue Signed-off-by: Di Wang <[email protected]>

fix wording. Signed-off-by: Di Wang <[email protected]>

daosbuild1 · 2025-01-31T21:40:59Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15820/4/testReport/

fix mem leak Signed-off-by: Di Wang <[email protected]>

daosbuild1 · 2025-01-31T23:02:46Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15820/5/testReport/

Revert some log changes to please CI Run-GHA: true Allow-unstable-test: true Signed-off-by: Di Wang <[email protected]>

daosbuild1 · 2025-02-01T03:04:56Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15820/6/testReport/

frostedcmos · 2025-02-01T07:51:58Z

src/cart/crt_bulk.c

@@ -203,7 +205,8 @@ crt_bulk_bind_transfer(struct crt_bulk_desc *bulk_desc,

 	rc = crt_hg_bulk_transfer(bulk_desc, complete_cb, arg, opid, true);
 	if (rc != 0)
-		D_ERROR("crt_hg_bulk_transfer() failed, rc: %d.\n", rc);
+		DL_ERROR(rc, "%p:%s crt_hg_bulk_transfer() failed.\n", bulk_desc->bd_rpc,


need to remove "\n" from the message as DL_ERROR will append one on its own

frostedcmos · 2025-02-01T07:56:50Z

src/cart/crt_internal.h

@@ -26,6 +27,34 @@
 #include "crt_self_test.h"
 #include "crt_swim.h"

+static inline char *


This should go into crt_internal_fns.h for declaration and crt_rpc.c for the implementation. I don't think there is much benefit from inlining this function anyway

frostedcmos · 2025-02-01T07:58:08Z

src/cart/crt_internal.h

+
+	rc = HG_Addr_to_string(hg_info->hg_class, addr, (hg_size_t *)&addr_size, hg_info->addr);
+	if (rc != 0)
+		return "None";


I think we should either print an error for each case or somehow differentiate None's.
Imagine you see "None" in log, that would be unexpected, but wouldn't tell us which one of those 3 calls failed

frostedcmos · 2025-02-01T08:03:47Z

src/include/cart/api.h

+ * \return			the origin address of the RPC
+ */
+char *
+crt_rpc_get_origin_addr(crt_rpc_t *rpc);


The api functions follow format of crt_[module][verb] (see crt_context_quota_limit_get, crt_context_create and others)
Also in public APIs we refer to 'rpc' as 'req' (e.g. crt_req_create(), crt_req_get_proto_ver() etc) in order to differentiate with crt_rpc functions that are internal.
So based on those 2 things, this should be renamed to something like crt_req_origin_addr_get()

wangdi1 requested review from jolivier23 and frostedcmos January 30, 2025 23:02

wangdi1 requested a review from soumagne January 30, 2025 23:02

wangdi1 force-pushed the wangdi/google_26_rpc_debug branch from db7cb67 to 24f7c0c Compare January 30, 2025 23:24

frostedcmos reviewed Jan 31, 2025

View reviewed changes

johannlombardi reviewed Jan 31, 2025

View reviewed changes

DAOS-16971 cart: add RPC origin address

6d0153b

Add cart RPC origin address to daos engine log. Run-GHA: true Allow-unstable-test: true Signed-off-by: Di Wang <[email protected]>

wangdi1 force-pushed the wangdi/google_26_rpc_debug branch from 24f7c0c to 6d0153b Compare January 31, 2025 19:19

wangdi1 requested review from johannlombardi and frostedcmos January 31, 2025 19:22

frostedcmos reviewed Jan 31, 2025

View reviewed changes

wangdi1 added 2 commits January 31, 2025 20:52

DAOS-16971 cart: fix style

86270c6

fix style issue Signed-off-by: Di Wang <[email protected]>

DAOS-16971 cart: fix wording

b7d4b59

fix wording. Signed-off-by: Di Wang <[email protected]>

DAOS-16971 cart: fix mem leak

c036a00

fix mem leak Signed-off-by: Di Wang <[email protected]>

DAOS-16971 object: revert some log changes

ee2ba3f

Revert some log changes to please CI Run-GHA: true Allow-unstable-test: true Signed-off-by: Di Wang <[email protected]>

frostedcmos reviewed Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16971 cart: add RPC origin address #15820

DAOS-16971 cart: add RPC origin address #15820

wangdi1 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

frostedcmos left a comment

frostedcmos Jan 30, 2025

wangdi1 Jan 31, 2025

frostedcmos Jan 30, 2025

wangdi1 Jan 31, 2025

frostedcmos Jan 30, 2025

wangdi1 Jan 31, 2025

frostedcmos Jan 30, 2025

wangdi1 Jan 31, 2025

frostedcmos Jan 30, 2025

wangdi1 Jan 31, 2025

wangdi1 Jan 31, 2025

frostedcmos Jan 30, 2025

github-actions bot commented Jan 31, 2025 •

edited

Loading

johannlombardi left a comment

frostedcmos commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

wangdi1 commented Jan 31, 2025

frostedcmos left a comment

frostedcmos Jan 31, 2025

wangdi1 Jan 31, 2025

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Feb 1, 2025

frostedcmos Feb 1, 2025

frostedcmos Feb 1, 2025

frostedcmos Feb 1, 2025

frostedcmos Feb 1, 2025

DAOS-16971 cart: add RPC origin address #15820

Are you sure you want to change the base?

DAOS-16971 cart: add RPC origin address #15820

Conversation

wangdi1 commented Jan 30, 2025

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 30, 2025

frostedcmos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2025 • edited Loading

Functional on EL 8.8 Test Results

johannlombardi left a comment

Choose a reason for hiding this comment

frostedcmos commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

Functional Hardware Large Test Results

github-actions bot commented Jan 31, 2025

Functional Hardware Medium Test Results

github-actions bot commented Jan 31, 2025

Functional Hardware Medium Verbs Provider Test Results

wangdi1 commented Jan 31, 2025

frostedcmos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Jan 31, 2025

daosbuild1 commented Feb 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2025 •

edited

Loading