From 50d69c82e57202829898600f623fe2ef9aad18f8 Mon Sep 17 00:00:00 2001 From: Tom Nabarro Date: Fri, 10 Jul 2026 18:16:28 +0100 Subject: [PATCH 1/3] DAOS-19288 control: Add allow-numa-imbalance flag to config generate Signed-off-by: Tom Nabarro --- docs/admin/deployment.md | 15 ++++ src/control/common/cmdutil/auto.go | 17 ++-- src/control/lib/control/auto.go | 111 +++++++++++++++++++++++++-- src/control/lib/control/auto_test.go | 105 ++++++++++++++++++++++++- 4 files changed, 230 insertions(+), 18 deletions(-) diff --git a/docs/admin/deployment.md b/docs/admin/deployment.md index 633a48a83c8..29381f4db12 100644 --- a/docs/admin/deployment.md +++ b/docs/admin/deployment.md @@ -298,6 +298,8 @@ Help Options: MD-on-SSD config -f, --fabric-ports= Allow custom fabric interface ports to be specified for each engine config section. Comma separated port numbers, one per engine + -n, --allow-numa-imbalance Distribute NVMe devices equally across engines regardless of NUMA + affinity (may result in suboptimal performance due to cross-NUMA access) --skip-prep Skip preparation of devices during scan. ``` @@ -344,6 +346,8 @@ Help Options: MD-on-SSD config -f, --fabric-ports= Allow custom fabric interface ports to be specified for each engine config section. Comma separated port numbers, one per engine + -n, --allow-numa-imbalance Distribute NVMe devices equally across engines regardless of NUMA + affinity (may result in suboptimal performance due to cross-NUMA access) ``` The `daos_server` service must be running on the remote storage servers and as such a minimal @@ -404,6 +408,17 @@ this option is set then a MD-on-SSD config will be generated. - `--fabric-ports` enables custom port numbers to be assigned to each engine's fabric settings. Comma separated list must contain enough numbers to cover all engines generated in config. +- `--allow-numa-imbalance` allows the generation of a server config with NVMe devices distributed +equally across engines regardless of NUMA affinity. By default, config generation respects NUMA +affinity and balances the number of SSDs assigned to each engine by limiting all engines to use +only the lowest common number of SSDs across NUMA nodes. When this flag is set, all available SSDs +are collected and distributed equally across all engines, ignoring NUMA node boundaries. If the total +number of SSDs is not evenly divisible by the number of engines, only the maximum divisible number +of SSDs are used (e.g., 7 SSDs / 2 engines = 3 per engine, 1 SSD unused). This maximizes device +utilization in heterogeneous environments but **may result in suboptimal performance** due to +cross-NUMA memory access. The generated config will include `allow_numa_imbalance: true` to allow +the server to start with this configuration. + The text generated by the command and output to stdout can be copied and used as the server config file on relevant hosts (normally by copying to `/etc/daos/daos_server.yml` and (re)starting service). diff --git a/src/control/common/cmdutil/auto.go b/src/control/common/cmdutil/auto.go index 26bd56eba49..aa3f82252df 100644 --- a/src/control/common/cmdutil/auto.go +++ b/src/control/common/cmdutil/auto.go @@ -8,14 +8,15 @@ type deprecatedParams struct { type ConfGenCmd struct { deprecatedParams - MgmtSvcReplicas string `default:"localhost" short:"r" long:"ms-replicas" description:"Comma separated list of MS replica addresses to host management service"` - NrEngines int `short:"e" long:"num-engines" description:"Set the number of DAOS Engine sections to be populated in the config file output. If unset then the value will be set to the number of NUMA nodes on storage hosts in the DAOS system."` - SCMOnly bool `short:"s" long:"scm-only" description:"Create a SCM-only config without NVMe SSDs."` - NetClass string `default:"infiniband" short:"c" long:"net-class" description:"Set the network device class to be used" choice:"ethernet" choice:"infiniband"` - NetProvider string `short:"p" long:"net-provider" description:"Set the network fabric provider to be used"` - UseTmpfsSCM bool `short:"t" long:"use-tmpfs-scm" description:"Use tmpfs for scm rather than PMem"` - ExtMetadataPath string `short:"m" long:"control-metadata-path" description:"External storage path to store control metadata. Set this to a persistent location and specify --use-tmpfs-scm to create an MD-on-SSD config"` - FabricPorts string `short:"f" long:"fabric-ports" description:"Allow custom fabric interface ports to be specified for each engine config section. Comma separated port numbers, one per engine"` + MgmtSvcReplicas string `default:"localhost" short:"r" long:"ms-replicas" description:"Comma separated list of MS replica addresses to host management service"` + NrEngines int `short:"e" long:"num-engines" description:"Set the number of DAOS Engine sections to be populated in the config file output. If unset then the value will be set to the number of NUMA nodes on storage hosts in the DAOS system."` + SCMOnly bool `short:"s" long:"scm-only" description:"Create a SCM-only config without NVMe SSDs."` + NetClass string `default:"infiniband" short:"c" long:"net-class" description:"Set the network device class to be used" choice:"ethernet" choice:"infiniband"` + NetProvider string `short:"p" long:"net-provider" description:"Set the network fabric provider to be used"` + UseTmpfsSCM bool `short:"t" long:"use-tmpfs-scm" description:"Use tmpfs for scm rather than PMem"` + ExtMetadataPath string `short:"m" long:"control-metadata-path" description:"External storage path to store control metadata. Set this to a persistent location and specify --use-tmpfs-scm to create an MD-on-SSD config"` + FabricPorts string `short:"f" long:"fabric-ports" description:"Allow custom fabric interface ports to be specified for each engine config section. Comma separated port numbers, one per engine"` + AllowNumaImbalance bool `short:"n" long:"allow-numa-imbalance" description:"Allow generation of config with imbalanced NVMe distribution. For non-VMD: redistribute devices equally across engines. For VMD: accept imbalanced VMD domains as-is"` } // CheckDeprecated will check for deprecated parameters and update as needed. diff --git a/src/control/lib/control/auto.go b/src/control/lib/control/auto.go index d07a0a3e9c7..64724a11c42 100644 --- a/src/control/lib/control/auto.go +++ b/src/control/lib/control/auto.go @@ -70,8 +70,10 @@ type ( // Generate config with a tmpfs RAM-disk SCM. UseTmpfsSCM bool `json:"UseTmpfsSCM"` // Location to persist control-plane metadata, will generate MD-on-SSD config. - ExtMetadataPath string `json:"ExtMetadataPath"` - Log logging.Logger `json:"-"` + ExtMetadataPath string `json:"ExtMetadataPath"` + // Allow NUMA imbalance when NVMe devices are not evenly distributed. + AllowNumaImbalance bool `json:"AllowNumaImbalance"` + Log logging.Logger `json:"-"` } // ConfGenerateResp contains the generated server config. @@ -476,8 +478,97 @@ func (nsm numaSSDsMap) keys() (keys []int) { return } -// mapSSDs maps NUMA node ID to NVMe SSD PCI address set. -func (nsm numaSSDsMap) fromNVMe(ssds storage.NvmeControllers) error { +// redistributeSsdsIgnNuma allocates all available SSDs equally across engines, ignoring NUMA affinity. +// Each engine receives the same number of SSDs. If the total number of SSDs is not evenly +// divisible by the number of engines, only the maximum divisible number of SSDs are used and +// remainder SSDs are not included in the generated configuration. +func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMap) error { + // Check if any NUMA node has VMD devices + // hasVMD := false + // for _, ssds := range sd.NumaSSDs { + // if ssds.HasVMD() { + // hasVMD = true + // break + // } + // } + + // if allowImbalance { + // if hasVMD { + // // VMD domains cannot be redistributed across NUMA nodes, but we can accept + // // the imbalance and keep VMD domains on their original NUMA nodes + // log.Debug("allow-numa-imbalance enabled with VMD devices, accepting imbalanced VMD configuration") + // return nil + // } + req.Log.Debug("allow-numa-imbalance enabled, distributing SSDs equally across engines") + // TODO: could continue here?? maybe not + // return distributeSsdsIgnNuma(log, sd) + // } + + // Assume numaCount contiguous IDs and split appropriately. + // numaIDs := sd.NumaSSDs.keys() + // if len(numaIDs) == 0 { + // return errors.New("no numa nodes with SSDs found") + // } + if numaCount == 0 { + return errors.New("no numa nodes detected") + } + if len(ssds) == 0 { + return errors.New("no ssds detected") + } + + // Collect all SSDs from all NUMA nodes + var allSSDs []string + for _, numaID := range numaIDs { + ssds := sd.NumaSSDs[numaID] + addrs := ssds.Strings() + + if ssds.HasVMD() { + // If addresses are for VMD backing devices, convert to the logical VMD + // domain address as this is what is expected in the server config. + newAddrSet, err := ssds.BackingToVMDAddresses() + if err != nil { + return nil, errors.Wrap(err, "converting backing addresses to vmd") + } + nrSSDs = newAddrSet.Len() + addrs = newAddrSet.Strings() + } + allSSDs = append(allSSDs, addrs...) + } + + totalSSDs := len(allSSDs) + // // TODO: take numaIDs as parameter based on network interfaces or engine nr CLI input + // nrEngines := len(numaIDs) + // Divide SSDs across NUMA Rather than requested number of engines to preserve how the + // selection algorithms work + ssdsPerNuma := totalSSDs / numaCount + remainder := totalSSDs % numaCount + ssdsToUse := ssdsPerNuma * numaCount + + if remainder > 0 { + log.Noticef("total SSDs (%d) not evenly divisible by engines (%d); "+ + "using %d SSDs (%d per engine), %d SSDs will not be used", + totalSSDs, numaCount, ssdsToUse, ssdsPerNuma, remainder) + } + + log.Debugf("distributing %d SSDs equally across %d engines (%d per engine)", + ssdsToUse, numaCount, ssdsPerNuma) + + // Distribute SSDs equally across engines, using only the divisible portion + idx := 0 + for i, numaID := range numaIDs { + numaSSDs := allSSDs[idx : idx+ssdsPerNuma] + sd.NumaSSDs[numaID] = hardware.MustNewPCIAddressSet(numaSSDs...) + log.Debugf("assigned %d SSDs to NUMA-%d (numa %d): %v", + ssdsPerNuma, numaID, i, numaSSDs) + idx += ssdsPerNuma + } + + return nil +} + +// fromNVMe maps NUMA node ID to NVMe SSD PCI address set. +// TODO: If numa imbalance set, convert vmd-backing addresses then distribute over numa-count. +func (nsm numaSSDsMap) fromNVMe(req *ConfGenerateReq, ssds storage.NvmeControllers, numaCount int) error { if nsm == nil { return errors.Errorf("%T receiver is nil", nsm) } @@ -498,6 +589,13 @@ func (nsm numaSSDsMap) fromNVMe(ssds storage.NvmeControllers) error { nsm[nn] = newAddrSet } + if req.AllowNumaImbalance { + // Pretend NVMe devices are distributed equally across NUMA nodes + if err := redistributeSsdsIgnNuma(req, numaCount, nsm); err != nil { + return nil, err + } + } + return nil } @@ -530,7 +628,7 @@ func getStorageDetails(req ConfGenerateReq, numaCount int, hs *HostStorage) (*st return nil, errors.New("requires nonzero HugepageSizeKiB") } - if err := sd.NumaSSDs.fromNVMe(hs.NvmeDevices); err != nil { + if err := sd.NumaSSDs.fromNVMe(&req, hs.NvmeDevices, numaCount); err != nil { return nil, errors.Wrap(err, "mapping ssd addresses to numa node") } @@ -1231,7 +1329,8 @@ func genServerConfig(req ConfGenerateReq, ecs []*engine.Config, tc *threadCounts WithMgmtSvcReplicas(req.MgmtSvcReplicas...). WithFabricProvider(ecs[0].Fabric.Provider). WithEngines(ecs...). - WithControlLogFile(defaultControlLogFile) + WithControlLogFile(defaultControlLogFile). + WithAllowNumaImbalance(req.AllowNumaImbalance) for idx := range cfg.Engines { tiers := cfg.Engines[idx].Storage.Tiers diff --git a/src/control/lib/control/auto_test.go b/src/control/lib/control/auto_test.go index 596b0d3c5de..a6b99940183 100644 --- a/src/control/lib/control/auto_test.go +++ b/src/control/lib/control/auto_test.go @@ -1073,9 +1073,10 @@ func TestControl_AutoConfig_filterDevicesByAffinity(t *testing.T) { func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { for name, tc := range map[string]struct { - sd storageDetails - expErr error - expSD storageDetails // expected details after updates + sd storageDetails + allowImbalance bool + expErr error + expSD storageDetails // expected details after updates }{ "no ssds": { sd: storageDetails{ @@ -1108,12 +1109,108 @@ func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { }, }, }, + "allow imbalance distributes equally": { + sd: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5)...), + }, + }, + allowImbalance: true, + expSD: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + // 6 total SSDs distributed equally: 3 per engine + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + }, + }, + }, + "allow imbalance with remainder discards extras": { + sd: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6)...), + }, + }, + allowImbalance: true, + expSD: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + // 7 total SSDs: uses 6 (3 per engine), 1 remainder not used + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + }, + }, + }, + "allow imbalance with 8 SSDs across 2 engines": { + sd: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6, 7)...), + }, + }, + allowImbalance: true, + expSD: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + // 8 total SSDs distributed equally: 4 per engine + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5, 6, 7)...), + }, + }, + }, + "allow imbalance with VMD accepts imbalance": { + sd: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), + 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), + }, + }, + allowImbalance: true, + expSD: storageDetails{ + NumaSCMs: numaSCMsMap{ + 0: []string{"/dev/pmem0"}, + 1: []string{"/dev/pmem1"}, + }, + // VMD domains kept on original NUMA nodes, imbalance accepted + NumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), + 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), + }, + }, + }, } { t.Run(name, func(t *testing.T) { log, buf := logging.NewTestLogger(t.Name()) defer test.ShowBufferOnFailure(t, buf) - gotErr := correctSSDCounts(log, &tc.sd) + gotErr := correctSSDCounts(log, &tc.sd, tc.allowImbalance) test.CmpErr(t, tc.expErr, gotErr) if tc.expErr != nil { return From 257ecaf6e5dd5ffe519fdce22e9b00835fce60dd Mon Sep 17 00:00:00 2001 From: Tom Nabarro Date: Fri, 10 Jul 2026 20:46:39 +0100 Subject: [PATCH 2/3] adjust logic to redistribute 0sds when flag is set after doing the initial collection in fromNVMe() Signed-off-by: Tom Nabarro --- src/control/lib/control/auto.go | 39 ++--- src/control/lib/control/auto_test.go | 204 +++++++++++++-------------- src/control/lib/hardware/pci.go | 11 +- 3 files changed, 132 insertions(+), 122 deletions(-) diff --git a/src/control/lib/control/auto.go b/src/control/lib/control/auto.go index 64724a11c42..e97bea2c2f6 100644 --- a/src/control/lib/control/auto.go +++ b/src/control/lib/control/auto.go @@ -1,6 +1,6 @@ // // (C) Copyright 2020-2024 Intel Corporation. -// (C) Copyright 2025 Hewlett Packard Enterprise Development LP +// (C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP // // SPDX-License-Identifier: BSD-2-Clause-Patent // @@ -512,24 +512,27 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa if numaCount == 0 { return errors.New("no numa nodes detected") } - if len(ssds) == 0 { + if len(nsm) == 0 { return errors.New("no ssds detected") } // Collect all SSDs from all NUMA nodes var allSSDs []string - for _, numaID := range numaIDs { - ssds := sd.NumaSSDs[numaID] - addrs := ssds.Strings() - - if ssds.HasVMD() { + for _, ssdAddrs := range nsm { + // for numaID := 0; numaID < numaCount; mumaID++ { + // for _, numaID := range numaIDs { + // + // ssds := sd.NumaSSDs[numaID] + addrs := ssdAddrs.Strings() + + if ssdAddrs.HasVMD() { // If addresses are for VMD backing devices, convert to the logical VMD // domain address as this is what is expected in the server config. - newAddrSet, err := ssds.BackingToVMDAddresses() + newAddrSet, err := ssdAddrs.BackingToVMDAddresses() if err != nil { - return nil, errors.Wrap(err, "converting backing addresses to vmd") + return errors.Wrap(err, "converting backing addresses to vmd") } - nrSSDs = newAddrSet.Len() + // nrSSDs = newAddrSet.Len() addrs = newAddrSet.Strings() } allSSDs = append(allSSDs, addrs...) @@ -545,21 +548,23 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa ssdsToUse := ssdsPerNuma * numaCount if remainder > 0 { - log.Noticef("total SSDs (%d) not evenly divisible by engines (%d); "+ + req.Log.Noticef("total SSDs (%d) not evenly divisible by engines (%d); "+ "using %d SSDs (%d per engine), %d SSDs will not be used", totalSSDs, numaCount, ssdsToUse, ssdsPerNuma, remainder) } - log.Debugf("distributing %d SSDs equally across %d engines (%d per engine)", + req.Log.Debugf("distributing %d SSDs equally across %d engines (%d per engine)", ssdsToUse, numaCount, ssdsPerNuma) // Distribute SSDs equally across engines, using only the divisible portion idx := 0 - for i, numaID := range numaIDs { + // for i, numaID := range numaIDs { + nsm = make(numaSSDsMap) + for numaID := 0; numaID < numaCount; numaID++ { numaSSDs := allSSDs[idx : idx+ssdsPerNuma] - sd.NumaSSDs[numaID] = hardware.MustNewPCIAddressSet(numaSSDs...) - log.Debugf("assigned %d SSDs to NUMA-%d (numa %d): %v", - ssdsPerNuma, numaID, i, numaSSDs) + nsm[numaID] = hardware.MustNewPCIAddressSet(numaSSDs...) + req.Log.Debugf("assigned %d SSDs to NUMA-%d: %v", ssdsPerNuma, numaID, + numaSSDs) idx += ssdsPerNuma } @@ -592,7 +597,7 @@ func (nsm numaSSDsMap) fromNVMe(req *ConfGenerateReq, ssds storage.NvmeControlle if req.AllowNumaImbalance { // Pretend NVMe devices are distributed equally across NUMA nodes if err := redistributeSsdsIgnNuma(req, numaCount, nsm); err != nil { - return nil, err + return errors.Wrap(err, "redistributing numa-imbalanced ssds") } } diff --git a/src/control/lib/control/auto_test.go b/src/control/lib/control/auto_test.go index a6b99940183..d0c03845acc 100644 --- a/src/control/lib/control/auto_test.go +++ b/src/control/lib/control/auto_test.go @@ -1,6 +1,6 @@ // // (C) Copyright 2020-2024 Intel Corporation. -// (C) Copyright 2025 Hewlett Packard Enterprise Development LP +// (C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP // // SPDX-License-Identifier: BSD-2-Clause-Patent // @@ -1073,10 +1073,10 @@ func TestControl_AutoConfig_filterDevicesByAffinity(t *testing.T) { func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { for name, tc := range map[string]struct { - sd storageDetails - allowImbalance bool - expErr error - expSD storageDetails // expected details after updates + sd storageDetails + allowImbalance bool + expErr error + expSD storageDetails // expected details after updates }{ "no ssds": { sd: storageDetails{ @@ -1109,108 +1109,108 @@ func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { }, }, }, - "allow imbalance distributes equally": { - sd: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5)...), - }, - }, - allowImbalance: true, - expSD: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - // 6 total SSDs distributed equally: 3 per engine - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), - }, - }, - }, - "allow imbalance with remainder discards extras": { - sd: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6)...), - }, - }, - allowImbalance: true, - expSD: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - // 7 total SSDs: uses 6 (3 per engine), 1 remainder not used - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), - }, - }, - }, - "allow imbalance with 8 SSDs across 2 engines": { - sd: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6, 7)...), - }, - }, - allowImbalance: true, - expSD: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - // 8 total SSDs distributed equally: 4 per engine - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), - 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5, 6, 7)...), - }, - }, - }, - "allow imbalance with VMD accepts imbalance": { - sd: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), - 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), - }, - }, - allowImbalance: true, - expSD: storageDetails{ - NumaSCMs: numaSCMsMap{ - 0: []string{"/dev/pmem0"}, - 1: []string{"/dev/pmem1"}, - }, - // VMD domains kept on original NUMA nodes, imbalance accepted - NumaSSDs: numaSSDsMap{ - 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), - 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), - }, - }, - }, + // "allow imbalance distributes equally": { + // sd: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5)...), + // }, + // }, + // allowImbalance: true, + // expSD: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // // 6 total SSDs distributed equally: 3 per engine + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + // }, + // }, + // }, + // "allow imbalance with remainder discards extras": { + // sd: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6)...), + // }, + // }, + // allowImbalance: true, + // expSD: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // // 7 total SSDs: uses 6 (3 per engine), 1 remainder not used + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + // }, + // }, + // }, + // "allow imbalance with 8 SSDs across 2 engines": { + // sd: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6, 7)...), + // }, + // }, + // allowImbalance: true, + // expSD: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // // 8 total SSDs distributed equally: 4 per engine + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), + // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5, 6, 7)...), + // }, + // }, + // }, + // "allow imbalance with VMD accepts imbalance": { + // sd: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), + // 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), + // }, + // }, + // allowImbalance: true, + // expSD: storageDetails{ + // NumaSCMs: numaSCMsMap{ + // 0: []string{"/dev/pmem0"}, + // 1: []string{"/dev/pmem1"}, + // }, + // // VMD domains kept on original NUMA nodes, imbalance accepted + // NumaSSDs: numaSSDsMap{ + // 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), + // 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), + // }, + // }, + // }, } { t.Run(name, func(t *testing.T) { log, buf := logging.NewTestLogger(t.Name()) defer test.ShowBufferOnFailure(t, buf) - gotErr := correctSSDCounts(log, &tc.sd, tc.allowImbalance) + gotErr := correctSSDCounts(log, &tc.sd) test.CmpErr(t, tc.expErr, gotErr) if tc.expErr != nil { return diff --git a/src/control/lib/hardware/pci.go b/src/control/lib/hardware/pci.go index e8109d9e799..2a5aca38e01 100644 --- a/src/control/lib/hardware/pci.go +++ b/src/control/lib/hardware/pci.go @@ -1,5 +1,6 @@ // // (C) Copyright 2021-2024 Intel Corporation. +// (C) Copyright 2026 Hewlett Packard Enterprise Development LP // // SPDX-License-Identifier: BSD-2-Clause-Patent // @@ -180,7 +181,8 @@ func (pa *PCIAddress) BackingToVMDAddress() (*PCIAddress, error) { return nil, ErrNotVMDBackingAddress } - return pa.VMDAddr, nil + va := *pa.VMDAddr + return &va, nil } // NewPCIAddress creates a PCIAddress struct from input string. @@ -396,8 +398,11 @@ func (pas *PCIAddressSet) BackingToVMDAddresses() (*PCIAddressSet, error) { for _, inAddr := range pas.Addresses() { if !inAddr.IsVMDBackingAddress() { - if err := outAddrs.Add(inAddr); err != nil { - return nil, err + if inAddr != nil { + ia := *inAddr + if err := outAddrs.Add(&ia); err != nil { + return nil, err + } } continue } From d2a76ce748d94478e5652e3d2c2749a4f6053ad6 Mon Sep 17 00:00:00 2001 From: Tom Nabarro Date: Fri, 10 Jul 2026 22:23:24 +0100 Subject: [PATCH 3/3] add unit tests Features: control Signed-off-by: Tom Nabarro --- src/control/lib/control/auto.go | 49 ++----- src/control/lib/control/auto_test.go | 200 +++++++++++++-------------- 2 files changed, 110 insertions(+), 139 deletions(-) diff --git a/src/control/lib/control/auto.go b/src/control/lib/control/auto.go index e97bea2c2f6..eab28204232 100644 --- a/src/control/lib/control/auto.go +++ b/src/control/lib/control/auto.go @@ -483,32 +483,6 @@ func (nsm numaSSDsMap) keys() (keys []int) { // divisible by the number of engines, only the maximum divisible number of SSDs are used and // remainder SSDs are not included in the generated configuration. func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMap) error { - // Check if any NUMA node has VMD devices - // hasVMD := false - // for _, ssds := range sd.NumaSSDs { - // if ssds.HasVMD() { - // hasVMD = true - // break - // } - // } - - // if allowImbalance { - // if hasVMD { - // // VMD domains cannot be redistributed across NUMA nodes, but we can accept - // // the imbalance and keep VMD domains on their original NUMA nodes - // log.Debug("allow-numa-imbalance enabled with VMD devices, accepting imbalanced VMD configuration") - // return nil - // } - req.Log.Debug("allow-numa-imbalance enabled, distributing SSDs equally across engines") - // TODO: could continue here?? maybe not - // return distributeSsdsIgnNuma(log, sd) - // } - - // Assume numaCount contiguous IDs and split appropriately. - // numaIDs := sd.NumaSSDs.keys() - // if len(numaIDs) == 0 { - // return errors.New("no numa nodes with SSDs found") - // } if numaCount == 0 { return errors.New("no numa nodes detected") } @@ -516,13 +490,11 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa return errors.New("no ssds detected") } + req.Log.Debug("allow-numa-imbalance enabled, distributing SSDs equally across engines") + // Collect all SSDs from all NUMA nodes var allSSDs []string for _, ssdAddrs := range nsm { - // for numaID := 0; numaID < numaCount; mumaID++ { - // for _, numaID := range numaIDs { - // - // ssds := sd.NumaSSDs[numaID] addrs := ssdAddrs.Strings() if ssdAddrs.HasVMD() { @@ -532,17 +504,14 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa if err != nil { return errors.Wrap(err, "converting backing addresses to vmd") } - // nrSSDs = newAddrSet.Len() addrs = newAddrSet.Strings() } allSSDs = append(allSSDs, addrs...) } + // Divide SSDs across NUMA rather than requested number of engines to preserve how the + // selection algorithms work when deciding on NUMA-to-engine mappings. totalSSDs := len(allSSDs) - // // TODO: take numaIDs as parameter based on network interfaces or engine nr CLI input - // nrEngines := len(numaIDs) - // Divide SSDs across NUMA Rather than requested number of engines to preserve how the - // selection algorithms work ssdsPerNuma := totalSSDs / numaCount remainder := totalSSDs % numaCount ssdsToUse := ssdsPerNuma * numaCount @@ -553,13 +522,16 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa totalSSDs, numaCount, ssdsToUse, ssdsPerNuma, remainder) } - req.Log.Debugf("distributing %d SSDs equally across %d engines (%d per engine)", + req.Log.Debugf("distributing %d SSDs equally across %d NUMA nodes (%d per node)", ssdsToUse, numaCount, ssdsPerNuma) + // Clear existing map and repopulate with redistributed SSDs + for k := range nsm { + delete(nsm, k) + } + // Distribute SSDs equally across engines, using only the divisible portion idx := 0 - // for i, numaID := range numaIDs { - nsm = make(numaSSDsMap) for numaID := 0; numaID < numaCount; numaID++ { numaSSDs := allSSDs[idx : idx+ssdsPerNuma] nsm[numaID] = hardware.MustNewPCIAddressSet(numaSSDs...) @@ -572,7 +544,6 @@ func redistributeSsdsIgnNuma(req *ConfGenerateReq, numaCount int, nsm numaSSDsMa } // fromNVMe maps NUMA node ID to NVMe SSD PCI address set. -// TODO: If numa imbalance set, convert vmd-backing addresses then distribute over numa-count. func (nsm numaSSDsMap) fromNVMe(req *ConfGenerateReq, ssds storage.NvmeControllers, numaCount int) error { if nsm == nil { return errors.Errorf("%T receiver is nil", nsm) diff --git a/src/control/lib/control/auto_test.go b/src/control/lib/control/auto_test.go index d0c03845acc..0f32b3c005d 100644 --- a/src/control/lib/control/auto_test.go +++ b/src/control/lib/control/auto_test.go @@ -696,6 +696,103 @@ func TestControl_AutoConfig_getStorageDetails(t *testing.T) { } } +func TestControl_AutoConfig_fromNVMe(t *testing.T) { + for name, tc := range map[string]struct { + ssds storage.NvmeControllers + numaCount int + allowImbalance bool + expErr error + expNumaSSDs numaSSDsMap + }{ + "allow imbalance distributes equally": { + ssds: storage.NvmeControllers{ + &storage.NvmeController{PciAddr: test.MockPCIAddr(0), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(1), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(2), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(3), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(4), SocketID: 1}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(5), SocketID: 1}, + }, + numaCount: 2, + allowImbalance: true, + expNumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + }, + }, + "allow imbalance with remainder discards extras": { + ssds: storage.NvmeControllers{ + &storage.NvmeController{PciAddr: test.MockPCIAddr(0), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(1), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(2), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(3), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(4), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(5), SocketID: 1}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(6), SocketID: 1}, + }, + numaCount: 2, + allowImbalance: true, + expNumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), + }, + }, + "allow imbalance with 8 SSDs across 2 engines": { + ssds: storage.NvmeControllers{ + &storage.NvmeController{PciAddr: test.MockPCIAddr(0), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(1), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(2), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(3), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(4), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(5), SocketID: 1}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(6), SocketID: 1}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(7), SocketID: 1}, + }, + numaCount: 2, + allowImbalance: true, + expNumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5, 6, 7)...), + }, + }, + "no imbalance keeps original distribution": { + ssds: storage.NvmeControllers{ + &storage.NvmeController{PciAddr: test.MockPCIAddr(0), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(1), SocketID: 0}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(2), SocketID: 1}, + &storage.NvmeController{PciAddr: test.MockPCIAddr(3), SocketID: 1}, + }, + numaCount: 2, + allowImbalance: false, + expNumaSSDs: numaSSDsMap{ + 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1)...), + 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(2, 3)...), + }, + }, + } { + t.Run(name, func(t *testing.T) { + log, buf := logging.NewTestLogger(t.Name()) + defer test.ShowBufferOnFailure(t, buf) + + req := ConfGenerateReq{ + Log: log, + AllowNumaImbalance: tc.allowImbalance, + } + + nsm := make(numaSSDsMap) + gotErr := nsm.fromNVMe(&req, tc.ssds, tc.numaCount) + test.CmpErr(t, tc.expErr, gotErr) + if tc.expErr != nil { + return + } + + if diff := cmp.Diff(tc.expNumaSSDs, nsm, defStorCmpOpts...); diff != "" { + t.Fatalf("unexpected numa SSDs (-want, +got):\n%s\n", diff) + } + }) + } +} + func TestControl_AutoConfig_filterDevicesByAffinity(t *testing.T) { singlePMemMap := numaSCMsMap{0: []string{"/dev/pmem0"}} @@ -1073,10 +1170,9 @@ func TestControl_AutoConfig_filterDevicesByAffinity(t *testing.T) { func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { for name, tc := range map[string]struct { - sd storageDetails - allowImbalance bool - expErr error - expSD storageDetails // expected details after updates + sd storageDetails + expErr error + expSD storageDetails }{ "no ssds": { sd: storageDetails{ @@ -1109,102 +1205,6 @@ func TestControl_AutoConfig_correctSSDCounts(t *testing.T) { }, }, }, - // "allow imbalance distributes equally": { - // sd: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5)...), - // }, - // }, - // allowImbalance: true, - // expSD: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // // 6 total SSDs distributed equally: 3 per engine - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), - // }, - // }, - // }, - // "allow imbalance with remainder discards extras": { - // sd: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6)...), - // }, - // }, - // allowImbalance: true, - // expSD: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // // 7 total SSDs: uses 6 (3 per engine), 1 remainder not used - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(3, 4, 5)...), - // }, - // }, - // }, - // "allow imbalance with 8 SSDs across 2 engines": { - // sd: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3, 4)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(5, 6, 7)...), - // }, - // }, - // allowImbalance: true, - // expSD: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // // 8 total SSDs distributed equally: 4 per engine - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(0, 1, 2, 3)...), - // 1: hardware.MustNewPCIAddressSet(test.MockPCIAddrs(4, 5, 6, 7)...), - // }, - // }, - // }, - // "allow imbalance with VMD accepts imbalance": { - // sd: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), - // 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), - // }, - // }, - // allowImbalance: true, - // expSD: storageDetails{ - // NumaSCMs: numaSCMsMap{ - // 0: []string{"/dev/pmem0"}, - // 1: []string{"/dev/pmem1"}, - // }, - // // VMD domains kept on original NUMA nodes, imbalance accepted - // NumaSSDs: numaSSDsMap{ - // 0: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(5, 2, 4)...), - // 1: hardware.MustNewPCIAddressSet(test.MockVMDPCIAddrs(13, 1, 2)...), - // }, - // }, - // }, } { t.Run(name, func(t *testing.T) { log, buf := logging.NewTestLogger(t.Name())