Adds new config to make script checks opt-in, updates documentation. #3284
Conversation
agent/agent_test.go
Outdated
| } | ||
|
|
||
| // Ensure we don't have a check mapping | ||
| _, ok := a.state.Checks()["mem"] |
agent/agent_test.go
Outdated
| t.Parallel() | ||
|
|
||
| cfg := TestConfig() | ||
| cfg.CheckEnableExec = false |
There was a problem hiding this comment.
Isn't that the default?
Update: I think this should use the normal default and tests that uses Script tests should set this flag to `true. The test config should use faster timings and in-mem storage but should otherwise have the same behavior than the default runtime config.
agent/config.go
Outdated
| @@ -625,6 +625,10 @@ type Config struct { | |||
| // true, we ignore the leave, and rejoin the cluster on start. | |||
| RejoinAfterLeave bool `mapstructure:"rejoin_after_leave"` | |||
|
|
|||
| // CheckEnableExec controls whether health checks which execute scripts | |||
| // are enabled. This includes regular script checks and Docker checks. | |||
| CheckEnableExec bool `mapstructure:"check_enable_exec"` | |||
There was a problem hiding this comment.
All other toggles are EnableXXX or DisableXXX. We also have a DisableRemoteExec flag which seems to be related to events but I'm not sure whether it relates to this as well. I suggest EnableScriptChecks (or Check singular) instead unless you have other checks in mind which involve execution of external commands.
There was a problem hiding this comment.
That's good feedback. I went around on this a bit and sort of went with something that would go with a check in structure if we ever made one but your suggestion fits more with what we have. I'll change this.
agent/consul/structs/check_type.go
Outdated
| @@ -47,6 +47,11 @@ func (c *CheckType) Valid() bool { | |||
| return c.IsTTL() || c.IsMonitor() || c.IsHTTP() || c.IsTCP() || c.IsDocker() | |||
| } | |||
|
|
|||
| // IsExec checks if this is a check that execs some kind of script. | |||
| func (c *CheckType) IsExec() bool { | |||
agent/testagent.go
Outdated
| @@ -314,6 +314,7 @@ func TestConfig() *Config { | |||
| cfg.Datacenter = "dc1" | |||
| cfg.Bootstrap = true | |||
| cfg.Server = true | |||
| cfg.CheckEnableExec = true | |||
There was a problem hiding this comment.
why? can't we just add it to the handful of tests that need it? It is the new default after all. We should see what breaks.
testutil/server.go
Outdated
| @@ -124,7 +125,8 @@ func defaultServerConfig() *TestServerConfig { | |||
| Server: randomPort(), | |||
| RPC: randomPort(), | |||
| }, | |||
| ReadyTimeout: 10 * time.Second, | |||
| CheckEnableExec: true, | |||
| The check should be paired with an invocation interval. The shell on which the check | ||
| has to be performed is configurable which makes it possible to run containers which | ||
| have different shells on the same host. Check output for Docker is limited to | ||
| 4K. Any output larger than this will be truncated. In Consul 0.9.0 and later, the agent |
| have different shells on the same host. Check output for Docker is limited to | ||
| 4K. Any output larger than this will be truncated. In Consul 0.9.0 and later, the agent | ||
| must be configured with [`check_enable_exec`](/docs/agent/options.html#_check_enable_exec) | ||
| set to `true` in order to enable script checks. |
There was a problem hiding this comment.
Is there any other way to perform (other than a script check) to perform this Docker check? If not, then the last sentence should be to enable Docker health checks.
| @@ -81,7 +81,7 @@ All together, these settings yield a | |||
| ```text | |||
| vagrant@n1:~$ consul agent -server -bootstrap-expect=1 \ | |||
| -data-dir=/tmp/consul -node=agent-one -bind=172.20.20.10 \ | |||
| -config-dir=/etc/consul.d | |||
| -check-enable-exec=true -config-dir=/etc/consul.d | |||
There was a problem hiding this comment.
Is this required for this example?
There was a problem hiding this comment.
Yeah we add a script check on the following example so it's needed to make that work. Same for the other one.
There was a problem hiding this comment.
I added a note about why it's there in-line.
| @@ -102,7 +102,7 @@ All together, these settings yield a | |||
|
|
|||
| ```text | |||
| vagrant@n2:~$ consul agent -data-dir=/tmp/consul -node=agent-two \ | |||
| -bind=172.20.20.11 -config-dir=/etc/consul.d | |||
| -bind=172.20.20.11 -check-enable-exec=true -config-dir=/etc/consul.d | |||
|
LGTM |
Fixes #3087.