DAOS-6970 iv: avoid pool connect/disconnect failure #4903

wangdi1 · 2021-03-07T21:39:12Z

Ignore -DER_NONEXIST failure for IV during pool connect and
disconnect, in case some ranks are being stopped at that
time.

Signed-off-by: Di Wang di.wang@intel.com

daosbuild1 · 2021-03-07T21:40:55Z

src/container/srv_container.c

+		if (rc == -DER_NONEXIST) {
+			/* If one of rank is being stopped, it may
+			 * return -DER_NONEXIST, which can be ignored
+			 * during capablity invalidate.


Suggested change

* during capablity invalidate.

* during capability invalidate.

daosbuild1 · 2021-03-07T21:40:58Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4903/1/execution/node/72/log

daosbuild1 · 2021-03-08T00:20:34Z

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4903/1/execution/node/1389/log

liw · 2021-03-08T08:42:55Z

src/cart/crt_corpc.c

@@ -110,7 +110,7 @@ crt_corpc_initiate(struct crt_rpc_priv *rpc_priv)
 		} else {
 			D_ERROR("crt_grp_lookup_grpid: %s failed.\n",
 				co_hdr->coh_grpid);
-			D_GOTO(out, rc = -DER_INVAL);
+			D_GOTO(out, rc = -DER_NONEXIST);


-DER_NONEXIST is probably an error that is too common, especially outside of crt. It'd be safer to find or define a new crt error.

DER_OOG? any other suggestion? @frostedcmos @liw ?

I'd suggest using a new -DER_GRP or -DER_GRPNOTFOUND, with a very specific error comment, so that people will unlikely use it for other purposes. For instance, write "crt group not found", instead of "group not found", so that it will unlikely be used for "redundancy groups", "placement groups", etc.

liw · 2021-03-08T08:55:23Z

src/container/srv_container.c

@@ -1635,6 +1635,16 @@ cont_close_recs(crt_context_t ctx, struct cont_svc *svc,
 		rc = cont_iv_capability_invalidate(
 				svc->cs_pool->sp_iv_ns,
 				recs[i].tcr_hdl, CRT_IV_SYNC_EAGER);
+		if (rc == -DER_NONEXIST) {


If the underlying CoRPC encountered two errors,

one on rank X, who had the pool started, due to some network trouble, and

one on rank Y, who didn't have the pool started,

then will the "group doesn't exist" error from rank Y hide the "network issue" error from rank X? If it will, when will rank X invalidate the capa in its memory?

Overall I am not sure I understand these changes.

If some node in the spanning tree happens to return DER_NONEXIST as it does not have the group then invalidate won't propagate past it to other nodes that will have the group. How is this condition being handled?

Isn't there some retry mechanism that should be kicking in to ensure invalidate request reaches all nodes eventually?

If rank stopping will cause the IV not being broadcast to all nodes, then it will need retry indeed. Hmm, return -DER_OOG will cause iv to retry internally.

@liw if one rank is not part of the pool, then iv requests or corpc will not send rpc to this rank? Not sure I follow the case here. The intention here is to not fail for stopping node, but I am not sure which is the better choice, ignore or retry.

Alex's example is clearer.

Generally, we should retry until this rank is excluded. The gap is the rank may be running but not have the pool started (i.e., not have the pool's SG created). In this case, we don't have a mechanism to trigger a pool map change for excluding this rank. I think we can introduce in-line failure detection (i.e., if a rank does not respond to an RPC in certain ways over certain numbers of retries, we ask the PS to decide whether this rank shall be excluded).

Specifically, the pool handle IV update case is okay, while the invalidation case is problematic. We would have to address the specific capa protocol if we choose not to follow the general rule.

Thoughts?

Updated patch

daosbuild1 · 2021-03-09T20:23:41Z

src/container/srv_container.c

+		if (rc == -DER_SHUTDOWN) {
+			/* If one of rank is being stopped, it may
+			 * return -DER_SHUTDOWN, which can be ignored
+			 * during capablity invalidate.


Suggested change

* during capablity invalidate.

* during capability invalidate.

daosbuild1 · 2021-03-09T20:23:45Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4903/2/execution/node/72/log

liuxuezhao · 2021-03-11T05:37:35Z

src/cart/crt_corpc.c

+			 */
+			D_ERROR("crt_grp_lookup_grpid: %s failed: %d\n",
+				co_hdr->coh_grpid, -DER_GRPVER);
+			D_GOTO(out, rc = -DER_GRPVER);


just curious how can it happen? when pool created the sub-group should be created already, why in bcast the crt_grp_lookup_grpid() possibly fail?

if the rank is being stopped at the same time, then crt_grp_lookup_grpid() on this rank will fail. I posted the log on the ticket you can check, thanks.

liuxuezhao · 2021-03-11T05:45:27Z

src/pool/srv_pool.c

-	if (rc)
+	if (rc) {
+		if (rc == -DER_SHUTDOWN) {
+			D_DEBUG(DF_DSMS, DF_UUID":"DF_UUID" some rank stop.\n",


s/ran stop/ranks stopped/ :)

liw · 2021-03-11T07:42:14Z

src/engine/server_iv.c

 			D_DEBUG(DB_MD, DF_UUID" stopping\n",
 				DP_UUID(ns->iv_pool_uuid));
-			return NULL;
+			return -DER_SHUTDOWN;


Sending -DER_SHUTDOWN from one rank to another sounds strange, if not potentially confusing. It is used to indicate that, locally, a lower layer has encountered an error (e.g., ENOMEM, EIO), and upper layers of the service (e.g., Pool Service) should trigger a shutdown, so that a different service replica could take over. In this PR's case, "the IV NS is shutting down" should not imply that "a different rank should shut down", right?

TBH, I do not see much of confusion to use SHUTDOWN outside of service change. To me shutdown just means the service is being shutdown somewhere, either switch/ignore/failure depend on the caller.

I am not sure we need invite a new error code here, what error do you suggest?

@liw any error code suggestion?

1. If the rank is being stopped, then iv operation might return DER_SHUTDOWN, let's ignore such failure for pool disconnect/connect. 2. Return GRPVER if the group can not be found in local SG, so let's the caller retry until the rank is excluded from the pool map or the pool service is stopped. Signed-off-by: Di Wang <di.wang@intel.com>

wangdi1 · 2021-03-17T17:35:53Z

only updated the patch to fix the spelling error. Please recheck. Thanks. @liw @liuxuezhao

Updated patch

daosbuild1

LGTM. No errors found by checkpatch.

1. If the rank is being stopped, then iv operation might return DER_SHUTDOWN, let's ignore such failure for pool disconnect/connect. 2. Return GRPVER if the group can not be found in local SG, so let's the caller retry until the rank is excluded from the pool map or the pool service is stopped. Signed-off-by: Di Wang <di.wang@intel.com>

wangdi1 requested review from liw, frostedcmos and liuxuezhao March 7, 2021 21:39

daosbuild1 previously requested changes Mar 7, 2021

View reviewed changes

liw reviewed Mar 8, 2021

View reviewed changes

wangdi1 force-pushed the daos_6970 branch from 15a4a97 to a2116e9 Compare March 9, 2021 20:21

wangdi1 requested a review from liw March 9, 2021 20:22

daosbuild1 previously requested changes Mar 9, 2021

View reviewed changes

liuxuezhao previously approved these changes Mar 11, 2021

View reviewed changes

liw reviewed Mar 11, 2021

View reviewed changes

wangdi1 requested a review from liw March 17, 2021 00:56

liw previously approved these changes Mar 17, 2021

View reviewed changes

wangdi1 dismissed stale reviews from liw and liuxuezhao via 2141969 March 17, 2021 17:34

wangdi1 force-pushed the daos_6970 branch from a2116e9 to 2141969 Compare March 17, 2021 17:34

wangdi1 requested review from liw and liuxuezhao March 17, 2021 17:35

daosbuild1 reviewed Mar 17, 2021

View reviewed changes

liw approved these changes Mar 19, 2021

View reviewed changes

liuxuezhao approved these changes Mar 21, 2021

View reviewed changes

jolivier23 merged commit 52e4828 into master Mar 21, 2021

jolivier23 deleted the daos_6970 branch March 21, 2021 16:44

ashleypittman mentioned this pull request Apr 28, 2021

1.2 dfuse single #5582

Closed

ashleypittman mentioned this pull request May 20, 2021

1.2 proc #5753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-6970 iv: avoid pool connect/disconnect failure #4903

DAOS-6970 iv: avoid pool connect/disconnect failure #4903

wangdi1 commented Mar 7, 2021

daosbuild1 Mar 7, 2021

daosbuild1 commented Mar 7, 2021

daosbuild1 commented Mar 8, 2021

liw Mar 8, 2021

wangdi1 Mar 8, 2021

liw Mar 9, 2021

liw Mar 8, 2021

frostedcmos Mar 8, 2021

wangdi1 Mar 8, 2021

liw Mar 9, 2021

daosbuild1 Mar 9, 2021

daosbuild1 commented Mar 9, 2021

liuxuezhao Mar 11, 2021

wangdi1 Mar 11, 2021

liuxuezhao Mar 11, 2021

liw Mar 11, 2021

wangdi1 Mar 11, 2021

wangdi1 Mar 17, 2021

wangdi1 commented Mar 17, 2021

daosbuild1 left a comment

	* during capablity invalidate.
	* during capability invalidate.

DAOS-6970 iv: avoid pool connect/disconnect failure #4903

DAOS-6970 iv: avoid pool connect/disconnect failure #4903

Conversation

wangdi1 commented Mar 7, 2021

Choose a reason for hiding this comment

daosbuild1 commented Mar 7, 2021

daosbuild1 commented Mar 8, 2021

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

daosbuild1 commented Mar 9, 2021

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

wangdi1 commented Mar 17, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment