DAOS-13558 control: Fix bdev count check on scan with vmd (#12376) (#…

…12435)

Bdev count check fails during dmg storage query usage when scanning
storage because the comparison is made between the number of detected
backing devices behind VMD rather than the number of VMDs (as
specified in the config file). Fix by resolving the backing devices
back to the VMD devices before comparing the counts.

Previous PR for this change was reverted due to the introduction of
intermittent unit test failures. Additional unit test coverage exposed
a bug in lib/harware/pci.go:PCIAdress.LessThan() where VMD backing
device addresses are improperly compared. Fix is applied in this PR.

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

src/control/lib/control/auto.go

@@ -891,7 +891,7 @@ func correctSSDCounts(log logging.Logger, sd *storageDetails) error {
  if ssds.HasVMD() {
  // If addresses are for VMD backing devices, convert to the logical VMD
  // endpoint address as this is what is expected in the server config.
- newAddrSet, err := ssds.BackingToVMDAddresses(log)
+ newAddrSet, err := ssds.BackingToVMDAddresses()
  if err != nil {
  return errors.Wrap(err, "converting backing addresses to vmd")
  }

src/control/lib/hardware/pci.go

@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2021-2022 Intel Corporation.
+// (C) Copyright 2021-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -16,8 +16,6 @@ import (
 
  "github.com/dustin/go-humanize"
  "github.com/pkg/errors"
-
- "github.com/daos-stack/daos/src/control/logging"
 )
 
 const (
@@ -141,8 +139,15 @@ func (pa *PCIAddress) LessThan(other *PCIAddress) bool {
  return false
  }
 
- return pa.VMDAddr.LessThan(other.VMDAddr) ||
- pa.Domain < other.Domain ||
+ // If VMD backing device address, return early on domain comparison if VMD domains are not
+ // equal. If equal, proceed to sort on backing device address BDF.
+ if pa.VMDAddr != nil && other.VMDAddr != nil {
+ if !pa.VMDAddr.Equals(other.VMDAddr) {
+ return pa.VMDAddr.LessThan(other.VMDAddr)
+ }
+ }
+
+ return pa.Domain < other.Domain ||
  pa.Domain == other.Domain && pa.Bus < other.Bus ||
  pa.Domain == other.Domain && pa.Bus == other.Bus && pa.Device < other.Device ||
  pa.Domain == other.Domain && pa.Bus == other.Bus && pa.Device == other.Device &&
@@ -374,7 +379,7 @@ func (pas *PCIAddressSet) HasVMD() bool {
 // e.g. [5d0505:01:00.0, 5d0505:03:00.0] -> [0000:5d:05.5].
 //
 // Many assumptions are made as to the input and output PCI address structure in the conversion.
-func (pas *PCIAddressSet) BackingToVMDAddresses(log logging.Logger) (*PCIAddressSet, error) {
+func (pas *PCIAddressSet) BackingToVMDAddresses() (*PCIAddressSet, error) {
  if pas == nil {
  return nil, errors.New("PCIAddressSet is nil")
  }
@@ -394,7 +399,6 @@ func (pas *PCIAddressSet) BackingToVMDAddresses(log logging.Logger) (*PCIAddress
  return nil, err
  }
 
- log.Debugf("replacing backing device %s with vmd %s", inAddr, vmdAddr)
  if err := outAddrs.Add(vmdAddr); err != nil {
  return nil, err
  }

src/control/lib/hardware/pci_test.go

@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2020-2022 Intel Corporation.
+// (C) Copyright 2020-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -14,7 +14,6 @@ import (
  "github.com/pkg/errors"
 
  "github.com/daos-stack/daos/src/control/common/test"
- "github.com/daos-stack/daos/src/control/logging"
 )
 
 func mockPCIBus(args ...uint8) *PCIBus {
@@ -179,6 +178,21 @@ func TestHardware_NewPCIAddressSet(t *testing.T) {
  "0000:80:00.0", "0000:81:00.0", "5d0505:01:00.0",
  },
  },
+ "multiple vmd backing device addresses": {
+ addrStrs: []string{
+ "d70505:03:00.0",
+ "d70505:01:00.0",
+ "5d0505:03:00.0",
+ "5d0505:01:00.0",
+ },
+ expAddrStr: "5d0505:01:00.0 5d0505:03:00.0 d70505:01:00.0 d70505:03:00.0",
+ expAddrStrs: []string{
+ "5d0505:01:00.0",
+ "5d0505:03:00.0",
+ "d70505:01:00.0",
+ "d70505:03:00.0",
+ },
+ },
  "vmd backing device": {
  addrStrs: []string{
  "050505:03:00.0", "050505:01:00.0",
@@ -210,6 +224,41 @@ func TestHardware_NewPCIAddressSet(t *testing.T) {
  }
 }
 
+func TestHardware_PCIAddressSet_Addresses(t *testing.T) {
+ for name, tc := range map[string]struct {
+ addrStrs []string
+ expAddrs []*PCIAddress
+ expErr error
+ }{
+ "multiple vmd backing device addresses": {
+ addrStrs: []string{
+ "d70505:03:00.0",
+ "d70505:01:00.0",
+ "5d0505:03:00.0",
+ "5d0505:01:00.0",
+ },
+ expAddrs: []*PCIAddress{
+ MustNewPCIAddress("5d0505:01:00.0"),
+ MustNewPCIAddress("5d0505:03:00.0"),
+ MustNewPCIAddress("d70505:01:00.0"),
+ MustNewPCIAddress("d70505:03:00.0"),
+ },
+ },
+ } {
+ t.Run(name, func(t *testing.T) {
+ addrSet, err := NewPCIAddressSet(tc.addrStrs...)
+ test.CmpErr(t, tc.expErr, err)
+ if tc.expErr != nil {
+ return
+ }
+
+ if diff := cmp.Diff(tc.expAddrs, addrSet.Addresses()); diff != "" {
+ t.Fatalf("unexpected list (-want, +got):\n%s\n", diff)
+ }
+ })
+ }
+}
+
 func TestHardware_PCIAddressSet_Intersect(t *testing.T) {
  for name, tc := range map[string]struct {
  addrStrs []string
@@ -305,26 +354,31 @@ func TestHardware_PCIAddressSet_BackingToVMDAddresses(t *testing.T) {
  inAddrs: []string{"5d0505:01:00.0"},
  expOutAddrs: []string{"0000:5d:05.5"},
  },
- "multiple vmd address": {
- inAddrs: []string{"5d0505:01:00.0", "5d0505:03:00.0"},
- expOutAddrs: []string{"0000:5d:05.5"},
+ "multiple vmd backing device addresses": {
+ inAddrs: []string{
+ "d70505:01:00.0",
+ "d70505:03:00.0",
+ "5d0505:01:00.0",
+ "5d0505:03:00.0",
+ },
+ expOutAddrs: []string{
+ "0000:5d:05.5",
+ "0000:d7:05.5",
+ },
  },
  "short vmd domain in address": {
  inAddrs: []string{"5d055:01:00.0"},
  expOutAddrs: []string{"0000:05:d0.55"},
  },
  } {
  t.Run(name, func(t *testing.T) {
- log, buf := logging.NewTestLogger(t.Name())
- defer test.ShowBufferOnFailure(t, buf)
-
  addrSet, gotErr := NewPCIAddressSet(tc.inAddrs...)
  test.CmpErr(t, tc.expErr, gotErr)
  if tc.expErr != nil {
  return
  }
 
- gotAddrs, gotErr := addrSet.BackingToVMDAddresses(log)
+ gotAddrs, gotErr := addrSet.BackingToVMDAddresses()
  test.CmpErr(t, tc.expErr, gotErr)
  if tc.expErr != nil {
  return

src/control/lib/spdk/spdk.go

@@ -30,7 +30,7 @@ type EnvOptions struct {
  EnableVMD bool // flag if VMD functionality should be enabled
 }
 
-func (eo *EnvOptions) sanitizeAllowList(log logging.Logger) error {
+func (eo *EnvOptions) sanitizeAllowList() error {
  if eo == nil {
  return errors.New("nil EnvOptions")
  }
@@ -39,7 +39,7 @@ func (eo *EnvOptions) sanitizeAllowList(log logging.Logger) error {
  }
 
  // DPDK will not accept VMD backing device addresses so convert to VMD addresses
- newSet, err := eo.PCIAllowList.BackingToVMDAddresses(log)
+ newSet, err := eo.PCIAllowList.BackingToVMDAddresses()
  if err != nil {
  return err
  }

src/control/lib/spdk/spdk_default.go

@@ -86,7 +86,7 @@ func (ei *EnvImpl) InitSPDKEnv(log logging.Logger, opts *EnvOptions) error {
  // Only print error and more severe to stderr.
  C.spdk_log_set_print_level(C.SPDK_LOG_ERROR)
 
- if err := opts.sanitizeAllowList(log); err != nil {
+ if err := opts.sanitizeAllowList(); err != nil {
  return errors.Wrap(err, "sanitizing PCI include list")
  }
 

src/control/server/storage/bdev.go

@@ -340,6 +340,9 @@ type NvmeController struct {
 
 // UpdateSmd adds or updates SMD device entry for an NVMe Controller.
 func (nc *NvmeController) UpdateSmd(newDev *SmdDevice) {
+ if nc == nil {
+ return
+ }
  for _, exstDev := range nc.SmdDevices {
  if newDev.UUID == exstDev.UUID {
  *exstDev = *newDev
@@ -352,6 +355,9 @@ func (nc *NvmeController) UpdateSmd(newDev *SmdDevice) {
 
 // Capacity returns the cumulative total bytes of all namespace sizes.
 func (nc *NvmeController) Capacity() (tb uint64) {
+ if nc == nil {
+ return 0
+ }
  for _, n := range nc.Namespaces {
  tb += n.Size
  }
@@ -442,6 +448,17 @@ func (ncs *NvmeControllers) Update(ctrlrs ...NvmeController) {
  }
 }
 
+// Addresses returns a hardware.PCIAddressSet pointer to controller addresses.
+func (ncs NvmeControllers) Addresses() (*hardware.PCIAddressSet, error) {
+ pas := hardware.MustNewPCIAddressSet()
+ for _, c := range ncs {
+ if err := pas.AddStrings(c.PciAddr); err != nil {
+ return nil, err
+ }
+ }
+ return pas, nil
+}
+
 // NvmeAioDevice returns struct representing an emulated NVMe AIO device (file or kdev).
 type NvmeAioDevice struct {
  Path string `json:"path"`

src/control/server/storage/bdev/backend_vmd.go

@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2021-2022 Intel Corporation.
+// (C) Copyright 2021-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -21,24 +21,17 @@ import (
  "github.com/daos-stack/daos/src/control/server/storage"
 )
 
-// getVMD returns VMD endpoint address when provided string is a VMD backing device PCI address.
-// If the input string is not a VMD backing device PCI address, hardware.ErrNotVMDBackingAddress
-// is returned.
-func getVMD(inAddr string) (*hardware.PCIAddress, error) {
- addr, err := hardware.NewPCIAddress(inAddr)
- if err != nil {
- return nil, errors.Wrap(err, "controller pci address invalid")
- }
-
- return addr.BackingToVMDAddress()
-}
-
 // mapVMDToBackingDevs stores found vmd backing device details under vmd address key.
 func mapVMDToBackingDevs(foundCtrlrs storage.NvmeControllers) (map[string]storage.NvmeControllers, error) {
  vmds := make(map[string]storage.NvmeControllers)
 
  for _, ctrlr := range foundCtrlrs {
- vmdAddr, err := getVMD(ctrlr.PciAddr)
+ addr, err := hardware.NewPCIAddress(ctrlr.PciAddr)
+ if err != nil {
+ return nil, errors.Wrap(err, "controller pci address invalid")
+ }
+
+ vmdAddr, err := addr.BackingToVMDAddress()
  if err != nil {
  if err == hardware.ErrNotVMDBackingAddress {
  continue
@@ -61,8 +54,13 @@ func mapVMDToBackingDevs(foundCtrlrs storage.NvmeControllers) (map[string]storag
 func mapVMDToBackingAddrs(foundCtrlrs storage.NvmeControllers) (map[string]*hardware.PCIAddressSet, error) {
  vmds := make(map[string]*hardware.PCIAddressSet)
 
- for _, ctrlr := range foundCtrlrs {
- vmdAddr, err := getVMD(ctrlr.PciAddr)
+ ctrlrAddrs, err := foundCtrlrs.Addresses()
+ if err != nil {
+ return nil, err
+ }
+
+ for _, addr := range ctrlrAddrs.Addresses() {
+ vmdAddr, err := addr.BackingToVMDAddress()
  if err != nil {
  if err == hardware.ErrNotVMDBackingAddress {
  continue
@@ -75,7 +73,7 @@ func mapVMDToBackingAddrs(foundCtrlrs storage.NvmeControllers) (map[string]*hard
  }
 
  // add backing device address to vmd address key in map
- if err := vmds[vmdAddr.String()].AddStrings(ctrlr.PciAddr); err != nil {
+ if err := vmds[vmdAddr.String()].Add(addr); err != nil {
  return nil, err
  }
  }
@@ -218,14 +216,13 @@ func vmdFilterAddresses(log logging.Logger, inReq *storage.BdevPrepareRequest, v
  }
 
  // Convert any VMD backing device addresses to endpoint addresses as the input vmdPCIAddrs
- // are what we are using for filters and these are VMD endpoint addresses.
- // FIXME: This imposes a limitation in that individual backing devices cannot be allowed or
- // blocked independently, see if this can be mitigated against.
- inAllowList, err = inAllowList.BackingToVMDAddresses(log)
+ // are what we are using for filters and these are VMD endpoint addresses. This imposes a
+ // limitation in that individual backing devices cannot be allowed or blocked independently.
+ inAllowList, err = inAllowList.BackingToVMDAddresses()
  if err != nil {
  return
  }
- inBlockList, err = inBlockList.BackingToVMDAddresses(log)
+ inBlockList, err = inBlockList.BackingToVMDAddresses()
  if err != nil {
  return
  }