Fix/mount idempotent improvements

[shay-berman]

This PR in conjunction with IBM/ubiquity-k8s#177 (it provides some mounter side facilities to be used in the ubiquity-k8s flex side to address flex side idempotent aspects)

This PR introduce the following:

1. fixes idempotent issues in the MountDeviceFlow:
   1.1. Skip mount if already mounted to the right mountpoint. (refactor IsDeviceMounted to return also the mountpoints of the mounted device)
   1.2. If mounted to wrong mountpoint it should fail.
   (note: set 20second timeout for identify if its already mounted)

2. Add timeout to "mount" command executions to prevent any potential hanging during the mount command. 2.1. mount to identify if device is mounted - timeout set to 20seconds.
   2.2. mount for actually mounting device set timeout to 2 minutes.

3. New interface mounter_factory.go [ubiquity/remote/mounter/mounter_factory.go] to get the relevant remote backend by backend name.
   (used by ubiquity-k8s/controller/controller.go)

4. Add new methods to the Executer interface : Lstat, IsDir, Symlink, IsSlink.
   (used by ubiquity-k8s/controller/controller.go to handle idempotent aspects in the flex mount side of thing)

5. Full unit testing coverage to all the items above.

6. Added few TODO lines for potential Idempotent items to improve in the umount flow.

How to test
Simulate the relevant idempotent issue in the mount flow, Including hanging mount commands to validate the timeouts as well.

---

This change is 

[coveralls]

Coverage decreased (-2.0%) to 52.759% when pulling 019bdfe on fix/idempotent_issues_in_umount into 7c17ff8 on dev.

[shay-berman]

Passed ubiquity sanity (with both this PR and IBM/ubiquity-k8s#177)

[tzurE]

Review status: 0 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

remote/mounter/mounter_factory.go, line 35 at r1 (raw file):

	} else if backend == resources.SCBE {
		return NewScbeMounter(pluginConfig.ScbeRemoteConfig), nil
	} else {

You don't need this else, if all the others are false it will just result in the last one.

---

Comments from Reviewable

[ranhrl]

Could we in any way use this module? https://github.com/kubernetes/kubernetes/blob/master/pkg/util/mount/mount.go

---

Review status: 0 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

remote/mounter/mounter_factory.go, line 35 at r1 (raw file):

Previously, tzurE wrote…

You don't need this else, if all the others are false it will just result in the last one.

Right, but I think it's more readable this way and prevents a mistake of adding more code at the end of the function by mistake.

---

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

const (
	NotMountedErrorMessage = "not mounted" // Error while umount device that is already unmounted
	TimeoutMilisecondMountCmdIsDeviceMounted = 20 * 1000 // max to wait for mount command

how did you determine the timeouts?

---

Comments from Reviewable

[ranhrl]

or even https://golang.org/src/syscall/syscall_linux.go?s=20289:20384#L786

---

Review status: 0 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[shay-berman]

way use this module
hi @ran, I also reviewed this k8s module as part of this PR for getting some code reuse (BTW there are other code we can reuse from k8s util.).
But i didn't want to add a new k8s dependency inside the ubiquity backend and it also a bit over kill. Potentially we can use it to reuse the code, I think its a potential refactoring then we can replace some other actions to make it more use full like mount, get device and others.

So yes we can use it, but we should do it as refactoring task later on (mentioned it in our refactor list).

---

Review status: 0 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

remote/mounter/mounter_factory.go, line 35 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

Right, but I think it's more readable this way and prevents a mistake of adding more code at the end of the function by mistake.

yes, explicit is better than implicit (PEP 20)

---

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

Previously, ranhrl (Ran Harel) wrote…

how did you determine the timeouts?

Just by experience, mount command to show the devices should be immidiate (so 20s it more then enough, I can do shorter)
and mount command also should be quick (its just mount and not creating file system, I could put it also 20s but I preferred to take higher number)

Agree?

---

Comments from Reviewable

[shay-berman]

@ranhrl review it today and approved.