Added a timeout for an execute call, and a timeout for sg_inq. + unit…

[tzurE]

…tests

---

This change is 

[coveralls]

Coverage decreased (-0.2%) to 54.773% when pulling d337b20 on fix/detach_cleanup into 838801b on dev.

[shay-berman]

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

---

utils/executor.go, line 74 at r1 (raw file):

}

func (e *executor) ExecuteWithTimeout(mSeconds int ,command string, args []string) ([]byte, error) {

maybe its better to get it in seconds(i don't see us playing with miliseconds, so one line below instead of *time.Millisecond you should use *time.Second

---

utils/executor.go, line 80 at r1 (raw file):

exec.CommandContext

i didn't find a way to kill it with sigKill(-9)
So we need to check if sigTerm(-15) is enough to kill the hanging sg_inq

---

utils/executor.go, line 86 at r1 (raw file):

	stdErr := stderr.Bytes()
	stdOut := stdout.Bytes()
	e.logger.Debug(

Please add before this line

   exceeded_timeout=false
   timeout_happened=""
if ctx.Err() == context.DeadlineExceeded {
            exceeded_timeout=true
	timeout_message="The command [%s] reached timeout setting [%s]msec, There for it was automatically killed"
}

and please add more logs to the logs.Args as follow:
{'timeout mSeconds', mSeconds},
{"exceeded_timeout", exceeded_timeout},
{"timeout_message",timeout_message }

---

Comments from Reviewable

[shay-berman]

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

---

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

	args := []string{"-p",  "0x83", dev}
	// add timeout in case the call never comes back.
	outputBytes, err := b.exec.ExecuteWithTimeout(1000, sgInqCmd, args)

1000 msec is only 1 second, I thing its too short. change it to 5 seconds(5000)
Alon\Ran do you agree?

---

utils/executor.go, line 95 at r1 (raw file):

		})

	return stdOut, err

BTW i am not sure if we reach to timeout, is the Run() return error if timeout exceeded? because if not there you have a bug here, so u need to return a specific error lets say with timeout_message

---

Comments from Reviewable

[ranhrl]

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

---

utils/executor.go, line 74 at r1 (raw file):

Previously, shay-berman wrote…

maybe its better to get it in seconds(i don't see us playing with miliseconds, so one line below instead of *time.Millisecond you should use *time.Second

I disagree, this is a better granularity, we might reduct the timeout to less then a second in the future and also in other future reuses of this class.

---

utils/executor.go, line 86 at r1 (raw file):

Previously, shay-berman wrote…

Please add before this line

   exceeded_timeout=false
   timeout_happened=""
if ctx.Err() == context.DeadlineExceeded {
            exceeded_timeout=true
	timeout_message="The command [%s] reached timeout setting [%s]msec, There for it was automatically killed"
}

and please add more logs to the logs.Args as follow:
{'timeout mSeconds', mSeconds},
{"exceeded_timeout", exceeded_timeout},
{"timeout_message",timeout_message }

I think that the proper way to do it as I found in the docs is using select:

select {
// case <-ctx.Done():
// return ctx.Err()
// case out <- v:
// }

---

Comments from Reviewable

[ranhrl]

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

---

utils/executor.go, line 95 at r1 (raw file):

Previously, shay-berman wrote…

BTW i am not sure if we reach to timeout, is the Run() return error if timeout exceeded? because if not there you have a bug here, so u need to return a specific error lets say with timeout_message

Shay please look at https://golang.org/pkg/os/exec/#CommandContext

---

Comments from Reviewable

[ranhrl]

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

---

utils/executor.go, line 80 at r1 (raw file):

Previously, shay-berman wrote…

exec.CommandContext

i didn't find a way to kill it with sigKill(-9)
So we need to check if sigTerm(-15) is enough to kill the hanging sg_inq

it is using kill https://golang.org/pkg/os/#Process.Kill not signal

---

Comments from Reviewable

[ranhrl]

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

---

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

Previously, shay-berman wrote…

1000 msec is only 1 second, I thing its too short. change it to 5 seconds(5000)
Alon\Ran do you agree?

sg_inq returns quickly if the device is attached. I guess you are afraid of iSCSI latencies? Maybe we can extend it to 2-3 seconds?

---

Comments from Reviewable

[ranhrl]

utils/executor.go, line 86 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

I think that the proper way to do it as I found in the docs is using select:

select {
// case <-ctx.Done():
// return ctx.Err()
// case out <- v:
// }

My mistake, just ignore this one

---

Comments from Reviewable

[ranhrl]

---

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

---

Comments from Reviewable

[ranhrl]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[shay-berman]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

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

Previously, ranhrl (Ran Harel) wrote…

sg_inq returns quickly if the device is attached. I guess you are afraid of iSCSI latencies? Maybe we can extend it to 2-3 seconds?

ok so the average is 4 seconds :-)
Tzur please change from 1 to 4 please.

---

utils/executor.go, line 80 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

it is using kill https://golang.org/pkg/os/#Process.Kill not signal

below it send signal to the process

---

utils/executor.go, line 86 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

My mistake, just ignore this one

I saw this select option, its alternative, but no need to do it.

---

utils/executor.go, line 92 at r1 (raw file):

			{"args", args},
			{"error", string(stdErr[:])},
			{"output", string(stdOut[:])},

I see that we are not printing the error of the Run command, its not good.
Please if u already touching this, please add line
{"exit status", err.Error()}

---

utils/executor.go, line 95 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

Shay please look at https://golang.org/pkg/os/exec/#CommandContext

why u refering me to this link?
the question is if the timeout expired does the Run command return error? and if so which?
I am not sure that it will return error

---

Comments from Reviewable

[tzurE]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

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

Previously, shay-berman wrote…

ok so the average is 4 seconds :-)
Tzur please change from 1 to 4 please.

Done.

---

utils/executor.go, line 74 at r1 (raw file):

Previously, ranhrl (Ran Harel) wrote…

I disagree, this is a better granularity, we might reduct the timeout to less then a second in the future and also in other future reuses of this class.

I agree with Ran. Also, milliseconds seems like a standard when it comes to timeouts of function calls. Lets keep it this way.

---

utils/executor.go, line 80 at r1 (raw file):

Previously, shay-berman wrote…

below it send signal to the process

It should work. I'll try it and tell you when I go over the logs.

---

utils/executor.go, line 86 at r1 (raw file):

Previously, shay-berman wrote…

I saw this select option, its alternative, but no need to do it.

Done.

---

utils/executor.go, line 92 at r1 (raw file):

Previously, shay-berman wrote…

I see that we are not printing the error of the Run command, its not good.
Please if u already touching this, please add line
{"exit status", err.Error()}

Done.

---

utils/executor.go, line 95 at r1 (raw file):

Previously, shay-berman wrote…

why u refering me to this link?
the question is if the timeout expired does the Run command return error? and if so which?
I am not sure that it will return error

In the link you can see that in case timeout exceeded, then run returns an error. we return this error.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.3%) to 54.667% when pulling 5648f56 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.3%) to 54.676% when pulling 9269b90 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.3%) to 54.676% when pulling f103be6 on fix/detach_cleanup into 838801b on dev.

