DAOS-16902 pool: Polish map change logging #15744

liw · 2025-01-17T06:09:21Z

No logic changes; just adding pool UUID and pool map address to pool map change logging, and logging important messages to both stdout and log.

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:

No logic changes; just adding pool UUID and pool map address to pool map change logging, and logging important messages to both stdout and log. Signed-off-by: Li Wei <[email protected]> Required-githooks: true

github-actions · 2025-01-17T06:09:40Z

Ticket title is 'LRZ: rebuild of pool is hanging'
Status is 'Awaiting Verification'
Labels: 'LRZ'
https://daosio.atlassian.net/browse/DAOS-16902

liw · 2025-01-20T00:47:43Z

Since the CI run has taken 3 days and is still in progress, I'm prematurely marking this PR as ready for review.

knard38

LGTM

kccain

Hope I haven't overthought this patch with some of the suggestions that are independent potential fixes for existing log messages (as opposed to the intent of this patch to provide pool UUID / map pointer context to messages).

kccain · 2025-01-21T20:29:24Z

src/pool/srv_pool_map.c

 			return -DER_BUSY;
 		case PO_COMP_ST_UPIN:
-			D_DEBUG(DB_MD, "change "DF_TARGET" to DRAIN %p\n",
-				DP_TARGET(target), map);
+			D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DRAIN %p\n",


map is now printed twice in this format string (DF_MAP and final %p that perhaps can be removed?)

And no change requested for this: at first I started to think this D_DEBUG call (and similar instances) could be removed, since the subsequent D_PRINT() and D_INFO() cover the logging needed. But DP_TARGET() logged before and after the target changes will show the status and failure sequence transitions.

Removed %p at the end of the message.

Since I did not design this code, I'll leave the potential removal of this D_DEBUG to you and @liuxuezhao.

kccain · 2025-01-21T20:42:20Z

src/pool/srv_pool_map.c

@@ -101,29 +106,28 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DRAIN:
 		case PO_COMP_ST_DOWNOUT:
 			/* Nothing to do, already excluded / draining */
-			D_INFO("Skip drain down target (rank %u idx %u)\n",
-			       target->ta_comp.co_rank,
+			D_INFO(DF_MAP ": Skip drain down target (rank %u idx %u)\n",


no change requested (unless it is obvious) - I'm assuming DF_TARGET/DP_TARGET are somehow not appropriate to use in this particular state change?

I don't see any reason; changed to DF_TARGET.

kccain · 2025-01-21T20:45:44Z

src/pool/srv_pool_map.c

 			return -DER_BUSY;
 		case PO_COMP_ST_DOWN:
 			target->ta_comp.co_flags |= PO_COMPF_DOWN2UP;
 		case PO_COMP_ST_DOWNOUT:
-			D_DEBUG(DB_MD, "change "DF_TARGET" to UP %p\n",
-				DP_TARGET(target), map);
+			D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to UP %p\n",


redundant printing of map pointer value in the format?

Removed the %p at the end of the message.

kccain · 2025-01-21T20:50:03Z

src/pool/srv_pool_map.c

@@ -213,12 +217,13 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_NEW:
 		case PO_COMP_ST_UP:
 			/* Nothing to do, already UPIN */
-			D_INFO("Skip ADD_IN UPIN "DF_TARGET"\n",
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),


Suggested change

D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),

D_INFO(DF_MAP ": Skip EXCLUDE_OUT " DF_TARGET "\n", DP_MAP(pool_uuid, map),

and maybe the comment above meant to say already downout

Changed to reflect the cases better. Please double check.

kccain · 2025-01-21T21:00:02Z

src/pool/srv_pool_map.c

@@ -229,16 +234,17 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DOWNOUT:
 		case PO_COMP_ST_NEW:
 			/* Nothing to do, already UPIN */
-			D_INFO("Skip ADD_IN UPIN "DF_TARGET"\n",
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),


Suggested change

D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),

D_INFO(DF_MAP ": Skip FINISH_REBUILD DF_TARGET "\n", DP_MAP(pool_uuid, map),

Changed to reflect the cases better. Please double check.

kccain · 2025-01-21T21:02:48Z

src/pool/srv_pool_map.c

@@ -250,8 +256,8 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DOWN:
 		case PO_COMP_ST_NEW:
 			/* Nothing to do, already UPIN and DOWNOUT. DOWN can not be revert. */
-			D_INFO("Skip ADD_IN UPIN DOWN "DF_TARGET"\n",
-			       DP_TARGET(target));
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN DOWN " DF_TARGET "\n",


Suggested change

D_INFO(DF_MAP ": Skip ADD_IN UPIN DOWN " DF_TARGET "\n",

D_INFO(DF_MAP ": Skip REVERT_REBUILD UPIN DOWN " DF_TARGET "\n",

Changed to reflect the cases better. Please double check.

kccain · 2025-01-21T21:08:15Z

src/pool/srv_pool_map.c

 				target->ta_comp.co_status = PO_COMP_ST_NEW;
 				target->ta_comp.co_in_ver = 0;
 				++(*version);
 			} else {
-				D_DEBUG(DB_MD, "change "DF_TARGET" to DOWNOUT %p fseq %u\n",
-					DP_TARGET(target), map, target->ta_comp.co_fseq);
+				D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWNOUT fseq %u\n",


Suggested change

D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWNOUT fseq %u\n",

D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWN or DOWNOUT fseq %u\n",

Changed this to two messages, one for each if branch. Please check.

kccain · 2025-01-21T21:12:08Z

src/pool/srv_pool_map.c

-				D_PRINT(DF_TARGET" is excluded.\n", DP_TARGET(target));
-			else
-				D_INFO(DF_TARGET" is excluded.\n", DP_TARGET(target));
+				D_PRINT(DF_MAP ": " DF_TARGET " is excluded.\n",


This one is more difficult for me to interpret quickly, and then I suppose get the log message perfectly accurate ("excluded" not always the case?). It seems the state may have changed to NEW, DOWN, or DOWNOUT. Maybe not worth perfecting it right now (if my understanding is accurate to begin with).

Changed to "reverted". Please double check.

That seems good, though I think the change will be needed also in the D_PRINT() in addition to the D_INFO() call.

Oops, sorry.

kccain · 2025-01-21T21:16:46Z

src/pool/srv_pool_map.c

 			return -DER_NONEXIST;
 		}

-		rc = update_one_tgt(map, target, opc, &version, print_changes);
+		rc = update_one_tgt(pool_uuid, map, target, opc, &version, print_changes);
 		if (rc < 0)
 			return rc;

 		/* if the target status does not need to change */


not sure but maybe the comment is intended to say this?

Suggested change

/* if the target status does not need to change */

/* if the rank status does not need to change */

Changed to "rank".

Required-githooks: true

Signed-off-by: Li Wei <[email protected]> Required-githooks: true

liw

@kccain, thanks.

liw · 2025-01-30T05:00:50Z

src/pool/srv_pool_map.c

@@ -101,29 +106,28 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DRAIN:
 		case PO_COMP_ST_DOWNOUT:
 			/* Nothing to do, already excluded / draining */
-			D_INFO("Skip drain down target (rank %u idx %u)\n",
-			       target->ta_comp.co_rank,
+			D_INFO(DF_MAP ": Skip drain down target (rank %u idx %u)\n",


I don't see any reason; changed to DF_TARGET.

liw · 2025-01-30T05:03:30Z

src/pool/srv_pool_map.c

 			return -DER_BUSY;
 		case PO_COMP_ST_UPIN:
-			D_DEBUG(DB_MD, "change "DF_TARGET" to DRAIN %p\n",
-				DP_TARGET(target), map);
+			D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DRAIN %p\n",


Removed %p at the end of the message.

Since I did not design this code, I'll leave the potential removal of this D_DEBUG to you and @liuxuezhao.

liw · 2025-01-30T05:05:18Z

src/pool/srv_pool_map.c

 			return -DER_BUSY;
 		case PO_COMP_ST_DOWN:
 			target->ta_comp.co_flags |= PO_COMPF_DOWN2UP;
 		case PO_COMP_ST_DOWNOUT:
-			D_DEBUG(DB_MD, "change "DF_TARGET" to UP %p\n",
-				DP_TARGET(target), map);
+			D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to UP %p\n",


Removed the %p at the end of the message.

liw · 2025-01-30T05:12:58Z

src/pool/srv_pool_map.c

@@ -213,12 +217,13 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_NEW:
 		case PO_COMP_ST_UP:
 			/* Nothing to do, already UPIN */
-			D_INFO("Skip ADD_IN UPIN "DF_TARGET"\n",
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),


Changed to reflect the cases better. Please double check.

liw · 2025-01-30T05:13:35Z

src/pool/srv_pool_map.c

@@ -229,16 +234,17 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DOWNOUT:
 		case PO_COMP_ST_NEW:
 			/* Nothing to do, already UPIN */
-			D_INFO("Skip ADD_IN UPIN "DF_TARGET"\n",
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),


Changed to reflect the cases better. Please double check.

liw · 2025-01-30T05:13:52Z

src/pool/srv_pool_map.c

@@ -250,8 +256,8 @@ update_one_tgt(struct pool_map *map, struct pool_target *target,
 		case PO_COMP_ST_DOWN:
 		case PO_COMP_ST_NEW:
 			/* Nothing to do, already UPIN and DOWNOUT. DOWN can not be revert. */
-			D_INFO("Skip ADD_IN UPIN DOWN "DF_TARGET"\n",
-			       DP_TARGET(target));
+			D_INFO(DF_MAP ": Skip ADD_IN UPIN DOWN " DF_TARGET "\n",


Changed to reflect the cases better. Please double check.

liw · 2025-01-30T05:18:11Z

src/pool/srv_pool_map.c

 				target->ta_comp.co_status = PO_COMP_ST_NEW;
 				target->ta_comp.co_in_ver = 0;
 				++(*version);
 			} else {
-				D_DEBUG(DB_MD, "change "DF_TARGET" to DOWNOUT %p fseq %u\n",
-					DP_TARGET(target), map, target->ta_comp.co_fseq);
+				D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWNOUT fseq %u\n",


Changed this to two messages, one for each if branch. Please check.

liw · 2025-01-30T05:20:52Z

src/pool/srv_pool_map.c

-				D_PRINT(DF_TARGET" is excluded.\n", DP_TARGET(target));
-			else
-				D_INFO(DF_TARGET" is excluded.\n", DP_TARGET(target));
+				D_PRINT(DF_MAP ": " DF_TARGET " is excluded.\n",


Changed to "reverted". Please double check.

liw · 2025-01-30T05:22:20Z

src/pool/srv_pool_map.c

 			return -DER_NONEXIST;
 		}

-		rc = update_one_tgt(map, target, opc, &version, print_changes);
+		rc = update_one_tgt(pool_uuid, map, target, opc, &version, print_changes);
 		if (rc < 0)
 			return rc;

 		/* if the target status does not need to change */


Changed to "rank".

kccain · 2025-01-30T16:38:16Z

src/pool/srv_pool_map.c

-				D_PRINT(DF_TARGET" is excluded.\n", DP_TARGET(target));
-			else
-				D_INFO(DF_TARGET" is excluded.\n", DP_TARGET(target));
+				D_PRINT(DF_MAP ": " DF_TARGET " is excluded.\n",


That seems good, though I think the change will be needed also in the D_PRINT() in addition to the D_INFO() call.

Signed-off-by: Li Wei <[email protected]> Required-githooks: true

Allow-unstable-test: true Required-githooks: true

liw · 2025-01-31T08:14:11Z

Merged master to add Allow-unstable-test: true because of unrelated NLT warnings.

daosbuild1 · 2025-01-31T08:58:40Z

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-15744/4/testReport/

[liw] Unrelated Valgrind errors in command daos: DAOS-17016

liw · 2025-02-06T00:58:55Z

@daos-stack/daos-gatekeeper, please note that the latest build has been tested with Allow-unstable-test: true, and the only failure is the NLT Valgrind one (DAOS-17016).

DAOS-16902 pool: Polish map change logging

91137b1

No logic changes; just adding pool UUID and pool map address to pool map change logging, and logging important messages to both stdout and log. Signed-off-by: Li Wei <[email protected]> Required-githooks: true

liw marked this pull request as ready for review January 20, 2025 00:43

liw requested review from a team as code owners January 20, 2025 00:43

liw requested review from liuxuezhao, kccain and knard38 January 20, 2025 00:44

knard38 previously approved these changes Jan 20, 2025

View reviewed changes

liuxuezhao previously approved these changes Jan 20, 2025

View reviewed changes

kccain requested changes Jan 21, 2025

View reviewed changes

liw added 2 commits January 30, 2025 13:54

Merge branch 'master' into liw/polish-map-logging

f5a5760

Required-githooks: true

Address review comments

5f3ec8c

Signed-off-by: Li Wei <[email protected]> Required-githooks: true

liw dismissed stale reviews from liuxuezhao and knard38 via 5f3ec8c January 30, 2025 05:48

liw commented Jan 30, 2025

View reviewed changes

kccain requested changes Jan 30, 2025

View reviewed changes

liw added 2 commits January 31, 2025 08:36

Address review comments

3237e2c

Signed-off-by: Li Wei <[email protected]> Required-githooks: true

Merge branch 'master' into liw/polish-map-logging

0780adf

Allow-unstable-test: true Required-githooks: true

kccain approved these changes Jan 31, 2025

View reviewed changes

liw requested review from knard38 and liuxuezhao February 3, 2025 00:11

knard38 approved these changes Feb 3, 2025

View reviewed changes

liw requested a review from a team February 4, 2025 01:26

liuxuezhao approved these changes Feb 7, 2025

View reviewed changes

	D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),
	D_INFO(DF_MAP ": Skip EXCLUDE_OUT " DF_TARGET "\n", DP_MAP(pool_uuid, map),

	D_INFO(DF_MAP ": Skip ADD_IN UPIN " DF_TARGET "\n", DP_MAP(pool_uuid, map),
	D_INFO(DF_MAP ": Skip FINISH_REBUILD DF_TARGET "\n", DP_MAP(pool_uuid, map),

	D_INFO(DF_MAP ": Skip ADD_IN UPIN DOWN " DF_TARGET "\n",
	D_INFO(DF_MAP ": Skip REVERT_REBUILD UPIN DOWN " DF_TARGET "\n",

	D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWNOUT fseq %u\n",
	D_DEBUG(DB_MD, DF_MAP ": change " DF_TARGET " to DOWN or DOWNOUT fseq %u\n",

	/* if the target status does not need to change */
	/* if the rank status does not need to change */

DAOS-16902 pool: Polish map change logging #15744

Are you sure you want to change the base?

DAOS-16902 pool: Polish map change logging #15744

Conversation

liw commented Jan 17, 2025

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 17, 2025

liw commented Jan 20, 2025

knard38 left a comment

Choose a reason for hiding this comment

kccain 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

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

liw 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

liw commented Jan 31, 2025

daosbuild1 commented Jan 31, 2025 • edited by liw Loading

liw commented Feb 6, 2025

daosbuild1 commented Jan 31, 2025 •

edited by liw

Loading