Improve umount flow tolerance

[shay-berman]

---

This change is 

[coveralls]

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

[ranhrl]

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]

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

[shay-berman]

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.

• 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

---

Comments from Reviewable

[coveralls]

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

[ranhrl]

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]

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]

---

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

---

Comments from Reviewable