DAOS-19772 control: Handle non-zero PCI domains in hotplug filter by tanabarr · Pull Request #19259 · daos-stack/daos

tanabarr · 2026-10-13T11:08:20Z

bdev_busid_range assumed PCI domain 0x0000 and only encoded an 8-bit
bus-ID range. On multi-PCI-segment platforms with non-zero domains,
this silently filtered the wrong bus-IDs (DAOS-19772: a hot-plugged
drive was claimed by the wrong engine).

Add bdev_hotplug_range, a config array of PCI domain + bus-ID-range
entries (one domain per entry; plain hex, no "0x" prefix, matching
ordinary PCI address formatting), replacing the legacy domain-0x00-
only bdev_busid_range (kept for backward compatibility). When unset,
ranges are now auto-derived directly from bdev_list's actual PCI
addresses instead of NUMA-node topology, collapsing only truly-
contiguous bus-IDs so the filter never admits an address absent from
bdev_list; VMD still allows all. Thread the resulting ranges through
to the SPDK JSON config and bio_config.c's hotplug_filter_fn, which
now matches domain+bus-ID per entry instead of a single bus-ID range.

Update daos_server.yml/admin guide to describe only the automatic
behavior; manual syntax is retained only in the design doc and
src/control/server/storage/README.md as an undocumented, experimental
escape hatch. Decimal notation is no longer accepted, hex only.

Add design/control/hotplug-pci-domain-design.md.

Features: control

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

github-actions · 2026-10-13T11:08:38Z

Ticket title is 'Hot-plugged drive claimed by the wrong engine'
Status is 'In Review'
Labels: 'request_for_2.8.2'
https://daosio.atlassian.net/browse/DAOS-19772

daosbuild3 · 2026-10-14T17:22:00Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19259/1/testReport/

daosbuild3 · 2026-10-15T14:22:07Z

Test stage Unit Test completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19259/5/testReport/

daosbuild3 · 2026-10-16T10:57:05Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19259/6/testReport/

tanabarr · 2026-10-20T19:17:19Z

CI build 6 known failures with Features: control

FTEST_control.DmgNetworkScanTest

Fix validated on Trout, Hotplug polling and replacement works as expected, manual verification process documented in ticket DAOS-19772.

kjacque

Only looked at the applicable commit... LGTM. My one comment is very minor. I guess this will need to be rebased before merging to remove the system erase commit?

daosbuild3 · 2026-10-22T07:45:34Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19259/7/testReport/

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

The base branch was changed.

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

…in' into tanabarr/control-hotplug-pcidomain Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

daosbuild3 · 2026-11-05T13:26:26Z

Test stage Unit Test completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19259/19/testReport/

knard38

If my concerns are valid they should be fixed before landing

knard38 · 2026-11-05T14:45:40Z

From my understanding, the previous hwloc-based derivation returned the bus range of the PCI host bridges of the whole NUMA node, so the filter did not depend on the bdev tiers. Now it is derived from bdev_list, which only lists the devices of one tier, and setHotplugRanges() is only called for the first bdev tier (idx != 0 is skipped).

For example, with the configuration below the filter written to daos_nvme.conf is only 0000:e3, so the SSDs of the second tier (0000:e4, 0000:e5 and 0000:e6) would be refused by their own engine.

storage: - class: ram scm_mount: /mnt/dram1 - class: nvme # first bdev tier (idx 0) bdev_roles: [wal] bdev_list: ["0000:e3:00.0"] - class: nvme # second bdev tier (idx 1) bdev_roles: [meta, data] bdev_list: ["0000:e4:00.0", "0000:e5:00.0", "0000:e6:00.0"]

knard38 · 2026-11-05T15:03:58Z

+// TestStorage_BdevHotplugRange_JSON is a regression test for JSON marshal/unmarshal of
+// BdevHotplugRange.
+func TestStorage_BdevHotplugRange_JSON(t *testing.T) {
+	hr, err := NewBdevHotplugRange("0x01:0x00-0xFF")


From my investigation with copilot, this test fails on the current head: the grammar of this PR no longer accepts the 0x prefix, so NewBdevHotplugRange("0x01:0x00-0xFF") returns parse identifier "0x01", expected plain hex notation and the test stops at its first t.Fatal. The Jenkins Unit Test stage of the head fails on exactly this test.

Suggested change

hr, err := NewBdevHotplugRange("0x01:0x00-0xFF")

hr, err := NewBdevHotplugRange("01:00-FF")

knard38 · 2026-11-05T16:12:37Z

Minor, non-blocking: from copilot, a few statements of the design document and of the admin guide do not seem to match the branch (each checked by copilot against the current head):

Section 5 and the PR description say the manual syntax is documented in src/control/server/storage/README.md, but that file is not part of the PR and does not exist in the repository, so this document is currently the only place describing bdev_hotplug_range. Should the README be added, or the references removed?

Section 7 says daos_server.yml gains a bdev_hotplug_range parameter, which contradicts section 5 and the diff (the diff only removes the bdev_busid_range lines from daos_server.yml).

Smaller inaccuracies: section 4.1 names setHotplugRange() (the function is setHotplugRanges); section 4.4 calls spdk_json_decode_array an existing pattern of bio_config.c (this PR adds its only use, in decode_daos_array); section 8 lists overlapping-range validation and C-side tests that I could not find in the PR; section 6 says the file is regenerated "on every engine start" (it is regenerated when the old file cannot be parsed, or at format).

The admin guide (docs/admin/administration.md) says the filter is "precise to exactly the devices the engine was formatted with" and "always succeeds"; this does not hold for engines with several bdev tiers (see my comment on provider.go) nor beyond 64 entries (HOTPLUG_FILTER_MAX_ENTRIES in bio_config.c).

The legacy bdev_busid_range still works as an alias of domain 0000 but is no longer documented in daos_server.yml or the admin guide, and its use is only logged at DEBUG. Would a one-line "deprecated, applies to domain 0000 only" note in daos_server.yml (and perhaps a NOTICE in the log) help people who still have it in their configuration?

kjacque

Thanks for the format updates. I think the issues Cedric found are valid and should be addressed.

kjacque · 2026-11-05T19:24:28Z

+	if strings.Contains(parts[0], "-") {
+		return nil, errors.Errorf(
+			"invalid hotplug range domain %q in %q, only a single domain is supported "+
+				"per entry", parts[0], entryStr)
+	}


This would be detected as simply a bad string format when the domain value is parsed, right? I don't think it needs to be called out specifically.

kjacque · 2026-11-05T19:42:01Z

+		ranges, err := deriveHotplugRangesFromBdevList(tier.Bdev.DeviceList)
 		if err != nil {
-			return errors.Wrapf(err, "get busid range limits")
+			return errors.Wrap(err, "derive hotplug filter ranges from bdev_list")
 		}
+		req.HotplugRanges = ranges


I think Cedric's comment is right, this is only getting the hotplug range for a single tier and overriding the range list. I think it would be fine to append them instead, since the caller is looping over the tiers.

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

knard38 · 2026-11-06T06:55:48Z

+		log.Debug("auto-deriving hotplug filter ranges from bdev_list of all bdev tiers")
+		ranges, err := deriveHotplugRangesFromBdevList(bdevCfgs.NVMeBdevs())
 		if err != nil {


From my investigation with copilot, your fix introduced a regression with emulated dev tiers and copilot proposed the following fix which makes sense to me:

Suggested change

log.Debug("auto-deriving hotplug filter ranges from bdev_list of all bdev tiers")

ranges, err := deriveHotplugRangesFromBdevList(bdevCfgs.NVMeBdevs())

if err != nil {

if !bdevCfgs.HaveRealNVMe() && bdevCfgs.HaveEmulatedNVMe() {

// Only emulated bdevs (class file or kdev): there is no PCI address to derive a filter from.

log.Debug("emulated bdevs only: no hotplug filter ranges configured")

req.HotplugRanges = nil

return nil

}

log.Debug("auto-deriving hotplug filter ranges from bdev_list of all bdev tiers")

ranges, err := deriveHotplugRangesFromBdevList(bdevCfgs.NVMeBdevs())

if err != nil {

thank you, resolved

knard38 · 2026-11-06T07:13:12Z

+			tier.Bdev.BusidRange.IsZero()) {
+			continue
+		}
+		if manualTier != nil {


NIT, it could be more user friendly to have this check in TierConfigs.Validate() to inform the user as soon as possible that a manual bdev_hotplug_range (or the legacy bdev_busid_range) is tolerated on one bdev tier only per engine.

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

daosbuild3 · 2026-11-06T12:48:06Z

Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-19259/20/execution/node/1463/log

tanabarr · 2026-11-06T13:24:41Z

@kjacque @knard38 have resolved comments, please could you rereview

knard38

LGTM

daltonbohning · 2026-11-06T21:33:01Z

@tanabarr Requested to land and will fix clang-format in a separate PR.

mchaarawi · 2026-11-06T21:59:09Z

@tanabarr Requested to land and will fix clang-format in a separate PR.

why? can we please just fix it here

mjmac

Approving because I think this is a much better approach now and I have limited review bandwidth for the next week or two. It does seem like the BdevHotplugRange implementation could be cleaned up to match the C struct, though, unless I'm missing something.

Also, it seems like parts of the design document were written from the perspective of how the design was changed from the first approach instead of what it changes from master. That's not really useful context to include in the final result, IMO.

mjmac · 2026-11-07T02:30:19Z

+type BdevHotplugRange struct {
+	LowAddress  hardware.PCIAddress
+	HighAddress hardware.PCIAddress
+}


Out of curiosity, why use two PCIAddress members instead of something like this, which also matches the C struct:

type BdevHotplugRange struct { Domain uint16 BusBegin uint8 BusEnd uint8 }

I'm all for type reuse when it makes sense, but the current type has unused fields and the possibility of accidentally setting different domains on the low/high addresses.

thank you . I will follow up on your review comments after landing

tanabarr · 2026-11-07T13:40:44Z

@tanabarr Requested to land and will fix clang-format in a separate PR.

why? can we please just fix it here

Sure I will push formatting fix, hopefully we can land on Monday without having to get reviews again given it's just clang formatting that will be fixed and we already have three reviews? @daltonbohning does that sound okay?

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr self-assigned this Oct 13, 2026

tanabarr requested review from NiuYawei, kjacque, knard38 and mjmac October 13, 2026 11:09

tanabarr marked this pull request as ready for review October 15, 2026 12:07

tanabarr requested review from a team as code owners October 15, 2026 12:07

tanabarr force-pushed the tanabarr/control-hotplug-pcidomain branch 3 times, most recently from c81a73a to 2dec464 Compare October 15, 2026 13:41

tanabarr changed the base branch from master to tanabarr/control-spdk-devtype October 15, 2026 13:46

tanabarr force-pushed the tanabarr/control-hotplug-pcidomain branch from 2dec464 to d225cee Compare October 15, 2026 14:39

tanabarr force-pushed the tanabarr/control-hotplug-pcidomain branch from d225cee to 17d0124 Compare October 21, 2026 13:31

tanabarr requested a review from a team as a code owner October 21, 2026 13:31

kjacque previously approved these changes Oct 21, 2026

View reviewed changes

Comment thread src/control/server/storage/bdev/backend_json.go Resolved

DAOS-19772 control: Handle non-zero PCI domains in hotplug filter

c7fa0ff

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr force-pushed the tanabarr/control-hotplug-pcidomain branch from 17d0124 to c7fa0ff Compare October 22, 2026 10:58

tanabarr requested review from a team as code owners October 22, 2026 10:58

tanabarr changed the base branch from tanabarr/control-spdk-devtype to master October 22, 2026 10:59

fix regression in BdevHotplugRange by reverting use of hardware.PCIBus

be159f0

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr dismissed kjacque’s stale review via be159f0 November 5, 2026 12:23

tanabarr added 2 commits November 5, 2026 12:45

address review comments from kjacque, no 0x prefix, single domain spec

5b7359f

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

Merge remote-tracking branch 'origin/tanabarr/control-hotplug-pcidoma…

99f4c3e

…in' into tanabarr/control-hotplug-pcidomain Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr requested a review from kjacque November 5, 2026 12:48

knard38 requested changes Nov 5, 2026

View reviewed changes

kjacque reviewed Nov 5, 2026

View reviewed changes

address more review comments

d2c559e

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

knard38 reviewed Nov 6, 2026

View reviewed changes

address review comments related to bug when multiple bdev tiers

60b308c

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr requested review from kjacque and knard38 November 6, 2026 13:20

knard38 previously approved these changes Nov 6, 2026

View reviewed changes

kjacque previously approved these changes Nov 6, 2026

View reviewed changes

daltonbohning added the release-3.0.0 Targeted for release 3.0.0 label Nov 6, 2026

mjmac previously approved these changes Nov 7, 2026

View reviewed changes

tanabarr requested a review from a team November 7, 2026 11:03

purely whitespace formatting clang fixes

dc6e047

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr dismissed stale reviews from mjmac, kjacque, and knard38 via dc6e047 November 7, 2026 13:47

tanabarr requested review from kjacque, knard38 and mjmac November 7, 2026 13:48

	hr, err := NewBdevHotplugRange("0x01:0x00-0xFF")
	hr, err := NewBdevHotplugRange("01:00-FF")

Conversation

tanabarr commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Oct 14, 2026

Uh oh!

daosbuild3 commented Oct 15, 2026

Uh oh!

daosbuild3 commented Oct 16, 2026

Uh oh!

tanabarr commented Oct 20, 2026

Uh oh!

kjacque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daosbuild3 commented Oct 22, 2026

Uh oh!

daosbuild3 commented Nov 5, 2026

Uh oh!

knard38 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjacque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daosbuild3 commented Nov 6, 2026

Uh oh!

tanabarr commented Nov 6, 2026

Uh oh!

knard38 left a comment

Choose a reason for hiding this comment

Uh oh!

daltonbohning commented Nov 6, 2026

Uh oh!

mchaarawi commented Nov 6, 2026

Uh oh!

mjmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanabarr commented Nov 7, 2026

Uh oh!

This branch has not been deployed

tanabarr commented Oct 13, 2026 •

edited

Loading

github-actions Bot commented Oct 13, 2026 •

edited

Loading