UB-1475: returning the actual error from the Discover instead of masking it as Volume not found

[olgashtivelman]

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.)

---

This change is 

[coveralls]

Coverage decreased (-3.8%) to 48.144% when pulling 015b4af on fix/UB-1475_umount_is_stuck_when_umounting_a_pod_with_faulty_device into dacf163 on dev.

[beckmani]

---

remote/mounter/block_device_utils/mpath.go, line 137 at r1 (raw file):

			wwn, err := b.GetWwnByScsiInq(mpathFullPath)
			if err != nil {
				return "", b.logger.ErrorRet(err, "failed")				

Extra spaces?

[beckmani]

---

remote/mounter/block_device_utils/mpath.go, line 259 at r1 (raw file):

	}
	if err := b.exec.IsExecutable(multipathCmd); err != nil {
		return b.logger.ErrorRet(&commandNotFoundError{multipathCmd, err}, "failed")

Don't you want to rename this as well? (To start with capital 'C')

[beckmani]

---

remote/mounter/block_device_utils/mpath.go, line 127 at r1 (raw file):

	}
	dev := ""
	

remove this empty line

[beckmani]

---

remote/mounter/block_device_utils/block_device_utils_test.go, line 207 at r1 (raw file):

							- 34:0:0:1 sdc 8:32 active ready running`
			fakeExec.ExecuteReturnsOnCall(0, []byte(mpathOutput),
				nil)

No need for new line here.

[beckmani]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @olgashtivelman and @shay-berman)

[olgashtivelman]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @beckmani, @olgashtivelman, and @shay-berman)

---

remote/mounter/block_device_utils/block_device_utils_test.go, line 207 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

No need for new line here.

Done.

---

remote/mounter/block_device_utils/mpath.go, line 127 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

remove this empty line

Done.

---

remote/mounter/block_device_utils/mpath.go, line 137 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

Extra spaces?

Done.

---

remote/mounter/block_device_utils/mpath.go, line 259 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

Don't you want to rename this as well? (To start with capital 'C')

I change the CommandExecuteError, because that was the one I used. there are many other errors in the code that are in small letters (and cannot be exported) , I think we can change this one when we will need to.

[beckmani]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @shay-berman)

[beckmani]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @shay-berman)

[shay-berman]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @olgashtivelman)

---

remote/mounter/block_device_utils/block_device_utils_test.go, line 199 at r2 (raw file):

			Expect(err).To(HaveOccurred())
		})
		FIt("should return actual error when sg_inq command fails", func() {

Olga, please remove the FIt and put It instead. So it will not run only this unit test all the time.

---

remote/mounter/block_device_utils/mpath.go, line 136 at r2 (raw file):

			wwn, err := b.GetWwnByScsiInq(mpathFullPath)
			if err != nil {
				return "", b.logger.ErrorRet(&VolumeNotFoundError{volumeWwn}, "failed")

are you sure its better for you to move the identification of the error to the upper level?

could you please specific the ubiqutiy-k8s PR that will do that instead?