Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "timeout" to CheckConfig #781

Closed
grepory opened this issue Jan 2, 2018 · 5 comments
Closed

Add "timeout" to CheckConfig #781

grepory opened this issue Jan 2, 2018 · 5 comments
Assignees
Milestone

Comments

@grepory
Copy link
Contributor

grepory commented Jan 2, 2018

CheckConfigs should have a Timeout field that specifies the maximum duration that a check process can take to return--after which we should terminate the external process and publish a check result with a status of 2 and an output of "Execution timed out".

@nikkictl
Copy link

nikkictl commented Jan 12, 2018

golang/go#22485, golang/go#18874, and golang/go#23019 document a similar issue I discovered in the command.go function ExecuteCommand. exec.CommandContextonly kills the immediate child process and not any additional spawned processes, causing cmd.Wait() to hang indefinitely. This does not impact invocations of single commands for ex. sleep 10, but will hang if execution.Command spawns multiple processes, ex. sleep 10; echo hanging. Some solutions include killing the process group which has OS specific implications, forcefully escaping cmd.Wait() which potentially leaves orphaned processes running, or not invoking the shell.

@nikkictl
Copy link

nikkictl commented Jan 16, 2018

I also find this 1.x bug particularly interesting... @cwjohnston @agoddard I'd love your input if you have some context on the 1.x issue, as it seems like it could be hit from a similar use case here. From golang/go#18874, "The difference in shell behavior here is that bash, zsh, etc will exec a single command in place, where as dash always executes in a sub process."

@grepory
Copy link
Contributor Author

grepory commented Jan 16, 2018

Similar behavior but different cause.

@nikkictl
Copy link

nikkictl commented Jan 16, 2018

@grepory ah.. I found it curious since we were still invoking a shell https://github.com/sensu/sensu-spawn/blob/f7149f9660f530ce777ae8ed4fb29b28eb2593a1/lib/sensu/spawn.rb#L83-L97.. trying to dig into how we kill child processes in 1.x...

How we appear to kill child processes on Windows and Unix in Sensu 1.x.

@palourde
Copy link
Contributor

Here's a common solution we found after our quick pairing this morning: https://github.com/hashicorp/consul/tree/master/agent/exec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants