diff --git a/DESIGN.md b/DESIGN.md index 0a30937..66630f8 100644 --- a/DESIGN.md +++ b/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) diff --git a/pkg/implementation/logicalvolumegetter/storcli2_test.go b/pkg/implementation/logicalvolumegetter/storcli2_test.go index 552dd6b..df105b7 100644 --- a/pkg/implementation/logicalvolumegetter/storcli2_test.go +++ b/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() diff --git a/pkg/implementation/physicaldrivegetter/ssacli.go b/pkg/implementation/physicaldrivegetter/ssacli.go index b012470..e65bfba 100644 --- a/pkg/implementation/physicaldrivegetter/ssacli.go +++ b/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 diff --git a/pkg/implementation/physicaldrivegetter/ssacli_test.go b/pkg/implementation/physicaldrivegetter/ssacli_test.go index e258eb9..d2b5eac 100644 --- a/pkg/implementation/physicaldrivegetter/ssacli_test.go +++ b/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) +} diff --git a/pkg/implementation/physicaldrivegetter/storcli2.go b/pkg/implementation/physicaldrivegetter/storcli2.go index 2333f1e..b324b12 100644 --- a/pkg/implementation/physicaldrivegetter/storcli2.go +++ b/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 = "" } } diff --git a/pkg/implementation/physicaldrivegetter/storcli2_test.go b/pkg/implementation/physicaldrivegetter/storcli2_test.go index d94e22d..2e258f1 100644 --- a/pkg/implementation/physicaldrivegetter/storcli2_test.go +++ b/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 diff --git a/pkg/implementation/raidcontroller/megaraid/logicalvolume.go b/pkg/implementation/raidcontroller/megaraid/logicalvolume.go index ba25a54..87e918b 100644 --- a/pkg/implementation/raidcontroller/megaraid/logicalvolume.go +++ b/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 } diff --git a/pkg/implementation/raidcontroller/megaraid/logicalvolume_internal_test.go b/pkg/implementation/raidcontroller/megaraid/logicalvolume_internal_test.go new file mode 100644 index 0000000..c052bc1 --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/implementation/raidcontroller/megaraid/megaraid_test.go b/pkg/implementation/raidcontroller/megaraid/megaraid_test.go index f5774bc..e5aa0e0 100644 --- a/pkg/implementation/raidcontroller/megaraid/megaraid_test.go +++ b/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")) diff --git a/pkg/implementation/raidcontroller/megaraid/testdata/logicalvolumes/show/v300.json b/pkg/implementation/raidcontroller/megaraid/testdata/logicalvolumes/show/v300.json new file mode 100644 index 0000000..cc591a6 --- /dev/null +++ b/pkg/implementation/raidcontroller/megaraid/testdata/logicalvolumes/show/v300.json @@ -0,0 +1,66 @@ +{ + "Controllers": [ + { + "Command Status": { + "Controller": 0, + "Status": "Success", + "Description": "None" + }, + "Response Data": { + "/c0/v300": [ + { + "DG/VD": "12/300", + "TYPE": "RAID1", + "State": "Dgrd", + "Access": "RW", + "Consist": "Yes", + "Cache": "RWTD", + "Cac": "-", + "sCC": "ON", + "Size": "16.370 TB", + "Name": "" + } + ], + "PDs for VD 300": [ + { + "EID:Slt": "251:1", + "DID": 1, + "State": "Onln", + "DG": 12, + "Size": "16.370 TB", + "Intf": "SAS", + "Med": "HDD", + "Model": "ST18000NM000D ", + "Type": "-" + }, + { + "EID:Slt": "251:2", + "DID": 2, + "State": "Failed", + "DG": 12, + "Size": "16.370 TB", + "Intf": "SAS", + "Med": "HDD", + "Model": "ST18000NM000D ", + "Type": "-" + } + ], + "VD300 Properties": { + "Strip Size": "256 KB", + "Number of Blocks": 35155607552, + "VD has Emulated PD": "Yes", + "Span Depth": 1, + "Number of Drives Per Span": 2, + "Write Cache(initial setting)": "WriteThrough", + "Disk Cache Policy": "Disk's Default", + "Exposed to OS": "Yes", + "Emulation type": "default", + "Is LD Ready for OS Requests": "Yes", + "OS Drive Name": "/dev/sdb", + "SCSI NAA Id": "600062b212da5d402bd3b493e1699377", + "Unmap Enabled": "No" + } + } + } + ] +}