DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC #13382

tanabarr · 2023-11-23T02:08:02Z

Enable fetching of NVMe controller (SSD) details over dRPC. This is
required to get updated SPDK discovery results after an NVMe SSD is
hotplugged as the newly added device will be claimed by the engine.
Once claimed the device cannot be accessed by the control-plane.

This change also enables the reduction of complexity in the
control-plane by moving to a position where the bdev scan cache,
which was previously implemented to mitigate the situation described
above, can be removed. This removal will be performed in a subsequent
change.

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:

github-actions · 2023-11-23T02:08:21Z

Bug-tracker data:
Ticket title is 'Remove bdev scan cache'
Status is 'In Review'
Labels: 'SPDK,drpc'
Errors are Unknown component
https://daosio.atlassian.net/browse/DAOS-14181

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-11-23T02:51:06Z

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

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

knard38 · 2023-11-23T16:28:51Z

src/control/server/ctl_storage_rpc_test.go

-		})
-	}
-}
+//func TestServer_CtlSvc_adjustNvmeSize(t *testing.T) {


Should be removed ?

will be in the follow-up.

knard38 · 2023-11-23T16:29:22Z

src/control/server/ctl_storage_rpc_test.go

-		})
-	}
-}
+//func TestServer_CtlSvc_StorageScan_PostEngineStart(t *testing.T) {


Should be removed ?

I will remove in the follow-up, I'm leaving commented to remind me to make sure everything is covered.

daosbuild1

LGTM. No errors found by checkpatch.

tanabarr · 2023-11-23T16:32:25Z

Notes to reviewers:

NVMe controller info is returned in SMD list dRPC query and protobuf and native go structs link a SMD with a controller and a single namespace. The changes are in the following areas:

control-plane SPDK bindings src/control/lib/spdk
drpc smd-list stack in engine mgmt and bio modules
drpc smd-list stack in control-plane

As a guidance so as not to burden anyone with too much to review I suggest the following (feel free to review more):
@NiuYawei and @wangshilong could you please review 2 and optionally 1.
@kjacque could you please review 1 and 3.
@mjmac and @knard-intel could you please review 3.

The PR doesn't introduce any outside visible change and attempts keeping parity with the existing functionality whilst adding info to the dRPC SMD list devs messages that isn't yet consumed in the control plane. The following PR will remove the bdev scan cache and instead update live over dRPC.

A number of go unit tests are removed in src/control/server/ctl_storage_rpc_test.go and will be reinstated or replaced during the bdev scan removal in the following PR. It didn't make sense to first fix up a large number of tests just to then remove them.

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-11-23T17:21:11Z

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

daosbuild1 · 2023-11-23T19:42:40Z

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13382/4/execution/node/1162/log

knard38

Mostly OK for what I understand.
Just a couple of questions.

knard38 · 2023-11-24T08:09:24Z

src/mgmt/srv_internal.h

@@ -119,6 +119,8 @@ struct mgmt_bio_health {

 int ds_mgmt_bio_health_query(struct mgmt_bio_health *mbh, uuid_t uuid);
 int ds_mgmt_smd_list_devs(Ctl__SmdDevResp *resp);
+void


Strange indentation

mandated by clang-format, not going to fight the linter we have in place to make formatting consistent

knard38 · 2023-11-24T08:14:53Z

src/mgmt/srv_query.c

+		return -DER_INVAL;
+	}
+
+	len = strnlen(src, NVME_DETAIL_BUFLEN);


The return value of strnlen() should be tested.

Suggested change

len = strnlen(src, NVME_DETAIL_BUFLEN);

if (len = strnlen(src, NVME_DETAIL_BUFLEN)) >= NVME_DETAIL_BUFLEN) {

D_ERROR("attempting to copy an invalid source");

return -DER_INVAL;

};

why would you test the implementation of a c built-in function? that's already tested by the C library It is a part of. You have to be able to rely on the functionality of the c lib functions being correct IMO. strnlen will only return the maximum of the second parameter.

From my understanding, if len == NVME_DETAIL_BUFLEN, then it means that src is too long and thus some part of it will not be copied at line 315.
However, if we are sure that src is always correct, then I got not issue to test len or even use strlen.

