IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

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

Closed olgashtivelman closed 6 years ago

olgashtivelman commented 6 years ago

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 Reviewable

coveralls commented 6 years ago

Coverage Status

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

beckmani commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

  }
  dev := ""

remove this empty line

beckmani commented 6 years ago

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.