Handle permanent and device path for megaraid volumes

pkg/implementation/logicalvolumegetter/mdadm.go

@@ -295,9 +295,10 @@ func ParseMDADMExportOutput(output []byte) ([]*MDADMExportDetails, error) {
 					device = currentDetails.Devices[deviceName]
 				}
 
-				if matches[2] == "ROLE" {
+				switch matches[2] {
+				case "ROLE":
 					device.Role = matches[3]
-				} else if matches[2] == "DEV" {
+				case "DEV":
 					device.Path = matches[3]
 				}
 

pkg/implementation/raidcontroller/megaraid/logicalvolume.go

@@ -166,7 +166,7 @@ func selectorLV(m *logicalvolume.Metadata) (string, error) {
 
 // logicalVolume returns a logical volume for a given logical volume metadata.
 //
-//nolint:funlen // The function is not too complex
+//nolint:funlen,cyclop // The function is not too complex
 func (a *Adapter) logicalVolume(
 	metadata *logicalvolume.Metadata) (
 	*logicalvolume.LogicalVolume, error,
@@ -224,14 +224,21 @@ func (a *Adapter) logicalVolume(
 		return nil, errors.Wrap(err, "failed to unmarshal VD properties")
 	}
 
-	permanentPath, err := vdProperties.permanentPath()
+	// Get the physical drives from the metadata
+	// NOTE: This is needed as fallback to get the device path
+	pds, err := a.fillPhysicalDrives(pdsMetadata)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to fill physical drives")
+	}
+
+	devicePath, permanentPath, err := getPaths(vdProperties, pds)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to get permanent path")
+		return nil, errors.Wrap(err, "failed to get paths")
 	}
 
 	logicalVolume := &logicalvolume.LogicalVolume{
 		Metadata:        metadata,
-		DevicePath:      vdProperties.OSDriveName,
+		DevicePath:      devicePath,
 		RAIDLevel:       logicalvolume.RAIDLevelMap(vd.Type),
 		PDrivesMetadata: pdsMetadata,
 		CacheOptions:    cacheOptions,
@@ -542,19 +549,43 @@ func hasMatchingPDs(lvPDs []*physicaldrive.Metadata, pdSlots map[string]struct{}
 	return false
 }
 
-// CustomEvalSymlinks is a variable that holds a function that evaluates symlinks.
-// It is used to mock the filepath.EvalSymlinks function in tests.
+// It is used to mock the functions in tests.
 // nolint: gochecknoglobals // This is a variable that is used to mock a function in tests.
-var CustomEvalSymlinks = filepath.EvalSymlinks
+var (
+	CustomEvalSymlinks = filepath.EvalSymlinks
+	CustomFileExists   = utils.FileExists
+)
+
+// getPaths returns the device path and a permanent paths for the logical volumes.
+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")
+		}
 
-// permanentPath returns the permanent path of a virtual drive.
-func (vdp *VDProperties) permanentPath() (string, error) {
-	sysPath := fmt.Sprintf("/dev/disk/by-id/wwn-0x%s", vdp.SCSINAAID)
+		pd := pdrives[0]
 
-	permanentPath, err := CustomEvalSymlinks(sysPath)
-	if err != nil {
-		return "", errors.Wrap(err, "failed to evaluate symlink")
+		permanentPath = computePermanentPath(pd)
+		if !CustomFileExists(permanentPath) {
+			return devicePath, "", errors.New("failed to get permanent path from physical drive")
+		}
+	}
+
+	// If the devicePath is empty let's retrieve it from the permanent path
+	if devicePath == "" {
+		devicePath, err = CustomEvalSymlinks(permanentPath)
+		if err != nil {
+			return "", "", errors.Wrap(err, "failed to evaluate symlink")
+		}
 	}
 
-	return permanentPath, nil
+	return devicePath, permanentPath, nil
 }

pkg/implementation/raidcontroller/megaraid/megaraid_test.go

@@ -30,6 +30,9 @@ type UnitTestSuite struct {
 
 	wasCreateLVCalledOnce bool
 
+	// fileExistsFunc is the original FileExists function
+	// It's used to restore the original function after mocking it
+	fileExistsFunc func(string) bool
 	// evalSymlinksFunc is the original EvalSymlinks function
 	// It's used to restore the original function after mocking it
 	evalSymlinksFunc func(string) (string, error)
@@ -191,26 +194,19 @@ func (s *UnitTestSuite) generalMockCalls(args []string) (*megaraid2.CmdOutput, e
 	return mockReturn(filename)
 }
 
-// setupMockCallsEvalSymlinksList sets up the mock calls for EvalSymlinks.
-func (s *UnitTestSuite) setupMockCallsEvalSymlinksList() {
-	ids := []string{
-		"600062b212da5d402bd3b43351207e5f",
-		"600062b212da5d402bd3b47dc0433389",
-		"600062b212da5d402bd3b47fc3685d94",
-		"600062b212da5d402bd3b481c696852b",
-		"600062b212da5d402bd3b483c9c3e053",
-		"600062b212da5d402bd3b486ccf8622c",
-		"600062b212da5d402bd3b488d03a89ca",
-		"600062b212da5d402bd3b48ad38fb43e",
-		"600062b212da5d402bd3b48cd6db37db",
-		"600062b212da5d402bd3b48eda2832e2",
-		"600062b212da5d402bd3b491dd849969",
-		"600062b212da5d402bd3b493e1699377",
-	}
-
-	for _, id := range ids {
-		s.mockPathResolver.On("EvalSymlinks", "/dev/disk/by-id/wwn-0x"+id).Return("/dev/sda", nil)
-	}
+// restoreCustomFileExists restores the custom FileExists function
+// to the original one.
+// It's used to mock the FileExists function.
+func (s *UnitTestSuite) restoreCustomFileExists() {
+	megaraid2.CustomFileExists = s.fileExistsFunc
+}
+
+// setupCustomFileExists sets up the custom FileExists function
+// to be used in the tests.
+// It's used to mock the FileExists function.
+func (s *UnitTestSuite) setupCustomFileExists() {
+	s.fileExistsFunc = megaraid2.CustomFileExists
+	megaraid2.CustomFileExists = s.mockPathResolver.FileExists
 }
 
 // restoreCustomEvalSymlinks restores the custom EvalSymlinks function
@@ -393,7 +389,11 @@ func (s *UnitTestSuite) TestPhysicalDrive() {
 
 func (s *UnitTestSuite) TestLogicalVolumes() {
 	s.setupMockCalls()
-	s.setupMockCallsEvalSymlinksList()
+	s.mockPathResolver.On("FileExists", mock.Anything).Return(true)
+	s.mockPathResolver.On("EvalSymlinks", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return("/dev/sda", nil)
+
+	s.setupCustomFileExists()
+	defer s.restoreCustomFileExists()
 
 	s.setupCustomEvalSymlinks()
 	defer s.restoreCustomEvalSymlinks()
@@ -450,8 +450,12 @@ func (s *UnitTestSuite) TestLogicalVolumes() {
 
 func (s *UnitTestSuite) TestLogicalVolume() {
 	s.setupMockCalls()
+	s.mockPathResolver.On("FileExists", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return(true)
 	s.mockPathResolver.On("EvalSymlinks", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return("/dev/sda", nil)
 
+	s.setupCustomFileExists()
+	defer s.restoreCustomFileExists()
+
 	s.setupCustomEvalSymlinks()
 	defer s.restoreCustomEvalSymlinks()
 
@@ -559,8 +563,12 @@ func (s *UnitTestSuite) TestDisableJBOD() {
 func (s *UnitTestSuite) TestSetLVCacheOptions() {
 	s.setupMockCalls()
 
+	s.mockPathResolver.On("FileExists", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return(true)
 	s.mockPathResolver.On("EvalSymlinks", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return("/dev/sda", nil)
 
+	s.setupCustomFileExists()
+	defer s.restoreCustomFileExists()
+
 	s.setupCustomEvalSymlinks()
 	defer s.restoreCustomEvalSymlinks()
 
@@ -614,7 +622,11 @@ func (s *UnitTestSuite) TestSetLVCacheOptions() {
 
 func (s *UnitTestSuite) TestCreateLV() {
 	s.setupMockCallsCreateLV()
-	s.setupMockCallsEvalSymlinksList()
+	s.mockPathResolver.On("FileExists", mock.Anything).Return(true)
+	s.mockPathResolver.On("EvalSymlinks", "/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377").Return("/dev/sda", nil)
+
+	s.setupCustomFileExists()
+	defer s.restoreCustomFileExists()
 
 	s.setupCustomEvalSymlinks()
 	defer s.restoreCustomEvalSymlinks()
@@ -691,7 +703,7 @@ func (s *UnitTestSuite) TestCreateLV() {
 			s.Equal(logicalvolume.ReadPolicyReadAhead, newLv.CacheOptions.ReadPolicy)
 			s.Equal(logicalvolume.WritePolicyWriteThrough, newLv.CacheOptions.WritePolicy)
 			s.Equal(logicalvolume.IOPolicyDirect, newLv.CacheOptions.IOPolicy)
-			s.Equal("/dev/sda", newLv.PermanentPath)
+			s.Equal("/dev/disk/by-id/wwn-0x600062b212da5d402bd3b493e1699377", newLv.PermanentPath)
 		}
 	}
 }

pkg/implementation/raidcontroller/megaraid/pathresolver.go

@@ -2,8 +2,11 @@ package megaraid
 
 // PathResolver is an interface that defines the EvalSymlinks method.
 //
+// FileExists checks if a file exists at the given path.
+// It is used to mock the os package in tests.
 // EvalSymlinks returns the path name after the evaluation of any symbolic links.
 // It is used to mock the filepath.EvalSymlinks function in tests.
 type PathResolver interface {
+	FileExists(string) bool
 	EvalSymlinks(string) (string, error)
 }

pkg/implementation/raidcontroller/megaraid/physicaldrive.go

@@ -2,6 +2,7 @@ package megaraid
 
 import (
 	"fmt"
+	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
@@ -66,7 +67,7 @@ func (a *Adapter) physicaldrives(metadata *raidcontroller.Metadata) (
 
 // physicalDrive returns a physical drive for a given physical drive metadata.
 //
-//nolint:funlen // no good reason to split it for now
+//nolint:funlen,gocognit // no good reason to split it for now
 func (a *Adapter) physicalDrive(
 	metadata *physicaldrive.Metadata) (
 	*physicaldrive.PhysicalDrive, error,
@@ -123,11 +124,21 @@ func (a *Adapter) physicalDrive(
 		Status:   pd.PDStatus(),
 		JBOD:     jbod,
 		// TODO : fill those fields
-		// PermanentPath: "",
-		// DevicePath:    "",
 		// Reason:   "",
 	}
 
+	if physicalDrive.JBOD {
+		physicalDrive.PermanentPath = computePermanentPath(physicalDrive)
+		if !utils.FileExists((physicalDrive.PermanentPath)) {
+			return nil, errors.New("permanent path does not exist")
+		}
+
+		physicalDrive.DevicePath, err = filepath.EvalSymlinks(physicalDrive.PermanentPath)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to evaluate symlinks")
+		}
+	}
+
 	return physicalDrive, nil
 }
 
@@ -247,3 +258,10 @@ func (a *Adapter) setJBOD(
 
 	return nil
 }
+
+// computePermanentPath computes the permanent path for a given physical drive.
+// The permanent path is a string that represents the physical drive
+// this may not exists if the drive is not in JBOD.
+func computePermanentPath(pd *physicaldrive.PhysicalDrive) string {
+	return fmt.Sprintf("/dev/disk/by-id/scsi-S%s_%s_%s", pd.Vendor, pd.Model, pd.Serial)
+}

pkg/implementation/raidcontroller/megaraid/structs.go

@@ -65,10 +65,12 @@ type (
 
 	// Physical Drive.
 	PD struct {
-		EIDSlot             string `json:"EID:Slt"`
-		DeviceID            int    `json:"DID"`
-		State               string `json:"State"`
-		DeviceGroup         int    `json:"DG"`
+		EIDSlot  string `json:"EID:Slt"`
+		DeviceID int    `json:"DID"`
+		State    string `json:"State"`
+		// DG can be either a "-" or a int, since we are
+		// not using it today let's just comment it out
+		// DeviceGroup         int    `json:"DG"`
 		Size                string `json:"Size"` // Size (humanized)
 		Interface           string `json:"Intf"`
 		MediaType           string `json:"Med"` // Media Type (HDD, SSD, NVMe)

pkg/implementation/raidcontroller/megaraid/testdata/logicalvolumes/show/v228.json

@@ -1 +1 @@
-{"Controllers":[{"Command Status":{"CLI Version":"007.1616.0000.0000 Dec 24, 2020","Operating system":"Linux 4.18.0-553.22.1.el8_10.x86_64","Controller":0,"Status":"Success","Description":"None"},"Response Data":{"/c0/v228":[{"DG/VD":"11/228","TYPE":"RAID0","State":"Optl","Access":"RW","Consist":"Yes","Cache":"RWTD","Cac":"-","sCC":"ON","Size":"16.370 TB","Name":""}],"PDs for VD 228":[{"EID:Slt":"251:12","DID":1,"State":"Onln","DG":11,"Size":"16.370 TB","Intf":"SAS","Med":"HDD","SED":"N","PI":"N","SeSz":"512B","Model":"ST18000NM000D   ","Sp":"U","Type":"-"}],"VD228 Properties":{"Strip Size":"256 KB","Number of Blocks":35155607552,"VD has Emulated PD":"Yes","Span Depth":1,"Number of Drives Per Span":1,"Write Cache(initial setting)":"WriteThrough","Disk Cache Policy":"Disk's Default","Encryption":"None","Data Protection":"None","Active Operations":"None","Exposed to OS":"Yes","OS Drive Name":"/dev/sda","Creation Date":"20-04-2023","Creation Time":"08:30:11 AM","Emulation type":"default","Cachebypass size":"Cachebypass-64k","Cachebypass Mode":"Cachebypass Intelligent","Is LD Ready for OS Requests":"Yes","SCSI NAA Id":"600062b212da5d402bd3b493e1699377","Unmap Enabled":"No"}}}]}
+{"Controllers":[{"Command Status":{"CLI Version":"007.1616.0000.0000 Dec 24, 2020","Operating system":"Linux 4.18.0-553.22.1.el8_10.x86_64","Controller":0,"Status":"Success","Description":"None"},"Response Data":{"/c0/v228":[{"DG/VD":"11/228","TYPE":"RAID0","State":"Optl","Access":"RW","Consist":"Yes","Cache":"RWTD","Cac":"-","sCC":"ON","Size":"16.370 TB","Name":""}],"PDs for VD 228":[{"EID:Slt":"251:12","DID":1,"State":"Onln","DG":11,"Size":"16.370 TB","Intf":"SAS","Med":"HDD","SED":"N","PI":"N","SeSz":"512B","Model":"ST18000NM000D   ","Sp":"U","Type":"-"}],"VD228 Properties":{"Strip Size":"256 KB","Number of Blocks":35155607552,"VD has Emulated PD":"Yes","Span Depth":1,"Number of Drives Per Span":1,"Write Cache(initial setting)":"WriteThrough","Disk Cache Policy":"Disk's Default","Encryption":"None","Data Protection":"None","Active Operations":"None","Exposed to OS":"Yes","Creation Date":"20-04-2023","Creation Time":"08:30:11 AM","Emulation type":"default","Cachebypass size":"Cachebypass-64k","Cachebypass Mode":"Cachebypass Intelligent","Is LD Ready for OS Requests":"Yes","SCSI NAA Id":"600062b212da5d402bd3b493e1699377","Unmap Enabled":"No"}}}]}

pkg/utils/path.go

@@ -0,0 +1,9 @@
+package utils
+
+import "os"
+
+// FileExists checks if a file exists at the given path.
+func FileExists(path string) bool {
+	_, err := os.Stat(path)
+	return !os.IsNotExist(err)
+}