UB-1407: added timeouts to umount operations

[olgashtivelman]

added timeout to some commands as part of the unmount process as follows:

unmount (timeout 30s)
clean up command : dmsetup and multipath -f (each should be with timeout of 30s)
multipath -ll (timeout 20s)
multipath -r (timeout 60s)
rescan -r (timeout 2 minutes)
iscsiadm rescan (timeout 1 minutes)

---

This change is 

[coveralls]

Coverage increased (+3.8%) to 51.961% when pulling f7fadb9 on fix/UB-1407_add_timeout_to_commands into 382fc6e on dev.

[beckmani]

---

remote/mounter/block_device_utils/fs.go, line 95 at r1 (raw file):

	args := []string{mpoint}
	if _, err := b.exec.ExecuteWithTimeout(30*1000, umountCmd, args); err != nil {

Please define as const (umountTimeout) - see MultipathTimeout in mpath.go

[beckmani]

---

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

const multipathCmd = "multipath"
const MultipathTimeout = 60*1000

Nice :-)

[beckmani]

---

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

	}
	args := []string{"-ll"}
	outputBytes, err := b.exec.ExecuteWithTimeout(20*1000, multipathCmd, args)

Should use MultipathTimeout

[beckmani]

---

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

func (b *blockDeviceUtils) Cleanup(mpath string) error {
	defer b.logger.Trace(logs.DEBUG)()
	cleanupTimeout := 30*1000

Should be defined as const

[beckmani]

---

remote/mounter/block_device_utils/rescan.go, line 44 at r1 (raw file):

	}
	args := []string{"-m", "session", "--rescan"}
	if _, err := b.exec.ExecuteWithTimeout(1*60*1000, rescanCmd, args); err != nil {

Should be defined as const

[beckmani]

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

[olgashtivelman]

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

---

remote/mounter/block_device_utils/fs.go, line 95 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

Please define as const (umountTimeout) - see MultipathTimeout in mpath.go

I defined const for numbers used more then once otherwise i just think its just abit of an overkill
but sure i will change it...

---

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

Previously, beckmani (Isaac Beckman) wrote…

Nice :-)

Done.

---

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

Previously, beckmani (Isaac Beckman) wrote…

Should use MultipathTimeout

Done.

---

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

Previously, beckmani (Isaac Beckman) wrote…

Should be defined as const

Done.

---

remote/mounter/block_device_utils/rescan.go, line 44 at r1 (raw file):

Previously, beckmani (Isaac Beckman) wrote…

Should be defined as const

Done.

[shay-berman]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @beckmani and @olgashtivelman)

---

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

			Expect(cmd).To(Equal("mount")) // second check is the umount
		})
		It("UmountFs fails if umount command missing", func() {

please add a test that will simulate timeout reached. at least for one command line.
So you will cover it as well in unit testing.

---

remote/mounter/block_device_utils/fs.go, line 34 at r2 (raw file):

	TimeoutMilisecondMountCmdIsDeviceMounted = 20 * 1000     // max to wait for mount command
	TimeoutMilisecondMountCmdMountFs         = 120 * 1000    // max to wait for mounting device
	UmountFsTimeout                          = 30 * 1000

please continue the terminolagy of the const name - TimeoutMilisecondUnmountFs and also add short comment please.
thanks

---

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

const multipathCmd = "multipath"
const MultipathTimeout = 60 * 1000
const Discovertimeout = 20 * 1000

small typo - try to use T capital.

[shay-berman]

@olgashtivelman finished to review - please apply few comments and then it LGTM

[olgashtivelman]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @beckmani and @shay-berman)

---

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

Previously, shay-berman wrote…

please add a test that will simulate timeout reached. at least for one command line.
So you will cover it as well in unit testing.

Done.

---

remote/mounter/block_device_utils/fs.go, line 34 at r2 (raw file):

Previously, shay-berman wrote…

please continue the terminolagy of the const name - TimeoutMilisecondUnmountFs and also add short comment please.
thanks

Done.

---

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

Previously, shay-berman wrote…

small typo - try to use T capital.

Done.

[shay-berman]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @beckmani)