[ranhrl]

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

---

utils/executor.go, line 75 at r4 (raw file):

func (e *executor) ExecuteWithTimeout(mSeconds int ,command string, args []string) ([]byte, error) {
	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(mSeconds)*time.Millisecond)

What does the time.Duration doing? I don't see that they use it in the example https://golang.org/pkg/os/exec/#CommandContext

---

Comments from Reviewable

[ranhrl]

utils/executor.go, line 95 at r1 (raw file):

What does the time.Duration doing? I
exactly

---

Comments from Reviewable

[tzurE]

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

---

utils/executor.go, line 75 at r4 (raw file):

Previously, ranhrl (Ran Harel) wrote…

What does the time.Duration doing? I don't see that they use it in the example https://golang.org/pkg/os/exec/#CommandContext

https://stackoverflow.com/questions/17573190/how-to-multiply-duration-by-integer

I saw it here.
But I also tried with 4000*time.milliseconds

could be that it's not accurate, I'm not sure actually.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.3%) to 54.676% when pulling c981ce2 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage increased (+4.9%) to 59.868% when pulling 6f6fad7 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage increased (+4.9%) to 59.869% when pulling 13725b6 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.5%) to 54.547% when pulling ed982e0 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.5%) to 54.559% when pulling d4c7e3a on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.5%) to 54.489% when pulling 11975a3 on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.6%) to 54.442% when pulling 5e5e45f on fix/detach_cleanup into 838801b on dev.

[coveralls]

Coverage decreased (-0.2%) to 54.771% when pulling 12703c5 on fix/detach_cleanup into 838801b on dev.