search

IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

Fix/mount idempotent improvements #196

Closed shay-berman closed 6 years ago

shay-berman commented 6 years ago

This PR in conjunction with https://github.com/IBM/ubiquity-k8s/pull/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 Reviewable

coveralls commented 6 years ago

Coverage Status

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

shay-berman commented 6 years ago

Passed ubiquity sanity (with both this PR and https://github.com/IBM/ubiquity-k8s/pull/177)

tzurE commented 6 years ago

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

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

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

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

@ranhrl review it today and approved.