UB-1475: returning the actual error from the Discover instead of mask…

…ing it as Volume not found
IBM · Aug 16, 2018 · e6b8132 · e6b8132
1 parent 525b20c
commit e6b8132
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 22 deletions.
diff --git a/remote/mounter/block_device_utils/block_device_utils_test.go b/remote/mounter/block_device_utils/block_device_utils_test.go
@@ -28,6 +28,7 @@ import (
 	"io/ioutil"
 	"strings"
 	"testing"
+	"os/exec"
 )
 
 var _ = Describe("block_device_utils_test", func() {
@@ -195,6 +196,22 @@ var _ = Describe("block_device_utils_test", func() {
 			_, err := bdUtils.Discover(volumeId, true)
 			Expect(err).To(HaveOccurred())
 		})
+		FIt("should return actual error when sg_inq command fails", func() {
+			volumeId := "0x6001738cfc9035eb0000000000cea5f6"
+			mpathOutput := `mpathhe (36001738cfc9035eb0000000000cea5f6) dm-3 IBM     ,2810XIV
+							size=19G features='1 queue_if_no_path' hwhandler='0' wp=rw
+							-+- policy='service-time 0' prio=1 status=active
+							|- 33:0:0:1 sdb 8:16 active ready running
+							- 34:0:0:1 sdc 8:32 active ready running`
+			fakeExec.ExecuteReturnsOnCall(0, []byte(mpathOutput),
+				nil)
+			returnError :=  &exec.ExitError{}
+			//this execute with timeout makes the GetWwnByScsiInq to return an error
+			fakeExec.ExecuteWithTimeoutReturns([]byte(""),returnError)
+			_, err := bdUtils.Discover(volumeId, true)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(Equal(&block_device_utils.CommandExecuteError{"sg_inq",returnError}))
+		})
 	})
 	Context(".DiscoverBySgInq", func() {
 		It("should return mpathhe", func() {
@@ -258,6 +275,20 @@ mpathhb (36001738cfc9035eb0000000000cea###) dm-3 ##,##
 			_, err := bdUtils.DiscoverBySgInq(mpathOutput, wwn)
 			Expect(err).To(HaveOccurred())
 		})
+		It("should return actual error when sg_inq command fails", func() {
+			mpathOutput := `mpathhe (36001738cfc9035eb0000000000cea5f6) dm-3 IBM     ,2810XIV
+							size=19G features='1 queue_if_no_path' hwhandler='0' wp=rw
+							-+- policy='service-time 0' prio=1 status=active
+							|- 33:0:0:1 sdb 8:16 active ready running
+							- 34:0:0:1 sdc 8:32 active ready running`
+			wwn := "wwn"
+			returnError :=  &exec.ExitError{}
+			//this execute with timeout makes the GetWwnByScsiInq to return an error
+			fakeExec.ExecuteWithTimeoutReturns([]byte(""),returnError)
+			_, err := bdUtils.DiscoverBySgInq(mpathOutput, wwn)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(Equal(&block_device_utils.CommandExecuteError{"sg_inq",returnError}))
+		})
 	})
 	Context(".GetWwnByScsiInq", func() {
 		It("GetWwnByScsiInq fails if sg_inq command fails", func() {

diff --git a/remote/mounter/block_device_utils/errors.go b/remote/mounter/block_device_utils/errors.go
@@ -27,13 +27,13 @@ func (e *commandNotFoundError) Error() string {
 	return fmt.Sprintf("command [%v] is not found [%v]", e.cmd, e.err)
 }
 
-type commandExecuteError struct {
-	cmd string
-	err error
+type CommandExecuteError struct {
+	Cmd string
+	Err error
 }
 
-func (e *commandExecuteError) Error() string {
-	return fmt.Sprintf("command [%v] execution failure [%v]", e.cmd, e.err)
+func (e *CommandExecuteError) Error() string {
+	return fmt.Sprintf("command [%v] execution failure [%v]", e.Cmd, e.Err)
 }
 
 type VolumeNotFoundError struct {

diff --git a/remote/mounter/block_device_utils/fs.go b/remote/mounter/block_device_utils/fs.go
@@ -47,7 +47,7 @@ func (b *blockDeviceUtils) CheckFs(mpath string) (bool, error) {
 			// TODO we can improve it by double check the fs type of this device and maybe log warning if its not the same fstype we expacted
 			needFs = true
 		} else {
-			return false, b.logger.ErrorRet(&commandExecuteError{blkidCmd, err}, "failed")
+			return false, b.logger.ErrorRet(&CommandExecuteError{blkidCmd, err}, "failed")
 		}
 	}
 	b.logger.Info("checked", logs.Args{{"needFs", needFs}, {"mpath", mpath}, {blkidCmd, outputBytes}})
@@ -62,7 +62,7 @@ func (b *blockDeviceUtils) MakeFs(mpath string, fsType string) error {
 	}
 	args := []string{"-t", fsType, mpath}
 	if _, err := b.exec.Execute(mkfsCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{mkfsCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{mkfsCmd, err}, "failed")
 	}
 	b.logger.Info("created", logs.Args{{"fsType", fsType}, {"mpath", mpath}})
 	return nil
@@ -76,7 +76,7 @@ func (b *blockDeviceUtils) MountFs(mpath string, mpoint string) error {
 	}
 	args := []string{mpath, mpoint}
 	if _, err := b.exec.ExecuteWithTimeout(TimeoutMilisecondMountCmdMountFs, mountCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{mountCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{mountCmd, err}, "failed")
 	}
 	b.logger.Info("mounted", logs.Args{{"mpoint", mpoint}})
 	return nil
@@ -101,7 +101,7 @@ func (b *blockDeviceUtils) UmountFs(mpoint string) error {
 			b.logger.Info("Device already unmounted.", logs.Args{{"mpoint", mpoint}})
 			return nil
 		}
-		return b.logger.ErrorRet(&commandExecuteError{umountCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{umountCmd, err}, "failed")
 	}
 	b.logger.Info("umounted", logs.Args{{"mpoint", mpoint}})
 	return nil
@@ -110,7 +110,7 @@ func (b *blockDeviceUtils) UmountFs(mpoint string) error {
 func (b *blockDeviceUtils) executeMountCmdToViewMountpoints() ([]byte, error) {
 	/*
 	   Check if mount command exist (if not return error commandNotFoundError)
-	   then trigger the mount command with no params (if failed return error commandExecuteError)
+	   then trigger the mount command with no params (if failed return error CommandExecuteError)
 	   and return the output as []byte.
 	*/
 
@@ -122,7 +122,7 @@ func (b *blockDeviceUtils) executeMountCmdToViewMountpoints() ([]byte, error) {
 
 	outputBytes, err := b.exec.ExecuteWithTimeout(TimeoutMilisecondMountCmdIsDeviceMounted, mountCmd, nil)
 	if err != nil {
-		return nil, b.logger.ErrorRet(&commandExecuteError{mountCmd, err}, "failed")
+		return nil, b.logger.ErrorRet(&CommandExecuteError{mountCmd, err}, "failed")
 	}
 
 	return outputBytes, nil

diff --git a/remote/mounter/block_device_utils/mpath.go b/remote/mounter/block_device_utils/mpath.go
@@ -39,13 +39,13 @@ func (b *blockDeviceUtils) ReloadMultipath() error {
 	args := []string{}
 	_, err := b.exec.ExecuteWithTimeout(MultipathTimeout, multipathCmd, args)
 	if err != nil  {
-		return b.logger.ErrorRet(&commandExecuteError{multipathCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{multipathCmd, err}, "failed")
 	}
 
 	args = []string{"-r"}
 	_, err = b.exec.ExecuteWithTimeout(MultipathTimeout, multipathCmd, args)
 	if err != nil  {
-		return b.logger.ErrorRet(&commandExecuteError{multipathCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{multipathCmd, err}, "failed")
 	}
 
 	return nil
@@ -59,7 +59,7 @@ func (b *blockDeviceUtils) Discover(volumeWwn string, deepDiscovery bool) (strin
 	args := []string{"-ll"}
 	outputBytes, err := b.exec.Execute(multipathCmd, args)
 	if err != nil {
-		return "", b.logger.ErrorRet(&commandExecuteError{multipathCmd, err}, "failed")
+		return "", b.logger.ErrorRet(&CommandExecuteError{multipathCmd, err}, "failed")
 	}
 	scanner := bufio.NewScanner(strings.NewReader(string(outputBytes[:])))
 	pattern := "(?i)" + volumeWwn
@@ -84,7 +84,7 @@ func (b *blockDeviceUtils) Discover(volumeWwn string, deepDiscovery bool) (strin
 		dev, err = b.DiscoverBySgInq(string(outputBytes[:]), volumeWwn)
 		if err != nil {
 			b.logger.Debug(fmt.Sprintf("mpath device was NOT found for WWN [%s] even after sg_inq on all mpath devices.", volumeWwn))
-			return "", b.logger.ErrorRet(&VolumeNotFoundError{volumeWwn}, "failed")
+			return "", b.logger.ErrorRet(err, "failed")
 		} else {
 			b.logger.Warning(fmt.Sprintf("device [%s] found for WWN [%s] after running sg_inq on all mpath devices although it was not found in multipath -ll. (Note: Could indicate multipathing issue).", dev, volumeWwn))
 			mpath = b.mpathDevFullPath(dev)
@@ -95,7 +95,7 @@ func (b *blockDeviceUtils) Discover(volumeWwn string, deepDiscovery bool) (strin
 		// Validate that we have the correct wwn.
 		SqInqWwn, err := b.GetWwnByScsiInq(mpath)
 		if err != nil {
-			return "", b.logger.ErrorRet(&commandExecuteError{"sg_inq", err}, "failed")
+			return "", b.logger.ErrorRet(&CommandExecuteError{"sg_inq", err}, "failed")
 		}
 
 		if strings.ToLower(SqInqWwn) != strings.ToLower(volumeWwn) {
@@ -124,6 +124,7 @@ func (b *blockDeviceUtils) DiscoverBySgInq(mpathOutput string, volumeWwn string)
 		return "", b.logger.ErrorRet(err, "failed")
 	}
 	dev := ""
+
 	for scanner.Scan() {
 		line := scanner.Text()
 		b.logger.Debug(fmt.Sprintf("%s", line))
@@ -133,7 +134,7 @@ func (b *blockDeviceUtils) DiscoverBySgInq(mpathOutput string, volumeWwn string)
 			mpathFullPath := b.mpathDevFullPath(dev)
 			wwn, err := b.GetWwnByScsiInq(mpathFullPath)
 			if err != nil {
-				return "", b.logger.ErrorRet(&VolumeNotFoundError{volumeWwn}, "failed")
+				return "", b.logger.ErrorRet(err, "failed")				
 			}
 			if strings.ToLower(wwn) == strings.ToLower(volumeWwn) {
 				return dev, nil
@@ -188,7 +189,7 @@ func (b *blockDeviceUtils) GetWwnByScsiInq(dev string) (string, error) {
 	b.logger.Debug(fmt.Sprintf("Calling [%s] with timeout", sgInqCmd))
 	outputBytes, err := b.exec.ExecuteWithTimeout(3000, sgInqCmd, args)
 	if err != nil {
-		return "", b.logger.ErrorRet(&commandExecuteError{sgInqCmd, err}, "failed")
+		return "", b.logger.ErrorRet(&CommandExecuteError{sgInqCmd, err}, "failed")
 	}
 	wwnRegex := "(?i)" + `\[0x(.*?)\]`
 	wwnRegexCompiled, err := regexp.Compile(wwnRegex)
@@ -252,14 +253,14 @@ func (b *blockDeviceUtils) Cleanup(mpath string) error {
 
 	args := []string{"message", dev, "0", "fail_if_no_path"}
 	if _, err := b.exec.Execute(dmsetupCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{dmsetupCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{dmsetupCmd, err}, "failed")
 	}
 	if err := b.exec.IsExecutable(multipathCmd); err != nil {
 		return b.logger.ErrorRet(&commandNotFoundError{multipathCmd, err}, "failed")
 	}
 	args = []string{"-f", dev}
 	if _, err := b.exec.Execute(multipathCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{multipathCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{multipathCmd, err}, "failed")
 	}
 	b.logger.Info("flushed", logs.Args{{"mpath", mpath}})
 	return nil

diff --git a/remote/mounter/block_device_utils/rescan.go b/remote/mounter/block_device_utils/rescan.go
@@ -42,7 +42,7 @@ func (b *blockDeviceUtils) RescanISCSI() error {
 	}
 	args := []string{"-m", "session", "--rescan"}
 	if _, err := b.exec.Execute(rescanCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{rescanCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{rescanCmd, err}, "failed")
 	}
 	return nil
 }
@@ -62,7 +62,7 @@ func (b *blockDeviceUtils) RescanSCSI() error {
 	}
 	args := []string{"-r"} // TODO should use -r only in clean up
 	if _, err := b.exec.Execute(rescanCmd, args); err != nil {
-		return b.logger.ErrorRet(&commandExecuteError{rescanCmd, err}, "failed")
+		return b.logger.ErrorRet(&CommandExecuteError{rescanCmd, err}, "failed")
 	}
 	return nil
 }