DAOS-14985 bio: Handle NVMe unplugged in list devs #13614

tanabarr · 2024-01-16T14:18:46Z

dmg storage query list-devices fails if run after a NVMe device is
unplugged during a physical hot-remove. The failure is due to
UNPLUGGED NVMe device state resulting in a SMD object not being
populated with a NVMe controller reference. Unplugged devices should
be reported in list-devices and ignored in storage scan so handle
UNPLUGGED device state without returning an error.

Features: control nvme
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:

Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

github-actions · 2024-01-16T14:19:07Z

Bug-tracker data:
Ticket title is 'dmg storage query list-devices fails if device is unplugged'
Status is 'In Review'
Labels: 'hotplug'
https://daosio.atlassian.net/browse/DAOS-14985

NiuYawei

Is this a regression introduced by removing device cache on control plane?

Besides the 'unplugged' case, please be aware that there is an 'unused' case: a device is plugged but not used by DAOS (not presented in SMD), is this case handled properly?

daosbuild1 · 2024-01-17T02:40:52Z

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

mjmac · 2024-01-17T15:18:44Z

src/control/server/instance_storage_rpc.go

@@ -198,6 +198,9 @@ func scanEngineBdevsOverDrpc(ctx context.Context, engine Engine, pbReq *ctlpb.Sc
 			return nil, errors.Errorf("smd %q has no ctrlr ref", sd.Uuid)
 		}

