fix: a single failed drive no longer blanks a controller's device paths

DESIGN.md

@@ -127,6 +127,16 @@ flags. This is not for idempotency but to minimize real mutations and to skip
 fields the (lossy) getter reports as `Unknown` when the caller did not change
 them — avoiding a spurious "unsettable" rejection on an untouched field.
 
+Getter read paths degrade gracefully. When a drive or volume in a multi-member
+array cannot be fully resolved — e.g. a degraded-but-online array whose udev
+`by-id` link is missing, or a failed JBOD drive whose device node is gone — the
+getter returns that entity with its status and empty `DevicePath`/`PermanentPath`
+rather than failing, so one unhealthy drive does not drop a controller's whole
+inventory. (The megaraid create/settle path is deliberately stricter: a
+just-created volume whose device node has not yet appeared is treated as
+not-ready and retried after a bus rescan, so there a failed path resolution is
+still an error.)
+
 ### Adapters
 
 #### MegaRAID / PERC (storcli, perccli)

pkg/implementation/logicalvolumegetter/storcli2_test.go

@@ -70,6 +70,39 @@ func TestStorCLI2LogicalVolumes(t *testing.T) {
 	assert.Equal(t, 0, first.PDrivesMetadata[0].CtrlMetadata.ID)
 }
 
+// TestStorCLI2LogicalVolumesDegraded guards the resilience property for
+// storcli2 RAID disks: a degraded volume (one member drive failed) is still
+// returned with its device and permanent path intact, and does not abort
+// discovery for the whole controller. Unlike the legacy megaraid getter,
+// storcli2 reads the OS drive name directly and never probes the filesystem,
+// so a not-optimal array keeps its path.
+func TestStorCLI2LogicalVolumesDegraded(t *testing.T) {
+	t.Parallel()
+
+	const payload = `{"Controllers":[{"Command Status":{"Status":"Success"},` +
+		`"Response Data":{"Virtual Drives":[{` +
+		`"VD Info":{"DG/VD":"0/1","TYPE":"RAID1","State":"Dgrd","CurrentCache":"NR,WB",` +
+		`"Size":"9.094 TiB"},` +
+		`"PDs":[{"EID:Slt":"306:0"},{"EID:Slt":"306:1"}],` +
+		`"VD Properties":{"OS Drive Name":"/dev/sdb",` +
+		`"SCSI NAA Id":"600062b22066d54069faf124ced57e62"}}]}}]}`
+
+	mockRunner := new(MockCommandRunner)
+	mockRunner.On("Run", []string{"/c0/vall", "show", "all"}).Return([]byte(payload), nil)
+
+	s := NewStorCLI2(mockRunner)
+
+	volumes, err := s.LogicalVolumes(&raidcontroller.Metadata{ID: 0})
+	require.NoError(t, err)
+	require.Len(t, volumes, 1)
+
+	vol := volumes[0]
+	assert.Equal(t, logicalvolume.LVStatusDegraded, vol.Status)
+	assert.Equal(t, "/dev/sdb", vol.DevicePath)
+	assert.Equal(t, "/dev/disk/by-id/wwn-0x600062b22066d54069faf124ced57e62", vol.PermanentPath)
+	assert.Len(t, vol.PDrivesMetadata, 2)
+}
+
 func TestStorCLI2LogicalVolume(t *testing.T) {
 	t.Parallel()
 

pkg/implementation/physicaldrivegetter/ssacli.go

@@ -243,12 +243,12 @@ func (s *SSACLI) parsePDLine( //nolint:funlen // This function is long and not c
 	case "Disk Name":
 		physicalDrive.DevicePath = value
 
-		blockDevice, err := s.getBlockDevice(value)
-		if err != nil {
-			return errors.Wrapf(err, "failed to get block device for %s", value)
-		}
-
-		if isBlockDeviceUsed(blockDevice) {
+		// getBlockDevice only refines the status: a mounted or formatted device
+		// is in use. A drive whose device node has disappeared (e.g. a failed or
+		// pulled drive) makes the lsblk lookup fail; that must not abort
+		// discovery for the whole controller, so a lookup failure leaves the
+		// status ssacli already reported untouched.
+		if blockDevice, err := s.getBlockDevice(value); err == nil && isBlockDeviceUsed(blockDevice) {
 			physicalDrive.Status = physicaldrive.PDStatusUsed
 		}
 		// TODO miss permanent path

pkg/implementation/physicaldrivegetter/ssacli_test.go

@@ -1,6 +1,7 @@
 package physicaldrivegetter
 
 import (
+	"errors"
 	"os"
 	"strconv"
 	"testing"
@@ -271,3 +272,29 @@ func TestSSACLIPhysicalDriveStatus(t *testing.T) {
 		})
 	}
 }
+
+// TestSSACLIParsePDLineDiskNameLsblkFailureIsNotFatal checks that a failed or
+// pulled drive whose device node has disappeared makes the lsblk lookup fail.
+// That lookup only refines the status, so it must
+// not abort discovery for the whole controller. The drive keeps its device
+// path and the status ssacli already reported.
+func TestSSACLIParsePDLineDiskNameLsblkFailureIsNotFatal(t *testing.T) {
+	mockRunner := new(MockCommandRunner)
+	mockRunner.On("Run", mock.AnythingOfType("[]string")).
+		Return([]byte(nil), errors.New("lsblk: device not found"))
+
+	s := &SSACLI{LSBLK: mockRunner}
+
+	pd := &physicaldrive.PhysicalDrive{
+		Metadata: &physicaldrive.Metadata{CtrlMetadata: &raidcontroller.Metadata{}},
+		Slot:     &physicaldrive.Slot{},
+		Status:   physicaldrive.PDStatusFailed,
+		Reason:   "Failed",
+	}
+
+	err := s.parsePDLine(pd, "   Disk Name: /dev/sdz")
+
+	assert.NoError(t, err)
+	assert.Equal(t, "/dev/sdz", pd.DevicePath)
+	assert.Equal(t, physicaldrive.PDStatusFailed, pd.Status)
+}

pkg/implementation/physicaldrivegetter/storcli2.go

@@ -238,9 +238,14 @@ func parseDrive(entry storcli2DrivesListEntry, ctrl *raidcontroller.Metadata) (
 	// missing) may have lost its device node and must not fail the whole
 	// inventory. ComputePaths reads the real filesystem (utils.FileExists), so
 	// the healthy-JBOD path is exercised on hardware rather than in unit tests.
+	//
+	// A resolution failure (e.g. a drive whose udev by-id link is missing or
+	// has not settled yet) must not drop the drive and abort discovery for the
+	// whole controller: keep it with empty paths instead.
 	if physicalDrive.JBOD && physicalDrive.Status == physicaldrive.PDStatusUsed {
 		if err := physicalDrive.ComputePaths(); err != nil {
-			return nil, errors.Wrap(err, "failed to compute paths")
+			physicalDrive.DevicePath = ""
+			physicalDrive.PermanentPath = ""
 		}
 	}
 

pkg/implementation/physicaldrivegetter/storcli2_test.go

@@ -181,6 +181,37 @@ func TestStorCLI2PhysicalDrivesEmptyInventory(t *testing.T) {
 	}
 }
 
+// TestStorCLI2PhysicalDrivesComputePathsFailureIsNotFatal checks that a healthy
+// JBOD "Used" drive whose identifiers do not resolve to any /dev/disk/by-id
+// link (e.g. the udev link is missing or has not settled yet) makes
+// ComputePaths fail. That must not abort discovery for the whole
+// controller; the drive is kept with empty paths. The fake vendor/serial/WWN
+// below cannot match a real by-id link on the build host, so ComputePaths fails
+// deterministically.
+func TestStorCLI2PhysicalDrivesComputePathsFailureIsNotFatal(t *testing.T) {
+	t.Parallel()
+
+	const payload = `{"Controllers":[{"Command Status":{"Status":"Success"},` +
+		`"Response Data":{"Drives List":[{` +
+		`"Drive Information":{"EID:Slt":"306:4","Model":"ST10000NM018B","Med":"HDD",` +
+		`"Size":"9.094 TiB","State":"JBOD","Status":"Online"},` +
+		`"Drive Detailed Information":{"Vendor":"RMTEST","Serial Number":"RMTEST0000000000",` +
+		`"WWN":"5000C500DEADBEEF"}}]}}]}`
+
+	mockRunner := new(MockCommandRunner)
+	mockRunner.On("Run", []string{"/c0/eall/sall", "show", "all"}).Return([]byte(payload), nil)
+
+	s := NewStorCLI2(mockRunner)
+
+	drives, err := s.PhysicalDrives(&raidcontroller.Metadata{ID: 0})
+	require.NoError(t, err)
+	require.Len(t, drives, 1)
+	assert.True(t, drives[0].JBOD)
+	assert.Equal(t, physicaldrive.PDStatusUsed, drives[0].Status)
+	assert.Empty(t, drives[0].DevicePath)
+	assert.Empty(t, drives[0].PermanentPath)
+}
+
 // TestStorCLI2PhysicalDrivesJBOD pins the JBOD mapping at the entity level
 // with a synthetic payload (the captured fixtures contain no JBOD drive): a
 // JBOD drive that is not functioning (here "Missing") keeps JBOD=true, maps to

pkg/implementation/raidcontroller/megaraid/logicalvolume.go

@@ -689,38 +689,63 @@ var (
 	CustomFileExists   = utils.FileExists
 )
 
-// getPaths returns the device path and a permanent paths for the logical volumes.
+// getPaths returns the device path and permanent path for a logical volume.
+//
+// A resolvable /dev/disk/by-id/wwn-* link is the preferred permanent path, but
+// its absence is not fatal: a degraded-but-online RAID volume (e.g. one whose
+// data drive has failed) still exposes a valid OS device path and still serves
+// I/O, and its udev by-id link may be missing while the array is not optimal.
+// In that case the OS device path is returned with a best-effort empty
+// permanent path rather than failing, so a single failed drive can no longer
+// abort discovery for the whole controller.
 func getPaths(vdp *VDProperties, pdrives []*physicaldrive.PhysicalDrive) (
 	devicePath, permanentPath string, err error,
 ) {
-	devicePath = vdp.OSDriveName
-
-	permanentPath = fmt.Sprintf("/dev/disk/by-id/wwn-0x%s", vdp.SCSINAAID)
-	if !CustomFileExists(permanentPath) {
-		// If the permanent path is not found and there is only one physical drive,
-		// we will try to get the path from the physical drive information
-		// otherwise let's error here
-		if len(pdrives) != 1 {
-			return devicePath, "", errors.New("failed to get permanent path")
-		}
+	devicePath, permanentPath, ok, err := resolveWWNPath(vdp)
+	if ok {
+		return devicePath, permanentPath, err
+	}
 
+	// No resolvable by-id/wwn permanent path. For a single-drive volume both
+	// paths can still be derived from the backing physical drive.
+	if len(pdrives) == 1 {
 		pd := pdrives[0]
 
-		err = pd.ComputePaths()
-		if err != nil {
-			return devicePath, "", errors.Wrap(err, "failed to compute paths from physical drive")
+		if err = pd.ComputePaths(); err != nil {
+			return vdp.OSDriveName, "", errors.Wrap(err, "failed to compute paths from physical drive")
 		}
 
 		return pd.DevicePath, pd.PermanentPath, nil
 	}
 
-	// If the devicePath is empty let's retrieve it from the permanent path
+	// Multi-drive volume without a resolvable permanent path. The OS device
+	// path (when reported) is still valid, so return it with an empty permanent
+	// path instead of failing the whole controller's discovery.
+	return vdp.OSDriveName, "", nil
+}
+
+// resolveWWNPath resolves a volume's paths from its /dev/disk/by-id/wwn-* link.
+// It reports ok=true when that link exists (whether or not the device path then
+// resolves); ok=false means the caller should fall back to another strategy.
+func resolveWWNPath(vdp *VDProperties) (devicePath, permanentPath string, ok bool, err error) {
+	if vdp.SCSINAAID == "" {
+		return "", "", false, nil
+	}
+
+	permanentPath = fmt.Sprintf("/dev/disk/by-id/wwn-0x%s", vdp.SCSINAAID)
+	if !CustomFileExists(permanentPath) {
+		return "", "", false, nil
+	}
+
+	// Fill the device path from the permanent path when the controller did not
+	// report an OS drive name.
+	devicePath = vdp.OSDriveName
 	if devicePath == "" {
 		devicePath, err = CustomEvalSymlinks(permanentPath)
 		if err != nil {
-			return "", "", errors.Wrap(err, "failed to evaluate symlink")
+			return "", "", true, errors.Wrap(err, "failed to evaluate symlink")
 		}
 	}
 
-	return devicePath, permanentPath, nil
+	return devicePath, permanentPath, true, nil
 }

pkg/implementation/raidcontroller/megaraid/logicalvolume_internal_test.go

@@ -0,0 +1,97 @@
+package megaraid
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/scality/raidmgmt/pkg/domain/entities/physicaldrive"
+)
+
+// TestGetPaths pins the device/permanent path resolution for a logical volume,
+// in particular that a degraded-but-online multi-drive volume whose udev
+// by-id/wwn link is missing still yields its OS device path instead of failing
+// discovery for the whole controller.
+func TestGetPaths(t *testing.T) {
+	const wwnLink = "/dev/disk/by-id/wwn-0xabc123"
+
+	twoDrives := []*physicaldrive.PhysicalDrive{{}, {}}
+
+	tests := []struct {
+		name          string
+		vdp           *VDProperties
+		pdrives       []*physicaldrive.PhysicalDrive
+		fileExists    func(string) bool
+		evalSymlinks  func(string) (string, error)
+		wantDevice    string
+		wantPermanent string
+		wantErr       bool
+	}{
+		{
+			name:          "wwn link present, os drive name reported",
+			vdp:           &VDProperties{OSDriveName: "/dev/sdb", SCSINAAID: "abc123"},
+			pdrives:       twoDrives,
+			fileExists:    func(string) bool { return true },
+			wantDevice:    "/dev/sdb",
+			wantPermanent: wwnLink,
+		},
+		{
+			name:          "wwn link present, os drive name empty, resolved via symlink",
+			vdp:           &VDProperties{OSDriveName: "", SCSINAAID: "abc123"},
+			pdrives:       twoDrives,
+			fileExists:    func(string) bool { return true },
+			evalSymlinks:  func(string) (string, error) { return "/dev/sdb", nil },
+			wantDevice:    "/dev/sdb",
+			wantPermanent: wwnLink,
+		},
+		{
+			name:          "degraded multi-drive volume, wwn link missing, keeps os drive name",
+			vdp:           &VDProperties{OSDriveName: "/dev/sdb", SCSINAAID: "abc123"},
+			pdrives:       twoDrives,
+			fileExists:    func(string) bool { return false },
+			wantDevice:    "/dev/sdb",
+			wantPermanent: "",
+		},
+		{
+			name:    "multi-drive volume, no scsi naa id, keeps os drive name",
+			vdp:     &VDProperties{OSDriveName: "/dev/sdb", SCSINAAID: ""},
+			pdrives: twoDrives,
+			fileExists: func(string) bool {
+				t.Helper()
+				require.Fail(t, "FileExists must not be called for an empty SCSI NAA Id")
+				return false
+			},
+			wantDevice:    "/dev/sdb",
+			wantPermanent: "",
+		},
+	}
+
+	origFileExists, origEvalSymlinks := CustomFileExists, CustomEvalSymlinks
+	defer func() {
+		CustomFileExists = origFileExists
+		CustomEvalSymlinks = origEvalSymlinks
+	}()
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			CustomFileExists = tc.fileExists
+			CustomEvalSymlinks = origEvalSymlinks
+
+			if tc.evalSymlinks != nil {
+				CustomEvalSymlinks = tc.evalSymlinks
+			}
+
+			device, permanent, err := getPaths(tc.vdp, tc.pdrives)
+
+			if tc.wantErr {
+				require.Error(t, err)
+
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tc.wantDevice, device)
+			require.Equal(t, tc.wantPermanent, permanent)
+		})
+	}
+}

