search

cloud-gov / kubernetes-broker

Broker for kubernetes based services
Apache License 2.0
5 stars 6 forks source link

Wrap redis check in timeout #49

Closed cnelson closed 7 years ago

cnelson commented 7 years ago

When attempting to accept https://favro.com/card/1e11108a2da81e3bd7153a7a/18F-5060; I noticed that if the redis server is locked up (I simulated by sending a STOP signal to the process) that the redis-cli healthcheck will block forever and k8s won't ever get a return code to validate against.

This wraps the redis-cli call with timeout(1) to ensure it fails if redis doesn't respond within 1000ms

jmcarp commented 7 years ago

This makes sense, but I thought we wouldn't need it:

timeoutSeconds: Number of seconds after which the probe times out. Defaults to 1 second. Minimum value is 1.

It looks like timeoutSeconds is valid for exec probes, but maybe I'm misreading, or maybe that's not the case for 1.5. Anyway, merging.

cnelson commented 7 years ago

I thought the same thing, but in testing the k8s timeout doesn't seem to actually do anything? I let a container sit there for 10+ minutes and it was never marked unhealthy when in this state.

WDYT about doing the same for elastic? My testing shows the same failure mode with it's wget based health check

jmcarp commented 7 years ago

WDYT about doing the same for elastic? My testing shows the same failure mode with it's wget based health check

SGTM, but maybe we can also make a note about investigating this later.

cnelson commented 7 years ago

I think timeout doesn't work for exec (unless I'm misreading these issues/PRs) even though the docs indicate otherwise...

https://github.com/kubernetes/kubernetes/pull/27956#issuecomment-250717535

https://github.com/kubernetes/kubernetes/pull/33366

https://github.com/kubernetes/kubernetes/pull/35893