Add script checks #12
Conversation
|
Seems the tests have failed since 10 months ago. Will look into it. |
README.md
Outdated
| 504 Gateway Not Found`. | ||
|
|
||
| ``` | ||
| health-checker --listener "0.0.0.0:6000" --script /path/to/script.sh |
There was a problem hiding this comment.
Can you show an example of how you'd pass args to the script?
|
Now that I think of it, we might also want to consider adding the HTTP check after all, especially from the ZK point of view. Then we could monitor ZK with a script and Exhibitor with HTTP. |
|
Does it make more sense to find a way to specify more than one script and then the script can do a simple CURL call itself? |
|
Definitely doable, we already support specifying multiple |
|
Agreed. You can see how I originally handled parallelization here. |
|
This is now pretty much done and ready for review. I think the test now cover most of the use cases, including the unhappy paths. |
README.md
Outdated
| @@ -14,15 +15,23 @@ a single TCP port, or an HTTP(S) endpoint. As a result, our use case just isn't | |||
| We wrote health-checker so that we could run a daemon on the server that reports the true health of the server by | |||
| attempting to open a TCP connection to more than one port when it receives an inbound HTTP request on the given listener. | |||
|
|
|||
| Using the `--script` -option, the `health-checker` can be extended to check many other targets. One concrete exeample is monitoring | |||
| | `--version` | Show the program's version | | | ||
|
|
||
| #### Example | ||
| If you execute a shell script, ensure you have a `shebang` line in your script, otherwise the script will fail with an `exec format error`. |
There was a problem hiding this comment.
Could our code catch that? Seems like a simple strings.HasPrefix("#!") would do the trick to avoid some confusing errors?
There was a problem hiding this comment.
Catch here is that we can execute other targets than shell scripts. We would then first have to identify the script as shell script (via the extension or some other way) and only after that check the prefix within. I think it would unnecessarily clutter the code and potentially lead to even more confusing errors 😄
| scripts := options.ParseScripts(scriptArr) | ||
|
|
||
| if len(ports) == 0 && len(scripts) == 0 { | ||
| return nil, OneOfParamsRequired{portFlag.Name, scriptFlag.Name} |
| scriptParams := []string{} | ||
| if len(commandArr) > 1 { | ||
| scriptParams = commandArr[1:] | ||
| } |
There was a problem hiding this comment.
For non-empty arrays, can be simplified to:
scriptName, scriptArgs := commandArr[0], commandArr[1:]
server/server.go
Outdated
| logger.Infof("Script %v successful", script) | ||
| } | ||
|
|
||
| waitGroup.Done() |
There was a problem hiding this comment.
Shouldn't this be defered right at the top of this function?
There was a problem hiding this comment.
You're right, it should.
Purpose of this PR is to document planned changes for adding a capability to execute health check script targets. PR #9 addresses this as part of the changes.
However, the idea with a smaller change is to have the capability available faster and add the advanced features (such as YAML config) incrementally. Feel free to disagree with the approach :)