ok I understand your comment now, fixed

knard38 · 2023-11-24T08:36:57Z

src/mgmt/srv_query.c

+
+	return 0;
+}
+
 int
 ds_mgmt_smd_list_devs(Ctl__SmdDevResp *resp)


NIT, this function is quite big; it would help to understand and maintain it, if it was refactorize in smaller functions.

ok, I will refactor

knard38 · 2023-11-24T08:52:55Z

src/control/server/instance_storage_rpc.go

+	for _, sd := range listDevsResp.Devices {
+		if sd != nil {
+			rResp.Devices = append(rResp.Devices, sd)
+			//&ctlpb.SmdQueryResp_SmdDeviceWithHealth{Details: sd})


To remove ?

removed in PR-13385, done

knard38 · 2023-11-24T09:47:42Z

src/bio/bio_config.c

@@ -376,11 +385,12 @@ load_vmd_subsystem_config(struct json_config_ctx *ctx, bool *vmd_enabled)

 	D_ASSERT(ctx->config_it != NULL);
 	D_ASSERT(vmd_enabled != NULL);
+	D_ASSERT(*vmd_enabled == false);


Not an expert, but not understand why VMD could be enabled in only one VMD subsystem.
I would expect the opposite to be enabled for all or nothing.

this is just Ensuring the correct flow, at this point the vmd_enabled should not have been set yet

As the call of this function in check_vmd_status() is done in a loop, I was expecting that we could find the NVME_CONF_ENABLE_VMD environment variable set in different subsystem.

there should only be one vmd subsystem in a config but I don't think there is any need to put a restriction on that in this code so I've removed this check, thanks

knard38 · 2023-11-24T10:36:11Z

src/control/lib/spdk/src/nvme_control_common.c


-	if (copy_ascii(cdst->fw_rev, sizeof(cdst->fw_rev), cdata->fr,
-		       sizeof(cdata->fr)) != 0)
+	len  = sizeof(src);


Not sure to understand how the sizeof could retrieve the length of the array holds by src.
In this case it will only returns the size of a void* pointer?

this looks like a bug, thanks, fixed

…move-bdev-scan-cache-pt1-proto

Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-11-27T13:37:20Z

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

…move-bdev-scan-cache-pt1-proto Required-githooks: true

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

kjacque

Go changes look good. A couple questions on C stuff.

src/mgmt/srv_query.c

kjacque · 2023-12-04T22:32:39Z

src/control/lib/spdk/src/nvme_control_common.c

+	if (copy_ascii(*dst, len + 1, src, len) != 0) {
+		perror("copy_ascii");
 		return -NVMEC_ERR_CHK_SIZE;


Since it's a static function with a pretty limited scope, that seems okay to me. It makes it a bit harder to keep track mentally of the state of the memory, though.

wangshilong

C parts looks good for me. ^_^

daosbuild1 · 2023-12-07T01:47:30Z

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13382/17/execution/node/1378/log

daosbuild1 · 2023-12-07T07:04:13Z

Test stage Functional Hardware Large completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13382/17/testReport/

tanabarr · 2023-12-07T14:36:12Z

https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13382/17/tests/ build failed with known failures:

https://daosio.atlassian.net/browse/DAOS-14718 ConfigGenerateRun
https://daosio.atlassian.net/browse/DAOS-14592 DmgSystemReformatTest

daosbuild1 · 2023-12-10T00:56:10Z

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13382/18/execution/node/354/log

daosbuild1 · 2023-12-10T22:17:00Z

Test stage Functional Hardware Large completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13382/18/testReport/

tanabarr · 2023-12-11T16:27:34Z

https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13382/18/tests/ build failed with known failures:

https://daosio.atlassian.net/browse/DAOS-14718 ConfigGenerateRun
https://daosio.atlassian.net/browse/DAOS-14592 DmgSystemReformatTest

tanabarr · 2023-12-12T16:30:56Z

@NiuYawei @daos-stack/daos-gatekeeper can this PR be force landed please, the consistent known failures identified in the comments are expected when running with Features: control Commit pragma.

tanabarr · 2023-12-12T23:36:23Z

@phender has advised me (after direct questioning) that there are @daos-stack/daos-gatekeeper concerns regarding lack of testing in this PR. First of all why has this not been commented in the PR that this is a concern after 5 days? This should have been brought up earlier and transparently communicated. Please can we have more transparency in gatekeeping to make the process more efficient. Thanks in advance.

With regards to testing, the PR has extensive unit test updates and storage scan is effectively covered in functional tests.

mchaarawi · 2023-12-12T23:52:11Z

@phender has advised me (after direct questioning) that there are @daos-stack/daos-gatekeeper concerns regarding lack of testing in this PR. First of all why has this not been commented in the PR that this is a concern after 5 days? This should have been brought up earlier and transparently communicated. Please can we have more transparency in gatekeeping to make the process more efficient. Thanks in advance.

With regards to testing, the PR has extensive unit test updates and storage scan is effectively covered in functional tests.

I am on the gatekeeping channel, and I asked a question there if testing was enough for this PR. Im not sure how this was translated as a concern that there is not enough testing. I do not have enough knowledge of this code base and it was just a genuine question to other gatekeepers who might be more familiar. anyway this was just a question and not a concern.
but the main concern is that this is a huge PR and is missing a +1 from Niu to review the BIO code. Niu is also a gatekeeper and i believe this is why several gatekeepers (including me) have deferred on this PR in addition to the concern that the failing tests do look in the same area as the PR (not saying they are).

also please consider breaking PRs into smaller ones; otherwise you are getting more into the feature branch territory ;-)

tanabarr · 2023-12-13T08:22:39Z

@phender has advised me (after direct questioning) that there are @daos-stack/daos-gatekeeper concerns regarding lack of testing in this PR. First of all why has this not been commented in the PR that this is a concern after 5 days? This should have been brought up earlier and transparently communicated. Please can we have more transparency in gatekeeping to make the process more efficient. Thanks in advance.
With regards to testing, the PR has extensive unit test updates and storage scan is effectively covered in functional tests.

I am on the gatekeeping channel, and I asked a question there if testing was enough for this PR. Im not sure how this was translated as a concern that there is not enough testing. I do not have enough knowledge of this code base and it was just a genuine question to other gatekeepers who might be more familiar. anyway this was just a question and not a concern. but the main concern is that this is a huge PR and is missing a +1 from Niu to review the BIO code. Niu is also a gatekeeper and i believe this is why several gatekeepers (including me) have deferred on this PR in addition to the concern that the failing tests do look in the same area as the PR (not saying they are).

also please consider breaking PRs into smaller ones; otherwise you are getting more into the feature branch territory ;-)

The tests have already been identified as known issues that exist within the subset run Features: control as acknowledged by @phender . I have also broken the PR out from #13385 and been discussing extensively with the control-plane team (hence why it has 3 approvals). it is a fundamental change in the DRPC plumbing as required by hotplug for a 2.6 required for ticket.

NiuYawei

The BIO change looks good to me. Sorry, I overlooked prior comments.

…move-bdev-scan-cache-pt1-proto Features: control Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

tanabarr · 2023-12-13T09:49:03Z

I have triggered tests again, there are no code changes in this PR between CI runs 17-19 and the failures are the same known issues as mentioned in previous comments. I believe the PR can be force landed based on CI test coverage, unit test coverage and review approvals. TIA

tanabarr · 2023-12-13T16:36:59Z

@mchaarawi can we please land this PR now we have an extensive list of review approvals and both https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13382/17/pipeline and https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13382/18/pipeline only failed on known issues that are expected to occur when Features: control pragma is used. build 19 is likely to take another few days.

daosbuild1 · 2023-12-13T17:58:18Z

Test stage Functional Hardware Large completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13382/19/testReport/

tanabarr self-assigned this Nov 23, 2023

tanabarr requested a review from knard38 November 23, 2023 02:08

daosbuild1 reviewed Nov 23, 2023

View reviewed changes

DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC

729d730

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

tanabarr force-pushed the tanabarr/control-remove-bdev-scan-cache-pt1-proto branch from bba5921 to 00e5ca5 Compare November 23, 2023 16:23

daosbuild1 reviewed Nov 23, 2023

View reviewed changes

tanabarr requested review from mjmac, wangshilong and NiuYawei November 23, 2023 16:26

tanabarr marked this pull request as ready for review November 23, 2023 16:28

tanabarr requested a review from a team as a code owner November 23, 2023 16:28

tanabarr requested review from kjacque and removed request for a team November 23, 2023 16:28

knard38 reviewed Nov 23, 2023

View reviewed changes

tanabarr force-pushed the tanabarr/control-remove-bdev-scan-cache-pt1-proto branch from 00e5ca5 to 87d69cf Compare November 23, 2023 16:29

daosbuild1 reviewed Nov 23, 2023

View reviewed changes

tanabarr force-pushed the tanabarr/control-remove-bdev-scan-cache-pt1-proto branch from 87d69cf to 729d730 Compare November 23, 2023 16:40

daosbuild1 reviewed Nov 23, 2023

View reviewed changes

knard38 reviewed Nov 24, 2023

View reviewed changes

tanabarr mentioned this pull request Nov 24, 2023

DAOS-14181 control: Remove bdev scan cache #13256

Closed

18 tasks

tanabarr added 2 commits November 27, 2023 11:26

Merge remote-tracking branch 'origin/master' into tanabarr/control-re…

6549812

…move-bdev-scan-cache-pt1-proto

address review comments from knard-intel pt1

86310ad

Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1 reviewed Nov 27, 2023

View reviewed changes

tanabarr added 2 commits December 4, 2023 02:50

Merge remote-tracking branch 'origin/master' into tanabarr/control-re…

7ce581d

…move-bdev-scan-cache-pt1-proto Required-githooks: true

address review comments from wangshilong

6360b73

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1 reviewed Dec 4, 2023

View reviewed changes

tanabarr requested a review from wangshilong December 4, 2023 17:34

kjacque reviewed Dec 4, 2023

View reviewed changes

tanabarr requested a review from kjacque December 5, 2023 11:49

kjacque approved these changes Dec 5, 2023

View reviewed changes

wangshilong approved these changes Dec 6, 2023

View reviewed changes

knard38 approved these changes Dec 6, 2023

View reviewed changes

tanabarr requested a review from a team December 7, 2023 14:37

tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Dec 7, 2023

NiuYawei approved these changes Dec 13, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into tanabarr/control-re…

7afc2f7

…move-bdev-scan-cache-pt1-proto Features: control Signed-off-by: Tom Nabarro <[email protected]>

daosbuild1 reviewed Dec 13, 2023

View reviewed changes

mjmac approved these changes Dec 13, 2023

View reviewed changes

mchaarawi merged commit f34040a into master Dec 13, 2023
42 of 45 checks passed

mchaarawi deleted the tanabarr/control-remove-bdev-scan-cache-pt1-proto branch December 13, 2023 22:50

DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC #13382

DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC #13382

Conversation

tanabarr commented Nov 23, 2023 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Nov 23, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 23, 2023

daosbuild1 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

daosbuild1 left a comment

Choose a reason for hiding this comment

tanabarr commented Nov 23, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 23, 2023

daosbuild1 commented Nov 23, 2023

knard38 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

knard38 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanabarr Nov 27, 2023 • 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

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 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 27, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

kjacque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangshilong left a comment

Choose a reason for hiding this comment

daosbuild1 commented Dec 7, 2023

daosbuild1 commented Dec 7, 2023

tanabarr commented Dec 7, 2023

daosbuild1 commented Dec 10, 2023

daosbuild1 commented Dec 10, 2023

tanabarr commented Dec 11, 2023

tanabarr commented Dec 12, 2023

tanabarr commented Dec 12, 2023

mchaarawi commented Dec 12, 2023 • edited Loading

tanabarr commented Dec 13, 2023

NiuYawei left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

tanabarr commented Dec 13, 2023

tanabarr commented Dec 13, 2023

daosbuild1 commented Dec 13, 2023

tanabarr commented Nov 23, 2023 •

edited

Loading

github-actions bot commented Nov 23, 2023 •

edited

Loading

tanabarr commented Nov 23, 2023 •

edited

Loading

knard38 Nov 27, 2023 •

edited

Loading

tanabarr Nov 27, 2023 •

edited

Loading

mchaarawi commented Dec 12, 2023 •

edited

Loading