pkg/implementation/raidcontroller/megaraid/megaraid_test.go

@@ -514,6 +514,39 @@ func (s *UnitTestSuite) TestLogicalVolume() {
 	}
 }
 
+// TestLogicalVolumeDegradedKeepsDevicePath is the end-to-end guard for the
+// legacy megaraid v1 read path: a degraded multi-drive volume whose
+// /dev/disk/by-id/wwn link is absent (FileExists == false) must
+// still resolve through logicalVolume() -> fillPhysicalDrives() -> getPaths()
+// and return the volume with its OS device path intact, rather than erroring
+// and blanking the whole controller.
+func (s *UnitTestSuite) TestLogicalVolumeDegradedKeepsDevicePath() {
+	s.setupMockCalls()
+	// The by-id/wwn link is missing while the array is degraded: the exact
+	// original-bug trigger for a multi-drive volume.
+	s.mockPathResolver.On("FileExists", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").
+		Return(false)
+
+	s.setupCustomFileExists()
+	defer s.restoreCustomFileExists()
+
+	s.setupCustomEvalSymlinks()
+	defer s.restoreCustomEvalSymlinks()
+
+	lv, err := s.a.LogicalVolume(&logicalvolume.Metadata{
+		CtrlMetadata: &raidcontroller.Metadata{ID: 0},
+		ID:           "300",
+	})
+
+	s.NoError(err)
+	s.Require().NotNil(lv)
+	s.Equal("300", lv.ID)
+	s.Equal(logicalvolume.LVStatusDegraded, lv.Status)
+	s.Equal("/dev/sdb", lv.DevicePath)
+	s.Empty(lv.PermanentPath)
+	s.Len(lv.PDrivesMetadata, 2)
+}
+
 func (s *UnitTestSuite) TestEnableJBOD() {
 	s.mockRunner.On("Run", []string{"/c0/e251/s6", "set", "jbod"}).
 		Return(mockReturn("physicaldrives/jbod/enable/fail"))