+		if sd.Ctrlr.DevState == ctlpb.NvmeDevState_UNPLUGGED {


I wonder if would be safer to skip the controller unless its state is in a set of known-good/expected states.

I have updated the PR to address this.

…ged-state Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

tanabarr · 2024-01-18T14:19:04Z

Is this a regression introduced by removing device cache on control plane?

Besides the 'unplugged' case, please be aware that there is an 'unused' case: a device is plugged but not used by DAOS (not presented in SMD), is this case handled properly?

I don't think this is specifically a regression introduced by a recent PR, it's not something we currently test in CI. @shimizukko is working on adding these tests using hotplug event emulation. When a device is plugged but not used by DAOS it will not be reported by either list-devices or dmg storage scan when engines are running. It will be returned if engines are running and either no bdevs specified in the server config or it's PCI address is specified in the server config. Does that sound acceptable?

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

mjmac · 2024-01-18T14:50:08Z

src/control/server/instance_storage_rpc.go

+		if sd.Ctrlr.DevState != ctlpb.NvmeDevState_NORMAL &&
+			sd.Ctrlr.DevState != ctlpb.NvmeDevState_NEW &&
+			sd.Ctrlr.DevState != ctlpb.NvmeDevState_EVICTED {


This will work, but if it were me I'd add a method to check this on NvmeController, e.g. sd.Ctrlr.Scannable(). That keeps the definition of what makes the controller scannable in one place, so it's easier to maintain when the inevitable future changes happen (e.g. to protobufs, etc).

I can do this, probably should also do the same for the list of states that can be queried for health as that is different.

…ged-state Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

tanabarr · 2024-01-18T21:23:21Z

[tanabarr@wolf-314 daos]$ install-rocky/bin/dmg storage query list-devices -i
---------
localhost
---------
  Devices
    UUID:39e0f1a3-42f2-45f2-b8b3-86482af5a407 [TrAddr:]
      Roles: Targets:[0 2 4 6] Rank:0 State:UNPLUGGED LED:OFF
    UUID:af75dfdd-2c49-4722-9e67-d8559bd1f863 [TrAddr:5d0505:03:00.0]
      Roles: Targets:[1 3 5 7] Rank:0 State:NORMAL LED:OFF

knard-intel

LGTM

daosbuild1 · 2024-01-19T23:06:56Z

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

…ged-state Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

wangshilong · 2024-01-22T01:59:55Z

src/mgmt/srv_query.c

+	}
+	D_ALLOC_PTR(ctrlr->namespaces[0]);
+	if (ctrlr->namespaces[0] == NULL) {
+		return -DER_NOMEM;


[defect] ctrlr->namespaces leaked?

yes, thanks for spotting, fixed in the relevant free function

Features: nvme control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

wangshilong

C parts looks good.

wangshilong · 2024-01-23T01:16:27Z

src/mgmt/srv_drpc.c

 			D_FREE(dev->ctrlr->namespaces[0]);
+			D_FREE(dev->ctrlr->namespaces);
+			dev->ctrlr->namespaces   = NULL;


[style] D_FREE() inside will assign NULL after free.

Thank you I was following the pattern in list_devs but I will remove both NULL assignments when I visit this area next.

daosbuild1 · 2024-01-23T18:57:36Z

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

tanabarr · 2024-01-23T22:28:33Z

https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13614/4/tests/

server.ObjectMetadata failed 3 tests due to https://daosio.atlassian.net/browse/DAOS-14517
DAOS_Nvme_Recov failed 6 tests due to https://daosio.atlassian.net/browse/DAOS-15038

CI run with pragmas "Features: control nvme"

daosbuild1 · 2024-01-23T22:42:09Z

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

tanabarr · 2024-01-24T21:44:49Z

@NiuYawei could you please approve the PR, @mchaarawi is asking for this before it can be force landed. Thanks

tanabarr · 2024-01-25T14:52:34Z

GATEKEEPER: Please use the PR title and description above as the commit message when landing. TIA

This reverts commit e86a3b3.

DAOS-14985 bio: Handle NVMe unplugged in list devs

15a47c4

Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

tanabarr requested a review from a team as a code owner January 16, 2024 14:18

tanabarr requested review from mjmac and kjacque and removed request for a team January 16, 2024 14:18

tanabarr self-assigned this Jan 16, 2024

tanabarr added bug hotplug labels Jan 16, 2024

tanabarr requested review from wangshilong, NiuYawei, knard-intel, a team and shimizukko January 16, 2024 14:21

NiuYawei reviewed Jan 17, 2024

View reviewed changes

mjmac reviewed Jan 17, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into tanabarr/bio-unplug…

42dc80a

…ged-state Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

address review comments from mjmac

14846c2

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

tanabarr requested review from NiuYawei and mjmac January 18, 2024 14:38

mjmac reviewed Jan 18, 2024

View reviewed changes

tanabarr added 3 commits January 18, 2024 17:17

Merge remote-tracking branch 'origin/master' into tanabarr/bio-unplug…

444019d

…ged-state Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

add NvmeController helpers to detect acceptable state

7b86208

Features: control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

don't populate pci address if unplugged

8c95d78

Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

mjmac previously approved these changes Jan 18, 2024

View reviewed changes

knard-intel previously approved these changes Jan 19, 2024

View reviewed changes

shimizukko previously approved these changes Jan 20, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into tanabarr/bio-unplug…

a9c9ece

…ged-state Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

wangshilong reviewed Jan 22, 2024

View reviewed changes

address memory leak as per code review comment from wangshilong

b423777

Features: nvme control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

tanabarr dismissed stale reviews from shimizukko, knard-intel, and mjmac via b423777 January 22, 2024 18:33

tanabarr requested review from wangshilong, mjmac, shimizukko and knard-intel January 22, 2024 18:34

wangshilong approved these changes Jan 23, 2024

View reviewed changes

shimizukko approved these changes Jan 23, 2024

View reviewed changes

knard-intel approved these changes Jan 23, 2024

View reviewed changes

tanabarr requested a review from a team January 23, 2024 23:13

tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 23, 2024

NiuYawei approved these changes Jan 25, 2024

View reviewed changes

tanabarr requested a review from a team January 25, 2024 11:56

mchaarawi merged commit e86a3b3 into master Jan 25, 2024
48 of 51 checks passed

mchaarawi deleted the tanabarr/bio-unplugged-state branch January 25, 2024 15:01

mjmac added a commit that referenced this pull request Jan 26, 2024

Revert "DAOS-14985 bio: Handle NVMe unplugged in list devs (#13614)"

d7728e2

This reverts commit e86a3b3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-14985 bio: Handle NVMe unplugged in list devs #13614

DAOS-14985 bio: Handle NVMe unplugged in list devs #13614

tanabarr commented Jan 16, 2024 •

edited

Loading

github-actions bot commented Jan 16, 2024

NiuYawei left a comment

daosbuild1 commented Jan 17, 2024

mjmac Jan 17, 2024

tanabarr Jan 18, 2024

tanabarr commented Jan 18, 2024

mjmac Jan 18, 2024

tanabarr Jan 18, 2024

tanabarr commented Jan 18, 2024

knard-intel left a comment

daosbuild1 commented Jan 19, 2024

wangshilong Jan 22, 2024

tanabarr Jan 22, 2024

wangshilong left a comment

wangshilong Jan 23, 2024

tanabarr Jan 23, 2024

daosbuild1 commented Jan 23, 2024

tanabarr commented Jan 23, 2024 •

edited

Loading

daosbuild1 commented Jan 23, 2024

tanabarr commented Jan 24, 2024

tanabarr commented Jan 25, 2024

DAOS-14985 bio: Handle NVMe unplugged in list devs #13614

DAOS-14985 bio: Handle NVMe unplugged in list devs #13614

Conversation

tanabarr commented Jan 16, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 16, 2024

NiuYawei left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanabarr commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanabarr commented Jan 18, 2024

knard-intel left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 23, 2024

tanabarr commented Jan 23, 2024 • edited Loading

daosbuild1 commented Jan 23, 2024

tanabarr commented Jan 24, 2024

tanabarr commented Jan 25, 2024

tanabarr commented Jan 16, 2024 •

edited

Loading

tanabarr commented Jan 23, 2024 •

edited

Loading