search

IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

Improve umount flow tolerance #163

Closed shay-berman closed 6 years ago

shay-berman commented 6 years ago

This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.9%) to 51.752% when pulling a34aac4ed6adf75afab313ab1bd8127018b87791 on fix/umountflow_ignore_already_unmounted_or_cleanned_mp into 3f7871a4ba5846b9f43f158c53997faaa3e78dab on dev.

ranhrl commented 6 years ago

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

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

  if _, err := b.exec.Execute(umountCmd, args); err != nil {
      if b.regExAlreadyMounted.MatchString(err.Error()) {
          b.logger.Info("Already umounted, so skip the umount operation.", logs.Args{{"mpoint", mpoint}})

Is it wise to count on specific error messages to return as text? I suggest 2 other options, either check the return code or check if it is mounted before running unmount (or after). What if the error message will change? For example, on mac you get "not currently mounted" which does not match your regexp.

Comments from Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 53.789% when pulling e2f555f9211d39ec201bb9928b349470ad09279c on fix/umountflow_ignore_already_unmounted_or_cleanned_mp into 3f7871a4ba5846b9f43f158c53997faaa3e78dab on dev.

shay-berman commented 6 years ago

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

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

Previously, ranhrl (Ran Harel) wrote…
Is it wise to count on specific error messages to return as text? I suggest 2 other options, either check the return code or check if it is mounted before running unmount (or after). What if the error message will change? For example, on mac you get "not currently mounted" which does not match your regexp.

Comments from Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 54.06% when pulling 55412d024989114be50fc043e8c1735aa814adae on fix/umountflow_ignore_already_unmounted_or_cleanned_mp into 3f7871a4ba5846b9f43f158c53997faaa3e78dab on dev.

ranhrl commented 6 years ago

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

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

Previously, shay-berman wrote…
- we support only ubuntu and rhel, and in both this is the error message in this case - anyway, it took me 3 hours to change it from identify mounted based on error message to identify it via mount command. Took me while because I am in jet leg time :-) - Ran please review again

Looks good to me. The only philosophical question I have is do we want to check if the mount point exists before umount every time or just if umount fails. Since most of the times we do expect it to be mounted, it might be wiser, what do you think?

Comments from Reviewable

shay-berman commented 6 years ago

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

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

Previously, ranhrl (Ran Harel) wrote…
Looks good to me. The only philosophical question I have is do we want to check if the mount point exists before umount every time or just if umount fails. Since most of the times we do expect it to be mounted, it might be wiser, what do you think?

you are killing me :-)

I think i agree with you on that. Please tell lior\Tzur to apply this change if u want. I have no time for this now.

Comments from Reviewable

ranhrl commented 6 years ago
:lgtm:

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

Comments from Reviewable