Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Commit

Permalink
UB-1475: returning the actual error from the Discover instead of mask…
Browse files Browse the repository at this point in the history
…ing it as Volume not found (#236)

This PR will return the actual error that is returned from sg_inq and not mask it as a volume not found error .
this is important because if we mask the error from sg_inq we might stumble into an idempotent case and continue the umount even thought we should not and we will be stuck.
(this can happen when there is a faulty device so sg_inq returns an error and we return a volume not found error in this case, and after that because we know to continue with the unmount in case of a volume not found we will continue the umount even though other steps did not happen and this is not really the case of a volume not found because it was already unmounted.)
olgashtivelman authored Aug 19, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 17e3da6 commit 382fc6e
Showing 5 changed files with 52 additions and 22 deletions.
30 changes: 30 additions & 0 deletions remote/mounter/block_device_utils/block_device_utils_test.go
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@ import (
"io/ioutil"
"strings"
"testing"
"os/exec"
)

var _ = Describe("block_device_utils_test", func() {
@@ -195,6 +196,21 @@ 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 +274,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() {
10 changes: 5 additions & 5 deletions remote/mounter/block_device_utils/errors.go
Original file line number Diff line number Diff line change
@@ -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 {
12 changes: 6 additions & 6 deletions remote/mounter/block_device_utils/fs.go
Original file line number Diff line number Diff line change
@@ -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
18 changes: 9 additions & 9 deletions remote/mounter/block_device_utils/mpath.go
Original file line number Diff line number Diff line change
@@ -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) {
@@ -133,7 +133,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 +188,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 +252,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
4 changes: 2 additions & 2 deletions remote/mounter/block_device_utils/rescan.go
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit 382fc6e

Please sign in to comment.