DAOS-15540 cart: Change proto query default timeout #14382

jolivier23 · 2024-05-15T21:58:06Z

Set it to 5 seconds so we can try a different target more quickly if a rank is down.

Features: rebuild

Required-githooks: true

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:

Set it to 5 seconds so we can try a different target more quickly if a rank is down. Features: rebuild Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

github-actions · 2024-05-15T21:58:24Z

Ticket title is 'Engine unresponsive at scale with tcp and multi-recv enabled'
Status is 'In Progress'
Labels: 'GCP,scrubbed,triaged'
https://daosio.atlassian.net/browse/DAOS-15540

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

frostedcmos · 2024-05-16T02:05:31Z

src/cart/crt_register.c

@@ -629,7 +629,7 @@ crt_proto_query_int(crt_endpoint_t *tgt_ep, crt_opcode_t base_opc, uint32_t *ver
 	proto_query->pq_user_arg = arg;
 	proto_query->pq_coq->coq_base = base_opc;

-	rc = crt_req_set_timeout(rpc_req, 5);
+	rc = crt_req_set_timeout(rpc_req, timeout);


I would prefer here if timeout passed is 0 to not crt_req_set_timeout() at all. This way callers like crt_rpc.c can then call with 0 and use whatever defaults were already set via CRT_TIMEOUT instead of having to specify 60 seconds (for fi/ctl proto queries below).

you mean timeout passed from the cart tests, but not the daos proto query from init? i agree in that case that passing 0 should just be a no-op and not do anything in this case.

mchaarawi · 2024-05-16T03:16:16Z

src/cart/crt_register.c

@@ -629,7 +629,7 @@ crt_proto_query_int(crt_endpoint_t *tgt_ep, crt_opcode_t base_opc, uint32_t *ver
 	proto_query->pq_user_arg = arg;
 	proto_query->pq_coq->coq_base = base_opc;

-	rc = crt_req_set_timeout(rpc_req, 5);
+	rc = crt_req_set_timeout(rpc_req, timeout);


you mean timeout passed from the cart tests, but not the daos proto query from init? i agree in that case that passing 0 should just be a no-op and not do anything in this case.

mchaarawi · 2024-05-16T03:18:21Z

src/client/api/rpc.c

@@ -114,8 +115,10 @@ query_cb(struct crt_proto_query_cb_info *cb_info)

 	if (daos_rpc_retryable_rc(cb_info->pq_rc)) {
 		rproto->ep.ep_rank = (rproto->ep.ep_rank + 1) % rproto->nr_ranks;
+		if (rproto->timeout < 30)


it would be better here that instead of 30 to use the default timeout that cart was inited with. not sure if there is an api to get default timeout from cart, but should be easy to add one and use that instead of 30 and if current timeout exceeds the default to just use the default.

@frostedcmos is there a way to get the current timeout setting from cart?

nope there isnt a single/simple way, as there are multiple timeouts involved (global, per context, per rpc).

mchaarawi · 2024-05-16T03:20:49Z

src/cart/crt_rpc.c

@@ -441,8 +441,7 @@ crt_register_proto_fi(crt_endpoint_t *ep)
 	if (rc != 0)
 		return -DER_MISC;

-	rc = crt_proto_query(ep, cpf.cpf_base, &cpf.cpf_ver,
-			     1, crt_pfi_cb, &pfi);
+	rc = crt_proto_query(ep, cpf.cpf_base, &cpf.cpf_ver, 1, 60, crt_pfi_cb, &pfi);


would be better to pass 0 here and in cart we just not set the timeout of the value passed is 0.

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 · 2024-05-16T13:47:32Z

sorry, @mchaarawi I noticed I didn't like the if statement after removing the API call

daltonbohning

ftest LGTM

frostedcmos · 2024-05-16T14:15:06Z

src/cart/crt_register.c

-	/** Should only fail if invalid parameter */
-	D_ASSERT(rc == 0);
+	if (timeout != 0) {
+		if (timeout < crt_gdata.cg_timeout) {


i would remove this line, as crt_gdata.cg_timeout isnt necessarily the one being used either if per-rpc timeout isnt set. the order is -- global timeout (crt_gdata.cg_timeout), per-context timeout, per-rpc timeout.

So we should use the crt_req_get_timeout() instead to get the default?

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

Set it to 3 seconds initially and increase it as we try other targets so we can get going more quickly when a rank is down. Signed-off-by: Jeff Olivier <jeffolivier@google.com>

Pick from all possible ranks for protocol query handling Backports the following patches, last one fixes the issue but prior two are benign and make the backport work without conflict DAOS-13717 client: use d_rand() to generate random number (#12418) DAOS-15120 crt: Misc d_rand-related fixes (#13738) DAOS-15476 client: do not use rsvc for proto query rank selection (#14131) DAOS-15540 cart: Change proto query default timeout (#14382) Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com> Signed-off-by: Wang Shilong <shilong.wang@intel.com> Signed-off-by: Li Wei <wei.g.li@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com>

DAOS-623 cart: Change proto query default timeout

dd592f0

Set it to 5 seconds so we can try a different target more quickly if a rank is down. Features: rebuild Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 requested review from a team as code owners May 15, 2024 21:58

jolivier23 added 2 commits May 15, 2024 16:00

Fix typo

6b2871c

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

Update to increase timeout on retry

4f10858

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 requested review from a team as code owners May 15, 2024 22:33

jolivier23 changed the title ~~DAOS-623 cart: Change proto query default timeout~~ DAOS-15540 cart: Change proto query default timeout May 15, 2024

jolivier23 requested review from wangshilong, mchaarawi and frostedcmos May 15, 2024 22:35

wangshilong previously approved these changes May 16, 2024

View reviewed changes

frostedcmos reviewed May 16, 2024

View reviewed changes

mchaarawi reviewed May 16, 2024

View reviewed changes

jolivier23 added 2 commits May 16, 2024 07:09

Merge branch 'master' into jvolivie/timeout

15d95dd

Address review comments

7d0773c

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 dismissed wangshilong’s stale review via 7d0773c May 16, 2024 13:42

jolivier23 requested review from wangshilong, frostedcmos and mchaarawi May 16, 2024 13:44

mchaarawi previously approved these changes May 16, 2024

View reviewed changes

Clean up logic a bit

095f422

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 dismissed mchaarawi’s stale review via 095f422 May 16, 2024 13:46

jolivier23 requested a review from mchaarawi May 16, 2024 13:47

mchaarawi previously approved these changes May 16, 2024

View reviewed changes

daltonbohning reviewed May 16, 2024

View reviewed changes

frostedcmos reviewed May 16, 2024

View reviewed changes

Use the crt_req_timeout_get api to get the timeout setting

4592e42

Required-githooks: true Signed-off-by: Jeff Olivier <jeffolivier@google.com>

jolivier23 dismissed mchaarawi’s stale review via 4592e42 May 16, 2024 14:55

jolivier23 requested a review from frostedcmos May 16, 2024 14:55

frostedcmos approved these changes May 16, 2024

View reviewed changes

mchaarawi approved these changes May 16, 2024

View reviewed changes

jolivier23 mentioned this pull request May 16, 2024

DAOS-14908 vos: Reduce aggregation conflicts #14143

Merged

18 tasks

wangshilong approved these changes May 17, 2024

View reviewed changes

jolivier23 merged commit 3375a34 into master May 17, 2024
51 checks passed

jolivier23 deleted the jvolivie/timeout branch May 17, 2024 18:16

jolivier23 mentioned this pull request May 17, 2024

DAOS-15476 client: Better protocol query handling #14392

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-15540 cart: Change proto query default timeout #14382

DAOS-15540 cart: Change proto query default timeout #14382

jolivier23 commented May 15, 2024

github-actions bot commented May 15, 2024 •

edited

frostedcmos May 16, 2024

mchaarawi May 16, 2024

mchaarawi May 16, 2024

mchaarawi May 16, 2024

jolivier23 May 16, 2024

frostedcmos May 16, 2024

mchaarawi May 16, 2024

jolivier23 commented May 16, 2024

daltonbohning left a comment

frostedcmos May 16, 2024

jolivier23 May 16, 2024

DAOS-15540 cart: Change proto query default timeout #14382

DAOS-15540 cart: Change proto query default timeout #14382

Conversation

jolivier23 commented May 15, 2024

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented May 15, 2024 • edited

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

jolivier23 commented May 16, 2024

daltonbohning left a 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 May 15, 2024 •

edited