Added a double validation for multipath device WWN discovered via multipath -ll

[tzurE]

In order to provide more confident that the mpath device we discover in multipath -ll, we added double validation that the WWN behind the discovered mpath device is really the WWN that we expect it to be. This way we sure(by getting the WWN directly from the device) that we going to mount the expected device.

This PR provide another layer of validation that we are using the right device.

---

This change is 

[coveralls]

Coverage increased (+0.06%) to 54.995% when pulling fe3ce0187356feec9c2758db3017441c4f2aa4c5 on fix/validate_wwn_using_sg_inq into 0b13f4c9ce4c32e8dff48436cca9fe59c7c5122a on dev.

[ranhrl]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

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

  }
  if _, err = b.exec.Stat(mpath); err != nil {
      return "", b.logger.ErrorRet(err, "Stat failed")

I know it's not in this code change but can we do better with this error message? Stat failed for volume [%s] ?

---

Comments from Reviewable

[ranhrl]

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[shay-berman]

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.

---

remote/mounter/block_device_utils/errors.go, line 47 at r1 (raw file):

}

type wrongVolumeFoundError struct {

rename to wrongDeviceFoundError

---

remote/mounter/block_device_utils/errors.go, line 49 at r1 (raw file):

type wrongVolumeFoundError struct {
  volName string
  reqVolName string

Add also the dev path as argument

---

remote/mounter/block_device_utils/errors.go, line 52 at r1 (raw file):

}
func (e *wrongVolumeFoundError) Error() string {
  return fmt.Sprintf("volume [%v] is not found. instead found vol [%s].", e.reqVolName, e.volName)

fix it to "multipath device [%s] was found as WWN [%v] via multipath -ll command, BUT sg_inq identify this device as different WWN [%s]. Check your multipathd."

---

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

GetWwnByScsiInq please call the var, SqInqWwn

---

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

  }
  if strings.ToLower(wwn) != strings.ToLower(volumeWwn){
      return "", b.logger.ErrorRet(&wrongVolumeFoundError{wwn, volumeWwn}, "failed")

please add here also dev as argument to see the whole picture not only the WWN

---

Comments from Reviewable

[tzurE]

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.

---

remote/mounter/block_device_utils/errors.go, line 47 at r1 (raw file):

Previously, shay-berman wrote…

rename to wrongDeviceFoundError

Done.

---

remote/mounter/block_device_utils/errors.go, line 49 at r1 (raw file):

Previously, shay-berman wrote…

Add also the dev path as argument

Done.

---

remote/mounter/block_device_utils/errors.go, line 52 at r1 (raw file):

Previously, shay-berman wrote…

fix it to "multipath device [%s] was found as WWN [%v] via multipath -ll command, BUT sg_inq identify this device as different WWN [%s]. Check your multipathd."

Done.

---

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

Previously, shay-berman wrote…

> GetWwnByScsiInq please call the var, SqInqWwn

Done.

---

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

Previously, shay-berman wrote…

please add here also dev as argument to see the whole picture not only the WWN

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.08%) to 55.014% when pulling e0b0342e8de68fefeaa8fb246d3206727eb24247 on fix/validate_wwn_using_sg_inq into 0b13f4c9ce4c32e8dff48436cca9fe59c7c5122a on dev.

[shay-berman]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

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

          Expect(err.Error()).To(MatchRegexp(cmdErr.Error()))
      })
      It("Discover fails if wrong wwn is found", func() {

very good UT

---

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

          cmd, args = fakeExec.ExecuteArgsForCall(1)
          Expect(cmd).To(Equal("sg_inq"))
          Expect(args).To(Equal([]string{"-p",  "0x83", "/dev/mapper/mpath"}))

not must, but its better to use the "/dev/mapper/" + result

---

remote/mounter/block_device_utils/errors.go, line 52 at r1 (raw file):

Previously, tzurE wrote…

Done.

Do you think we should tell Dima to add this string to the troubleshooting section in the userguide?

---

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

Previously, ranhrl (Ran Harel) wrote…

I know it's not in this code change but can we do better with this error message? Stat failed for volume [%s] ?

the err it should will be find not found. so i think we are ok here. But yes we can add here "Stat fileX failed". BTW this is the same Stat we should do after remove the multipath device.

---

Comments from Reviewable

[tzurE]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

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

Previously, shay-berman wrote…

not must, but its better to use the "/dev/mapper/" + result

Done.

---

remote/mounter/block_device_utils/errors.go, line 52 at r1 (raw file):

Previously, shay-berman wrote…

Do you think we should tell Dima to add this string to the troubleshooting section in the userguide?

yes

---

Comments from Reviewable