Implement timeouts in checks and check hooks #893
Conversation
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
…u-go into feature/check-timeout Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
…u-go into feature/check-timeout
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
…u-go into feature/check-timeout Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
command/command.go
Outdated
| // Kill process and all of its children when the timeout has expired. | ||
| // context.WithTimeout will not kill child/grandchild processes | ||
| // (see issues tagged in https://github.com/sensu/sensu-go/issues/781), | ||
| // rather we will use a timer and utility_os package to perform full cleanup. |
There was a problem hiding this comment.
I think we can remove the reference to utility_os 😄
command/command.go
Outdated
| SetProcessGroup(cmd) | ||
| time.AfterFunc(time.Duration(execution.Timeout)*time.Second, func() { | ||
| if err := KillProcess(cmd); err != nil { | ||
| return |
There was a problem hiding this comment.
Should we log something here, in case we are not able to kill the process?
There was a problem hiding this comment.
I agree that something should happen here. We would return an error if the process is already done or if a failure writing to the process file occurs. Perhaps we should return immediately from the ExecuteCommand function, since this is an unexpected case.
There was a problem hiding this comment.
I'm ambivalent about whether or not we should return immediately, since the command executed successfully and it's more about cleaning up our mess 🤔
There was a problem hiding this comment.
I believe it would be something similar to https://github.com/enkessler/childprocess/blob/master/lib/childprocess/unix/process.rb#L22-L26?
There was a problem hiding this comment.
Thought about it some more, and on the rare instance that the process cleanly, or unexpectedly exits after the timeout but before we kill the process, we should still continue as normal with logging the error. 👍
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
|
Also, if possible, try to squash the commits when merging into master? |
What is this change?
Implements a timeout in check and check hook execution, adds the timeout field to CheckConfig.
Why is this change necessary?
Closes #781
Closes #879
Does your change need a Changelog entry?
Yas.
Do you need clarification on anything?
Nah.
Were there any complications while making this change?
There were several. Most of them are documented in #781.