DAOS-10250 control: Get enabled and disabled ranks with dmg pool query #14436

knard-intel · 2024-05-24T09:19:22Z

Description

This PP is fixing the following issues:

DAOS-10250: Makes enabled and disabled ranks option of dmg compatible.
DAOS-10253: Update and add cmocka unit tests of engine management related functions.
Extra: Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query(). There is no JIRA for this issue as it was discover during the dev of this PR.

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 · 2024-05-24T09:19:42Z

Ticket title is 'Update engine to get enabled and disabled ranks with drpc pool query'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-10250

daosbuild1 · 2024-05-24T10:01:24Z

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

daosbuild1 · 2024-05-24T10:33:07Z

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14436/2/testReport/

daosbuild1 · 2024-05-24T12:43:42Z

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

daosbuild1 · 2024-05-24T19:51:29Z

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14436/5/execution/node/373/log

daosbuild1 · 2024-05-24T19:54:47Z

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14436/5/execution/node/358/log

daosbuild1 · 2024-05-24T19:57:46Z

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14436/5/execution/node/318/log

daosbuild1 · 2024-05-24T19:58:04Z

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14436/5/execution/node/315/log

daosbuild1 · 2024-05-24T20:56:26Z

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

Makes enabled and disabled ranks option of dmg compatible. Update and add cmocka unit tests of engine management related functions. Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query(). Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

tanabarr

Nice work @knard-intel , comments are nonblocking and only minor suggestions.

tanabarr · 2024-05-28T08:19:12Z

src/mgmt/tests/mocks.c

 {
 	/* If function is to return with an error, pool_info and ranks will not be filled. */
 	if (ds_mgmt_pool_query_return != 0)
 		return ds_mgmt_pool_query_return;

 	uuid_copy(ds_mgmt_pool_query_uuid, pool_uuid);
 	ds_mgmt_pool_query_info_ptr = (void *)pool_info;
+	if (pool_info != NULL && (pool_info->pi_bits & DPI_ENGINES_ENABLED) != 0) {


nit: might look nicer to perform the pool_info NULL check once and indent the rest of the checks

Improve code readability

Improve code readability

Fixed with commit 29e8d0e

tanabarr · 2024-05-28T08:39:37Z

src/pool/srv_cli.c


-	if (ranks != NULL) {
-		bool	get_enabled = (info ? ((info->pi_bits & DPI_ENGINES_ENABLED) != 0) : false);
+	if (info != NULL && (info->pi_bits & DPI_ENGINES_ENABLED) != 0) {


Is a NULL daos_pool_info_t valid here? if so then maybe GOTO label above pool_query_reply_to_info might be useful to skip to in the case of NULL. It looks like we are using the info reference as both input and an output, if so then should we update the documentation for dsc_pool_svc_query to mark the field as in/out? It could be made more clear by reading the input bit-fields from info before explicitly specifying output values in a separate info_out. This suggestion could be considered as overkill but I've occasionally found mixing input output to be problematic.

Yep, I agree that the usage of the info var is confusing.
Thanks for your attention.

Fix the dsc_pool_svc_query() documentation

Refactor the info var

Yep, I agree that the usage of the info var is confusing. Thanks for your attention.

Fix the dsc_pool_svc_query() documentation

Refactor the info var

Fixed with commit 87ff98c

daosbuild1 · 2024-06-03T14:56:35Z

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-14436/16/display/redirect

daosbuild1 · 2024-06-03T15:37:43Z

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-14436/17/testReport/

…/daos-10250 Required-githooks: true

Fix reviewers comments: - Change rc != -DER_SUCCESS to rc != 0 Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

Fix reviewers comments: - Remove invalid assert Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-06-03T19:59:03Z

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-14436/18/testReport/

daosbuild1 · 2024-06-03T20:14:42Z

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-14436/19/testReport/

kjacque

LGTM. My comments are minor non-blocking suggestions. Good work.

kjacque · 2024-06-03T22:03:10Z

src/pool/srv_cli.c

 	}

 	if ((pi_bits & DPI_ENGINES_ENABLED) != 0) {
-		D_ASSERT(enabled_ranks != NULL);
+		if (enabled_ranks == NULL) {
+			DL_ERROR(-DER_INVAL, DF_UUID ": query pool with invalid params",


minor - might be nice to be a little more descriptive here. Something like "query pool requested enabled ranks, but ptr is NULL"?

Fix error message

Fix error message

Fixed with commit 533bf93

kjacque · 2024-06-03T22:03:22Z

src/pool/srv_cli.c

 		}
 		D_DEBUG(DB_MD, DF_UUID ": found %" PRIu32 " enabled ranks in pool map\n",
 			DP_UUID(pool_uuid), enabled_rank_list->rl_nr);
 	}

 	if ((pi_bits & DPI_ENGINES_DISABLED) != 0) {
-		D_ASSERT(disabled_ranks != NULL);
+		if (disabled_ranks == NULL) {
+			DL_ERROR(-DER_INVAL, DF_UUID ": query pool with invalid params",


same comment as above

Fix error message

Fix error message

Fixed with commit 533bf93

Fix reviewers comments: - Fix error message Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-06-04T07:16:02Z

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-14436/20/testReport/

daosbuild1 · 2024-06-04T10:17:22Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14436/20/execution/node/1189/log

mjmac

Looks good, thanks for addressing this. At google this was briefly a problem because our service control team wanted to query both enabled and disabled and were confused by the "not supported" error.

mjmac · 2024-06-04T15:24:14Z

src/mgmt/srv_drpc.c

+	char                   *enabled_ranks_str  = NULL;
+	char                   *disabled_ranks_str = NULL;


I don't feel too strongly either way, but just a thought -- I wonder if it makes more sense to convert the response message to return a pair of uint32 arrays instead of the range strings. It would simplify the engine-side code and let the caller decide how to present the lists.

I don't recall how many ranks can be in a pool. If it's a lot, the string may actually be the better way to transmit that value...

Well, in the case of Aurora, a pool could in theory span 2048 ranks. As far as I'm aware there's no hard-coded limit on the number of ranks in a pool, so it could go higher for larger systems.

You make a good point about the ranklist string being a more compact representation. As I said, I don't feel very strongly about this, I guess I was just reacting a little to what seemed like presentation-layer logic living in the drpc handler.

From my side, I have to admit that I am also not found of using a string, and I will indeed prefer to use a list of intervals.
However, I will prefer to do that in a follow up PR with a dedicated ticket.
Does it makes sense.

Lists of intervals might be a nice compromise here. If I'm understanding correctly, you're proposing something like:
enabled_ranks: [0,7,9,15] (equivalent to "[0-7,9-15]")

Or, best-case:
enabled_ranks: [0,15] (equivalent to "[0-15]")

Right? This would still be reasonably compact, and would still require some interpretation, but would at least eliminate the string manipulation steps for encode/decode.

In any case, I am fine with doing that as a follow-up rather than forcing it into this patch.

Keeping this simple and transferring uint32 enabled_ranks over protobuf would be my preferred solution if range string is not acceptable. Given the infrequency of the operation would ~2k*uint32s introduce a problem?

@knard-intel IMO better to solve this problem in a new PR associated with a new ticket. I think it's OK to leave the strings for now.

@tanabarr Fair point... I think it would be a good idea to test and see what the impact is.

From my side both solution (integer interval and explicit list of ranks) make sense.
Interval of integer should not be so hard to implement on the engine side as it is already building a list of intervals.
If it is OK for you, I will submit this PR as it for landing (as soon as the CI is OK), and I will continue investigation on this point in a follow-up PR.
If it is not OK for you, do not hesitate to ask for modifications.

Follow-up action regarding decoupling improvement (update of the protobuf structure) will be done in the ticket DAOS-15987

…/daos-10250 Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

tanabarr

Minor: spelling PP-PR in commit message

knard-intel · 2024-06-11T06:53:43Z

@daos-stack/daos-gatekeeper , please could you lend this PR with the following commit message:

Title: DAOS-10250 control: Get enabled and disabled ranks with dmg pool query

Body:

Allow enabled and disabled ranks option to be used simultaneously (DAOS-10250).
Update and add cmocka unit tests of engine management related functions (DAOS-10253).

Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query().

#14436) Allow enabled and disabled ranks option to be used simultaneously (DAOS-10250). Update and add cmocka unit tests of engine management related functions (DAOS-10253). Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query(). Required-githooks: true Change-Id: I27f5b3acb003faea2d53697e83f0afeb0e284080 Signed-off-by: Cedric Koch-Hofer <[email protected]>

#14436) (#14548) Allow enabled and disabled ranks option to be used simultaneously (DAOS-10250). Update and add cmocka unit tests of engine management related functions (DAOS-10253). Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query(). Signed-off-by: Cedric Koch-Hofer <[email protected]>

#14436) Allow enabled and disabled ranks option to be used simultaneously (DAOS-10250). Update and add cmocka unit tests of engine management related functions (DAOS-10253). Fix memory leaks of ranks string in function ds_mgmt_drpc_pool_query(). Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel force-pushed the ckochhof/fix/master/daos-10250 branch from 22d9e2f to 1916065 Compare May 24, 2024 09:30

knard-intel self-assigned this May 24, 2024

knard-intel force-pushed the ckochhof/fix/master/daos-10250 branch from 1916065 to 9d4d536 Compare May 24, 2024 11:19

knard-intel force-pushed the ckochhof/fix/master/daos-10250 branch from 9d4d536 to f3d6fe5 Compare May 24, 2024 19:46

knard-intel force-pushed the ckochhof/fix/master/daos-10250 branch from f3d6fe5 to 5cec650 Compare May 24, 2024 19:57

knard-intel force-pushed the ckochhof/fix/master/daos-10250 branch from 5cec650 to 52da7cd Compare May 24, 2024 22:42

knard-intel marked this pull request as ready for review May 27, 2024 08:21

knard-intel requested review from a team as code owners May 27, 2024 08:21

knard-intel requested review from liw, tanabarr, daltonbohning, kjacque and kccain May 27, 2024 08:24

tanabarr previously approved these changes May 28, 2024

View reviewed changes

kanard38 added 3 commits June 3, 2024 19:12

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

11f7dc5

…/daos-10250 Required-githooks: true

DAOS-10250 control: Get enabled and disabled ranks with dmg pool query

aab1abe

Fix reviewers comments: - Change rc != -DER_SUCCESS to rc != 0 Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

DAOS-10250 control: Get enabled and disabled ranks with dmg pool query

2622260

Fix reviewers comments: - Remove invalid assert Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

kjacque previously approved these changes Jun 3, 2024

View reviewed changes

DAOS-10250 control: Get enabled and disabled ranks with dmg pool query

533bf93

Fix reviewers comments: - Fix error message Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel dismissed kjacque’s stale review via 533bf93 June 4, 2024 06:32

mjmac reviewed Jun 4, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

bc31087

…/daos-10250 Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

mjmac approved these changes Jun 5, 2024

View reviewed changes

kjacque approved these changes Jun 6, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

5d9c9f3

…/daos-10250 Features: control dmg Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

tanabarr approved these changes Jun 10, 2024

View reviewed changes

knard-intel requested review from daltonbohning, liw and a team June 11, 2024 06:47

mjmac merged commit 5c8868f into master Jun 11, 2024
70 of 87 checks passed

mjmac deleted the ckochhof/fix/master/daos-10250 branch June 11, 2024 12:59

mjmac mentioned this pull request Nov 13, 2024

mjmac/DAOS 16787 google 2.6 #15498

Closed

mjmac mentioned this pull request Dec 12, 2024

mjmac/dead ranks #15609

Merged

		char *enabled_ranks_str = NULL;
		char *disabled_ranks_str = NULL;

DAOS-10250 control: Get enabled and disabled ranks with dmg pool query #14436

DAOS-10250 control: Get enabled and disabled ranks with dmg pool query #14436

Conversation

knard-intel commented May 24, 2024 • edited Loading

Description

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented May 24, 2024 • edited Loading

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

daosbuild1 commented May 24, 2024

tanabarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel May 29, 2024 • edited Loading

Choose a reason for hiding this comment

knard-intel May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 3, 2024

daosbuild1 commented Jun 3, 2024

daosbuild1 commented Jun 3, 2024

daosbuild1 commented Jun 3, 2024

kjacque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

mjmac 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

mjmac Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

tanabarr Jun 6, 2024 • edited Loading

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

knard-intel Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

tanabarr left a comment

Choose a reason for hiding this comment

knard-intel commented Jun 11, 2024

knard-intel commented May 24, 2024 •

edited

Loading

github-actions bot commented May 24, 2024 •

edited

Loading

knard-intel May 29, 2024 •

edited

Loading

knard-intel May 29, 2024 •

edited

Loading

knard-intel May 29, 2024 •

edited

Loading

knard-intel Jun 4, 2024 •

edited

Loading

knard-intel Jun 4, 2024 •

edited

Loading

mjmac Jun 5, 2024 •

edited

Loading

tanabarr Jun 6, 2024 •

edited

Loading

knard-intel Jun 7, 2024 •

edited

Loading