From be7c4c8ba14539c494cc6f5b8ab5fc84d4b3238e Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Tue, 7 Apr 2026 14:12:52 +0000 Subject: [PATCH 1/8] fix: Handle JBOD/passthrough disks behind RAID controllers gracefully When a disk is visible through a RAID's sysfs tree but has no corresponding virtual/logical drive entry (e.g. JBOD or passthrough mode), GetDiskType previously returned a hard error that aborted disk scanning entirely. This broke ScanAllDisks on systems with mixed RAID and JBOD configurations. Core change: - Introduce disko.ErrNotVirtualDrive so RAID drivers can distinguish "controller query succeeded but this path is not one of my VDs/LDs" from a genuine failure. - In linux/system.go, collapse ScanDisk's and GetDiskType's RAID+udev classification into a single resolveDiskType helper. On ErrNotVirtualDrive (wrapped or not) it logs and falls through to the existing udev-based getDiskType; real controller errors still propagate. ScanDisk now sets Attachment=RAID only when the controller claimed the device. - Replace the hard-coded call to IsSysPathRAID with an injectable linuxSystem.isSysPathRAID field (defaulting to the real helper) so resolveDiskType is unit-testable without touching /sys. Driver changes (megaraid, mpi3mr, smartpqi): - cachingStorCli / storCli2 / arcConf return ErrNotVirtualDrive once the controller has been queried successfully but the path matches no VD/LD. mpi3mr and smartpqi track this with an explicit isPhysical flag and accumulate query errors in queryErrs (mpi3mr also shadows the stdlib errors import cleanly, and fixes a latent bug where a non-SSD VD would fall through instead of returning HDD). - Drop the stub IsSysPathRAID(string) bool method from the MegaRaid, Mpi3mr and SmartPqi interfaces and their implementations. All three had been returning false unconditionally; the membership check is now the responsibility of linuxSystem only, removing dead code from the driver layer. linux package reorg: - Move IsSysPathRAID and GetSysPaths out of linux/util.go into a new linux/sysfs.go (same package, no API break); util.go is trimmed accordingly. - Remove IsSysPathRAID from the RAIDController interface. Tests: - linux/system_test.go (new): mock-based coverage of resolveDiskType for RAID VD match, ErrNotVirtualDrive fallback to udev, wrapped sentinel detection, genuine controller errors, multi-controller ordering, and a non-RAID/virtio disk falling straight through to getDiskType. - linux/sysfs_test.go (new): hermetic tempdir-based tests for GetSysPaths (single/multi BDF, missing dir, broken symlinks, module symlink skipped, malformed glob pattern) and IsSysPathRAID (no-host gate, /sys auto-prepend, unresolvable path, no-match fall-through, and a positive-match case that synthesises a fake driver dir whose BDF symlink targets a real /sys device - exercising the full path without requiring root or specific hardware). - megaraid/storcli_test.go: new coverage for the ErrNotVirtualDrive return path from cachingStorCli.GetDiskType. Signed-off-by: Andrei Aaron --- disk.go | 7 + linux/raidcontroller.go | 1 - linux/sysfs.go | 62 +++++++ linux/sysfs_test.go | 69 ++++++++ linux/system.go | 71 +++++--- linux/system_test.go | 347 +++++++++++++++++++++++++++++++++++++++ linux/util.go | 55 ------- megaraid/megaraid.go | 3 - megaraid/storcli.go | 12 +- megaraid/storcli_test.go | 243 +++++++++++++++++++++++++++ mpi3mr/mpi3mr.go | 3 - mpi3mr/storcli2.go | 34 ++-- smartpqi/arcconf.go | 23 ++- smartpqi/smartpqi.go | 3 - 14 files changed, 812 insertions(+), 121 deletions(-) create mode 100644 linux/sysfs.go create mode 100644 linux/sysfs_test.go diff --git a/disk.go b/disk.go index eccad53..373b29e 100644 --- a/disk.go +++ b/disk.go @@ -2,6 +2,7 @@ package disko import ( "encoding/json" + "errors" "fmt" "sort" "strings" @@ -9,6 +10,12 @@ import ( "machinerun.io/disko/partid" ) +// ErrNotVirtualDrive is returned by RAIDController.GetDiskType when the +// device path does not match any virtual/logical drive on the controller. +// This typically indicates a JBOD/passthrough disk that the OS sees through +// the RAID HBA sysfs tree but which has no corresponding VD/LD entry. +var ErrNotVirtualDrive = errors.New("device is not a virtual/logical drive on the RAID controller") + // DiskType enumerates supported disk types. type DiskType int diff --git a/linux/raidcontroller.go b/linux/raidcontroller.go index d5af3f0..a610f98 100644 --- a/linux/raidcontroller.go +++ b/linux/raidcontroller.go @@ -13,6 +13,5 @@ const ( type RAIDController interface { // Type() RAIDControllerType GetDiskType(string) (disko.DiskType, error) - IsSysPathRAID(string) bool DriverSysfsPath() string } diff --git a/linux/sysfs.go b/linux/sysfs.go new file mode 100644 index 0000000..44be5a2 --- /dev/null +++ b/linux/sysfs.go @@ -0,0 +1,62 @@ +package linux + +import ( + "fmt" + "path/filepath" + "strings" +) + +// IsSysPathRAID checks whether syspath (udevadm DEVPATH) belongs to a RAID +// controller whose PCI driver is registered at driverSysPath. +// +// syspath will look something like +// /devices/pci0000:3a/0000:3a:02.0/0000:3c:00.0/host0/target0:2:2/0:2:2:0/block/sdc +func IsSysPathRAID(syspath string, driverSysPath string) bool { + if !strings.HasPrefix(syspath, "/sys") { + syspath = "/sys" + syspath + } + + if !strings.Contains(syspath, "/host") { + return false + } + + fp, err := filepath.EvalSymlinks(syspath) + if err != nil { + fmt.Printf("seriously? %s\n", err) + return false + } + + for _, path := range GetSysPaths(driverSysPath) { + if strings.HasPrefix(fp, path) { + return true + } + } + + return false +} + +// GetSysPaths returns the resolved PCI device paths for a RAID driver. +func GetSysPaths(driverSysPath string) []string { + paths := []string{} + // a raid driver has directory entries for each of the scsi hosts on that controller. + // $cd /sys/bus/pci/drivers/ + // $ for d in *; do [ -d "$d" ] || continue; echo "$d -> $( cd "$d" && pwd -P )"; done + // 0000:3c:00.0 -> /sys/devices/pci0000:3a/0000:3a:02.0/0000:3c:00.0 + // module -> /sys/module/ + + // We take a hack path and consider anything with a ":" in that dir as a host path. + matches, err := filepath.Glob(driverSysPath + "/*:*") + + if err != nil { + fmt.Printf("errors: %s\n", err) + return paths + } + + for _, p := range matches { + if fp, err := filepath.EvalSymlinks(p); err == nil { + paths = append(paths, fp) + } + } + + return paths +} diff --git a/linux/sysfs_test.go b/linux/sysfs_test.go new file mode 100644 index 0000000..8af8793 --- /dev/null +++ b/linux/sysfs_test.go @@ -0,0 +1,69 @@ +package linux + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// buildFakeDriverDir creates a tempdir layout mirroring /sys/bus/pci when a +// RAID PCI driver is loaded: +// +// /drivers// -> /devices/pci0000/ +// /drivers//module -> /module/ +// +// It returns the driver directory and the resolved device paths that +// GetSysPaths should return for it. +func buildFakeDriverDir(t *testing.T, driver string, bdfs []string) (string, []string) { + t.Helper() + root := t.TempDir() + + driverDir := filepath.Join(root, "drivers", driver) + require.NoError(t, os.MkdirAll(driverDir, 0o755)) + + // Real driver dirs also contain a "module" symlink; GetSysPaths must + // skip it because its name has no ':' (the "*:*" glob excludes it). + modulePath := filepath.Join(root, "module", driver) + require.NoError(t, os.MkdirAll(modulePath, 0o755)) + require.NoError(t, os.Symlink(modulePath, filepath.Join(driverDir, "module"))) + + resolved := make([]string, 0, len(bdfs)) + for _, bdf := range bdfs { + dev := filepath.Join(root, "devices", "pci0000", bdf) + require.NoError(t, os.MkdirAll(dev, 0o755)) + require.NoError(t, os.Symlink(dev, filepath.Join(driverDir, bdf))) + + resolvedDev, err := filepath.EvalSymlinks(dev) + require.NoError(t, err) + resolved = append(resolved, resolvedDev) + } + + return driverDir, resolved +} + +func TestGetSysPaths_EmptyDriverDir(t *testing.T) { + driverDir, _ := buildFakeDriverDir(t, "megaraid_sas", nil) + assert.Empty(t, GetSysPaths(driverDir)) +} + +func TestGetSysPaths_SingleBoundDevice(t *testing.T) { + driverDir, resolved := buildFakeDriverDir(t, "megaraid_sas", + []string{"0000:3c:00.0"}) + + assert.ElementsMatch(t, resolved, GetSysPaths(driverDir)) +} + +func TestGetSysPaths_MultipleBoundDevices(t *testing.T) { + bdfs := []string{"0000:3c:00.0", "0000:82:00.0"} + driverDir, resolved := buildFakeDriverDir(t, "mpi3mr", bdfs) + + assert.ElementsMatch(t, resolved, GetSysPaths(driverDir)) +} + +func TestIsSysPathRAID_NoHostSegment(t *testing.T) { + assert.False(t, IsSysPathRAID("/sys/devices/virtual/block/loop0", "")) + assert.False(t, IsSysPathRAID("/devices/virtual/block/loop0", "")) +} diff --git a/linux/system.go b/linux/system.go index b972b6e..c3e18a3 100644 --- a/linux/system.go +++ b/linux/system.go @@ -1,6 +1,7 @@ package linux import ( + "errors" "fmt" "log" "os" @@ -18,6 +19,10 @@ import ( type linuxSystem struct { raidctrls []RAIDController + // isSysPathRAID resolves whether a udev DEVPATH belongs to the driver + // rooted at driverSysPath. It defaults to sysfs.IsSysPathRAID in + // production; tests inject a stub to avoid touching real /sys. + isSysPathRAID func(syspath, driverSysPath string) bool } // System returns an linux specific implementation of disko.System interface. @@ -28,6 +33,7 @@ func System() disko.System { smartpqi.ArcConf(), mpi3mr.StorCli2(), }, + isSysPathRAID: IsSysPathRAID, } } @@ -161,28 +167,14 @@ func (ls *linuxSystem) ScanDisk(devicePath string) (disko.Disk, error) { attachType = getAttachType(udInfo) - for _, ctrl := range ls.raidctrls { - if IsSysPathRAID(udInfo.Properties["DEVPATH"], ctrl.DriverSysfsPath()) { - // we know this is device is part of a raid, so if we cannot get - // disk type we must return an error - dType, err := ctrl.GetDiskType(devicePath) - if err != nil { - return disko.Disk{}, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", devicePath, err) - } - - attachType = disko.RAID - diskType = dType - - break - } + var onRAID bool + diskType, onRAID, err = ls.resolveDiskType(devicePath, udInfo) + if err != nil { + return disko.Disk{}, fmt.Errorf("error while getting disk type: %s", err) } - // check disk type if it wasn't on raid - if attachType != disko.RAID { - diskType, err = getDiskType(udInfo) - if err != nil { - return disko.Disk{}, fmt.Errorf("error while getting disk type: %s", err) - } + if onRAID { + attachType = disko.RAID } ro, err = getDiskReadOnly(name) @@ -298,15 +290,42 @@ func (ls *linuxSystem) Wipe(d disko.Disk) error { } func (ls *linuxSystem) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { + dType, _, err := ls.resolveDiskType(path, udInfo) + return dType, err +} + +// resolveDiskType classifies devicePath as a DiskType. If the device sits +// behind a known RAID controller and corresponds to a virtual/logical drive, +// the controller's classification is used and the returned bool is true. For +// devices that are either not on a RAID controller or are visible through a +// RAID HBA in JBOD/passthrough mode (ErrNotVirtualDrive), the function falls +// back to generic detection via getDiskType(udInfo) with the bool set to +// false. +func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) (disko.DiskType, bool, error) { + devpath := udInfo.Properties["DEVPATH"] + for _, ctrl := range ls.raidctrls { - if IsSysPathRAID(udInfo.Properties["DEVPATH"], ctrl.DriverSysfsPath()) { - dType, err := ctrl.GetDiskType(path) - if err != nil { - return disko.HDD, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", path, err) + if !ls.isSysPathRAID(devpath, ctrl.DriverSysfsPath()) { + continue + } + + dType, err := ctrl.GetDiskType(devicePath) + if err != nil { + if errors.Is(err, disko.ErrNotVirtualDrive) { + log.Printf("device %q on RAID sysfs path has no virtual drive (JBOD?), using generic detection", devicePath) + break } - return dType, nil + return disko.HDD, false, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", devicePath, err) } + + return dType, true, nil } - return getDiskType(udInfo) + + dType, err := getDiskType(udInfo) + if err != nil { + return disko.HDD, false, err + } + + return dType, false, nil } diff --git a/linux/system_test.go b/linux/system_test.go index 8750226..29e6c63 100644 --- a/linux/system_test.go +++ b/linux/system_test.go @@ -1,6 +1,7 @@ package linux import ( + "errors" "fmt" "os" "path" @@ -235,3 +236,349 @@ func TestCreatePartitions(t *testing.T) { ast.Equal(disko.FILESYSTEM, scannedDisk.Attachment) ast.Equal(disko.TYPEFILE, scannedDisk.Type) } + +type mockRAIDController struct { + diskType disko.DiskType + err error + sysfsPath string + isSysPathRAID bool + getDiskTypeCalled bool + getDiskTypePath string +} + +func (m *mockRAIDController) GetDiskType(path string) (disko.DiskType, error) { + m.getDiskTypeCalled = true + m.getDiskTypePath = path + return m.diskType, m.err +} + +func (m *mockRAIDController) DriverSysfsPath() string { + return m.sysfsPath +} + +// newTestLinuxSystem builds a linuxSystem whose sysfs lookup is stubbed by +// consulting the isSysPathRAID field on the matching mock (matched by +// DriverSysfsPath), so tests don't need a real /sys mount. +func newTestLinuxSystem(ctrls ...*mockRAIDController) *linuxSystem { + rc := make([]RAIDController, 0, len(ctrls)) + for _, c := range ctrls { + rc = append(rc, c) + } + + return &linuxSystem{ + raidctrls: rc, + isSysPathRAID: func(_, driverSysPath string) bool { + for _, c := range ctrls { + if c.sysfsPath == driverSysPath { + return c.isSysPathRAID + } + } + return false + }, + } +} + +func TestGetDiskTypeRAIDMatchHDD(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.HDD, + err: nil, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + dtype, err := ls.GetDiskType("/dev/sda", udInfo) + ast.NoError(err) + ast.Equal(disko.HDD, dtype) + ast.True(mock.getDiskTypeCalled) + ast.Equal("/dev/sda", mock.getDiskTypePath) +} + +func TestGetDiskTypeRAIDMatchSSD(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.SSD, + err: nil, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + dtype, err := ls.GetDiskType("/dev/sda", udInfo) + ast.NoError(err) + ast.Equal(disko.SSD, dtype) + ast.True(mock.getDiskTypeCalled) +} + +func TestGetDiskTypeJBODFallback(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.HDD, + err: disko.ErrNotVirtualDrive, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{ + Name: "sda", + Properties: map[string]string{ + "DEVPATH": "/devices/pci/host0/block/sda", + "ID_BUS": "scsi", + "DEVTYPE": "disk", + }, + } + + dtype, err := ls.GetDiskType("/dev/sda", udInfo) + + ast.True(mock.getDiskTypeCalled, "RAID controller should have been consulted") + ast.NoError(err, "ErrNotVirtualDrive should not propagate as a fatal error") + ast.Equal(disko.HDD, dtype, "should fall through to generic detection (HDD default)") +} + +func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { + ast := assert.New(t) + wrappedErr := fmt.Errorf("controller 0: %w", disko.ErrNotVirtualDrive) + + mock := &mockRAIDController{ + diskType: disko.HDD, + err: wrappedErr, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + _, err := ls.GetDiskType("/dev/sda", udInfo) + ast.NoError(err, "wrapped ErrNotVirtualDrive should still trigger fallback via errors.Is") +} + +func TestGetDiskTypeRAIDRealError(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.HDD, + err: fmt.Errorf("storcli binary crashed"), + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + _, err := ls.GetDiskType("/dev/sda", udInfo) + ast.Error(err) + ast.Contains(err.Error(), "failed to get diskType") + ast.Contains(err.Error(), "storcli binary crashed") + ast.False(errors.Is(err, disko.ErrNotVirtualDrive)) +} + +func TestGetDiskTypeNoRAIDMatch(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.HDD, + err: nil, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: false, + } + + ls := newTestLinuxSystem(mock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + _, err := ls.GetDiskType("/dev/sda", udInfo) + ast.False(mock.getDiskTypeCalled, "should not call GetDiskType when sysfs path doesn't match") + _ = err +} + +func TestGetDiskTypeMultiControllerJBODFallback(t *testing.T) { + ast := assert.New(t) + + megaraidMock := &mockRAIDController{ + diskType: disko.HDD, + err: disko.ErrNotVirtualDrive, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + smartpqiMock := &mockRAIDController{ + diskType: disko.HDD, + err: nil, + sysfsPath: "/sys/bus/pci/drivers/smartpqi", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(megaraidMock, smartpqiMock) + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + + _, err := ls.GetDiskType("/dev/sda", udInfo) + ast.NoError(err) + ast.True(megaraidMock.getDiskTypeCalled, "megaraid should have been tried") + ast.False(smartpqiMock.getDiskTypeCalled, "smartpqi should NOT be tried after break from megaraid ErrNotVirtualDrive") +} + +// udevInfoFallbackStub returns a UdevInfo that drives getDiskType's +// nvme-prefix short-circuit, so fallback tests get a deterministic +// (disko.NVME, nil) result without touching /dev or sysfs. The observed +// disk type is a fingerprint that the fallback ran; it is not itself under +// test. +func udevInfoFallbackStub(devpath string) disko.UdevInfo { + return disko.UdevInfo{ + Name: "nvme0n1", + Properties: map[string]string{ + "DEVPATH": devpath, + "DEVTYPE": "disk", + }, + } +} + +func TestResolveDiskType_RAIDMatchSSD(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.SSD, + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/sda", + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + ast.NoError(err) + ast.True(onRAID) + ast.Equal(disko.SSD, dType) + ast.True(mock.getDiskTypeCalled) + ast.Equal("/dev/sda", mock.getDiskTypePath) +} + +func TestResolveDiskType_RAIDMatchHDD(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.HDD, + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/sda", + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + ast.NoError(err) + ast.True(onRAID) + ast.Equal(disko.HDD, dType) +} + +// An ErrNotVirtualDrive from the controller must not propagate; resolveDiskType +// falls through to the udev classifier. +func TestResolveDiskType_JBODFallsThroughToUdev(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + err: disko.ErrNotVirtualDrive, + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", + udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + ast.NoError(err) + ast.False(onRAID) + ast.True(mock.getDiskTypeCalled) + ast.Equal(disko.NVME, dType) +} + +// Same contract as the previous test, but the sentinel is wrapped via +// fmt.Errorf("...: %w", ...); errors.Is must still trigger the fallback. +func TestResolveDiskType_WrappedJBODFallsThroughToUdev(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + err: fmt.Errorf("controller 0: %w", disko.ErrNotVirtualDrive), + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", + udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + ast.NoError(err) + ast.False(onRAID) + ast.Equal(disko.NVME, dType) +} + +// Non-sentinel controller errors must surface to the caller, not be swallowed +// into the udev fallback. +func TestResolveDiskType_RealErrorIsPropagated(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + err: fmt.Errorf("storcli binary crashed"), + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + _, onRAID, err := ls.resolveDiskType("/dev/sda", + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + ast.Error(err) + ast.False(onRAID) + ast.Contains(err.Error(), "storcli binary crashed") +} + +// When no controller claims the devpath via IsSysPathRAID, resolveDiskType +// skips the RAID path entirely and classifies the disk via udev. +func TestResolveDiskType_NoRAIDMatchFallsThroughToUdev(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + isSysPathRAID: false, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", + udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + ast.NoError(err) + ast.False(onRAID) + ast.False(mock.getDiskTypeCalled) + ast.Equal(disko.NVME, dType) +} + +// Once a matching controller reports ErrNotVirtualDrive, iteration stops (a +// JBOD on one HBA cannot simultaneously be a VD on another) and we fall +// through to the udev classifier exactly once. +func TestResolveDiskType_MultiControllerJBODStopsIteration(t *testing.T) { + ast := assert.New(t) + + first := &mockRAIDController{ + err: disko.ErrNotVirtualDrive, + isSysPathRAID: true, + } + second := &mockRAIDController{ + diskType: disko.SSD, + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(first, second) + + dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", + udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + ast.NoError(err) + ast.False(onRAID) + ast.True(first.getDiskTypeCalled) + ast.False(second.getDiskTypeCalled, "second controller should not be consulted after JBOD break") + ast.Equal(disko.NVME, dType) +} + +// A system with no RAID controllers configured still classifies disks via +// udev and never reports onRAID=true. +func TestResolveDiskType_NoControllersConfiguredUsesUdev(t *testing.T) { + ast := assert.New(t) + + ls := newTestLinuxSystem() + + dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", + udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + ast.NoError(err) + ast.False(onRAID) + ast.Equal(disko.NVME, dType) +} diff --git a/linux/util.go b/linux/util.go index 45c914d..d6f4ffe 100644 --- a/linux/util.go +++ b/linux/util.go @@ -291,34 +291,6 @@ func Floor(val, unit uint64) uint64 { return (val / unit) * unit } -// IsSysPathRAID - is this sys path (udevadm info's DEVPATH) on a scsi controller. -// -// syspath will look something like -// /devices/pci0000:3a/0000:3a:02.0/0000:3c:00.0/host0/target0:2:2/0:2:2:0/block/sdc -func IsSysPathRAID(syspath string, driverSysPath string) bool { - if !strings.HasPrefix(syspath, "/sys") { - syspath = "/sys" + syspath - } - - if !strings.Contains(syspath, "/host") { - return false - } - - fp, err := filepath.EvalSymlinks(syspath) - if err != nil { - fmt.Printf("seriously? %s\n", err) - return false - } - - for _, path := range GetSysPaths(driverSysPath) { - if strings.HasPrefix(fp, path) { - return true - } - } - - return false -} - // NameByDiskID - return the linux name (sda) for the disk with given DiskID func NameByDiskID(driverSysPath string, id int) (string, error) { // given ID, we expect a single file in: @@ -339,30 +311,3 @@ func NameByDiskID(driverSysPath string, id int) (string, error) { return path.Base(matches[0]), nil } - -func GetSysPaths(driverSysPath string) []string { - paths := []string{} - // a raid driver has directory entries for each of the scsi hosts on that controller. - // $cd /sys/bus/pci/drivers/ - // $ for d in *; do [ -d "$d" ] || continue; echo "$d -> $( cd "$d" && pwd -P )"; done - // 0000:3c:00.0 -> /sys/devices/pci0000:3a/0000:3a:02.0/0000:3c:00.0 - // module -> /sys/module/ - - // We take a hack path and consider anything with a ":" in that dir as a host path. - matches, err := filepath.Glob(driverSysPath + "/*:*") - - if err != nil { - fmt.Printf("errors: %s\n", err) - return paths - } - - for _, p := range matches { - fp, err := filepath.EvalSymlinks(p) - - if err == nil { - paths = append(paths, fp) - } - } - - return paths -} diff --git a/megaraid/megaraid.go b/megaraid/megaraid.go index e0d0d2f..c35c8e1 100644 --- a/megaraid/megaraid.go +++ b/megaraid/megaraid.go @@ -139,9 +139,6 @@ type MegaRaid interface { // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string - - // IsSysPathRAID - Check if sysfs path is a device on the controller - IsSysPathRAID(string) bool } // ErrNoController - Error reported by Query if no controller is found. diff --git a/megaraid/storcli.go b/megaraid/storcli.go index bc55232..d2510db 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -83,11 +83,6 @@ func (sc *storCli) GetDiskType(path string) (disko.DiskType, error) { return disko.HDD, fmt.Errorf("missing controller to run query") } -// not implemented in driver layer -func (sc *storCli) IsSysPathRAID(syspath string) bool { - return false -} - func newController(cID int, cxDxOut string, cxVxOut string) (Controller, error) { const pathPropName = "OS Drive Name" @@ -634,6 +629,8 @@ func (csc *cachingStorCli) GetDiskType(path string) (disko.DiskType, error) { return disko.HDD, nil } } + + return disko.HDD, disko.ErrNotVirtualDrive } else if err != ErrNoStorcli && err != ErrNoController && err != ErrUnsupported { return disko.HDD, err } @@ -644,8 +641,3 @@ func (csc *cachingStorCli) GetDiskType(path string) (disko.DiskType, error) { func (csc *cachingStorCli) DriverSysfsPath() string { return csc.mr.DriverSysfsPath() } - -// not implemented in the driver layer -func (csc *cachingStorCli) IsSysPathRAID(syspath string) bool { - return false -} diff --git a/megaraid/storcli_test.go b/megaraid/storcli_test.go index 518e5e1..3b58162 100644 --- a/megaraid/storcli_test.go +++ b/megaraid/storcli_test.go @@ -2,9 +2,14 @@ package megaraid import ( "encoding/json" + "errors" "reflect" "strings" "testing" + "time" + + "github.com/patrickmn/go-cache" + "machinerun.io/disko" ) var tableData1 = ` @@ -1163,6 +1168,119 @@ SCSI NAA Id = 6cc167e9730322c027dd6c90047c42a1 Unmap Enabled = No ` +// Cisco 12G Modular Raid Controller with all JBOD drives and no virtual drives. +// Captured from a real system running megaraid_sas with +// FW 5.190.00-3697. All five disks are in JBOD mode (1 SSD + 4 HDD). +var ciscoJBODCxShow = ` +Generating detailed summary of the adapter, it may take a while to complete. + +CLI Version = 007.1907.0000.0000 Sep 13, 2021 +Operating system = Linux 6.6.0 +Controller = 0 +Status = Success +Description = None + +Product Name = Cisco 12G Modular Raid Controller with 2GB cache (max 16 drives) +Serial Number = SKXXXXXXXX +SAS Address = 0000000000000000 +PCI Address = 00:3c:00:00 +System Time = 04/18/2026 07:25:04 +Mfg. Date = 07/23/22 +Controller Time = 04/18/2026 07:24:48 +FW Package Build = 51.19.0-4532 +BIOS Version = 7.19.00.0_0x07130200 +FW Version = 5.190.00-3697 +Driver Name = megaraid_sas +Driver Version = 07.725.01.00-rc1 +Current Personality = RAID-Mode +Vendor Id = 0x1000 +Device Id = 0x14 +SubVendor Id = 0x1137 +SubDevice Id = 0x20E +Host Interface = PCI-E +Device Interface = SAS-12G +Bus Number = 60 +Device Number = 0 +Function Number = 0 +Domain ID = 0 +Security Protocol = None +JBOD Drives = 5 + +JBOD LIST : +========= + +------------------------------------------------------------------------------ +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +------------------------------------------------------------------------------ +134:2 1 JBOD - 745.211 GB SAS SSD N N 512B KPM6XVUG800G U - +134:3 0 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:4 3 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:5 2 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:6 4 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +------------------------------------------------------------------------------ + +ID=JBOD Target ID|EID=Enclosure Device ID|Slt=Slot No|DID=Device ID|Onln=Online +Offln=Offline|Intf=Interface|Med=Media Type|SeSz=Sector Size +SED=Self Encryptive Drive|PI=Protection Info|Sp=Spun|U=Up|D=Down + +Physical Drives = 5 + +PD LIST : +======= + +------------------------------------------------------------------------------ +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +------------------------------------------------------------------------------ +134:2 1 JBOD - 745.211 GB SAS SSD N N 512B KPM6XVUG800G U - +134:3 0 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:4 3 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:5 2 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +134:6 4 JBOD - 2.182 TB SAS HDD N N 4 KB AL15SEB24EP U - +------------------------------------------------------------------------------ + +EID=Enclosure Device ID|Slt=Slot No|DID=Device ID|DG=DriveGroup +DHS=Dedicated Hot Spare|UGood=Unconfigured Good|GHS=Global Hotspare +UBad=Unconfigured Bad|Sntze=Sanitize|Onln=Online|Offln=Offline|Intf=Interface +Med=Media Type|SED=Self Encryptive Drive|PI=Protection Info +SeSz=Sector Size|Sp=Spun|U=Up|D=Down|T=Transition|F=Foreign +UGUnsp=UGood Unsupported|UGShld=UGood shielded|HSPShld=Hotspare shielded +CFShld=Configured shielded|Cpybck=CopyBack|CBShld=Copyback Shielded +UBUnsp=UBad Unsupported|Rbld=Rebuild + +Enclosures = 1 + +Enclosure LIST : +============== + +------------------------------------------------------------------------ +EID State Slots PD PS Fans TSs Alms SIM Port# ProdID VendorSpecific +------------------------------------------------------------------------ +134 OK 16 5 0 0 0 0 0 - VirtualSES +------------------------------------------------------------------------ + +EID=Enclosure Device ID | PD=Physical drive count | PS=Power Supply count +TSs=Temperature sensor count | Alms=Alarm count | SIM=SIM Count | ProdID=Product ID + + +Cachevault_Info : +=============== + +------------------------------------ +Model State Temp Mode MfgDate +------------------------------------ +CVPM05 Optimal 25C - 2022/05/21 +------------------------------------ + +` + +var ciscoJBODCxVallShowAll = ` +CLI Version = 007.1907.0000.0000 Sep 13, 2021 +Operating system = Linux 6.6.0 +Controller = 0 +Status = Success +Description = No VD's have been configured. +` + // this has a 'F' for a DriveGroup (foreign) // it is put together from old '/c0/dall show' output to // look like '/c0 show all' would. @@ -1289,3 +1407,128 @@ Model State Temp Mode MfgDate CVPM05 Optimal 34C - 2018/10/16 ------------------------------------ ` + +func TestParseCxShowCiscoJBOD(t *testing.T) { + vds, pds, err := parseCxShow(ciscoJBODCxShow) + if err != nil { + t.Fatalf("parseCxShow(ciscoJBODCxShow) returned error: %s", err) + } + + if len(vds) != 0 { + t.Errorf("expected 0 virtual drives, got %d", len(vds)) + } + + if len(pds) != 5 { + t.Errorf("expected 5 physical drives, got %d", len(pds)) + } + + for _, pd := range pds { + if pd.State != "JBOD" { + t.Errorf("drive %d: expected State=JBOD, got %q", pd.ID, pd.State) + } + + if pd.DriveGroup != -1 { + t.Errorf("drive %d: expected DriveGroup=-1 (JBOD), got %d", pd.ID, pd.DriveGroup) + } + } + + ssd := pds[1] + if ssd.MediaType != SSD { + t.Errorf("drive 1 (slot 2): expected SSD, got %s", ssd.MediaType) + } + + hdd := pds[0] + if hdd.MediaType != HDD { + t.Errorf("drive 0 (slot 3): expected HDD, got %s", hdd.MediaType) + } +} + +func TestParseVirtPropertiesCiscoJBOD(t *testing.T) { + propMap, err := parseVirtProperties(ciscoJBODCxVallShowAll) + if err != nil { + t.Fatalf("parseVirtProperties(ciscoJBODCxVallShowAll) returned error: %s", err) + } + + if len(propMap) != 0 { + t.Errorf("expected 0 VD properties, got %d", len(propMap)) + } +} + +func TestNewControllerCiscoJBOD(t *testing.T) { + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + if err != nil { + t.Fatalf("newController failed: %s", err) + } + + if ctrl.ID != 0 { + t.Errorf("expected controller ID 0, got %d", ctrl.ID) + } + + if len(ctrl.VirtDrives) != 0 { + t.Errorf("expected 0 VirtDrives, got %d", len(ctrl.VirtDrives)) + } + + if len(ctrl.Drives) != 5 { + t.Errorf("expected 5 Drives, got %d", len(ctrl.Drives)) + } + + if len(ctrl.DriveGroups) != 0 { + t.Errorf("expected 0 DriveGroups (JBOD has no drive groups), got %d", len(ctrl.DriveGroups)) + } +} + +type mockMegaRaid struct { + ctrl Controller + err error +} + +func (m *mockMegaRaid) Query(cID int) (Controller, error) { + return m.ctrl, m.err +} + +func (m *mockMegaRaid) GetDiskType(path string) (disko.DiskType, error) { + return disko.HDD, nil +} + +func (m *mockMegaRaid) DriverSysfsPath() string { + return "/sys/bus/pci/drivers/megaraid_sas" +} + +func TestGetDiskTypeJBODReturnsErrNotVirtualDrive(t *testing.T) { + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + if err != nil { + t.Fatalf("newController failed: %s", err) + } + + csc := &cachingStorCli{ + mr: &mockMegaRaid{ctrl: ctrl, err: nil}, + cache: cache.New(5*time.Minute, 5*time.Minute), + } + + for _, path := range []string{"/dev/sda", "/dev/sdb", "/dev/sdc"} { + dtype, err := csc.GetDiskType(path) + if !errors.Is(err, disko.ErrNotVirtualDrive) { + t.Errorf("GetDiskType(%q): expected ErrNotVirtualDrive, got %v", path, err) + } + + if dtype != disko.HDD { + t.Errorf("GetDiskType(%q): expected HDD default, got %d", path, dtype) + } + } +} + +func TestGetDiskTypeQueryFailureNotSentinel(t *testing.T) { + csc := &cachingStorCli{ + mr: &mockMegaRaid{ctrl: Controller{}, err: ErrNoStorcli}, + cache: cache.New(5*time.Minute, 5*time.Minute), + } + + _, err := csc.GetDiskType("/dev/sda") + if errors.Is(err, disko.ErrNotVirtualDrive) { + t.Error("GetDiskType should NOT return ErrNotVirtualDrive when storcli is missing") + } + + if err == nil { + t.Error("GetDiskType should return an error when storcli is missing") + } +} diff --git a/mpi3mr/mpi3mr.go b/mpi3mr/mpi3mr.go index 48c9144..3e3aae8 100644 --- a/mpi3mr/mpi3mr.go +++ b/mpi3mr/mpi3mr.go @@ -72,9 +72,6 @@ type Mpi3mr interface { // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string - - // IsSysPathRAID - Check if sysfs path is a device on the controller - IsSysPathRAID(string) bool } // ErrNoController - Error reported by Query if no controller is found. diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index 9607a6b..5dbb613 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -602,32 +602,42 @@ func (sc *storCli2) GetDiskType(path string) (disko.DiskType, error) { return disko.HDD, errors.Errorf("failed to get controller list: %s", err) } - errors := []error{} + // isPhysical becomes true once we have successfully inspected at least one + // controller. If we then finish the loop without matching path against any + // VirtualDrive, we know the device is visible through the HBA but is not a + // configured virtual drive -- i.e. it is a physical/passthrough (JBOD) disk. + isPhysical := false + queryErrs := []error{} + for _, cID := range cIDs { ctrl, err := sc.Query(cID) if err != nil { - errors = append(errors, fmt.Errorf("error while getting config for controller id:%d %s", cID, err)) + queryErrs = append(queryErrs, fmt.Errorf("error while getting config for controller id:%d %s", cID, err)) continue } + + isPhysical = true + for _, vDev := range ctrl.VirtualDrives { - if vDev.Path() == path && vDev.IsSSD() { - return disko.SSD, nil - } + if vDev.Path() == path { + if vDev.IsSSD() { + return disko.SSD, nil + } - return disko.HDD, nil + return disko.HDD, nil + } } } - for _, err := range errors { + for _, err := range queryErrs { if err != ErrNoStor2cli && err != ErrNoController && err != ErrUnsupported { return disko.HDD, err } } - return disko.HDD, fmt.Errorf("cannot determine diskt type for path %q", path) -} + if isPhysical { + return disko.HDD, disko.ErrNotVirtualDrive + } -// not implemented in driver layer -func (sc *storCli2) IsSysPathRAID(syspath string) bool { - return false + return disko.HDD, fmt.Errorf("cannot determine disk type for path %q", path) } diff --git a/smartpqi/arcconf.go b/smartpqi/arcconf.go index 92a2b24..52ede77 100644 --- a/smartpqi/arcconf.go +++ b/smartpqi/arcconf.go @@ -495,14 +495,22 @@ func (ac *arcConf) GetDiskType(path string) (disko.DiskType, error) { return disko.HDD, fmt.Errorf("failed to enumerate controllers: %s", err) } - errors := []error{} + // isPhysical becomes true once we have successfully inspected at least one + // controller. If we then finish the loop without matching path against any + // LogicalDrive, we know the device is visible through the HBA but is not a + // configured logical drive -- i.e. it is a physical/passthrough (JBOD) disk. + isPhysical := false + queryErrs := []error{} + for _, cID := range cIDs { ctrl, err := ac.GetConfig(cID) if err != nil { - errors = append(errors, fmt.Errorf("error while getting config for controller id:%d: %s", cID, err)) + queryErrs = append(queryErrs, fmt.Errorf("error while getting config for controller id:%d: %s", cID, err)) continue } + isPhysical = true + for _, lDrive := range ctrl.LogicalDrives { if lDrive.DiskName == path { if lDrive.IsSSD() { @@ -514,12 +522,16 @@ func (ac *arcConf) GetDiskType(path string) (disko.DiskType, error) { } } - for _, err := range errors { + for _, err := range queryErrs { if err != ErrNoArcconf && err != ErrNoController && err != ErrUnsupported { return disko.HDD, err } } + if isPhysical { + return disko.HDD, disko.ErrNotVirtualDrive + } + return disko.HDD, fmt.Errorf("cannot determine disk type") } @@ -527,11 +539,6 @@ func (ac *arcConf) DriverSysfsPath() string { return SysfsPCIDriversPath } -// not implemented at the driver level -func (ac *arcConf) IsSysPathRAID(path string) bool { - return false -} - func (ac *arcConf) GetConfig(cID int) (Controller, error) { var stdout, stderr []byte var rc int diff --git a/smartpqi/smartpqi.go b/smartpqi/smartpqi.go index a4b0b08..5b44c6f 100644 --- a/smartpqi/smartpqi.go +++ b/smartpqi/smartpqi.go @@ -124,9 +124,6 @@ type SmartPqi interface { // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string - - // IsSysPathRAID - Check if sysfs path is a device on the controller - IsSysPathRAID(string) bool } // ErrNoController - Error reported by Query if no controller is found. From d342650a7dbff85130dd9e0e34998a422565475c Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Fri, 24 Apr 2026 11:30:32 +0000 Subject: [PATCH 2/8] feat: Classify JBOD disks using per-controller identity correlation When a disk is owned by a RAID HBA but isn't part of a VD/LD (JBOD or passthrough), we currently return ErrNotVirtualDrive and let scanning fall through to generic udev detection - losing the controller's authoritative media-type information. Now that the controller query has already succeeded, correlate each JBOD with its matching controller record and use that record's medium, while still falling back when no unambiguous match exists. Interface change: RAIDController.GetDiskType gains a disko.UdevInfo parameter (breaking change, propagated to the MegaRaid, Mpi3mr and SmartPqi driver interfaces and their in-tree implementations and mocks). linux/system.go's resolveDiskType simply threads udInfo through. Per-driver correlation: - megaraid: parse the SCSI target T from /sys/block//device and match it against Drive.DID; classify via Drive.MediaType. - mpi3mr: same T, matched against PhysicalDrive.PID across all inspected controllers; classify via PhysicalDrive.Medium. - smartpqi: SCSI T doesn't match controller IDs here, so match udev ID_SERIAL_SHORT (with ID_SERIAL as fallback) against PhysicalDevice.SerialNumber; classify via PhysicalDevice.Type. Both SCSI-based drivers inject the helper via sysRoot + scsiTargetFn fields so tests can stub sysfs without touching /sys. Unit tests cover match, no-match, ambiguous-match, unresolved-target and missing-name cases per driver. Signed-off-by: Andrei Aaron --- disk.go | 12 +-- linux/raidcontroller.go | 2 +- linux/system.go | 16 ++-- linux/system_test.go | 28 +++--- megaraid/megaraid.go | 2 +- megaraid/scsi.go | 47 ++++++++++ megaraid/scsi_test.go | 113 +++++++++++++++++++++++ megaraid/storcli.go | 97 ++++++++++++++++---- megaraid/storcli_test.go | 149 ++++++++++++++++++++++++++++--- mpi3mr/mpi3mr.go | 2 +- mpi3mr/storcli2.go | 100 ++++++++++++++++----- mpi3mr/storcli2_test.go | 150 +++++++++++++++++++++++++++++++ smartpqi/arcconf.go | 108 ++++++++++++++++++---- smartpqi/arcconf_test.go | 187 +++++++++++++++++++++++++++++++++++++++ smartpqi/smartpqi.go | 2 +- 15 files changed, 919 insertions(+), 96 deletions(-) create mode 100644 megaraid/scsi.go create mode 100644 megaraid/scsi_test.go diff --git a/disk.go b/disk.go index 373b29e..46ba545 100644 --- a/disk.go +++ b/disk.go @@ -10,11 +10,13 @@ import ( "machinerun.io/disko/partid" ) -// ErrNotVirtualDrive is returned by RAIDController.GetDiskType when the -// device path does not match any virtual/logical drive on the controller. -// This typically indicates a JBOD/passthrough disk that the OS sees through -// the RAID HBA sysfs tree but which has no corresponding VD/LD entry. -var ErrNotVirtualDrive = errors.New("device is not a virtual/logical drive on the RAID controller") +// ErrDiskTypeUndetermined is returned by RAIDController.GetDiskType when +// the controller layer cannot determine the disk type. Typical cases: +// the device is on the controller's sysfs tree but is not a configured +// virtual/logical drive and cannot be matched as a JBOD/passthrough +// disk, or controller queries were inconclusive (e.g. controller tool +// unavailable). Callers should fall back to generic udev-based detection. +var ErrDiskTypeUndetermined = errors.New("RAID controller could not determine disk type") // DiskType enumerates supported disk types. type DiskType int diff --git a/linux/raidcontroller.go b/linux/raidcontroller.go index a610f98..675e6db 100644 --- a/linux/raidcontroller.go +++ b/linux/raidcontroller.go @@ -12,6 +12,6 @@ const ( type RAIDController interface { // Type() RAIDControllerType - GetDiskType(string) (disko.DiskType, error) + GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) DriverSysfsPath() string } diff --git a/linux/system.go b/linux/system.go index c3e18a3..c4a81f9 100644 --- a/linux/system.go +++ b/linux/system.go @@ -294,13 +294,9 @@ func (ls *linuxSystem) GetDiskType(path string, udInfo disko.UdevInfo) (disko.Di return dType, err } -// resolveDiskType classifies devicePath as a DiskType. If the device sits -// behind a known RAID controller and corresponds to a virtual/logical drive, -// the controller's classification is used and the returned bool is true. For -// devices that are either not on a RAID controller or are visible through a -// RAID HBA in JBOD/passthrough mode (ErrNotVirtualDrive), the function falls -// back to generic detection via getDiskType(udInfo) with the bool set to -// false. +// resolveDiskType classifies devicePath. Returns (type, true, nil) when a +// RAID controller owns the virtual/logical drive; otherwise (non-RAID or +// ErrDiskTypeUndetermined) falls back to getDiskType(udInfo) with bool=false. func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) (disko.DiskType, bool, error) { devpath := udInfo.Properties["DEVPATH"] @@ -309,10 +305,10 @@ func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) continue } - dType, err := ctrl.GetDiskType(devicePath) + dType, err := ctrl.GetDiskType(devicePath, udInfo) if err != nil { - if errors.Is(err, disko.ErrNotVirtualDrive) { - log.Printf("device %q on RAID sysfs path has no virtual drive (JBOD?), using generic detection", devicePath) + if errors.Is(err, disko.ErrDiskTypeUndetermined) { + log.Printf("RAID controller could not determine disk type for %q, using generic detection", devicePath) break } diff --git a/linux/system_test.go b/linux/system_test.go index 29e6c63..d0e190d 100644 --- a/linux/system_test.go +++ b/linux/system_test.go @@ -244,11 +244,13 @@ type mockRAIDController struct { isSysPathRAID bool getDiskTypeCalled bool getDiskTypePath string + getDiskTypeUdev disko.UdevInfo } -func (m *mockRAIDController) GetDiskType(path string) (disko.DiskType, error) { +func (m *mockRAIDController) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { m.getDiskTypeCalled = true m.getDiskTypePath = path + m.getDiskTypeUdev = udInfo return m.diskType, m.err } @@ -319,7 +321,7 @@ func TestGetDiskTypeJBODFallback(t *testing.T) { ast := assert.New(t) mock := &mockRAIDController{ diskType: disko.HDD, - err: disko.ErrNotVirtualDrive, + err: disko.ErrDiskTypeUndetermined, sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, } @@ -337,13 +339,13 @@ func TestGetDiskTypeJBODFallback(t *testing.T) { dtype, err := ls.GetDiskType("/dev/sda", udInfo) ast.True(mock.getDiskTypeCalled, "RAID controller should have been consulted") - ast.NoError(err, "ErrNotVirtualDrive should not propagate as a fatal error") + ast.NoError(err, "ErrDiskTypeUndetermined should not propagate as a fatal error") ast.Equal(disko.HDD, dtype, "should fall through to generic detection (HDD default)") } func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { ast := assert.New(t) - wrappedErr := fmt.Errorf("controller 0: %w", disko.ErrNotVirtualDrive) + wrappedErr := fmt.Errorf("controller 0: %w", disko.ErrDiskTypeUndetermined) mock := &mockRAIDController{ diskType: disko.HDD, @@ -356,7 +358,7 @@ func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} _, err := ls.GetDiskType("/dev/sda", udInfo) - ast.NoError(err, "wrapped ErrNotVirtualDrive should still trigger fallback via errors.Is") + ast.NoError(err, "wrapped ErrDiskTypeUndetermined should still trigger fallback via errors.Is") } func TestGetDiskTypeRAIDRealError(t *testing.T) { @@ -375,7 +377,7 @@ func TestGetDiskTypeRAIDRealError(t *testing.T) { ast.Error(err) ast.Contains(err.Error(), "failed to get diskType") ast.Contains(err.Error(), "storcli binary crashed") - ast.False(errors.Is(err, disko.ErrNotVirtualDrive)) + ast.False(errors.Is(err, disko.ErrDiskTypeUndetermined)) } func TestGetDiskTypeNoRAIDMatch(t *testing.T) { @@ -400,7 +402,7 @@ func TestGetDiskTypeMultiControllerJBODFallback(t *testing.T) { megaraidMock := &mockRAIDController{ diskType: disko.HDD, - err: disko.ErrNotVirtualDrive, + err: disko.ErrDiskTypeUndetermined, sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, } @@ -418,7 +420,7 @@ func TestGetDiskTypeMultiControllerJBODFallback(t *testing.T) { _, err := ls.GetDiskType("/dev/sda", udInfo) ast.NoError(err) ast.True(megaraidMock.getDiskTypeCalled, "megaraid should have been tried") - ast.False(smartpqiMock.getDiskTypeCalled, "smartpqi should NOT be tried after break from megaraid ErrNotVirtualDrive") + ast.False(smartpqiMock.getDiskTypeCalled, "smartpqi should NOT be tried after break from megaraid ErrDiskTypeUndetermined") } // udevInfoFallbackStub returns a UdevInfo that drives getDiskType's @@ -470,12 +472,12 @@ func TestResolveDiskType_RAIDMatchHDD(t *testing.T) { ast.Equal(disko.HDD, dType) } -// An ErrNotVirtualDrive from the controller must not propagate; resolveDiskType +// An ErrDiskTypeUndetermined from the controller must not propagate; resolveDiskType // falls through to the udev classifier. func TestResolveDiskType_JBODFallsThroughToUdev(t *testing.T) { ast := assert.New(t) mock := &mockRAIDController{ - err: disko.ErrNotVirtualDrive, + err: disko.ErrDiskTypeUndetermined, isSysPathRAID: true, } @@ -494,7 +496,7 @@ func TestResolveDiskType_JBODFallsThroughToUdev(t *testing.T) { func TestResolveDiskType_WrappedJBODFallsThroughToUdev(t *testing.T) { ast := assert.New(t) mock := &mockRAIDController{ - err: fmt.Errorf("controller 0: %w", disko.ErrNotVirtualDrive), + err: fmt.Errorf("controller 0: %w", disko.ErrDiskTypeUndetermined), isSysPathRAID: true, } @@ -543,14 +545,14 @@ func TestResolveDiskType_NoRAIDMatchFallsThroughToUdev(t *testing.T) { ast.Equal(disko.NVME, dType) } -// Once a matching controller reports ErrNotVirtualDrive, iteration stops (a +// Once a matching controller reports ErrDiskTypeUndetermined, iteration stops (a // JBOD on one HBA cannot simultaneously be a VD on another) and we fall // through to the udev classifier exactly once. func TestResolveDiskType_MultiControllerJBODStopsIteration(t *testing.T) { ast := assert.New(t) first := &mockRAIDController{ - err: disko.ErrNotVirtualDrive, + err: disko.ErrDiskTypeUndetermined, isSysPathRAID: true, } second := &mockRAIDController{ diff --git a/megaraid/megaraid.go b/megaraid/megaraid.go index c35c8e1..99a5998 100644 --- a/megaraid/megaraid.go +++ b/megaraid/megaraid.go @@ -135,7 +135,7 @@ type MegaRaid interface { Query(int) (Controller, error) // GetDiskType - Determine the disk type if controller owns disk - GetDiskType(string) (disko.DiskType, error) + GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string diff --git a/megaraid/scsi.go b/megaraid/scsi.go new file mode 100644 index 0000000..6c697e3 --- /dev/null +++ b/megaraid/scsi.go @@ -0,0 +1,47 @@ +package megaraid + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" +) + +// readSCSITarget returns T from /sys/block//device -> H:C:T:L. +// On megaraid_sas JBODs, T equals storcli's Drive.DID. +// +// ok=false (nil err) for non-SCSI devices (no "device" symlink, link does +// not end in H:C:T:L, e.g. NVMe, virtio). sysRoot is injectable for tests; +// production passes "/sys". +func readSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { + if kname == "" { + return 0, false, nil + } + + link := filepath.Join(sysRoot, "block", kname, "device") + + dest, err := os.Readlink(link) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return 0, false, nil + } + return 0, false, fmt.Errorf("readlink %q: %w", link, err) + } + + last := filepath.Base(dest) + parts := strings.Split(last, ":") + + const hctlFields = 4 + if len(parts) != hctlFields { + return 0, false, nil + } + + t, cerr := strconv.Atoi(parts[2]) + if cerr != nil { + return 0, false, fmt.Errorf("parse SCSI target from %q: %w", last, cerr) + } + + return t, true, nil +} diff --git a/megaraid/scsi_test.go b/megaraid/scsi_test.go new file mode 100644 index 0000000..655f52a --- /dev/null +++ b/megaraid/scsi_test.go @@ -0,0 +1,113 @@ +package megaraid + +import ( + "os" + "path/filepath" + "testing" +) + +// makeBlockDeviceSymlink wires up a fake sysfs entry at +// /block//device pointing at ../../scsi_device/. +func makeBlockDeviceSymlink(t *testing.T, root, kname, hctl string) { + t.Helper() + + blockDir := filepath.Join(root, "block", kname) + if err := os.MkdirAll(blockDir, 0o755); err != nil { + t.Fatalf("mkdir %q: %s", blockDir, err) + } + + scsiDir := filepath.Join(root, "scsi_device", hctl) + if err := os.MkdirAll(scsiDir, 0o755); err != nil { + t.Fatalf("mkdir %q: %s", scsiDir, err) + } + + link := filepath.Join(blockDir, "device") + target := filepath.Join("..", "..", "scsi_device", hctl) + if err := os.Symlink(target, link); err != nil { + t.Fatalf("symlink: %s", err) + } +} + +func TestReadSCSITargetJBOD(t *testing.T) { + root := t.TempDir() + makeBlockDeviceSymlink(t, root, "sdb", "0:2:3:0") + + target, ok, err := readSCSITarget(root, "sdb") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if !ok { + t.Fatalf("expected ok=true for a SCSI-backed block device") + } + if target != 3 { + t.Errorf("target: got %d, want 3", target) + } +} + +// An NVMe/virtio-style block device has no H:C:T:L "device" symlink. +// readSCSITarget must report ok=false (not an error) so the caller can +// fall through to generic udev detection. +func TestReadSCSITargetNoDevice(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "block", "nvme0n1"), 0o755); err != nil { + t.Fatalf("mkdir: %s", err) + } + + _, ok, err := readSCSITarget(root, "nvme0n1") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if ok { + t.Errorf("expected ok=false when device symlink is absent") + } +} + +// A symlink whose last segment isn't H:C:T:L (e.g. points at a PCI node) must +// not be mistaken for a SCSI device. readSCSITarget returns ok=false with no +// error so the caller falls through to udev. +func TestReadSCSITargetNonHCTL(t *testing.T) { + root := t.TempDir() + blockDir := filepath.Join(root, "block", "vda") + if err := os.MkdirAll(blockDir, 0o755); err != nil { + t.Fatalf("mkdir: %s", err) + } + other := filepath.Join(root, "devices", "virtio0") + if err := os.MkdirAll(other, 0o755); err != nil { + t.Fatalf("mkdir: %s", err) + } + if err := os.Symlink(filepath.Join("..", "..", "devices", "virtio0"), + filepath.Join(blockDir, "device")); err != nil { + t.Fatalf("symlink: %s", err) + } + + _, ok, err := readSCSITarget(root, "vda") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if ok { + t.Errorf("expected ok=false when link target is not H:C:T:L") + } +} + +func TestReadSCSITargetBadTarget(t *testing.T) { + root := t.TempDir() + makeBlockDeviceSymlink(t, root, "sdc", "0:2:notanum:0") + + _, ok, err := readSCSITarget(root, "sdc") + if err == nil { + t.Fatalf("expected parse error, got nil") + } + if ok { + t.Errorf("expected ok=false on parse error") + } +} + +func TestReadSCSITargetEmptyKname(t *testing.T) { + _, ok, err := readSCSITarget("/sys", "") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if ok { + t.Errorf("expected ok=false for empty kname") + } +} diff --git a/megaraid/storcli.go b/megaraid/storcli.go index d2510db..d6f9deb 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -2,6 +2,7 @@ package megaraid import ( "bytes" + "errors" "fmt" "os/exec" "regexp" @@ -79,7 +80,7 @@ func (sc *storCli) DriverSysfsPath() string { return SysfsPCIDriversPath } -func (sc *storCli) GetDiskType(path string) (disko.DiskType, error) { +func (sc *storCli) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { return disko.HDD, fmt.Errorf("missing controller to run query") } @@ -585,6 +586,10 @@ func getCommandErrorRCDefault(err error, rcError int) int { type cachingStorCli struct { mr MegaRaid cache *cache.Cache + // sysRoot: sysfs root for JBOD SCSI-target lookups ("/sys" in prod). + sysRoot string + // scsiTargetFn: kname -> SCSI target ID. Overridable for tests. + scsiTargetFn func(sysRoot, kname string) (int, bool, error) } // CachingStorCli - just a cache for a MegaRaid @@ -592,8 +597,10 @@ func CachingStorCli() MegaRaid { const longTime = 5 * time.Minute return &cachingStorCli{ - mr: &storCli{}, - cache: cache.New(longTime, longTime), + mr: &storCli{}, + cache: cache.New(longTime, longTime), + sysRoot: "/sys", + scsiTargetFn: readSCSITarget, } } @@ -617,25 +624,85 @@ func (csc *cachingStorCli) Query(cID int) (Controller, error) { return ctrl, err } -func (csc *cachingStorCli) GetDiskType(path string) (disko.DiskType, error) { +func (csc *cachingStorCli) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { ctrl, err := csc.Query(0) - if err == nil { - for _, vd := range ctrl.VirtDrives { - if vd.Path == path { - if ctrl.DriveGroups[vd.DriveGroup].IsSSD() { - return disko.SSD, nil - } + if err != nil { + if isSoftStorCliErr(err) { + // Controller tool unavailable or no controller. Fall + // through with the sentinel so the caller uses udev. + return disko.HDD, disko.ErrDiskTypeUndetermined + } + return disko.HDD, err + } - return disko.HDD, nil + for _, vd := range ctrl.VirtDrives { + if vd.Path == path { + if ctrl.DriveGroups[vd.DriveGroup].IsSSD() { + return disko.SSD, nil } + + return disko.HDD, nil } + } - return disko.HDD, disko.ErrNotVirtualDrive - } else if err != ErrNoStorcli && err != ErrNoController && err != ErrUnsupported { - return disko.HDD, err + // No VD matched path. Try JBOD/passthrough by matching SCSI target + // against Drive.DID; fall through with the sentinel on any failure. + if dType, ok := csc.jbodDiskTypeFromSCSI(ctrl, udInfo); ok { + return dType, nil + } + + return disko.HDD, disko.ErrDiskTypeUndetermined +} + +// isSoftStorCliErr returns true when err indicates that storcli simply +// cannot answer right now (binary missing, no controller present, or an +// unsupported controller). Callers should fall back to generic detection +// rather than treat these as fatal. +func isSoftStorCliErr(err error) bool { + return errors.Is(err, ErrNoStorcli) || + errors.Is(err, ErrNoController) || + errors.Is(err, ErrUnsupported) +} + +// jbodDiskTypeFromSCSI matches the Linux SCSI target T against Drive.DID +// in the PD LIST. Returns ok=false on missing kname, non-SCSI device, DID +// collision, or UnknownMedia. +func (csc *cachingStorCli) jbodDiskTypeFromSCSI(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { + if csc.scsiTargetFn == nil { + return disko.HDD, false + } + + kname := udInfo.Name + if kname == "" { + return disko.HDD, false + } + + target, ok, err := csc.scsiTargetFn(csc.sysRoot, kname) + if err != nil || !ok { + return disko.HDD, false + } + + var matches []*Drive + for _, d := range ctrl.Drives { + if d != nil && d.ID == target { + matches = append(matches, d) + } + } + + if len(matches) != 1 { + return disko.HDD, false + } + + switch matches[0].MediaType { + case SSD: + return disko.SSD, true + case HDD: + return disko.HDD, true + case UnknownMedia: + return disko.HDD, false } - return disko.HDD, fmt.Errorf("cannot determine disk type") + return disko.HDD, false } func (csc *cachingStorCli) DriverSysfsPath() string { diff --git a/megaraid/storcli_test.go b/megaraid/storcli_test.go index 3b58162..b5e8c66 100644 --- a/megaraid/storcli_test.go +++ b/megaraid/storcli_test.go @@ -3,6 +3,7 @@ package megaraid import ( "encoding/json" "errors" + "fmt" "reflect" "strings" "testing" @@ -1486,7 +1487,7 @@ func (m *mockMegaRaid) Query(cID int) (Controller, error) { return m.ctrl, m.err } -func (m *mockMegaRaid) GetDiskType(path string) (disko.DiskType, error) { +func (m *mockMegaRaid) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { return disko.HDD, nil } @@ -1494,7 +1495,7 @@ func (m *mockMegaRaid) DriverSysfsPath() string { return "/sys/bus/pci/drivers/megaraid_sas" } -func TestGetDiskTypeJBODReturnsErrNotVirtualDrive(t *testing.T) { +func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) if err != nil { t.Fatalf("newController failed: %s", err) @@ -1506,9 +1507,9 @@ func TestGetDiskTypeJBODReturnsErrNotVirtualDrive(t *testing.T) { } for _, path := range []string{"/dev/sda", "/dev/sdb", "/dev/sdc"} { - dtype, err := csc.GetDiskType(path) - if !errors.Is(err, disko.ErrNotVirtualDrive) { - t.Errorf("GetDiskType(%q): expected ErrNotVirtualDrive, got %v", path, err) + dtype, err := csc.GetDiskType(path, disko.UdevInfo{}) + if !errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Errorf("GetDiskType(%q): expected ErrDiskTypeUndetermined, got %v", path, err) } if dtype != disko.HDD { @@ -1517,18 +1518,146 @@ func TestGetDiskTypeJBODReturnsErrNotVirtualDrive(t *testing.T) { } } -func TestGetDiskTypeQueryFailureNotSentinel(t *testing.T) { +// A soft storcli failure (missing binary / no controller / unsupported) +// should return the sentinel so the caller falls back to udev. +func TestGetDiskTypeSoftQueryFailureReturnsSentinel(t *testing.T) { csc := &cachingStorCli{ mr: &mockMegaRaid{ctrl: Controller{}, err: ErrNoStorcli}, cache: cache.New(5*time.Minute, 5*time.Minute), } - _, err := csc.GetDiskType("/dev/sda") - if errors.Is(err, disko.ErrNotVirtualDrive) { - t.Error("GetDiskType should NOT return ErrNotVirtualDrive when storcli is missing") + _, err := csc.GetDiskType("/dev/sda", disko.UdevInfo{}) + if !errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Errorf("expected ErrDiskTypeUndetermined on soft storcli failure, got %v", err) } +} +// A hard storcli error must propagate unchanged (it is not the sentinel). +func TestGetDiskTypeHardQueryFailurePropagates(t *testing.T) { + hardErr := fmt.Errorf("storcli exploded") + csc := &cachingStorCli{ + mr: &mockMegaRaid{ctrl: Controller{}, err: hardErr}, + cache: cache.New(5*time.Minute, 5*time.Minute), + } + + _, err := csc.GetDiskType("/dev/sda", disko.UdevInfo{}) + if errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Error("hard storcli error should NOT be treated as the sentinel") + } if err == nil { - t.Error("GetDiskType should return an error when storcli is missing") + t.Error("hard storcli error must propagate") + } +} + +// newJBODCachingStorCli wires the Cisco JBOD fixture to a stub scsiTargetFn +// that returns (target, ok) for kname "sdb". +func newJBODCachingStorCli(t *testing.T, target int, ok bool) *cachingStorCli { + t.Helper() + + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + if err != nil { + t.Fatalf("newController failed: %s", err) + } + + return &cachingStorCli{ + mr: &mockMegaRaid{ctrl: ctrl, err: nil}, + cache: cache.New(5*time.Minute, 5*time.Minute), + sysRoot: "/unused-in-test", + scsiTargetFn: func(_, kname string) (int, bool, error) { + if kname == "sdb" { + return target, ok, nil + } + return 0, false, nil + }, + } +} + +// DID=1 is the SSD in the fixture. +func TestGetDiskTypeJBODSCSITargetMatchSSD(t *testing.T) { + csc := newJBODCachingStorCli(t, 1, true) + + ud := disko.UdevInfo{Name: "sdb"} + dtype, err := csc.GetDiskType("/dev/sdb", ud) + if err != nil { + t.Fatalf("expected nil err for SSD JBOD match, got %v", err) + } + if dtype != disko.SSD { + t.Errorf("GetDiskType: got %v, want SSD", dtype) + } +} + +// DID=0 is an HDD in the fixture. +func TestGetDiskTypeJBODSCSITargetMatchHDD(t *testing.T) { + csc := newJBODCachingStorCli(t, 0, true) + + ud := disko.UdevInfo{Name: "sdb"} + dtype, err := csc.GetDiskType("/dev/sdb", ud) + if err != nil { + t.Fatalf("expected nil err for HDD JBOD match, got %v", err) + } + if dtype != disko.HDD { + t.Errorf("GetDiskType: got %v, want HDD", dtype) + } +} + +// DID not in the fixture falls through to ErrDiskTypeUndetermined. +func TestGetDiskTypeJBODSCSITargetNoMatch(t *testing.T) { + csc := newJBODCachingStorCli(t, 42, true) + + ud := disko.UdevInfo{Name: "sdb"} + _, err := csc.GetDiskType("/dev/sdb", ud) + if !errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Errorf("expected ErrDiskTypeUndetermined for unknown DID, got %v", err) + } +} + +// Unresolvable sysfs H:C:T:L (NVMe, virtio, missing symlink). +func TestGetDiskTypeJBODSCSITargetUnresolved(t *testing.T) { + csc := newJBODCachingStorCli(t, 0, false) + + ud := disko.UdevInfo{Name: "sdb"} + _, err := csc.GetDiskType("/dev/sdb", ud) + if !errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Errorf("expected ErrDiskTypeUndetermined when SCSI target is unresolved, got %v", err) + } +} + +// Empty udev Name: sysfs helper has no kname to resolve. +func TestGetDiskTypeJBODNoUdevName(t *testing.T) { + csc := newJBODCachingStorCli(t, 1, true) + + _, err := csc.GetDiskType("/dev/sdb", disko.UdevInfo{}) + if !errors.Is(err, disko.ErrDiskTypeUndetermined) { + t.Errorf("expected ErrDiskTypeUndetermined with empty udev Name, got %v", err) + } +} + +// isSoftStorCliErr must recognise the three soft sentinels whether bare +// or wrapped with fmt.Errorf("%w"). +func TestIsSoftStorCliErr(t *testing.T) { + hard := errors.New("storcli blew up") + + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"bare ErrNoStorcli", ErrNoStorcli, true}, + {"bare ErrNoController", ErrNoController, true}, + {"bare ErrUnsupported", ErrUnsupported, true}, + {"wrapped ErrNoStorcli", fmt.Errorf("ctx: %w", ErrNoStorcli), true}, + {"wrapped ErrNoController", fmt.Errorf("ctx: %w", ErrNoController), true}, + {"wrapped ErrUnsupported", fmt.Errorf("ctx: %w", ErrUnsupported), true}, + {"unrelated hard error", hard, false}, + {"wrapped hard error", fmt.Errorf("ctx: %w", hard), false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSoftStorCliErr(tc.err); got != tc.want { + t.Errorf("isSoftStorCliErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) } } diff --git a/mpi3mr/mpi3mr.go b/mpi3mr/mpi3mr.go index 3e3aae8..ab558ba 100644 --- a/mpi3mr/mpi3mr.go +++ b/mpi3mr/mpi3mr.go @@ -68,7 +68,7 @@ type Mpi3mr interface { Query(int) (Controller, error) // GetDiskType - Determine the disk type if controller owns disk - GetDiskType(string) (disko.DiskType, error) + GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index 5dbb613..4b662c5 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -520,11 +520,18 @@ func getCommandErrorRCDefault(err error, rcError int) int { // Implement the mpi3mr Interface with storcli2 type storCli2 struct { + // sysRoot: sysfs root for JBOD SCSI-target lookups ("/sys" in prod). + sysRoot string + // scsiTargetFn: kname -> SCSI target ID. Overridable for tests. + scsiTargetFn func(sysRoot, kname string) (int, bool, error) } // StorCli returns a storcli2 specific implementation of Query for the Mpi3mr interface func StorCli2() Mpi3mr { - return &storCli2{} + return &storCli2{ + sysRoot: "/sys", + scsiTargetFn: readSCSITarget, + } } const ( @@ -596,48 +603,101 @@ func (sc *storCli2) DriverSysfsPath() string { return SysfsPCIDriversPath } -func (sc *storCli2) GetDiskType(path string) (disko.DiskType, error) { +func (sc *storCli2) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { cIDs, err := sc.List() if err != nil { + if isSoftStorCli2Err(err) { + return disko.HDD, disko.ErrDiskTypeUndetermined + } return disko.HDD, errors.Errorf("failed to get controller list: %s", err) } - // isPhysical becomes true once we have successfully inspected at least one - // controller. If we then finish the loop without matching path against any - // VirtualDrive, we know the device is visible through the HBA but is not a - // configured virtual drive -- i.e. it is a physical/passthrough (JBOD) disk. - isPhysical := false - queryErrs := []error{} + var queryErrs []error + var queriedCtrls []Controller for _, cID := range cIDs { ctrl, err := sc.Query(cID) if err != nil { - queryErrs = append(queryErrs, fmt.Errorf("error while getting config for controller id:%d %s", cID, err)) + queryErrs = append(queryErrs, fmt.Errorf("controller id %d: %w", cID, err)) continue } - isPhysical = true + queriedCtrls = append(queriedCtrls, ctrl) for _, vDev := range ctrl.VirtualDrives { - if vDev.Path() == path { - if vDev.IsSSD() { - return disko.SSD, nil - } - - return disko.HDD, nil + if vDev.Path() != path { + continue + } + if vDev.IsSSD() { + return disko.SSD, nil } + return disko.HDD, nil } } for _, err := range queryErrs { - if err != ErrNoStor2cli && err != ErrNoController && err != ErrUnsupported { + if !isSoftStorCli2Err(err) { return disko.HDD, err } } - if isPhysical { - return disko.HDD, disko.ErrNotVirtualDrive + // No VD matched path (or no controller responded). Try JBOD/ + // passthrough by matching SCSI target against PhysicalDrive.PID; + // fall through with the sentinel so the caller uses udev. + if dType, ok := sc.jbodDiskTypeFromSCSI(queriedCtrls, udInfo); ok { + return dType, nil + } + + return disko.HDD, disko.ErrDiskTypeUndetermined +} + +// isSoftStorCli2Err returns true when err indicates that storcli2 simply +// cannot answer right now (binary missing, no controller present, or an +// unsupported controller). Callers should fall back to generic detection +// rather than treat these as fatal. +func isSoftStorCli2Err(err error) bool { + return errors.Is(err, ErrNoStor2cli) || + errors.Is(err, ErrNoController) || + errors.Is(err, ErrUnsupported) +} + +// jbodDiskTypeFromSCSI matches the Linux SCSI target T against +// PhysicalDrive.PID across the given controllers. Returns ok=false on +// missing kname, non-SCSI device, PID collision, or unknown medium. +func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { + if sc.scsiTargetFn == nil { + return disko.HDD, false + } + + kname := udInfo.Name + if kname == "" { + return disko.HDD, false + } + + target, ok, err := sc.scsiTargetFn(sc.sysRoot, kname) + if err != nil || !ok { + return disko.HDD, false + } + + var matches []PhysicalDrive + for _, ctrl := range ctrls { + for _, pd := range ctrl.PhysicalDrives { + if pd.PID == target { + matches = append(matches, pd) + } + } + } + + if len(matches) != 1 { + return disko.HDD, false + } + + switch strings.ToUpper(strings.TrimSpace(matches[0].Medium)) { + case "SSD": + return disko.SSD, true + case "HDD": + return disko.HDD, true } - return disko.HDD, fmt.Errorf("cannot determine disk type for path %q", path) + return disko.HDD, false } diff --git a/mpi3mr/storcli2_test.go b/mpi3mr/storcli2_test.go index 141f25b..db1d993 100644 --- a/mpi3mr/storcli2_test.go +++ b/mpi3mr/storcli2_test.go @@ -2,9 +2,12 @@ package mpi3mr import ( "encoding/json" + "errors" + "fmt" "testing" "github.com/google/go-cmp/cmp" + "machinerun.io/disko" ) var showNoLogJData = ` @@ -446,3 +449,150 @@ func TestStorCli2ParsePhysicalDriveForeign(t *testing.T) { } } + +func jbodPhysCtrl() Controller { + return Controller{ + PhysicalDrives: PhysicalDriveSet{ + "10": PhysicalDrive{PID: 10, Medium: "HDD", State: "JBOD"}, + "20": PhysicalDrive{PID: 20, Medium: "SSD", State: "JBOD"}, + }, + } +} + +func TestStorCli2JBODDiskTypeHDD(t *testing.T) { + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + if kname == "sdb" { + return 10, true, nil + } + return 0, false, nil + }, + } + + dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}) + if !ok { + t.Fatalf("expected ok=true on HDD JBOD match") + } + if dType != disko.HDD { + t.Errorf("jbodDiskTypeFromSCSI: got %v, want HDD", dType) + } +} + +func TestStorCli2JBODDiskTypeSSD(t *testing.T) { + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 20, true, nil + }, + } + + dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}) + if !ok { + t.Fatalf("expected ok=true on SSD JBOD match") + } + if dType != disko.SSD { + t.Errorf("jbodDiskTypeFromSCSI: got %v, want SSD", dType) + } +} + +// SCSI target not reported by any controller. +func TestStorCli2JBODDiskTypeNoMatch(t *testing.T) { + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 42, true, nil + }, + } + + if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { + t.Errorf("expected ok=false when no PhysicalDrive matches SCSI target") + } +} + +// PID collision across controllers is ambiguous. +func TestStorCli2JBODDiskTypeAmbiguousPID(t *testing.T) { + other := Controller{ + PhysicalDrives: PhysicalDriveSet{ + "10": PhysicalDrive{PID: 10, Medium: "SSD", State: "JBOD"}, + }, + } + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 10, true, nil + }, + } + + if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl(), other}, disko.UdevInfo{Name: "sdb"}); ok { + t.Errorf("expected ok=false on PID collision across controllers") + } +} + +// Non-SCSI device (NVMe, virtio, missing symlink). +func TestStorCli2JBODDiskTypeUnresolvedTarget(t *testing.T) { + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 0, false, nil + }, + } + + if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { + t.Errorf("expected ok=false when SCSI target is unresolved") + } +} + +// Sysfs lookup error is swallowed as a no-match. +func TestStorCli2JBODDiskTypeLookupError(t *testing.T) { + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 0, false, errors.New("boom") + }, + } + + if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { + t.Errorf("expected ok=false when SCSI lookup errors") + } +} + +// Empty udev Name short-circuits before any sysfs call. +func TestStorCli2JBODDiskTypeEmptyName(t *testing.T) { + sc := &storCli2{sysRoot: "/unused", scsiTargetFn: readSCSITarget} + + if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{}); ok { + t.Errorf("expected ok=false when udev Name is empty") + } +} + +// isSoftStorCli2Err must recognise the three soft sentinels whether bare +// or wrapped with fmt.Errorf("%w"), so wrapped per-controller errors are +// still routed through the udev fallback instead of propagating as fatal. +func TestIsSoftStorCli2Err(t *testing.T) { + hard := errors.New("storcli2 blew up") + + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"bare ErrNoStor2cli", ErrNoStor2cli, true}, + {"bare ErrNoController", ErrNoController, true}, + {"bare ErrUnsupported", ErrUnsupported, true}, + {"wrapped ErrNoStor2cli", fmt.Errorf("controller id %d: %w", 0, ErrNoStor2cli), true}, + {"wrapped ErrNoController", fmt.Errorf("controller id %d: %w", 1, ErrNoController), true}, + {"wrapped ErrUnsupported", fmt.Errorf("controller id %d: %w", 2, ErrUnsupported), true}, + {"unrelated hard error", hard, false}, + {"wrapped hard error", fmt.Errorf("controller id %d: %w", 3, hard), false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSoftStorCli2Err(tc.err); got != tc.want { + t.Errorf("isSoftStorCli2Err(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/smartpqi/arcconf.go b/smartpqi/arcconf.go index 52ede77..445f475 100644 --- a/smartpqi/arcconf.go +++ b/smartpqi/arcconf.go @@ -2,6 +2,7 @@ package smartpqi import ( "bytes" + "errors" "fmt" "os/exec" "regexp" @@ -489,50 +490,119 @@ func (ac *arcConf) Query(cID int) (Controller, error) { return Controller{}, fmt.Errorf("unknown controller id %d", cID) } -func (ac *arcConf) GetDiskType(path string) (disko.DiskType, error) { +func (ac *arcConf) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { cIDs, err := ac.List() if err != nil { + if isSoftArcconfErr(err) { + return disko.HDD, disko.ErrDiskTypeUndetermined + } return disko.HDD, fmt.Errorf("failed to enumerate controllers: %s", err) } - // isPhysical becomes true once we have successfully inspected at least one - // controller. If we then finish the loop without matching path against any - // LogicalDrive, we know the device is visible through the HBA but is not a - // configured logical drive -- i.e. it is a physical/passthrough (JBOD) disk. - isPhysical := false - queryErrs := []error{} + var queryErrs []error + var queriedCtrls []Controller for _, cID := range cIDs { ctrl, err := ac.GetConfig(cID) if err != nil { - queryErrs = append(queryErrs, fmt.Errorf("error while getting config for controller id:%d: %s", cID, err)) + queryErrs = append(queryErrs, fmt.Errorf("controller id %d: %w", cID, err)) continue } - isPhysical = true + queriedCtrls = append(queriedCtrls, ctrl) for _, lDrive := range ctrl.LogicalDrives { - if lDrive.DiskName == path { - if lDrive.IsSSD() { - return disko.SSD, nil - } - - return disko.HDD, nil + if lDrive.DiskName != path { + continue } + if lDrive.IsSSD() { + return disko.SSD, nil + } + return disko.HDD, nil } } for _, err := range queryErrs { - if err != ErrNoArcconf && err != ErrNoController && err != ErrUnsupported { + if !isSoftArcconfErr(err) { return disko.HDD, err } } - if isPhysical { - return disko.HDD, disko.ErrNotVirtualDrive + // No LD matched path (or no controller responded). Try JBOD by + // matching udev serial against PhysicalDevice SerialNumber; fall + // through with the sentinel so the caller uses udev. + if dType, ok := jbodDiskTypeFromSerial(queriedCtrls, udInfo); ok { + return dType, nil + } + + return disko.HDD, disko.ErrDiskTypeUndetermined +} + +// isSoftArcconfErr returns true when err indicates that arcconf simply +// cannot answer right now (binary missing, no controller present, or an +// unsupported controller). Callers should fall back to generic detection +// rather than treat these as fatal. +func isSoftArcconfErr(err error) bool { + return errors.Is(err, ErrNoArcconf) || + errors.Is(err, ErrNoController) || + errors.Is(err, ErrUnsupported) +} + +// jbodDiskTypeFromSerial matches a udev serial against PhysicalDevice +// SerialNumber across the given controllers. Returns ok=false on missing +// udev serial, no match, collision, or unknown media. +func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { + serials := collectUdevSerials(udInfo) + if len(serials) == 0 { + return disko.HDD, false + } + + var matches []*PhysicalDevice + for _, ctrl := range ctrls { + for _, pDev := range ctrl.PhysicalDrives { + if pDev == nil { + continue + } + pdSerial := strings.TrimSpace(pDev.SerialNumber) + if pdSerial == "" { + continue + } + if _, ok := serials[pdSerial]; ok { + matches = append(matches, pDev) + } + } + } + + if len(matches) != 1 { + return disko.HDD, false + } + + switch matches[0].Type { + case SSD: + return disko.SSD, true + case HDD: + return disko.HDD, true + case NVME: + return disko.NVME, true + } + + return disko.HDD, false +} + +// collectUdevSerials returns the udev tokens to match against +// PhysicalDevice.SerialNumber: the raw ID_SERIAL_SHORT and, as fallback, +// the longer "__" ID_SERIAL. +func collectUdevSerials(udInfo disko.UdevInfo) map[string]struct{} { + out := map[string]struct{}{} + + for _, key := range []string{"ID_SERIAL_SHORT", "ID_SERIAL"} { + v := strings.TrimSpace(udInfo.Properties[key]) + if v != "" { + out[v] = struct{}{} + } } - return disko.HDD, fmt.Errorf("cannot determine disk type") + return out } func (ac *arcConf) DriverSysfsPath() string { diff --git a/smartpqi/arcconf_test.go b/smartpqi/arcconf_test.go index c1c04ce..495bc55 100644 --- a/smartpqi/arcconf_test.go +++ b/smartpqi/arcconf_test.go @@ -2,9 +2,13 @@ package smartpqi import ( "encoding/json" + "errors" + "fmt" "reflect" "strings" "testing" + + "machinerun.io/disko" ) var arcConfGetConfig = ` @@ -1577,3 +1581,186 @@ func TestSmartPqiNewControllerBadInput(t *testing.T) { } } } + +// jbodCtrlWithPDs: one HDD, one SSD, one NVMe keyed by distinct serials. +func jbodCtrlWithPDs() Controller { + return Controller{ + ID: 1, + PhysicalDrives: DriveSet{ + 10: &PhysicalDevice{ID: 10, SerialNumber: "SN-HDD-1", Type: HDD}, + 20: &PhysicalDevice{ID: 20, SerialNumber: "SN-SSD-1", Type: SSD}, + 30: &PhysicalDevice{ID: 30, SerialNumber: "SN-NVME-1", Type: NVME}, + }, + } +} + +func TestJBODDiskTypeSerialHDDShort(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-HDD-1"}, + } + + dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + if !ok { + t.Fatalf("expected ok=true when ID_SERIAL_SHORT matches") + } + if dType != disko.HDD { + t.Errorf("jbodDiskTypeFromSerial: got %v, want HDD", dType) + } +} + +// ID_SERIAL is the fallback when ID_SERIAL_SHORT is absent. +func TestJBODDiskTypeSerialSSDFallback(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL": "SN-SSD-1"}, + } + + dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + if !ok { + t.Fatalf("expected ok=true when ID_SERIAL matches as fallback") + } + if dType != disko.SSD { + t.Errorf("jbodDiskTypeFromSerial: got %v, want SSD", dType) + } +} + +func TestJBODDiskTypeSerialNVME(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-NVME-1"}, + } + + dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + if !ok { + t.Fatalf("expected ok=true for NVME serial match") + } + if dType != disko.NVME { + t.Errorf("jbodDiskTypeFromSerial: got %v, want NVME", dType) + } +} + +// No ID_SERIAL* property in udev. +func TestJBODDiskTypeSerialNoSerial(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_MODEL": "SomeModel"}, + } + + if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud); ok { + t.Errorf("expected ok=false when udev carries no ID_SERIAL*") + } +} + +// Serial not reported by any controller. +func TestJBODDiskTypeSerialNoMatch(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-UNKNOWN"}, + } + + if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud); ok { + t.Errorf("expected ok=false when no PhysicalDevice matches") + } +} + +// Duplicate serial across controllers is ambiguous. +func TestJBODDiskTypeSerialAmbiguous(t *testing.T) { + dup := Controller{ + ID: 2, + PhysicalDrives: DriveSet{ + 99: &PhysicalDevice{ID: 99, SerialNumber: "SN-HDD-1", Type: SSD}, + }, + } + + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-HDD-1"}, + } + + if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs(), dup}, ud); ok { + t.Errorf("expected ok=false on duplicate SerialNumber across controllers") + } +} + +// Matched PD with UnknownMedia cannot be classified. +func TestJBODDiskTypeSerialUnknownMedia(t *testing.T) { + ctrl := Controller{ + ID: 3, + PhysicalDrives: DriveSet{ + 10: &PhysicalDevice{ID: 10, SerialNumber: "SN-UNK", Type: UnknownMedia}, + }, + } + + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-UNK"}, + } + + if _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud); ok { + t.Errorf("expected ok=false when matched PD has UnknownMedia") + } +} + +// Blank controller-side SerialNumber never matches. +func TestJBODDiskTypeSerialEmptyPDSerial(t *testing.T) { + ctrl := Controller{ + ID: 4, + PhysicalDrives: DriveSet{ + 10: &PhysicalDevice{ID: 10, SerialNumber: " ", Type: SSD}, + }, + } + + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SN-X"}, + } + + if _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud); ok { + t.Errorf("expected ok=false when PhysicalDevice has blank SerialNumber") + } +} + +func TestCollectUdevSerialsPrefersBoth(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SERIAL_SHORT": "short1", + "ID_SERIAL": "long_short1", + "ID_MODEL": "ignored", + }, + } + + got := collectUdevSerials(ud) + if _, ok := got["short1"]; !ok { + t.Errorf("missing ID_SERIAL_SHORT token") + } + if _, ok := got["long_short1"]; !ok { + t.Errorf("missing ID_SERIAL token") + } + if _, ok := got["ignored"]; ok { + t.Errorf("collectUdevSerials must not include unrelated properties") + } +} + +// isSoftArcconfErr must recognise the three soft sentinels whether bare +// or wrapped with fmt.Errorf("%w"), so wrapped per-controller errors are +// still routed through the udev fallback instead of propagating as fatal. +func TestIsSoftArcconfErr(t *testing.T) { + hard := errors.New("arcconf blew up") + + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"bare ErrNoArcconf", ErrNoArcconf, true}, + {"bare ErrNoController", ErrNoController, true}, + {"bare ErrUnsupported", ErrUnsupported, true}, + {"wrapped ErrNoArcconf", fmt.Errorf("controller id %d: %w", 0, ErrNoArcconf), true}, + {"wrapped ErrNoController", fmt.Errorf("controller id %d: %w", 1, ErrNoController), true}, + {"wrapped ErrUnsupported", fmt.Errorf("controller id %d: %w", 2, ErrUnsupported), true}, + {"unrelated hard error", hard, false}, + {"wrapped hard error", fmt.Errorf("controller id %d: %w", 3, hard), false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSoftArcconfErr(tc.err); got != tc.want { + t.Errorf("isSoftArcconfErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/smartpqi/smartpqi.go b/smartpqi/smartpqi.go index 5b44c6f..08c0fa4 100644 --- a/smartpqi/smartpqi.go +++ b/smartpqi/smartpqi.go @@ -120,7 +120,7 @@ type SmartPqi interface { Query(int) (Controller, error) // GetDiskType - Determine the disk type if controller owns disk - GetDiskType(string) (disko.DiskType, error) + GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) // DriverSysfsPath - Return the sysfs path to the linux driver for this controller DriverSysfsPath() string From 4b46bbc0bb5201c946f4c1e731b1bc09e2133343 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Fri, 24 Apr 2026 11:58:32 +0000 Subject: [PATCH 3/8] refactor: Consolidate Linux sysfs helpers into linux/sysfs subpackage Move IsSysPathRAID, GetSysPaths and readSCSITarget into a new linux/sysfs package (with readSCSITarget exported as ReadSCSITarget) so driver packages and the top-level linux package can share one implementation. The subpackage location avoids an import cycle since linux already depends on megaraid/mpi3mr/smartpqi. Files: - linux/sysfs.go -> linux/sysfs/sysfs.go (+package doc) - linux/sysfs_test.go -> linux/sysfs/sysfs_test.go - megaraid/scsi.go -> linux/sysfs/scsi.go (renamed/exported, generic doc covering both megaraid DID and mpi3mr PID) - megaraid/scsi_test.go -> linux/sysfs/scsi_test.go Callers: - linux/system.go: default isSysPathRAID is now sysfs.IsSysPathRAID. - megaraid/storcli.go, mpi3mr/storcli2.go: default scsiTargetFn is now sysfs.ReadSCSITarget. Injection type is unchanged so existing stub tests keep compiling. - mpi3mr/storcli2_test.go: the one test that referenced the production helper directly now uses sysfs.ReadSCSITarget. - linux/util.go: add thin IsSysPathRAID and GetSysPaths wrappers that forward to the sysfs package, marked // Deprecated: so existing external callers of the top-level linux package keep working. Signed-off-by: Andrei Aaron --- {megaraid => linux/sysfs}/scsi.go | 11 +++++++---- {megaraid => linux/sysfs}/scsi_test.go | 16 ++++++++-------- linux/{ => sysfs}/sysfs.go | 6 +++++- linux/{ => sysfs}/sysfs_test.go | 2 +- linux/system.go | 3 ++- linux/util.go | 20 ++++++++++++++++++++ megaraid/storcli.go | 3 ++- mpi3mr/storcli2.go | 3 ++- mpi3mr/storcli2_test.go | 3 ++- 9 files changed, 49 insertions(+), 18 deletions(-) rename {megaraid => linux/sysfs}/scsi.go (68%) rename {megaraid => linux/sysfs}/scsi_test.go (88%) rename linux/{ => sysfs}/sysfs.go (84%) rename linux/{ => sysfs}/sysfs_test.go (99%) diff --git a/megaraid/scsi.go b/linux/sysfs/scsi.go similarity index 68% rename from megaraid/scsi.go rename to linux/sysfs/scsi.go index 6c697e3..3048ef7 100644 --- a/megaraid/scsi.go +++ b/linux/sysfs/scsi.go @@ -1,4 +1,4 @@ -package megaraid +package sysfs import ( "errors" @@ -9,13 +9,16 @@ import ( "strings" ) -// readSCSITarget returns T from /sys/block//device -> H:C:T:L. -// On megaraid_sas JBODs, T equals storcli's Drive.DID. +// ReadSCSITarget returns T from /sys/block//device -> H:C:T:L. +// For SCSI-attached JBOD/passthrough disks behind a RAID HBA, T matches +// the controller-reported drive ID (megaraid Drive.DID, mpi3mr +// PhysicalDrive.PID), which lets callers correlate a Linux block device +// with an entry in the controller's PD list. // // ok=false (nil err) for non-SCSI devices (no "device" symlink, link does // not end in H:C:T:L, e.g. NVMe, virtio). sysRoot is injectable for tests; // production passes "/sys". -func readSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { +func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { if kname == "" { return 0, false, nil } diff --git a/megaraid/scsi_test.go b/linux/sysfs/scsi_test.go similarity index 88% rename from megaraid/scsi_test.go rename to linux/sysfs/scsi_test.go index 655f52a..68ef315 100644 --- a/megaraid/scsi_test.go +++ b/linux/sysfs/scsi_test.go @@ -1,4 +1,4 @@ -package megaraid +package sysfs import ( "os" @@ -32,7 +32,7 @@ func TestReadSCSITargetJBOD(t *testing.T) { root := t.TempDir() makeBlockDeviceSymlink(t, root, "sdb", "0:2:3:0") - target, ok, err := readSCSITarget(root, "sdb") + target, ok, err := ReadSCSITarget(root, "sdb") if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -45,7 +45,7 @@ func TestReadSCSITargetJBOD(t *testing.T) { } // An NVMe/virtio-style block device has no H:C:T:L "device" symlink. -// readSCSITarget must report ok=false (not an error) so the caller can +// ReadSCSITarget must report ok=false (not an error) so the caller can // fall through to generic udev detection. func TestReadSCSITargetNoDevice(t *testing.T) { root := t.TempDir() @@ -53,7 +53,7 @@ func TestReadSCSITargetNoDevice(t *testing.T) { t.Fatalf("mkdir: %s", err) } - _, ok, err := readSCSITarget(root, "nvme0n1") + _, ok, err := ReadSCSITarget(root, "nvme0n1") if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -63,7 +63,7 @@ func TestReadSCSITargetNoDevice(t *testing.T) { } // A symlink whose last segment isn't H:C:T:L (e.g. points at a PCI node) must -// not be mistaken for a SCSI device. readSCSITarget returns ok=false with no +// not be mistaken for a SCSI device. ReadSCSITarget returns ok=false with no // error so the caller falls through to udev. func TestReadSCSITargetNonHCTL(t *testing.T) { root := t.TempDir() @@ -80,7 +80,7 @@ func TestReadSCSITargetNonHCTL(t *testing.T) { t.Fatalf("symlink: %s", err) } - _, ok, err := readSCSITarget(root, "vda") + _, ok, err := ReadSCSITarget(root, "vda") if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -93,7 +93,7 @@ func TestReadSCSITargetBadTarget(t *testing.T) { root := t.TempDir() makeBlockDeviceSymlink(t, root, "sdc", "0:2:notanum:0") - _, ok, err := readSCSITarget(root, "sdc") + _, ok, err := ReadSCSITarget(root, "sdc") if err == nil { t.Fatalf("expected parse error, got nil") } @@ -103,7 +103,7 @@ func TestReadSCSITargetBadTarget(t *testing.T) { } func TestReadSCSITargetEmptyKname(t *testing.T) { - _, ok, err := readSCSITarget("/sys", "") + _, ok, err := ReadSCSITarget("/sys", "") if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/linux/sysfs.go b/linux/sysfs/sysfs.go similarity index 84% rename from linux/sysfs.go rename to linux/sysfs/sysfs.go index 44be5a2..dda23db 100644 --- a/linux/sysfs.go +++ b/linux/sysfs/sysfs.go @@ -1,4 +1,8 @@ -package linux +// Package sysfs contains Linux-specific helpers for inspecting the sysfs +// hierarchy. The helpers are driver-agnostic so they can be shared by the +// top-level linux package and by individual RAID driver packages without +// creating an import cycle back into linux. +package sysfs import ( "fmt" diff --git a/linux/sysfs_test.go b/linux/sysfs/sysfs_test.go similarity index 99% rename from linux/sysfs_test.go rename to linux/sysfs/sysfs_test.go index 8af8793..12bd789 100644 --- a/linux/sysfs_test.go +++ b/linux/sysfs/sysfs_test.go @@ -1,4 +1,4 @@ -package linux +package sysfs import ( "os" diff --git a/linux/system.go b/linux/system.go index c4a81f9..b535b01 100644 --- a/linux/system.go +++ b/linux/system.go @@ -12,6 +12,7 @@ import ( "golang.org/x/sys/unix" "machinerun.io/disko" + "machinerun.io/disko/linux/sysfs" "machinerun.io/disko/megaraid" "machinerun.io/disko/mpi3mr" "machinerun.io/disko/smartpqi" @@ -33,7 +34,7 @@ func System() disko.System { smartpqi.ArcConf(), mpi3mr.StorCli2(), }, - isSysPathRAID: IsSysPathRAID, + isSysPathRAID: sysfs.IsSysPathRAID, } } diff --git a/linux/util.go b/linux/util.go index d6f4ffe..a64c665 100644 --- a/linux/util.go +++ b/linux/util.go @@ -15,6 +15,7 @@ import ( "github.com/pkg/errors" "machinerun.io/disko" + "machinerun.io/disko/linux/sysfs" ) // GetUdevInfo return a UdevInfo for the device with kernel name kname. @@ -291,6 +292,16 @@ func Floor(val, unit uint64) uint64 { return (val / unit) * unit } +// IsSysPathRAID checks whether syspath (udevadm DEVPATH) belongs to a RAID +// controller whose PCI driver is registered at driverSysPath. +// +// Deprecated: use sysfs.IsSysPathRAID from machinerun.io/disko/linux/sysfs +// instead. This wrapper exists only for backwards compatibility with callers +// that import the top-level linux package. +func IsSysPathRAID(syspath string, driverSysPath string) bool { + return sysfs.IsSysPathRAID(syspath, driverSysPath) +} + // NameByDiskID - return the linux name (sda) for the disk with given DiskID func NameByDiskID(driverSysPath string, id int) (string, error) { // given ID, we expect a single file in: @@ -311,3 +322,12 @@ func NameByDiskID(driverSysPath string, id int) (string, error) { return path.Base(matches[0]), nil } + +// GetSysPaths returns the resolved PCI device paths for a RAID driver. +// +// Deprecated: use sysfs.GetSysPaths from machinerun.io/disko/linux/sysfs +// instead. This wrapper exists only for backwards compatibility with callers +// that import the top-level linux package. +func GetSysPaths(driverSysPath string) []string { + return sysfs.GetSysPaths(driverSysPath) +} diff --git a/megaraid/storcli.go b/megaraid/storcli.go index d6f9deb..7b9cd61 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -13,6 +13,7 @@ import ( "github.com/patrickmn/go-cache" "machinerun.io/disko" + "machinerun.io/disko/linux/sysfs" ) type storCli struct { @@ -600,7 +601,7 @@ func CachingStorCli() MegaRaid { mr: &storCli{}, cache: cache.New(longTime, longTime), sysRoot: "/sys", - scsiTargetFn: readSCSITarget, + scsiTargetFn: sysfs.ReadSCSITarget, } } diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index 4b662c5..831c1b3 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -11,6 +11,7 @@ import ( "github.com/pkg/errors" "machinerun.io/disko" + "machinerun.io/disko/linux/sysfs" ) // parse JSON from 'storcli2 show nolog J' for List() method @@ -530,7 +531,7 @@ type storCli2 struct { func StorCli2() Mpi3mr { return &storCli2{ sysRoot: "/sys", - scsiTargetFn: readSCSITarget, + scsiTargetFn: sysfs.ReadSCSITarget, } } diff --git a/mpi3mr/storcli2_test.go b/mpi3mr/storcli2_test.go index db1d993..9ea423d 100644 --- a/mpi3mr/storcli2_test.go +++ b/mpi3mr/storcli2_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "machinerun.io/disko" + "machinerun.io/disko/linux/sysfs" ) var showNoLogJData = ` @@ -559,7 +560,7 @@ func TestStorCli2JBODDiskTypeLookupError(t *testing.T) { // Empty udev Name short-circuits before any sysfs call. func TestStorCli2JBODDiskTypeEmptyName(t *testing.T) { - sc := &storCli2{sysRoot: "/unused", scsiTargetFn: readSCSITarget} + sc := &storCli2{sysRoot: "/unused", scsiTargetFn: sysfs.ReadSCSITarget} if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{}); ok { t.Errorf("expected ok=false when udev Name is empty") From 7952c568fdf5fa3f1dfcefc2a7690e6d1d1a3ebe Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Fri, 24 Apr 2026 17:46:12 +0000 Subject: [PATCH 4/8] megaraid: classify JBOD drives by serial number Replace the SCSI-target/Drive.DID pairing used by cachingStorCli with serial-number matching, aligning megaraid with the smartpqi driver. Query issues a best-effort `storcli /cN/eall/sall show all` whose output is parsed into (EID, Slot) -> SN and stamped onto the new Drive.SerialNumber field. jbodDiskTypeFromSerial then matches udev ID_SERIAL_SHORT/ID_SERIAL against it and maps Drive.MediaType (HDD/SSD/new NVME) to disko.DiskType; sysRoot/scsiTargetFn and the linux/sysfs import are dropped. Tests cover the new parser, SN propagation through newController, and the serial-based GetDiskType paths. Signed-off-by: Andrei Aaron --- megaraid/megaraid.go | 20 +-- megaraid/storcli.go | 165 +++++++++++++++++++------ megaraid/storcli_test.go | 259 ++++++++++++++++++++++++++++++++------- smartpqi/arcconf.go | 8 +- smartpqi/arcconf_test.go | 35 ++++++ 5 files changed, 398 insertions(+), 89 deletions(-) diff --git a/megaraid/megaraid.go b/megaraid/megaraid.go index 99a5998..bdf526b 100644 --- a/megaraid/megaraid.go +++ b/megaraid/megaraid.go @@ -85,14 +85,15 @@ func (dgs DriveGroupSet) MarshalJSON() ([]byte, error) { // Drive - a megaraid (physical) Drive. type Drive struct { - ID int - DriveGroup int - EID int - Slot int - State string - MediaType MediaType - Model string - Raw map[string]string + ID int + DriveGroup int + EID int + Slot int + State string + MediaType MediaType + Model string + SerialNumber string + Raw map[string]string } // DriveSet - just a map of Drives by ID @@ -118,6 +119,9 @@ const ( // SSD - Solid State Disk SSD + + // NVME - Non-Volatile Memory Express + NVME ) func (t MediaType) String() string { diff --git a/megaraid/storcli.go b/megaraid/storcli.go index 7b9cd61..2a1c3b5 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -13,7 +13,6 @@ import ( "github.com/patrickmn/go-cache" "machinerun.io/disko" - "machinerun.io/disko/linux/sysfs" ) type storCli struct { @@ -46,10 +45,9 @@ type scResultSection struct { } func (sc *storCli) Query(cID int) (Controller, error) { - // run /c0 show all - // - get PDs and VDs - // run /c0/vall show all - // - populate VD Properties and Path + // run /cN show - get PDs and VDs + // run /cN/vall show all - populate VD Properties and Path + // run /cN/eall/sall show all - populate Drive.SerialNumber (best-effort) var stdout, stderr []byte var rc int @@ -74,7 +72,16 @@ func (sc *storCli) Query(cID int) (Controller, error) { cxVxOut := string(stdout) - return newController(cID, cxDxOut, cxVxOut) + // Best-effort: an error here leaves Drive.SerialNumber empty, which + // disables JBOD matching but does not break VD classification. + args = []string{fmt.Sprintf("/c%d/eall/sall", cID), "show", "all", "nolog"} + stdout, _, rc = storcli(args...) + cxEallSallOut := "" + if rc == 0 { + cxEallSallOut = string(stdout) + } + + return newController(cID, cxDxOut, cxVxOut, cxEallSallOut) } func (sc *storCli) DriverSysfsPath() string { @@ -85,7 +92,7 @@ func (sc *storCli) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskTy return disko.HDD, fmt.Errorf("missing controller to run query") } -func newController(cID int, cxDxOut string, cxVxOut string) (Controller, error) { +func newController(cID int, cxDxOut, cxVxOut, cxEallSallOut string) (Controller, error) { const pathPropName = "OS Drive Name" ctrl := Controller{ @@ -113,6 +120,15 @@ func newController(cID int, cxDxOut string, cxVxOut string) (Controller, error) ctrl.VirtDrives[vID].Path = vProps[pathPropName] } + // Best-effort: ignore parse errors; drives just keep empty SNs. + if serials, sErr := parseDriveSerials(cxEallSallOut); sErr == nil { + for _, drive := range pds { + if sn, ok := serials[driveKey{EID: drive.EID, Slot: drive.Slot}]; ok { + drive.SerialNumber = sn + } + } + } + for diskID, drive := range pds { dgID := drive.DriveGroup if dgID < 0 { @@ -131,9 +147,6 @@ func newController(cID int, cxDxOut string, cxVxOut string) (Controller, error) } } - // fmt.Printf("ctrl: %#v\n", propMap) - // fmt.Printf("ctrl: %#v\n", ctrl) - return ctrl, nil } @@ -448,6 +461,71 @@ func parseVirtProperties(cmdOut string) (map[int](map[string]string), error) { return vdmap, nil } +// driveKey identifies a physical drive by (enclosure, slot) for +// cross-referencing parsed 'storcli /cN' and '/cN/eall/sall' outputs. +type driveKey struct { + EID int + Slot int +} + +// parseDriveSerials extracts (EID, Slot) -> SN from the output of +// 'storcli /cN/eall/sall show all'. Returns an empty map (with nil error) +// for empty input, so callers can feed in a best-effort string. +func parseDriveSerials(cmdOut string) (map[driveKey]string, error) { + out := map[driveKey]string{} + + if strings.TrimSpace(cmdOut) == "" { + return out, nil + } + + // Surface Success/Failure/Unsupported via the standard header block. + for _, sect := range loadSections(cmdOut) { + if sect.Type == rsHeader { + if err := getHeaderError(parseKeyValData(sect.Lines)); err != nil { + return out, err + } + break + } + } + + // Any line starting with "Drive /cX/eY/sZ " updates the current + // drive context (covers the summary, state, attributes, etc. + // sub-sections). The drive's SN appears later in the attributes + // sub-section. + driveHdr := regexp.MustCompile(`^Drive /c\d+/e(\d+)/s(\d+)\b`) + snLine := regexp.MustCompile(`^SN\s*=\s*(.*\S)\s*$`) + + var cur *driveKey + + for _, line := range strings.Split(cmdOut, "\n") { + if m := driveHdr.FindStringSubmatch(line); m != nil { + eid, err := strconv.Atoi(m[1]) + if err != nil { + cur = nil + continue + } + slot, err := strconv.Atoi(m[2]) + if err != nil { + cur = nil + continue + } + k := driveKey{EID: eid, Slot: slot} + cur = &k + continue + } + + if cur == nil { + continue + } + + if m := snLine.FindStringSubmatch(line); m != nil { + out[*cur] = m[1] + } + } + + return out, nil +} + func parseIntOrDash(field string) (int, error) { if field == "-" { return -1, nil @@ -587,10 +665,6 @@ func getCommandErrorRCDefault(err error, rcError int) int { type cachingStorCli struct { mr MegaRaid cache *cache.Cache - // sysRoot: sysfs root for JBOD SCSI-target lookups ("/sys" in prod). - sysRoot string - // scsiTargetFn: kname -> SCSI target ID. Overridable for tests. - scsiTargetFn func(sysRoot, kname string) (int, bool, error) } // CachingStorCli - just a cache for a MegaRaid @@ -598,10 +672,8 @@ func CachingStorCli() MegaRaid { const longTime = 5 * time.Minute return &cachingStorCli{ - mr: &storCli{}, - cache: cache.New(longTime, longTime), - sysRoot: "/sys", - scsiTargetFn: sysfs.ReadSCSITarget, + mr: &storCli{}, + cache: cache.New(longTime, longTime), } } @@ -646,9 +718,10 @@ func (csc *cachingStorCli) GetDiskType(path string, udInfo disko.UdevInfo) (disk } } - // No VD matched path. Try JBOD/passthrough by matching SCSI target - // against Drive.DID; fall through with the sentinel on any failure. - if dType, ok := csc.jbodDiskTypeFromSCSI(ctrl, udInfo); ok { + // No VD matched path. Try JBOD/passthrough by matching udev serial + // against Drive.SerialNumber; fall through with the sentinel on any + // failure. + if dType, ok := jbodDiskTypeFromSerial(ctrl, udInfo); ok { return dType, nil } @@ -665,27 +738,25 @@ func isSoftStorCliErr(err error) bool { errors.Is(err, ErrUnsupported) } -// jbodDiskTypeFromSCSI matches the Linux SCSI target T against Drive.DID -// in the PD LIST. Returns ok=false on missing kname, non-SCSI device, DID -// collision, or UnknownMedia. -func (csc *cachingStorCli) jbodDiskTypeFromSCSI(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { - if csc.scsiTargetFn == nil { - return disko.HDD, false - } - - kname := udInfo.Name - if kname == "" { - return disko.HDD, false - } - - target, ok, err := csc.scsiTargetFn(csc.sysRoot, kname) - if err != nil || !ok { +// jbodDiskTypeFromSerial matches a udev serial against Drive.SerialNumber +// across the controller's Drives. Returns ok=false on missing udev serial, +// no match, collision, or unknown media. +func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { + serials := collectUdevSerials(udInfo) + if len(serials) == 0 { return disko.HDD, false } var matches []*Drive for _, d := range ctrl.Drives { - if d != nil && d.ID == target { + if d == nil { + continue + } + sn := strings.TrimSpace(d.SerialNumber) + if sn == "" { + continue + } + if _, ok := serials[sn]; ok { matches = append(matches, d) } } @@ -699,6 +770,8 @@ func (csc *cachingStorCli) jbodDiskTypeFromSCSI(ctrl Controller, udInfo disko.Ud return disko.SSD, true case HDD: return disko.HDD, true + case NVME: + return disko.NVME, true case UnknownMedia: return disko.HDD, false } @@ -706,6 +779,24 @@ func (csc *cachingStorCli) jbodDiskTypeFromSCSI(ctrl Controller, udInfo disko.Ud return disko.HDD, false } +// collectUdevSerials returns the udev tokens to match against +// Drive.SerialNumber. storcli reports the SCSI INQUIRY page-80 serial, +// which udev exposes as ID_SCSI_SERIAL; that's the primary key. +// ID_SERIAL_SHORT and ID_SERIAL are WWN-derived on most SAS/SATA drives, +// but they cover drives that don't expose a distinct VPD page-80 serial. +func collectUdevSerials(udInfo disko.UdevInfo) map[string]struct{} { + out := map[string]struct{}{} + + for _, key := range []string{"ID_SCSI_SERIAL", "ID_SERIAL_SHORT", "ID_SERIAL"} { + v := strings.TrimSpace(udInfo.Properties[key]) + if v != "" { + out[v] = struct{}{} + } + } + + return out +} + func (csc *cachingStorCli) DriverSysfsPath() string { return csc.mr.DriverSysfsPath() } diff --git a/megaraid/storcli_test.go b/megaraid/storcli_test.go index b5e8c66..9582d04 100644 --- a/megaraid/storcli_test.go +++ b/megaraid/storcli_test.go @@ -210,7 +210,7 @@ func TestNewController(t *testing.T) { var exVlen, exPlen, exDGlen = 5, 5, 5 var cID = 0 - ctrl, err := newController(cID, sys0CxShow, sys0CxVallShowAll) + ctrl, err := newController(cID, sys0CxShow, sys0CxVallShowAll, "") if err != nil { t.Fatalf("newController failed: %s", err) } @@ -1282,6 +1282,98 @@ Status = Success Description = No VD's have been configured. ` +// Output of 'storcli /c0/eall/sall show all' for the same JBOD setup. +// Abridged: per drive we keep the summary row and the Device attributes +// sub-section that carries "SN = ...". Serial numbers are synthetic +// (SNTEST00N) so this fixture contains no data from real systems. +var ciscoJBODCxEallSallShowAll = ` +CLI Version = 007.1907.0000.0000 Sep 13, 2021 +Operating system = Linux 6.6.0 +Controller = 0 +Status = Success +Description = Show Drive Information Succeeded. + + +Drive /c0/e134/s2 : +================= + +------------------------------------------------------------------------------ +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +------------------------------------------------------------------------------ +134:2 1 JBOD - 372.611 GB SAS SSD N N 512B TESTMODELSSD1 U - +------------------------------------------------------------------------------ + + +Drive /c0/e134/s2 Device attributes : +=================================== +SN = SNTEST001 +Model Number = TESTMODELSSD1 + + +Drive /c0/e134/s3 : +================= + +---------------------------------------------------------------------------- +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +---------------------------------------------------------------------------- +134:3 0 JBOD - 2.182 TB SAS HDD N N 4 KB TESTMODELHDD1 U - +---------------------------------------------------------------------------- + + +Drive /c0/e134/s3 Device attributes : +=================================== +SN = SNTEST002 +Model Number = TESTMODELHDD1 + + +Drive /c0/e134/s4 : +================= + +---------------------------------------------------------------------------- +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +---------------------------------------------------------------------------- +134:4 3 JBOD - 2.182 TB SAS HDD N N 4 KB TESTMODELHDD1 U - +---------------------------------------------------------------------------- + + +Drive /c0/e134/s4 Device attributes : +=================================== +SN = SNTEST003 +Model Number = TESTMODELHDD1 + + +Drive /c0/e134/s5 : +================= + +---------------------------------------------------------------------------- +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +---------------------------------------------------------------------------- +134:5 2 JBOD - 2.182 TB SAS HDD N N 4 KB TESTMODELHDD1 U - +---------------------------------------------------------------------------- + + +Drive /c0/e134/s5 Device attributes : +=================================== +SN = SNTEST004 +Model Number = TESTMODELHDD1 + + +Drive /c0/e134/s6 : +================= + +---------------------------------------------------------------------------- +EID:Slt DID State DG Size Intf Med SED PI SeSz Model Sp Type +---------------------------------------------------------------------------- +134:6 4 JBOD - 2.182 TB SAS HDD N N 4 KB TESTMODELHDD1 U - +---------------------------------------------------------------------------- + + +Drive /c0/e134/s6 Device attributes : +=================================== +SN = SNTEST005 +Model Number = TESTMODELHDD1 +` + // this has a 'F' for a DriveGroup (foreign) // it is put together from old '/c0/dall show' output to // look like '/c0 show all' would. @@ -1456,7 +1548,8 @@ func TestParseVirtPropertiesCiscoJBOD(t *testing.T) { } func TestNewControllerCiscoJBOD(t *testing.T) { - ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, + ciscoJBODCxEallSallShowAll) if err != nil { t.Fatalf("newController failed: %s", err) } @@ -1476,6 +1569,52 @@ func TestNewControllerCiscoJBOD(t *testing.T) { if len(ctrl.DriveGroups) != 0 { t.Errorf("expected 0 DriveGroups (JBOD has no drive groups), got %d", len(ctrl.DriveGroups)) } + + // Drives indexed by DID. Slot 2 is the SSD (DID=1); slot 3 is HDD (DID=0). + wantSerials := map[int]string{ + 1: "SNTEST001", // 134:2 SSD + 0: "SNTEST002", // 134:3 HDD + 3: "SNTEST003", // 134:4 + 2: "SNTEST004", // 134:5 + 4: "SNTEST005", // 134:6 + } + for did, want := range wantSerials { + d := ctrl.Drives[did] + if d == nil { + t.Fatalf("drive DID=%d missing", did) + } + if d.SerialNumber != want { + t.Errorf("drive DID=%d: SerialNumber=%q, want %q", did, d.SerialNumber, want) + } + } +} + +func TestParseDriveSerialsCiscoJBOD(t *testing.T) { + got, err := parseDriveSerials(ciscoJBODCxEallSallShowAll) + if err != nil { + t.Fatalf("parseDriveSerials returned error: %s", err) + } + + want := map[driveKey]string{ + {EID: 134, Slot: 2}: "SNTEST001", + {EID: 134, Slot: 3}: "SNTEST002", + {EID: 134, Slot: 4}: "SNTEST003", + {EID: 134, Slot: 5}: "SNTEST004", + {EID: 134, Slot: 6}: "SNTEST005", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("parseDriveSerials mismatch:\n got=%v\nwant=%v", got, want) + } +} + +func TestParseDriveSerialsEmpty(t *testing.T) { + got, err := parseDriveSerials("") + if err != nil { + t.Fatalf("parseDriveSerials(\"\") returned error: %s", err) + } + if len(got) != 0 { + t.Errorf("expected empty map, got %v", got) + } } type mockMegaRaid struct { @@ -1496,7 +1635,8 @@ func (m *mockMegaRaid) DriverSysfsPath() string { } func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { - ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, + ciscoJBODCxEallSallShowAll) if err != nil { t.Fatalf("newController failed: %s", err) } @@ -1506,6 +1646,7 @@ func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { cache: cache.New(5*time.Minute, 5*time.Minute), } + // No udev serial => JBOD matching fails => sentinel for udev fallback. for _, path := range []string{"/dev/sda", "/dev/sdb", "/dev/sdc"} { dtype, err := csc.GetDiskType(path, disko.UdevInfo{}) if !errors.Is(err, disko.ErrDiskTypeUndetermined) { @@ -1549,34 +1690,30 @@ func TestGetDiskTypeHardQueryFailurePropagates(t *testing.T) { } } -// newJBODCachingStorCli wires the Cisco JBOD fixture to a stub scsiTargetFn -// that returns (target, ok) for kname "sdb". -func newJBODCachingStorCli(t *testing.T, target int, ok bool) *cachingStorCli { +// newJBODCachingStorCli wires the Cisco JBOD fixture (with serials from +// the /eall/sall query) to a cachingStorCli for serial-based matching. +func newJBODCachingStorCli(t *testing.T) *cachingStorCli { t.Helper() - ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll) + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, + ciscoJBODCxEallSallShowAll) if err != nil { t.Fatalf("newController failed: %s", err) } return &cachingStorCli{ - mr: &mockMegaRaid{ctrl: ctrl, err: nil}, - cache: cache.New(5*time.Minute, 5*time.Minute), - sysRoot: "/unused-in-test", - scsiTargetFn: func(_, kname string) (int, bool, error) { - if kname == "sdb" { - return target, ok, nil - } - return 0, false, nil - }, + mr: &mockMegaRaid{ctrl: ctrl, err: nil}, + cache: cache.New(5*time.Minute, 5*time.Minute), } } -// DID=1 is the SSD in the fixture. -func TestGetDiskTypeJBODSCSITargetMatchSSD(t *testing.T) { - csc := newJBODCachingStorCli(t, 1, true) +// SSD serial (slot 2) resolves via ID_SERIAL_SHORT. +func TestGetDiskTypeJBODSerialMatchSSD(t *testing.T) { + csc := newJBODCachingStorCli(t) - ud := disko.UdevInfo{Name: "sdb"} + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST001"}, + } dtype, err := csc.GetDiskType("/dev/sdb", ud) if err != nil { t.Fatalf("expected nil err for SSD JBOD match, got %v", err) @@ -1586,11 +1723,13 @@ func TestGetDiskTypeJBODSCSITargetMatchSSD(t *testing.T) { } } -// DID=0 is an HDD in the fixture. -func TestGetDiskTypeJBODSCSITargetMatchHDD(t *testing.T) { - csc := newJBODCachingStorCli(t, 0, true) +// HDD serial (slot 3) resolves via ID_SERIAL_SHORT. +func TestGetDiskTypeJBODSerialMatchHDD(t *testing.T) { + csc := newJBODCachingStorCli(t) - ud := disko.UdevInfo{Name: "sdb"} + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST002"}, + } dtype, err := csc.GetDiskType("/dev/sdb", ud) if err != nil { t.Fatalf("expected nil err for HDD JBOD match, got %v", err) @@ -1600,35 +1739,73 @@ func TestGetDiskTypeJBODSCSITargetMatchHDD(t *testing.T) { } } -// DID not in the fixture falls through to ErrDiskTypeUndetermined. -func TestGetDiskTypeJBODSCSITargetNoMatch(t *testing.T) { - csc := newJBODCachingStorCli(t, 42, true) +// ID_SERIAL alone (no ID_SERIAL_SHORT) still matches when it equals the SN. +func TestGetDiskTypeJBODSerialViaIDSerialFallback(t *testing.T) { + csc := newJBODCachingStorCli(t) - ud := disko.UdevInfo{Name: "sdb"} - _, err := csc.GetDiskType("/dev/sdb", ud) - if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined for unknown DID, got %v", err) + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL": "SNTEST001"}, + } + dtype, err := csc.GetDiskType("/dev/sdb", ud) + if err != nil { + t.Fatalf("expected nil err for ID_SERIAL fallback match, got %v", err) + } + if dtype != disko.SSD { + t.Errorf("GetDiskType: got %v, want SSD", dtype) } } -// Unresolvable sysfs H:C:T:L (NVMe, virtio, missing symlink). -func TestGetDiskTypeJBODSCSITargetUnresolved(t *testing.T) { - csc := newJBODCachingStorCli(t, 0, false) +func TestGetDiskTypeJBODSerialViaIDSCSISerial(t *testing.T) { + csc := newJBODCachingStorCli(t) - ud := disko.UdevInfo{Name: "sdb"} + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SCSI_SERIAL": "SNTEST001", + "ID_SERIAL_SHORT": "deadbeefcafef001", + "ID_SERIAL": "3deadbeefcafef001", + }, + } + dtype, err := csc.GetDiskType("/dev/sdb", ud) + if err != nil { + t.Fatalf("expected nil err for ID_SCSI_SERIAL match, got %v", err) + } + if dtype != disko.SSD { + t.Errorf("GetDiskType: got %v, want SSD", dtype) + } +} + +// Serial not in fixture falls through to ErrDiskTypeUndetermined. +func TestGetDiskTypeJBODSerialNoMatch(t *testing.T) { + csc := newJBODCachingStorCli(t) + + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "NOSUCHSERIAL"}, + } _, err := csc.GetDiskType("/dev/sdb", ud) if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined when SCSI target is unresolved, got %v", err) + t.Errorf("expected ErrDiskTypeUndetermined for unknown serial, got %v", err) } } -// Empty udev Name: sysfs helper has no kname to resolve. -func TestGetDiskTypeJBODNoUdevName(t *testing.T) { - csc := newJBODCachingStorCli(t, 1, true) +// When the /eall/sall detail query was unavailable (empty string), drives +// carry no SerialNumber and JBOD matching correctly fails over to udev. +func TestGetDiskTypeJBODEmptyDetailOutputFallsThrough(t *testing.T) { + ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, "") + if err != nil { + t.Fatalf("newController failed: %s", err) + } + + csc := &cachingStorCli{ + mr: &mockMegaRaid{ctrl: ctrl, err: nil}, + cache: cache.New(5*time.Minute, 5*time.Minute), + } - _, err := csc.GetDiskType("/dev/sdb", disko.UdevInfo{}) + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST001"}, + } + _, err = csc.GetDiskType("/dev/sdb", ud) if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined with empty udev Name, got %v", err) + t.Errorf("expected ErrDiskTypeUndetermined with no serials, got %v", err) } } diff --git a/smartpqi/arcconf.go b/smartpqi/arcconf.go index 445f475..805643e 100644 --- a/smartpqi/arcconf.go +++ b/smartpqi/arcconf.go @@ -590,12 +590,14 @@ func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.Di } // collectUdevSerials returns the udev tokens to match against -// PhysicalDevice.SerialNumber: the raw ID_SERIAL_SHORT and, as fallback, -// the longer "__" ID_SERIAL. +// PhysicalDevice.SerialNumber. arcconf reports the SCSI INQUIRY page-80 +// serial, which udev exposes as ID_SCSI_SERIAL; that's the primary key. +// ID_SERIAL_SHORT and ID_SERIAL are WWN-derived on most SAS/SATA drives, +// but they cover drives that don't expose a distinct VPD page-80 serial. func collectUdevSerials(udInfo disko.UdevInfo) map[string]struct{} { out := map[string]struct{}{} - for _, key := range []string{"ID_SERIAL_SHORT", "ID_SERIAL"} { + for _, key := range []string{"ID_SCSI_SERIAL", "ID_SERIAL_SHORT", "ID_SERIAL"} { v := strings.TrimSpace(udInfo.Properties[key]) if v != "" { out[v] = struct{}{} diff --git a/smartpqi/arcconf_test.go b/smartpqi/arcconf_test.go index 495bc55..cb5841e 100644 --- a/smartpqi/arcconf_test.go +++ b/smartpqi/arcconf_test.go @@ -1734,6 +1734,41 @@ func TestCollectUdevSerialsPrefersBoth(t *testing.T) { } } +func TestCollectUdevSerialsIncludesSCSISerial(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SCSI_SERIAL": "TESTSN0PQI01", + "ID_SERIAL_SHORT": "deadbeefcafef001", + "ID_SERIAL": "3deadbeefcafef001", + }, + } + + got := collectUdevSerials(ud) + for _, want := range []string{ + "TESTSN0PQI01", + "deadbeefcafef001", + "3deadbeefcafef001", + } { + if _, ok := got[want]; !ok { + t.Errorf("collectUdevSerials: missing token %q", want) + } + } +} + +func TestJBODDiskTypeSerialViaIDSCSISerial(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{"ID_SCSI_SERIAL": "SN-HDD-1"}, + } + + dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + if !ok { + t.Fatalf("expected ok=true when ID_SCSI_SERIAL matches") + } + if dType != disko.HDD { + t.Errorf("jbodDiskTypeFromSerial: got %v, want HDD", dType) + } +} + // isSoftArcconfErr must recognise the three soft sentinels whether bare // or wrapped with fmt.Errorf("%w"), so wrapped per-controller errors are // still routed through the udev fallback instead of propagating as fatal. From 1b598cfe08a22380ee55f859ced8c278c43c76d9 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Fri, 24 Apr 2026 18:42:00 +0000 Subject: [PATCH 5/8] disko: add internal-only disko.Unknown for RAID driver error paths Drivers previously returned disko.HDD as a placeholder on every error path, which is indistinguishable from a real HDD classification. Add a new DiskType, Unknown, reserved for error returns. - disk.go: new Unknown constant, String()/StringToDiskType updated; default unknown-string fallback remains HDD (public contract intact). - megaraid/storcli.go, mpi3mr/storcli2.go, smartpqi/arcconf.go: all GetDiskType error returns switch HDD -> Unknown. arcconf.go also fixes one %s -> %w in the top-level wrap so errors.Is works consistently across drivers. - linux/system.go: resolveDiskType returns Unknown on error, and adds a defense-in-depth guard that rejects a (Unknown, nil) driver return with a hard error so Unknown never reaches disko.Disk.Type. - Tests: update driver and system mocks to match the new contract and add a case for the new leak guard. Unknown is paired with a non-nil error in every return path and is never written to disko.Disk.Type, so no downstream consumer (storage, bootmgr, spm, bootstrap, configmgr) needs any change. Signed-off-by: Andrei Aaron --- disk.go | 19 ++++++++++++++----- linux/system.go | 14 ++++++++++++-- linux/system_test.go | 31 +++++++++++++++++++++++++++---- megaraid/storcli.go | 16 ++++++++-------- megaraid/storcli_test.go | 4 ++-- mpi3mr/storcli2.go | 18 +++++++++--------- smartpqi/arcconf.go | 14 +++++++------- 7 files changed, 79 insertions(+), 37 deletions(-) diff --git a/disk.go b/disk.go index 46ba545..fd969d6 100644 --- a/disk.go +++ b/disk.go @@ -33,19 +33,28 @@ const ( // TYPEFILE - A file on disk, not a block device. TYPEFILE + + // Unknown is an internal disko placeholder returned alongside a + // non-nil error by RAID-controller drivers when they cannot classify + // a device. It MUST NOT leak out of disko onto disko.Disk.Type: the + // linux system layer either consumes it via udev fallback or + // propagates the accompanying error. External callers should never + // observe this value. + Unknown ) func (t DiskType) String() string { - return []string{"HDD", "SSD", "NVME", "FILE"}[t] + return []string{"HDD", "SSD", "NVME", "FILE", "UNKNOWN"}[t] } // StringToDiskType - convert a string to a disk type. func StringToDiskType(typeStr string) DiskType { kmap := map[string]DiskType{ - "HDD": HDD, - "SSD": SSD, - "NVME": NVME, - "FILE": TYPEFILE, + "HDD": HDD, + "SSD": SSD, + "NVME": NVME, + "FILE": TYPEFILE, + "UNKNOWN": Unknown, } if dtype, ok := kmap[typeStr]; ok { return dtype diff --git a/linux/system.go b/linux/system.go index b535b01..28938ae 100644 --- a/linux/system.go +++ b/linux/system.go @@ -313,7 +313,17 @@ func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) break } - return disko.HDD, false, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", devicePath, err) + return disko.Unknown, false, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", devicePath, err) + } + + // Defense in depth: disko.Unknown is an internal-only + // placeholder paired with a non-nil error. If a driver returns + // it with err == nil it has broken contract; surface as hard + // error rather than silently leak Unknown into disko.Disk.Type. + if dType == disko.Unknown { + return disko.Unknown, false, fmt.Errorf( + "RAID controller returned disko.Unknown with nil error for %q; driver contract violated", + devicePath) } return dType, true, nil @@ -321,7 +331,7 @@ func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) dType, err := getDiskType(udInfo) if err != nil { - return disko.HDD, false, err + return disko.Unknown, false, err } return dType, false, nil diff --git a/linux/system_test.go b/linux/system_test.go index d0e190d..02a325b 100644 --- a/linux/system_test.go +++ b/linux/system_test.go @@ -320,7 +320,7 @@ func TestGetDiskTypeRAIDMatchSSD(t *testing.T) { func TestGetDiskTypeJBODFallback(t *testing.T) { ast := assert.New(t) mock := &mockRAIDController{ - diskType: disko.HDD, + diskType: disko.Unknown, err: disko.ErrDiskTypeUndetermined, sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, @@ -348,7 +348,7 @@ func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { wrappedErr := fmt.Errorf("controller 0: %w", disko.ErrDiskTypeUndetermined) mock := &mockRAIDController{ - diskType: disko.HDD, + diskType: disko.Unknown, err: wrappedErr, sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, @@ -364,7 +364,7 @@ func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { func TestGetDiskTypeRAIDRealError(t *testing.T) { ast := assert.New(t) mock := &mockRAIDController{ - diskType: disko.HDD, + diskType: disko.Unknown, err: fmt.Errorf("storcli binary crashed"), sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, @@ -401,7 +401,7 @@ func TestGetDiskTypeMultiControllerJBODFallback(t *testing.T) { ast := assert.New(t) megaraidMock := &mockRAIDController{ - diskType: disko.HDD, + diskType: disko.Unknown, err: disko.ErrDiskTypeUndetermined, sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", isSysPathRAID: true, @@ -584,3 +584,26 @@ func TestResolveDiskType_NoControllersConfiguredUsesUdev(t *testing.T) { ast.False(onRAID) ast.Equal(disko.NVME, dType) } + +// disko.Unknown is reserved for error paths; a driver that returns it +// with err == nil has violated the contract. resolveDiskType must catch +// this and surface a hard error rather than let Unknown reach callers. +func TestResolveDiskType_UnknownWithNilErrorIsRejected(t *testing.T) { + ast := assert.New(t) + mock := &mockRAIDController{ + diskType: disko.Unknown, + err: nil, + sysfsPath: "/sys/bus/pci/drivers/megaraid_sas", + isSysPathRAID: true, + } + + ls := newTestLinuxSystem(mock) + + dType, onRAID, err := ls.resolveDiskType("/dev/sda", + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + ast.Error(err) + ast.Contains(err.Error(), "disko.Unknown") + ast.Contains(err.Error(), "contract violated") + ast.False(onRAID) + ast.Equal(disko.Unknown, dType) +} diff --git a/megaraid/storcli.go b/megaraid/storcli.go index 2a1c3b5..411778e 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -89,7 +89,7 @@ func (sc *storCli) DriverSysfsPath() string { } func (sc *storCli) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { - return disko.HDD, fmt.Errorf("missing controller to run query") + return disko.Unknown, fmt.Errorf("missing controller to run query") } func newController(cID int, cxDxOut, cxVxOut, cxEallSallOut string) (Controller, error) { @@ -703,9 +703,9 @@ func (csc *cachingStorCli) GetDiskType(path string, udInfo disko.UdevInfo) (disk if isSoftStorCliErr(err) { // Controller tool unavailable or no controller. Fall // through with the sentinel so the caller uses udev. - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } - return disko.HDD, err + return disko.Unknown, err } for _, vd := range ctrl.VirtDrives { @@ -725,7 +725,7 @@ func (csc *cachingStorCli) GetDiskType(path string, udInfo disko.UdevInfo) (disk return dType, nil } - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } // isSoftStorCliErr returns true when err indicates that storcli simply @@ -744,7 +744,7 @@ func isSoftStorCliErr(err error) bool { func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { serials := collectUdevSerials(udInfo) if len(serials) == 0 { - return disko.HDD, false + return disko.Unknown, false } var matches []*Drive @@ -762,7 +762,7 @@ func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskT } if len(matches) != 1 { - return disko.HDD, false + return disko.Unknown, false } switch matches[0].MediaType { @@ -773,10 +773,10 @@ func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskT case NVME: return disko.NVME, true case UnknownMedia: - return disko.HDD, false + return disko.Unknown, false } - return disko.HDD, false + return disko.Unknown, false } // collectUdevSerials returns the udev tokens to match against diff --git a/megaraid/storcli_test.go b/megaraid/storcli_test.go index 9582d04..5308a44 100644 --- a/megaraid/storcli_test.go +++ b/megaraid/storcli_test.go @@ -1653,8 +1653,8 @@ func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { t.Errorf("GetDiskType(%q): expected ErrDiskTypeUndetermined, got %v", path, err) } - if dtype != disko.HDD { - t.Errorf("GetDiskType(%q): expected HDD default, got %d", path, dtype) + if dtype != disko.Unknown { + t.Errorf("GetDiskType(%q): expected disko.Unknown placeholder, got %d", path, dtype) } } } diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index 831c1b3..a1b5f01 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -608,9 +608,9 @@ func (sc *storCli2) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskT cIDs, err := sc.List() if err != nil { if isSoftStorCli2Err(err) { - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } - return disko.HDD, errors.Errorf("failed to get controller list: %s", err) + return disko.Unknown, errors.Errorf("failed to get controller list: %s", err) } var queryErrs []error @@ -638,7 +638,7 @@ func (sc *storCli2) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskT for _, err := range queryErrs { if !isSoftStorCli2Err(err) { - return disko.HDD, err + return disko.Unknown, err } } @@ -649,7 +649,7 @@ func (sc *storCli2) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskT return dType, nil } - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } // isSoftStorCli2Err returns true when err indicates that storcli2 simply @@ -667,17 +667,17 @@ func isSoftStorCli2Err(err error) bool { // missing kname, non-SCSI device, PID collision, or unknown medium. func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { if sc.scsiTargetFn == nil { - return disko.HDD, false + return disko.Unknown, false } kname := udInfo.Name if kname == "" { - return disko.HDD, false + return disko.Unknown, false } target, ok, err := sc.scsiTargetFn(sc.sysRoot, kname) if err != nil || !ok { - return disko.HDD, false + return disko.Unknown, false } var matches []PhysicalDrive @@ -690,7 +690,7 @@ func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevIn } if len(matches) != 1 { - return disko.HDD, false + return disko.Unknown, false } switch strings.ToUpper(strings.TrimSpace(matches[0].Medium)) { @@ -700,5 +700,5 @@ func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevIn return disko.HDD, true } - return disko.HDD, false + return disko.Unknown, false } diff --git a/smartpqi/arcconf.go b/smartpqi/arcconf.go index 805643e..b03cd18 100644 --- a/smartpqi/arcconf.go +++ b/smartpqi/arcconf.go @@ -494,9 +494,9 @@ func (ac *arcConf) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskTy cIDs, err := ac.List() if err != nil { if isSoftArcconfErr(err) { - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } - return disko.HDD, fmt.Errorf("failed to enumerate controllers: %s", err) + return disko.Unknown, fmt.Errorf("failed to enumerate controllers: %w", err) } var queryErrs []error @@ -524,7 +524,7 @@ func (ac *arcConf) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskTy for _, err := range queryErrs { if !isSoftArcconfErr(err) { - return disko.HDD, err + return disko.Unknown, err } } @@ -535,7 +535,7 @@ func (ac *arcConf) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskTy return dType, nil } - return disko.HDD, disko.ErrDiskTypeUndetermined + return disko.Unknown, disko.ErrDiskTypeUndetermined } // isSoftArcconfErr returns true when err indicates that arcconf simply @@ -554,7 +554,7 @@ func isSoftArcconfErr(err error) bool { func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { serials := collectUdevSerials(udInfo) if len(serials) == 0 { - return disko.HDD, false + return disko.Unknown, false } var matches []*PhysicalDevice @@ -574,7 +574,7 @@ func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.Di } if len(matches) != 1 { - return disko.HDD, false + return disko.Unknown, false } switch matches[0].Type { @@ -586,7 +586,7 @@ func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.Di return disko.NVME, true } - return disko.HDD, false + return disko.Unknown, false } // collectUdevSerials returns the udev tokens to match against From 79132f02231ffed5a11081884e39b6930314d09d Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Mon, 27 Apr 2026 19:37:47 +0000 Subject: [PATCH 6/8] disko: implement review feedback - Hoist collectUdevSerials into UdevInfo.CollectSerials, dropping the duplicate copies in megaraid and smartpqi. - linux/sysfs.ReadSCSITarget: expand Host:Controller:Target:LUN doc with a symlink example, document the ID_SCSI=1 upstream-filter contract, rename internals, and return a real error on malformed link targets. - mpi3mr.jbodDiskTypeFromSCSI: short-circuit on ID_SCSI != "1" so virtio-blk, NVMe and ATA/SATA never trigger a sysfs lookup. - linux.resolveDiskType: trim the defense-in-depth comment and note the RAID-binding skip. - Migrate the tests to testify/assert and require; use realistic DEVPATHs in linux/system_test. Signed-off-by: Andrei Aaron --- disk.go | 24 ++++++ disk_test.go | 58 +++++++++++++ linux/sysfs/scsi.go | 44 ++++++---- linux/sysfs/scsi_test.go | 90 +++++++------------- linux/system.go | 9 +- linux/system_test.go | 32 +++---- megaraid/storcli.go | 20 +---- megaraid/storcli_test.go | 176 +++++++++++---------------------------- mpi3mr/storcli2.go | 15 +++- mpi3mr/storcli2_test.go | 89 +++++++++++++------- smartpqi/arcconf.go | 20 +---- smartpqi/arcconf_test.go | 107 +++++------------------- 12 files changed, 306 insertions(+), 378 deletions(-) diff --git a/disk.go b/disk.go index fd969d6..3e20d05 100644 --- a/disk.go +++ b/disk.go @@ -503,6 +503,30 @@ type UdevInfo struct { Properties map[string]string `json:"properties"` } +// CollectSerials returns the set of udev serial-style tokens that +// identify this device. Used by RAID drivers (megaraid, smartpqi) to +// correlate a Linux device with a controller-reported physical drive +// serial number. +// +// Controllers typically expose the SCSI INQUIRY page-80 serial, which +// udev surfaces as ID_SCSI_SERIAL on SCSI-class devices and is the +// primary key. ID_SERIAL_SHORT and ID_SERIAL are WWN-derived on most +// SAS/SATA drives but cover drives that don't expose a distinct VPD +// page-80 serial. All non-empty values are returned so callers can match +// any of them against a controller record. +func (u UdevInfo) CollectSerials() map[string]struct{} { + out := map[string]struct{}{} + + for _, key := range []string{"ID_SCSI_SERIAL", "ID_SERIAL_SHORT", "ID_SERIAL"} { + v := strings.TrimSpace(u.Properties[key]) + if v != "" { + out[v] = struct{}{} + } + } + + return out +} + // PartitionSet is a map of partition number to the partition. type PartitionSet map[uint]Partition diff --git a/disk_test.go b/disk_test.go index 4308bc2..b8c6889 100644 --- a/disk_test.go +++ b/disk_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" + "machinerun.io/disko" "machinerun.io/disko/partid" ) @@ -394,3 +396,59 @@ func TestMarshalProperties(t *testing.T) { } } } + +func TestUdevInfoCollectSerialsPrefersBoth(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SERIAL_SHORT": "short1", + "ID_SERIAL": "long_short1", + "ID_MODEL": "ignored", + }, + } + + got := ud.CollectSerials() + assert.Contains(t, got, "short1", "missing ID_SERIAL_SHORT token") + assert.Contains(t, got, "long_short1", "missing ID_SERIAL token") + assert.NotContains(t, got, "ignored", + "CollectSerials must not include unrelated properties") +} + +func TestUdevInfoCollectSerialsIncludesSCSISerial(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SCSI_SERIAL": "TESTSN0PQI01", + "ID_SERIAL_SHORT": "deadbeefcafef001", + "ID_SERIAL": "3deadbeefcafef001", + }, + } + + got := ud.CollectSerials() + for _, want := range []string{ + "TESTSN0PQI01", + "deadbeefcafef001", + "3deadbeefcafef001", + } { + assert.Contains(t, got, want, "CollectSerials: missing token %q", want) + } +} + +// Whitespace-only values must not be returned as serial tokens; otherwise +// a controller record with a blank SerialNumber could match every device. +func TestUdevInfoCollectSerialsTrimsWhitespace(t *testing.T) { + ud := disko.UdevInfo{ + Properties: map[string]string{ + "ID_SCSI_SERIAL": " ", + "ID_SERIAL_SHORT": "", + "ID_SERIAL": "real-serial", + }, + } + + got := ud.CollectSerials() + assert.Contains(t, got, "real-serial", "missing real serial token") + assert.Len(t, got, 1, "expected only the non-empty token") +} + +func TestUdevInfoCollectSerialsEmpty(t *testing.T) { + got := disko.UdevInfo{}.CollectSerials() + assert.Empty(t, got, "expected empty map for empty UdevInfo") +} diff --git a/linux/sysfs/scsi.go b/linux/sysfs/scsi.go index 3048ef7..92d9c7a 100644 --- a/linux/sysfs/scsi.go +++ b/linux/sysfs/scsi.go @@ -9,15 +9,24 @@ import ( "strings" ) -// ReadSCSITarget returns T from /sys/block//device -> H:C:T:L. -// For SCSI-attached JBOD/passthrough disks behind a RAID HBA, T matches -// the controller-reported drive ID (megaraid Drive.DID, mpi3mr -// PhysicalDrive.PID), which lets callers correlate a Linux block device -// with an entry in the controller's PD list. +// ReadSCSITarget returns Target from /sys/block//device -> +// Host:Controller:Target:LUN. For SCSI-attached JBOD/passthrough disks +// behind a RAID HBA, Target matches the controller-reported drive ID +// (megaraid Drive.DID, mpi3mr PhysicalDrive.PID), which lets callers +// correlate a Linux block device with an entry in the controller's PD list. // -// ok=false (nil err) for non-SCSI devices (no "device" symlink, link does -// not end in H:C:T:L, e.g. NVMe, virtio). sysRoot is injectable for tests; -// production passes "/sys". +// The "device" entry under a SCSI block device is a symlink into +// /sys/class/scsi_device/, e.g.: +// +// % ls -al /sys/block/sda/device +// lrwxrwxrwx 1 root root 0 Jan 31 16:11 /sys/block/sda/device -> ../../../2:0:0:0 +// +// Callers must only invoke ReadSCSITarget for devices that udev reports as +// SCSI (ID_SCSI=1); virtio-blk, NVMe, ATA/SATA, etc. do not expose this +// symlink and should be filtered upstream. ok=false with a nil error is +// reserved for benign cases (empty kname, missing "device" symlink); a +// malformed Host:Controller:Target:LUN link target is reported as an +// error. sysRoot is injectable for tests; production passes "/sys". func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { if kname == "" { return 0, false, nil @@ -33,17 +42,22 @@ func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { return 0, false, fmt.Errorf("readlink %q: %w", link, err) } - last := filepath.Base(dest) - parts := strings.Split(last, ":") + // Parse out the SCSI device id from the symlink. e.g. + // ../../../0:3:110:0 -> 0:3:110:0 + // matching an entry under /sys/class/scsi_device/. + scsiDeviceID := filepath.Base(dest) + hctlFields := strings.Split(scsiDeviceID, ":") - const hctlFields = 4 - if len(parts) != hctlFields { - return 0, false, nil + const requiredHCTLFields = 4 + if len(hctlFields) != requiredHCTLFields { + return 0, false, fmt.Errorf( + "invalid SCSI Host:Controller:Target:LUN value %q from %q: expected %d fields, got %d", + scsiDeviceID, link, requiredHCTLFields, len(hctlFields)) } - t, cerr := strconv.Atoi(parts[2]) + t, cerr := strconv.Atoi(hctlFields[2]) if cerr != nil { - return 0, false, fmt.Errorf("parse SCSI target from %q: %w", last, cerr) + return 0, false, fmt.Errorf("parse SCSI target from %q: %w", scsiDeviceID, cerr) } return t, true, nil diff --git a/linux/sysfs/scsi_test.go b/linux/sysfs/scsi_test.go index 68ef315..3584790 100644 --- a/linux/sysfs/scsi_test.go +++ b/linux/sysfs/scsi_test.go @@ -4,28 +4,26 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // makeBlockDeviceSymlink wires up a fake sysfs entry at -// /block//device pointing at ../../scsi_device/. +// /block//device pointing at +// ../../scsi_device/. func makeBlockDeviceSymlink(t *testing.T, root, kname, hctl string) { t.Helper() blockDir := filepath.Join(root, "block", kname) - if err := os.MkdirAll(blockDir, 0o755); err != nil { - t.Fatalf("mkdir %q: %s", blockDir, err) - } + require.NoError(t, os.MkdirAll(blockDir, 0o755), "mkdir %q", blockDir) scsiDir := filepath.Join(root, "scsi_device", hctl) - if err := os.MkdirAll(scsiDir, 0o755); err != nil { - t.Fatalf("mkdir %q: %s", scsiDir, err) - } + require.NoError(t, os.MkdirAll(scsiDir, 0o755), "mkdir %q", scsiDir) link := filepath.Join(blockDir, "device") target := filepath.Join("..", "..", "scsi_device", hctl) - if err := os.Symlink(target, link); err != nil { - t.Fatalf("symlink: %s", err) - } + require.NoError(t, os.Symlink(target, link), "symlink") } func TestReadSCSITargetJBOD(t *testing.T) { @@ -33,60 +31,40 @@ func TestReadSCSITargetJBOD(t *testing.T) { makeBlockDeviceSymlink(t, root, "sdb", "0:2:3:0") target, ok, err := ReadSCSITarget(root, "sdb") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if !ok { - t.Fatalf("expected ok=true for a SCSI-backed block device") - } - if target != 3 { - t.Errorf("target: got %d, want 3", target) - } + require.NoError(t, err) + require.True(t, ok, "expected ok=true for a SCSI-backed block device") + assert.Equal(t, 3, target, "target") } -// An NVMe/virtio-style block device has no H:C:T:L "device" symlink. +// An NVMe/virtio-style block device has no Host:Controller:Target:LUN +// "device" symlink. // ReadSCSITarget must report ok=false (not an error) so the caller can // fall through to generic udev detection. func TestReadSCSITargetNoDevice(t *testing.T) { root := t.TempDir() - if err := os.MkdirAll(filepath.Join(root, "block", "nvme0n1"), 0o755); err != nil { - t.Fatalf("mkdir: %s", err) - } + require.NoError(t, os.MkdirAll(filepath.Join(root, "block", "nvme0n1"), 0o755)) _, ok, err := ReadSCSITarget(root, "nvme0n1") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if ok { - t.Errorf("expected ok=false when device symlink is absent") - } + require.NoError(t, err) + assert.False(t, ok, "expected ok=false when device symlink is absent") } -// A symlink whose last segment isn't H:C:T:L (e.g. points at a PCI node) must -// not be mistaken for a SCSI device. ReadSCSITarget returns ok=false with no -// error so the caller falls through to udev. +// A "device" symlink whose last segment isn't Host:Controller:Target:LUN +// (e.g. points at a PCI node) is malformed for a SCSI block device and +// should be reported as an error so the caller can log/diagnose. Callers +// are expected to filter non-SCSI devices upstream via udev (ID_SCSI=1). func TestReadSCSITargetNonHCTL(t *testing.T) { root := t.TempDir() blockDir := filepath.Join(root, "block", "vda") - if err := os.MkdirAll(blockDir, 0o755); err != nil { - t.Fatalf("mkdir: %s", err) - } + require.NoError(t, os.MkdirAll(blockDir, 0o755)) other := filepath.Join(root, "devices", "virtio0") - if err := os.MkdirAll(other, 0o755); err != nil { - t.Fatalf("mkdir: %s", err) - } - if err := os.Symlink(filepath.Join("..", "..", "devices", "virtio0"), - filepath.Join(blockDir, "device")); err != nil { - t.Fatalf("symlink: %s", err) - } + require.NoError(t, os.MkdirAll(other, 0o755)) + require.NoError(t, os.Symlink(filepath.Join("..", "..", "devices", "virtio0"), + filepath.Join(blockDir, "device"))) _, ok, err := ReadSCSITarget(root, "vda") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if ok { - t.Errorf("expected ok=false when link target is not H:C:T:L") - } + require.Error(t, err, "expected error for malformed Host:Controller:Target:LUN link target") + assert.False(t, ok, "expected ok=false when link target is not Host:Controller:Target:LUN") } func TestReadSCSITargetBadTarget(t *testing.T) { @@ -94,20 +72,12 @@ func TestReadSCSITargetBadTarget(t *testing.T) { makeBlockDeviceSymlink(t, root, "sdc", "0:2:notanum:0") _, ok, err := ReadSCSITarget(root, "sdc") - if err == nil { - t.Fatalf("expected parse error, got nil") - } - if ok { - t.Errorf("expected ok=false on parse error") - } + require.Error(t, err, "expected parse error") + assert.False(t, ok, "expected ok=false on parse error") } func TestReadSCSITargetEmptyKname(t *testing.T) { _, ok, err := ReadSCSITarget("/sys", "") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if ok { - t.Errorf("expected ok=false for empty kname") - } + require.NoError(t, err) + assert.False(t, ok, "expected ok=false for empty kname") } diff --git a/linux/system.go b/linux/system.go index 28938ae..bea385a 100644 --- a/linux/system.go +++ b/linux/system.go @@ -290,6 +290,9 @@ func (ls *linuxSystem) Wipe(d disko.Disk) error { return udevSettle() } +// GetDiskType is the disko.System entry point for disk-type classification. +// resolveDiskType also reports whether a RAID controller owns the device, +// but external callers of GetDiskType only care about the type itself. func (ls *linuxSystem) GetDiskType(path string, udInfo disko.UdevInfo) (disko.DiskType, error) { dType, _, err := ls.resolveDiskType(path, udInfo) return dType, err @@ -302,6 +305,7 @@ func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) devpath := udInfo.Properties["DEVPATH"] for _, ctrl := range ls.raidctrls { + // skip any device that isn't bound to the raid controller if !ls.isSysPathRAID(devpath, ctrl.DriverSysfsPath()) { continue } @@ -316,10 +320,7 @@ func (ls *linuxSystem) resolveDiskType(devicePath string, udInfo disko.UdevInfo) return disko.Unknown, false, fmt.Errorf("failed to get diskType of %q from RAID controller: %s", devicePath, err) } - // Defense in depth: disko.Unknown is an internal-only - // placeholder paired with a non-nil error. If a driver returns - // it with err == nil it has broken contract; surface as hard - // error rather than silently leak Unknown into disko.Disk.Type. + // RAID Controller should never return disko.Unknown with err == nil. if dType == disko.Unknown { return disko.Unknown, false, fmt.Errorf( "RAID controller returned disko.Unknown with nil error for %q; driver contract violated", diff --git a/linux/system_test.go b/linux/system_test.go index 02a325b..b7db442 100644 --- a/linux/system_test.go +++ b/linux/system_test.go @@ -290,7 +290,7 @@ func TestGetDiskTypeRAIDMatchHDD(t *testing.T) { } ls := newTestLinuxSystem(mock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} dtype, err := ls.GetDiskType("/dev/sda", udInfo) ast.NoError(err) @@ -309,7 +309,7 @@ func TestGetDiskTypeRAIDMatchSSD(t *testing.T) { } ls := newTestLinuxSystem(mock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} dtype, err := ls.GetDiskType("/dev/sda", udInfo) ast.NoError(err) @@ -330,7 +330,7 @@ func TestGetDiskTypeJBODFallback(t *testing.T) { udInfo := disko.UdevInfo{ Name: "sda", Properties: map[string]string{ - "DEVPATH": "/devices/pci/host0/block/sda", + "DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda", "ID_BUS": "scsi", "DEVTYPE": "disk", }, @@ -355,7 +355,7 @@ func TestGetDiskTypeWrappedSentinelFallback(t *testing.T) { } ls := newTestLinuxSystem(mock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} _, err := ls.GetDiskType("/dev/sda", udInfo) ast.NoError(err, "wrapped ErrDiskTypeUndetermined should still trigger fallback via errors.Is") @@ -371,7 +371,7 @@ func TestGetDiskTypeRAIDRealError(t *testing.T) { } ls := newTestLinuxSystem(mock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} _, err := ls.GetDiskType("/dev/sda", udInfo) ast.Error(err) @@ -390,7 +390,7 @@ func TestGetDiskTypeNoRAIDMatch(t *testing.T) { } ls := newTestLinuxSystem(mock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} _, err := ls.GetDiskType("/dev/sda", udInfo) ast.False(mock.getDiskTypeCalled, "should not call GetDiskType when sysfs path doesn't match") @@ -415,7 +415,7 @@ func TestGetDiskTypeMultiControllerJBODFallback(t *testing.T) { } ls := newTestLinuxSystem(megaraidMock, smartpqiMock) - udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}} + udInfo := disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}} _, err := ls.GetDiskType("/dev/sda", udInfo) ast.NoError(err) @@ -448,7 +448,7 @@ func TestResolveDiskType_RAIDMatchSSD(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/sda", - disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}}) ast.NoError(err) ast.True(onRAID) ast.Equal(disko.SSD, dType) @@ -466,7 +466,7 @@ func TestResolveDiskType_RAIDMatchHDD(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/sda", - disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}}) ast.NoError(err) ast.True(onRAID) ast.Equal(disko.HDD, dType) @@ -484,7 +484,7 @@ func TestResolveDiskType_JBODFallsThroughToUdev(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", - udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + udevInfoFallbackStub("/devices/pci0000:00/0000:00:1c.4/0000:04:00.0/nvme/nvme0/nvme0n1")) ast.NoError(err) ast.False(onRAID) ast.True(mock.getDiskTypeCalled) @@ -503,7 +503,7 @@ func TestResolveDiskType_WrappedJBODFallsThroughToUdev(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", - udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + udevInfoFallbackStub("/devices/pci0000:00/0000:00:1c.4/0000:04:00.0/nvme/nvme0/nvme0n1")) ast.NoError(err) ast.False(onRAID) ast.Equal(disko.NVME, dType) @@ -521,7 +521,7 @@ func TestResolveDiskType_RealErrorIsPropagated(t *testing.T) { ls := newTestLinuxSystem(mock) _, onRAID, err := ls.resolveDiskType("/dev/sda", - disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}}) ast.Error(err) ast.False(onRAID) ast.Contains(err.Error(), "storcli binary crashed") @@ -538,7 +538,7 @@ func TestResolveDiskType_NoRAIDMatchFallsThroughToUdev(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", - udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + udevInfoFallbackStub("/devices/pci0000:00/0000:00:1c.4/0000:04:00.0/nvme/nvme0/nvme0n1")) ast.NoError(err) ast.False(onRAID) ast.False(mock.getDiskTypeCalled) @@ -563,7 +563,7 @@ func TestResolveDiskType_MultiControllerJBODStopsIteration(t *testing.T) { ls := newTestLinuxSystem(first, second) dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", - udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + udevInfoFallbackStub("/devices/pci0000:00/0000:00:1c.4/0000:04:00.0/nvme/nvme0/nvme0n1")) ast.NoError(err) ast.False(onRAID) ast.True(first.getDiskTypeCalled) @@ -579,7 +579,7 @@ func TestResolveDiskType_NoControllersConfiguredUsesUdev(t *testing.T) { ls := newTestLinuxSystem() dType, onRAID, err := ls.resolveDiskType("/dev/nvme0n1", - udevInfoFallbackStub("/devices/pci/host0/block/nvme0n1")) + udevInfoFallbackStub("/devices/pci0000:00/0000:00:1c.4/0000:04:00.0/nvme/nvme0/nvme0n1")) ast.NoError(err) ast.False(onRAID) ast.Equal(disko.NVME, dType) @@ -600,7 +600,7 @@ func TestResolveDiskType_UnknownWithNilErrorIsRejected(t *testing.T) { ls := newTestLinuxSystem(mock) dType, onRAID, err := ls.resolveDiskType("/dev/sda", - disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci/host0/block/sda"}}) + disko.UdevInfo{Properties: map[string]string{"DEVPATH": "/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host0/target0:2:0/0:2:0:0/block/sda"}}) ast.Error(err) ast.Contains(err.Error(), "disko.Unknown") ast.Contains(err.Error(), "contract violated") diff --git a/megaraid/storcli.go b/megaraid/storcli.go index 411778e..8e87307 100644 --- a/megaraid/storcli.go +++ b/megaraid/storcli.go @@ -742,7 +742,7 @@ func isSoftStorCliErr(err error) bool { // across the controller's Drives. Returns ok=false on missing udev serial, // no match, collision, or unknown media. func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { - serials := collectUdevSerials(udInfo) + serials := udInfo.CollectSerials() if len(serials) == 0 { return disko.Unknown, false } @@ -779,24 +779,6 @@ func jbodDiskTypeFromSerial(ctrl Controller, udInfo disko.UdevInfo) (disko.DiskT return disko.Unknown, false } -// collectUdevSerials returns the udev tokens to match against -// Drive.SerialNumber. storcli reports the SCSI INQUIRY page-80 serial, -// which udev exposes as ID_SCSI_SERIAL; that's the primary key. -// ID_SERIAL_SHORT and ID_SERIAL are WWN-derived on most SAS/SATA drives, -// but they cover drives that don't expose a distinct VPD page-80 serial. -func collectUdevSerials(udInfo disko.UdevInfo) map[string]struct{} { - out := map[string]struct{}{} - - for _, key := range []string{"ID_SCSI_SERIAL", "ID_SERIAL_SHORT", "ID_SERIAL"} { - v := strings.TrimSpace(udInfo.Properties[key]) - if v != "" { - out[v] = struct{}{} - } - } - - return out -} - func (csc *cachingStorCli) DriverSysfsPath() string { return csc.mr.DriverSysfsPath() } diff --git a/megaraid/storcli_test.go b/megaraid/storcli_test.go index 5308a44..cf7d0be 100644 --- a/megaraid/storcli_test.go +++ b/megaraid/storcli_test.go @@ -10,6 +10,9 @@ import ( "time" "github.com/patrickmn/go-cache" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "machinerun.io/disko" ) @@ -1503,72 +1506,35 @@ CVPM05 Optimal 34C - 2018/10/16 func TestParseCxShowCiscoJBOD(t *testing.T) { vds, pds, err := parseCxShow(ciscoJBODCxShow) - if err != nil { - t.Fatalf("parseCxShow(ciscoJBODCxShow) returned error: %s", err) - } + require.NoError(t, err, "parseCxShow(ciscoJBODCxShow)") - if len(vds) != 0 { - t.Errorf("expected 0 virtual drives, got %d", len(vds)) - } - - if len(pds) != 5 { - t.Errorf("expected 5 physical drives, got %d", len(pds)) - } + assert.Empty(t, vds, "virtual drives") + require.Len(t, pds, 5, "physical drives") for _, pd := range pds { - if pd.State != "JBOD" { - t.Errorf("drive %d: expected State=JBOD, got %q", pd.ID, pd.State) - } - - if pd.DriveGroup != -1 { - t.Errorf("drive %d: expected DriveGroup=-1 (JBOD), got %d", pd.ID, pd.DriveGroup) - } + assert.Equal(t, "JBOD", pd.State, "drive %d: State", pd.ID) + assert.Equal(t, -1, pd.DriveGroup, "drive %d: DriveGroup (JBOD)", pd.ID) } - ssd := pds[1] - if ssd.MediaType != SSD { - t.Errorf("drive 1 (slot 2): expected SSD, got %s", ssd.MediaType) - } - - hdd := pds[0] - if hdd.MediaType != HDD { - t.Errorf("drive 0 (slot 3): expected HDD, got %s", hdd.MediaType) - } + assert.Equal(t, SSD, pds[1].MediaType, "drive 1 (slot 2)") + assert.Equal(t, HDD, pds[0].MediaType, "drive 0 (slot 3)") } func TestParseVirtPropertiesCiscoJBOD(t *testing.T) { propMap, err := parseVirtProperties(ciscoJBODCxVallShowAll) - if err != nil { - t.Fatalf("parseVirtProperties(ciscoJBODCxVallShowAll) returned error: %s", err) - } - - if len(propMap) != 0 { - t.Errorf("expected 0 VD properties, got %d", len(propMap)) - } + require.NoError(t, err, "parseVirtProperties(ciscoJBODCxVallShowAll)") + assert.Empty(t, propMap, "VD properties") } func TestNewControllerCiscoJBOD(t *testing.T) { ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, ciscoJBODCxEallSallShowAll) - if err != nil { - t.Fatalf("newController failed: %s", err) - } - - if ctrl.ID != 0 { - t.Errorf("expected controller ID 0, got %d", ctrl.ID) - } - - if len(ctrl.VirtDrives) != 0 { - t.Errorf("expected 0 VirtDrives, got %d", len(ctrl.VirtDrives)) - } - - if len(ctrl.Drives) != 5 { - t.Errorf("expected 5 Drives, got %d", len(ctrl.Drives)) - } + require.NoError(t, err, "newController") - if len(ctrl.DriveGroups) != 0 { - t.Errorf("expected 0 DriveGroups (JBOD has no drive groups), got %d", len(ctrl.DriveGroups)) - } + assert.Equal(t, 0, ctrl.ID, "controller ID") + assert.Empty(t, ctrl.VirtDrives, "VirtDrives") + assert.Len(t, ctrl.Drives, 5, "Drives") + assert.Empty(t, ctrl.DriveGroups, "DriveGroups (JBOD has no drive groups)") // Drives indexed by DID. Slot 2 is the SSD (DID=1); slot 3 is HDD (DID=0). wantSerials := map[int]string{ @@ -1580,20 +1546,14 @@ func TestNewControllerCiscoJBOD(t *testing.T) { } for did, want := range wantSerials { d := ctrl.Drives[did] - if d == nil { - t.Fatalf("drive DID=%d missing", did) - } - if d.SerialNumber != want { - t.Errorf("drive DID=%d: SerialNumber=%q, want %q", did, d.SerialNumber, want) - } + require.NotNil(t, d, "drive DID=%d missing", did) + assert.Equal(t, want, d.SerialNumber, "drive DID=%d: SerialNumber", did) } } func TestParseDriveSerialsCiscoJBOD(t *testing.T) { got, err := parseDriveSerials(ciscoJBODCxEallSallShowAll) - if err != nil { - t.Fatalf("parseDriveSerials returned error: %s", err) - } + require.NoError(t, err, "parseDriveSerials") want := map[driveKey]string{ {EID: 134, Slot: 2}: "SNTEST001", @@ -1602,19 +1562,13 @@ func TestParseDriveSerialsCiscoJBOD(t *testing.T) { {EID: 134, Slot: 5}: "SNTEST004", {EID: 134, Slot: 6}: "SNTEST005", } - if !reflect.DeepEqual(got, want) { - t.Errorf("parseDriveSerials mismatch:\n got=%v\nwant=%v", got, want) - } + assert.Equal(t, want, got, "parseDriveSerials") } func TestParseDriveSerialsEmpty(t *testing.T) { got, err := parseDriveSerials("") - if err != nil { - t.Fatalf("parseDriveSerials(\"\") returned error: %s", err) - } - if len(got) != 0 { - t.Errorf("expected empty map, got %v", got) - } + require.NoError(t, err, `parseDriveSerials("")`) + assert.Empty(t, got, "expected empty map") } type mockMegaRaid struct { @@ -1637,9 +1591,7 @@ func (m *mockMegaRaid) DriverSysfsPath() string { func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, ciscoJBODCxEallSallShowAll) - if err != nil { - t.Fatalf("newController failed: %s", err) - } + require.NoError(t, err, "newController") csc := &cachingStorCli{ mr: &mockMegaRaid{ctrl: ctrl, err: nil}, @@ -1649,13 +1601,10 @@ func TestGetDiskTypeJBODReturnsErrDiskTypeUndetermined(t *testing.T) { // No udev serial => JBOD matching fails => sentinel for udev fallback. for _, path := range []string{"/dev/sda", "/dev/sdb", "/dev/sdc"} { dtype, err := csc.GetDiskType(path, disko.UdevInfo{}) - if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("GetDiskType(%q): expected ErrDiskTypeUndetermined, got %v", path, err) - } - - if dtype != disko.Unknown { - t.Errorf("GetDiskType(%q): expected disko.Unknown placeholder, got %d", path, dtype) - } + assert.ErrorIs(t, err, disko.ErrDiskTypeUndetermined, + "GetDiskType(%q): expected ErrDiskTypeUndetermined", path) + assert.Equal(t, disko.Unknown, dtype, + "GetDiskType(%q): expected disko.Unknown placeholder", path) } } @@ -1668,9 +1617,8 @@ func TestGetDiskTypeSoftQueryFailureReturnsSentinel(t *testing.T) { } _, err := csc.GetDiskType("/dev/sda", disko.UdevInfo{}) - if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined on soft storcli failure, got %v", err) - } + assert.ErrorIs(t, err, disko.ErrDiskTypeUndetermined, + "expected ErrDiskTypeUndetermined on soft storcli failure") } // A hard storcli error must propagate unchanged (it is not the sentinel). @@ -1682,12 +1630,9 @@ func TestGetDiskTypeHardQueryFailurePropagates(t *testing.T) { } _, err := csc.GetDiskType("/dev/sda", disko.UdevInfo{}) - if errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Error("hard storcli error should NOT be treated as the sentinel") - } - if err == nil { - t.Error("hard storcli error must propagate") - } + assert.NotErrorIs(t, err, disko.ErrDiskTypeUndetermined, + "hard storcli error should NOT be treated as the sentinel") + assert.Error(t, err, "hard storcli error must propagate") } // newJBODCachingStorCli wires the Cisco JBOD fixture (with serials from @@ -1697,9 +1642,7 @@ func newJBODCachingStorCli(t *testing.T) *cachingStorCli { ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, ciscoJBODCxEallSallShowAll) - if err != nil { - t.Fatalf("newController failed: %s", err) - } + require.NoError(t, err, "newController") return &cachingStorCli{ mr: &mockMegaRaid{ctrl: ctrl, err: nil}, @@ -1715,12 +1658,8 @@ func TestGetDiskTypeJBODSerialMatchSSD(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST001"}, } dtype, err := csc.GetDiskType("/dev/sdb", ud) - if err != nil { - t.Fatalf("expected nil err for SSD JBOD match, got %v", err) - } - if dtype != disko.SSD { - t.Errorf("GetDiskType: got %v, want SSD", dtype) - } + require.NoError(t, err, "expected nil err for SSD JBOD match") + assert.Equal(t, disko.SSD, dtype, "GetDiskType") } // HDD serial (slot 3) resolves via ID_SERIAL_SHORT. @@ -1731,12 +1670,8 @@ func TestGetDiskTypeJBODSerialMatchHDD(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST002"}, } dtype, err := csc.GetDiskType("/dev/sdb", ud) - if err != nil { - t.Fatalf("expected nil err for HDD JBOD match, got %v", err) - } - if dtype != disko.HDD { - t.Errorf("GetDiskType: got %v, want HDD", dtype) - } + require.NoError(t, err, "expected nil err for HDD JBOD match") + assert.Equal(t, disko.HDD, dtype, "GetDiskType") } // ID_SERIAL alone (no ID_SERIAL_SHORT) still matches when it equals the SN. @@ -1747,12 +1682,8 @@ func TestGetDiskTypeJBODSerialViaIDSerialFallback(t *testing.T) { Properties: map[string]string{"ID_SERIAL": "SNTEST001"}, } dtype, err := csc.GetDiskType("/dev/sdb", ud) - if err != nil { - t.Fatalf("expected nil err for ID_SERIAL fallback match, got %v", err) - } - if dtype != disko.SSD { - t.Errorf("GetDiskType: got %v, want SSD", dtype) - } + require.NoError(t, err, "expected nil err for ID_SERIAL fallback match") + assert.Equal(t, disko.SSD, dtype, "GetDiskType") } func TestGetDiskTypeJBODSerialViaIDSCSISerial(t *testing.T) { @@ -1766,12 +1697,8 @@ func TestGetDiskTypeJBODSerialViaIDSCSISerial(t *testing.T) { }, } dtype, err := csc.GetDiskType("/dev/sdb", ud) - if err != nil { - t.Fatalf("expected nil err for ID_SCSI_SERIAL match, got %v", err) - } - if dtype != disko.SSD { - t.Errorf("GetDiskType: got %v, want SSD", dtype) - } + require.NoError(t, err, "expected nil err for ID_SCSI_SERIAL match") + assert.Equal(t, disko.SSD, dtype, "GetDiskType") } // Serial not in fixture falls through to ErrDiskTypeUndetermined. @@ -1782,18 +1709,15 @@ func TestGetDiskTypeJBODSerialNoMatch(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "NOSUCHSERIAL"}, } _, err := csc.GetDiskType("/dev/sdb", ud) - if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined for unknown serial, got %v", err) - } + assert.ErrorIs(t, err, disko.ErrDiskTypeUndetermined, + "expected ErrDiskTypeUndetermined for unknown serial") } // When the /eall/sall detail query was unavailable (empty string), drives // carry no SerialNumber and JBOD matching correctly fails over to udev. func TestGetDiskTypeJBODEmptyDetailOutputFallsThrough(t *testing.T) { ctrl, err := newController(0, ciscoJBODCxShow, ciscoJBODCxVallShowAll, "") - if err != nil { - t.Fatalf("newController failed: %s", err) - } + require.NoError(t, err, "newController") csc := &cachingStorCli{ mr: &mockMegaRaid{ctrl: ctrl, err: nil}, @@ -1804,9 +1728,8 @@ func TestGetDiskTypeJBODEmptyDetailOutputFallsThrough(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SNTEST001"}, } _, err = csc.GetDiskType("/dev/sdb", ud) - if !errors.Is(err, disko.ErrDiskTypeUndetermined) { - t.Errorf("expected ErrDiskTypeUndetermined with no serials, got %v", err) - } + assert.ErrorIs(t, err, disko.ErrDiskTypeUndetermined, + "expected ErrDiskTypeUndetermined with no serials") } // isSoftStorCliErr must recognise the three soft sentinels whether bare @@ -1832,9 +1755,8 @@ func TestIsSoftStorCliErr(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := isSoftStorCliErr(tc.err); got != tc.want { - t.Errorf("isSoftStorCliErr(%v) = %v, want %v", tc.err, got, tc.want) - } + assert.Equal(t, tc.want, isSoftStorCliErr(tc.err), + "isSoftStorCliErr(%v)", tc.err) }) } } diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index a1b5f01..c43f9d8 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -662,14 +662,23 @@ func isSoftStorCli2Err(err error) bool { errors.Is(err, ErrUnsupported) } -// jbodDiskTypeFromSCSI matches the Linux SCSI target T against -// PhysicalDrive.PID across the given controllers. Returns ok=false on -// missing kname, non-SCSI device, PID collision, or unknown medium. +// jbodDiskTypeFromSCSI matches the Linux SCSI Target field (the third +// component of Host:Controller:Target:LUN read from +// /sys/block//device) against PhysicalDrive.PID across the given +// controllers. Returns ok=false for non-SCSI devices (udev ID_SCSI != "1"), +// missing kname, unresolved target, PID collision, or unknown medium. func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { if sc.scsiTargetFn == nil { return disko.Unknown, false } + // udev sets ID_SCSI=1 only for devices that present as SCSI on a + // host controller. virtio-blk, NVMe and ATA/SATA do not, so we can + // skip the sysfs lookup for them. + if udInfo.Properties["ID_SCSI"] != "1" { + return disko.Unknown, false + } + kname := udInfo.Name if kname == "" { return disko.Unknown, false diff --git a/mpi3mr/storcli2_test.go b/mpi3mr/storcli2_test.go index 9ea423d..dfea275 100644 --- a/mpi3mr/storcli2_test.go +++ b/mpi3mr/storcli2_test.go @@ -7,6 +7,9 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "machinerun.io/disko" "machinerun.io/disko/linux/sysfs" ) @@ -471,13 +474,10 @@ func TestStorCli2JBODDiskTypeHDD(t *testing.T) { }, } - dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}) - if !ok { - t.Fatalf("expected ok=true on HDD JBOD match") - } - if dType != disko.HDD { - t.Errorf("jbodDiskTypeFromSCSI: got %v, want HDD", dType) - } + dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + require.True(t, ok, "expected ok=true on HDD JBOD match") + assert.Equal(t, disko.HDD, dType, "jbodDiskTypeFromSCSI") } func TestStorCli2JBODDiskTypeSSD(t *testing.T) { @@ -488,13 +488,10 @@ func TestStorCli2JBODDiskTypeSSD(t *testing.T) { }, } - dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}) - if !ok { - t.Fatalf("expected ok=true on SSD JBOD match") - } - if dType != disko.SSD { - t.Errorf("jbodDiskTypeFromSCSI: got %v, want SSD", dType) - } + dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + require.True(t, ok, "expected ok=true on SSD JBOD match") + assert.Equal(t, disko.SSD, dType, "jbodDiskTypeFromSCSI") } // SCSI target not reported by any controller. @@ -506,9 +503,9 @@ func TestStorCli2JBODDiskTypeNoMatch(t *testing.T) { }, } - if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { - t.Errorf("expected ok=false when no PhysicalDrive matches SCSI target") - } + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + assert.False(t, ok, "expected ok=false when no PhysicalDrive matches SCSI target") } // PID collision across controllers is ambiguous. @@ -525,9 +522,9 @@ func TestStorCli2JBODDiskTypeAmbiguousPID(t *testing.T) { }, } - if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl(), other}, disko.UdevInfo{Name: "sdb"}); ok { - t.Errorf("expected ok=false on PID collision across controllers") - } + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl(), other}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + assert.False(t, ok, "expected ok=false on PID collision across controllers") } // Non-SCSI device (NVMe, virtio, missing symlink). @@ -539,9 +536,9 @@ func TestStorCli2JBODDiskTypeUnresolvedTarget(t *testing.T) { }, } - if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { - t.Errorf("expected ok=false when SCSI target is unresolved") - } + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + assert.False(t, ok, "expected ok=false when SCSI target is unresolved") } // Sysfs lookup error is swallowed as a no-match. @@ -553,17 +550,48 @@ func TestStorCli2JBODDiskTypeLookupError(t *testing.T) { }, } - if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{Name: "sdb"}); ok { - t.Errorf("expected ok=false when SCSI lookup errors") - } + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + assert.False(t, ok, "expected ok=false when SCSI lookup errors") } // Empty udev Name short-circuits before any sysfs call. func TestStorCli2JBODDiskTypeEmptyName(t *testing.T) { sc := &storCli2{sysRoot: "/unused", scsiTargetFn: sysfs.ReadSCSITarget} - if _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, disko.UdevInfo{}); ok { - t.Errorf("expected ok=false when udev Name is empty") + udInfo := disko.UdevInfo{Properties: map[string]string{"ID_SCSI": "1"}} + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, udInfo) + assert.False(t, ok, "expected ok=false when udev Name is empty") +} + +// Non-SCSI devices (ID_SCSI != "1") must short-circuit before any sysfs +// call so virtio-blk, NVMe, ATA/SATA etc. don't trigger spurious lookups. +func TestStorCli2JBODDiskTypeNonSCSI(t *testing.T) { + called := false + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, _ string) (int, bool, error) { + called = true + return 10, true, nil + }, + } + + cases := []struct { + name string + udInfo disko.UdevInfo + }{ + {"missing ID_SCSI", disko.UdevInfo{Name: "sdb"}}, + {"ID_SCSI=0", disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "0"}}}, + {"NVMe-shaped", disko.UdevInfo{Name: "nvme0n1", Properties: map[string]string{"ID_MODEL": "Some NVMe"}}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + called = false + _, ok := sc.jbodDiskTypeFromSCSI([]Controller{jbodPhysCtrl()}, tc.udInfo) + assert.False(t, ok, "expected ok=false") + assert.False(t, called, "expected scsiTargetFn not to be called") + }) } } @@ -591,9 +619,8 @@ func TestIsSoftStorCli2Err(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := isSoftStorCli2Err(tc.err); got != tc.want { - t.Errorf("isSoftStorCli2Err(%v) = %v, want %v", tc.err, got, tc.want) - } + assert.Equal(t, tc.want, isSoftStorCli2Err(tc.err), + "isSoftStorCli2Err(%v)", tc.err) }) } } diff --git a/smartpqi/arcconf.go b/smartpqi/arcconf.go index b03cd18..22569b3 100644 --- a/smartpqi/arcconf.go +++ b/smartpqi/arcconf.go @@ -552,7 +552,7 @@ func isSoftArcconfErr(err error) bool { // SerialNumber across the given controllers. Returns ok=false on missing // udev serial, no match, collision, or unknown media. func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.DiskType, bool) { - serials := collectUdevSerials(udInfo) + serials := udInfo.CollectSerials() if len(serials) == 0 { return disko.Unknown, false } @@ -589,24 +589,6 @@ func jbodDiskTypeFromSerial(ctrls []Controller, udInfo disko.UdevInfo) (disko.Di return disko.Unknown, false } -// collectUdevSerials returns the udev tokens to match against -// PhysicalDevice.SerialNumber. arcconf reports the SCSI INQUIRY page-80 -// serial, which udev exposes as ID_SCSI_SERIAL; that's the primary key. -// ID_SERIAL_SHORT and ID_SERIAL are WWN-derived on most SAS/SATA drives, -// but they cover drives that don't expose a distinct VPD page-80 serial. -func collectUdevSerials(udInfo disko.UdevInfo) map[string]struct{} { - out := map[string]struct{}{} - - for _, key := range []string{"ID_SCSI_SERIAL", "ID_SERIAL_SHORT", "ID_SERIAL"} { - v := strings.TrimSpace(udInfo.Properties[key]) - if v != "" { - out[v] = struct{}{} - } - } - - return out -} - func (ac *arcConf) DriverSysfsPath() string { return SysfsPCIDriversPath } diff --git a/smartpqi/arcconf_test.go b/smartpqi/arcconf_test.go index cb5841e..dea5a77 100644 --- a/smartpqi/arcconf_test.go +++ b/smartpqi/arcconf_test.go @@ -8,6 +8,9 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "machinerun.io/disko" ) @@ -1600,12 +1603,8 @@ func TestJBODDiskTypeSerialHDDShort(t *testing.T) { } dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) - if !ok { - t.Fatalf("expected ok=true when ID_SERIAL_SHORT matches") - } - if dType != disko.HDD { - t.Errorf("jbodDiskTypeFromSerial: got %v, want HDD", dType) - } + require.True(t, ok, "expected ok=true when ID_SERIAL_SHORT matches") + assert.Equal(t, disko.HDD, dType, "jbodDiskTypeFromSerial") } // ID_SERIAL is the fallback when ID_SERIAL_SHORT is absent. @@ -1615,12 +1614,8 @@ func TestJBODDiskTypeSerialSSDFallback(t *testing.T) { } dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) - if !ok { - t.Fatalf("expected ok=true when ID_SERIAL matches as fallback") - } - if dType != disko.SSD { - t.Errorf("jbodDiskTypeFromSerial: got %v, want SSD", dType) - } + require.True(t, ok, "expected ok=true when ID_SERIAL matches as fallback") + assert.Equal(t, disko.SSD, dType, "jbodDiskTypeFromSerial") } func TestJBODDiskTypeSerialNVME(t *testing.T) { @@ -1629,12 +1624,8 @@ func TestJBODDiskTypeSerialNVME(t *testing.T) { } dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) - if !ok { - t.Fatalf("expected ok=true for NVME serial match") - } - if dType != disko.NVME { - t.Errorf("jbodDiskTypeFromSerial: got %v, want NVME", dType) - } + require.True(t, ok, "expected ok=true for NVME serial match") + assert.Equal(t, disko.NVME, dType, "jbodDiskTypeFromSerial") } // No ID_SERIAL* property in udev. @@ -1643,9 +1634,8 @@ func TestJBODDiskTypeSerialNoSerial(t *testing.T) { Properties: map[string]string{"ID_MODEL": "SomeModel"}, } - if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud); ok { - t.Errorf("expected ok=false when udev carries no ID_SERIAL*") - } + _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + assert.False(t, ok, "expected ok=false when udev carries no ID_SERIAL*") } // Serial not reported by any controller. @@ -1654,9 +1644,8 @@ func TestJBODDiskTypeSerialNoMatch(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SN-UNKNOWN"}, } - if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud); ok { - t.Errorf("expected ok=false when no PhysicalDevice matches") - } + _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) + assert.False(t, ok, "expected ok=false when no PhysicalDevice matches") } // Duplicate serial across controllers is ambiguous. @@ -1672,9 +1661,8 @@ func TestJBODDiskTypeSerialAmbiguous(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SN-HDD-1"}, } - if _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs(), dup}, ud); ok { - t.Errorf("expected ok=false on duplicate SerialNumber across controllers") - } + _, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs(), dup}, ud) + assert.False(t, ok, "expected ok=false on duplicate SerialNumber across controllers") } // Matched PD with UnknownMedia cannot be classified. @@ -1690,9 +1678,8 @@ func TestJBODDiskTypeSerialUnknownMedia(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SN-UNK"}, } - if _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud); ok { - t.Errorf("expected ok=false when matched PD has UnknownMedia") - } + _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud) + assert.False(t, ok, "expected ok=false when matched PD has UnknownMedia") } // Blank controller-side SerialNumber never matches. @@ -1708,51 +1695,8 @@ func TestJBODDiskTypeSerialEmptyPDSerial(t *testing.T) { Properties: map[string]string{"ID_SERIAL_SHORT": "SN-X"}, } - if _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud); ok { - t.Errorf("expected ok=false when PhysicalDevice has blank SerialNumber") - } -} - -func TestCollectUdevSerialsPrefersBoth(t *testing.T) { - ud := disko.UdevInfo{ - Properties: map[string]string{ - "ID_SERIAL_SHORT": "short1", - "ID_SERIAL": "long_short1", - "ID_MODEL": "ignored", - }, - } - - got := collectUdevSerials(ud) - if _, ok := got["short1"]; !ok { - t.Errorf("missing ID_SERIAL_SHORT token") - } - if _, ok := got["long_short1"]; !ok { - t.Errorf("missing ID_SERIAL token") - } - if _, ok := got["ignored"]; ok { - t.Errorf("collectUdevSerials must not include unrelated properties") - } -} - -func TestCollectUdevSerialsIncludesSCSISerial(t *testing.T) { - ud := disko.UdevInfo{ - Properties: map[string]string{ - "ID_SCSI_SERIAL": "TESTSN0PQI01", - "ID_SERIAL_SHORT": "deadbeefcafef001", - "ID_SERIAL": "3deadbeefcafef001", - }, - } - - got := collectUdevSerials(ud) - for _, want := range []string{ - "TESTSN0PQI01", - "deadbeefcafef001", - "3deadbeefcafef001", - } { - if _, ok := got[want]; !ok { - t.Errorf("collectUdevSerials: missing token %q", want) - } - } + _, ok := jbodDiskTypeFromSerial([]Controller{ctrl}, ud) + assert.False(t, ok, "expected ok=false when PhysicalDevice has blank SerialNumber") } func TestJBODDiskTypeSerialViaIDSCSISerial(t *testing.T) { @@ -1761,12 +1705,8 @@ func TestJBODDiskTypeSerialViaIDSCSISerial(t *testing.T) { } dType, ok := jbodDiskTypeFromSerial([]Controller{jbodCtrlWithPDs()}, ud) - if !ok { - t.Fatalf("expected ok=true when ID_SCSI_SERIAL matches") - } - if dType != disko.HDD { - t.Errorf("jbodDiskTypeFromSerial: got %v, want HDD", dType) - } + require.True(t, ok, "expected ok=true when ID_SCSI_SERIAL matches") + assert.Equal(t, disko.HDD, dType, "jbodDiskTypeFromSerial") } // isSoftArcconfErr must recognise the three soft sentinels whether bare @@ -1793,9 +1733,8 @@ func TestIsSoftArcconfErr(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := isSoftArcconfErr(tc.err); got != tc.want { - t.Errorf("isSoftArcconfErr(%v) = %v, want %v", tc.err, got, tc.want) - } + assert.Equal(t, tc.want, isSoftArcconfErr(tc.err), + "isSoftArcconfErr(%v)", tc.err) }) } } From c5f63db0082ec5a45dcc3d9020c4818b50224810 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Tue, 28 Apr 2026 07:42:47 +0000 Subject: [PATCH 7/8] fix: H:C:T:L is actually Host:Channel:Target:LUN Signed-off-by: Andrei Aaron --- linux/sysfs/scsi.go | 6 +++--- linux/sysfs/scsi_test.go | 10 +++++----- mpi3mr/storcli2.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/linux/sysfs/scsi.go b/linux/sysfs/scsi.go index 92d9c7a..1326281 100644 --- a/linux/sysfs/scsi.go +++ b/linux/sysfs/scsi.go @@ -10,7 +10,7 @@ import ( ) // ReadSCSITarget returns Target from /sys/block//device -> -// Host:Controller:Target:LUN. For SCSI-attached JBOD/passthrough disks +// Host:Channel:Target:LUN (H:C:T:L). For SCSI-attached JBOD/passthrough disks // behind a RAID HBA, Target matches the controller-reported drive ID // (megaraid Drive.DID, mpi3mr PhysicalDrive.PID), which lets callers // correlate a Linux block device with an entry in the controller's PD list. @@ -25,7 +25,7 @@ import ( // SCSI (ID_SCSI=1); virtio-blk, NVMe, ATA/SATA, etc. do not expose this // symlink and should be filtered upstream. ok=false with a nil error is // reserved for benign cases (empty kname, missing "device" symlink); a -// malformed Host:Controller:Target:LUN link target is reported as an +// malformed Host:Channel:Target:LUN link target is reported as an // error. sysRoot is injectable for tests; production passes "/sys". func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { if kname == "" { @@ -51,7 +51,7 @@ func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { const requiredHCTLFields = 4 if len(hctlFields) != requiredHCTLFields { return 0, false, fmt.Errorf( - "invalid SCSI Host:Controller:Target:LUN value %q from %q: expected %d fields, got %d", + "invalid SCSI Host:Channel:Target:LUN value %q from %q: expected %d fields, got %d", scsiDeviceID, link, requiredHCTLFields, len(hctlFields)) } diff --git a/linux/sysfs/scsi_test.go b/linux/sysfs/scsi_test.go index 3584790..4d27cdd 100644 --- a/linux/sysfs/scsi_test.go +++ b/linux/sysfs/scsi_test.go @@ -11,7 +11,7 @@ import ( // makeBlockDeviceSymlink wires up a fake sysfs entry at // /block//device pointing at -// ../../scsi_device/. +// ../../scsi_device/. func makeBlockDeviceSymlink(t *testing.T, root, kname, hctl string) { t.Helper() @@ -36,7 +36,7 @@ func TestReadSCSITargetJBOD(t *testing.T) { assert.Equal(t, 3, target, "target") } -// An NVMe/virtio-style block device has no Host:Controller:Target:LUN +// An NVMe/virtio-style block device has no Host:Channel:Target:LUN // "device" symlink. // ReadSCSITarget must report ok=false (not an error) so the caller can // fall through to generic udev detection. @@ -49,7 +49,7 @@ func TestReadSCSITargetNoDevice(t *testing.T) { assert.False(t, ok, "expected ok=false when device symlink is absent") } -// A "device" symlink whose last segment isn't Host:Controller:Target:LUN +// A "device" symlink whose last segment isn't Host:Channel:Target:LUN // (e.g. points at a PCI node) is malformed for a SCSI block device and // should be reported as an error so the caller can log/diagnose. Callers // are expected to filter non-SCSI devices upstream via udev (ID_SCSI=1). @@ -63,8 +63,8 @@ func TestReadSCSITargetNonHCTL(t *testing.T) { filepath.Join(blockDir, "device"))) _, ok, err := ReadSCSITarget(root, "vda") - require.Error(t, err, "expected error for malformed Host:Controller:Target:LUN link target") - assert.False(t, ok, "expected ok=false when link target is not Host:Controller:Target:LUN") + require.Error(t, err, "expected error for malformed Host:Channel:Target:LUN link target") + assert.False(t, ok, "expected ok=false when link target is not Host:Channel:Target:LUN") } func TestReadSCSITargetBadTarget(t *testing.T) { diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index c43f9d8..c081e6b 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -663,7 +663,7 @@ func isSoftStorCli2Err(err error) bool { } // jbodDiskTypeFromSCSI matches the Linux SCSI Target field (the third -// component of Host:Controller:Target:LUN read from +// component of Host:Channel:Target:LUN read from // /sys/block//device) against PhysicalDrive.PID across the given // controllers. Returns ok=false for non-SCSI devices (udev ID_SCSI != "1"), // missing kname, unresolved target, PID collision, or unknown medium. From 40025f6c9b3b630084c01498205ab41d755767eb Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Tue, 28 Apr 2026 19:49:37 +0000 Subject: [PATCH 8/8] disko: tighten ReadSCSITarget contract and handle NVMe JBOD medium Reject empty kname in linux/sysfs.ReadSCSITarget with an explicit error; every production call site already filters on udInfo.Name, so an empty kname is a caller bug. In mpi3mr/storcli2.jbodDiskTypeFromSCSI, classify Medium="NVMe" as disko.NVME for parity with megaraid's jbodDiskTypeFromSerial. Signed-off-by: Andrei Aaron --- linux/sysfs/scsi.go | 11 ++++++----- linux/sysfs/scsi_test.go | 2 +- mpi3mr/storcli2.go | 2 ++ mpi3mr/storcli2_test.go | 23 +++++++++++++++++++++++ 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/linux/sysfs/scsi.go b/linux/sysfs/scsi.go index 1326281..4307aa9 100644 --- a/linux/sysfs/scsi.go +++ b/linux/sysfs/scsi.go @@ -23,13 +23,14 @@ import ( // // Callers must only invoke ReadSCSITarget for devices that udev reports as // SCSI (ID_SCSI=1); virtio-blk, NVMe, ATA/SATA, etc. do not expose this -// symlink and should be filtered upstream. ok=false with a nil error is -// reserved for benign cases (empty kname, missing "device" symlink); a -// malformed Host:Channel:Target:LUN link target is reported as an -// error. sysRoot is injectable for tests; production passes "/sys". +// symlink and should be filtered upstream. An empty kname is a caller bug +// and is rejected with an error; ok=false with a nil error is reserved for +// the benign missing-"device"-symlink case; a malformed +// Host:Channel:Target:LUN link target is reported as an error. sysRoot is +// injectable for tests; production passes "/sys". func ReadSCSITarget(sysRoot, kname string) (target int, ok bool, err error) { if kname == "" { - return 0, false, nil + return 0, false, fmt.Errorf("invalid empty kname parameter") } link := filepath.Join(sysRoot, "block", kname, "device") diff --git a/linux/sysfs/scsi_test.go b/linux/sysfs/scsi_test.go index 4d27cdd..483956d 100644 --- a/linux/sysfs/scsi_test.go +++ b/linux/sysfs/scsi_test.go @@ -78,6 +78,6 @@ func TestReadSCSITargetBadTarget(t *testing.T) { func TestReadSCSITargetEmptyKname(t *testing.T) { _, ok, err := ReadSCSITarget("/sys", "") - require.NoError(t, err) + require.Error(t, err, "expected error for empty kname") assert.False(t, ok, "expected ok=false for empty kname") } diff --git a/mpi3mr/storcli2.go b/mpi3mr/storcli2.go index c081e6b..5556c54 100644 --- a/mpi3mr/storcli2.go +++ b/mpi3mr/storcli2.go @@ -707,6 +707,8 @@ func (sc *storCli2) jbodDiskTypeFromSCSI(ctrls []Controller, udInfo disko.UdevIn return disko.SSD, true case "HDD": return disko.HDD, true + case "NVME": + return disko.NVME, true } return disko.Unknown, false diff --git a/mpi3mr/storcli2_test.go b/mpi3mr/storcli2_test.go index dfea275..7c9f804 100644 --- a/mpi3mr/storcli2_test.go +++ b/mpi3mr/storcli2_test.go @@ -494,6 +494,29 @@ func TestStorCli2JBODDiskTypeSSD(t *testing.T) { assert.Equal(t, disko.SSD, dType, "jbodDiskTypeFromSCSI") } +// Tri-mode controllers can report Medium="NVMe" for NVMe JBODs. The +// production short-circuit on ID_SCSI=1 keeps real NVMe drives from +// reaching this switch, but exercising the case directly with a fake +// fixture pins the parity with megaraid's jbodDiskTypeFromSerial. +func TestStorCli2JBODDiskTypeNVMe(t *testing.T) { + ctrl := Controller{ + PhysicalDrives: PhysicalDriveSet{ + "30": PhysicalDrive{PID: 30, Medium: "NVMe", State: "JBOD"}, + }, + } + sc := &storCli2{ + sysRoot: "/unused", + scsiTargetFn: func(_, kname string) (int, bool, error) { + return 30, true, nil + }, + } + + dType, ok := sc.jbodDiskTypeFromSCSI([]Controller{ctrl}, + disko.UdevInfo{Name: "sdb", Properties: map[string]string{"ID_SCSI": "1"}}) + require.True(t, ok, "expected ok=true on NVMe JBOD match") + assert.Equal(t, disko.NVME, dType, "jbodDiskTypeFromSCSI") +} + // SCSI target not reported by any controller. func TestStorCli2JBODDiskTypeNoMatch(t *testing.T) { sc := &storCli2{