internal: Enable Envoy healthchecks #759
Conversation
| @@ -50,8 +56,6 @@ spec: | |||
| command: ["contour"] | |||
| args: | |||
| - bootstrap | |||
| # Uncomment the statsd-enable to enable prometheus metrics | |||
| #- --statsd-enable | |||
| # Uncomment to set a custom stats emission address and port | |||
| #- --stats-address=0.0.0.0 | |||
| #- --stats-port=8002 | |||
There was a problem hiding this comment.
@stevesloka should we change the commented out stats-port to something besides 8002?
There was a problem hiding this comment.
We should actually remove those values, those are the defaults if nothing is specified.
| @@ -9,9 +9,6 @@ avoid exposing the entire admin interface to Prometheus (and other workloads in | |||
| the cluster), Contour configures a static listener that sends traffic to the | |||
| stats endpoint and nowhere else. | |||
|
|
|||
| To enable the static listener, set the `--statsd-enabled` flag on the Contour | |||
There was a problem hiding this comment.
Should we add docs here for --stats-address and --stats-port?
There was a problem hiding this comment.
Yup good point @rosskukulinski, I think we missed those docs when this was introduced initally.
|
@stevesloka metrics can still be enabled, just that the end-user must now specify an address & port. Right? |
|
Will this cause issues if statsd is not deployed? I believe that flag was introduced when we added the ability to configure a statsd sink. |
stevesloka
force-pushed
the
healthcheck
branch
from
13ef0e5
to
28127af
Compare
|
Although it seems like it was controlling more than just the statsd piece. |
|
My idea to reuse the stats listener was to reduce the number of listeners. I could rever this and introduce another port if folks think that makes sense. This now "enables" the stats listener, if the forwarder isn't turned on then its sort of a no-op. Specifying the the IP/Port is a way to override the default (if someone would like to). |
stevesloka
force-pushed
the
healthcheck
branch
from
28127af
to
c081381
Compare
|
I think reusing the port is fine. What might be an issue is unconditionally enabling Envoy's statsd feature where it pushes metrics to a statsd sink. What if statsd is not running? Does envoy error out? Is it just going to continue trying to push metrics even though nothing is listening for them? An alternative would be to move the "if statsdEnabled |
|
Yeah, we could do that. Right now just enabling the statsd interface will send UDP to nowhere. But if you don't want that turned on then I agree we should probably allow it to be disabled, however, the Prometheus stats interface is still available. |
|
I think my vote would go towards having the ability to enable the statsd-sink feature, instead of always blasting packets at a potentially non-existent statsd. |
|
+1 for disabling statsd-sink feature. It's a CPU hog at scale. |
stevesloka
force-pushed
the
healthcheck
branch
2 times, most recently
from
35859a7
to
193a4ce
Compare
|
@alexbrand @rosskukulinski I'm retesting this now, but should be updated so that the |
|
@stevesloka ping, is this ready to merge? |
Signed-off-by: Steve Sloka <steves@heptio.com>
stevesloka
force-pushed
the
healthcheck
branch
from
193a4ce
to
76c391d
Compare
|
yeah should be good now @davecheney, I just rebased it against master. |
|
LGTM. Thank you. |
Fixes #695 by enabling a
/healthzendpoint in Envoy to enable Kubernetes readiness probes. This also removes the ability to enable metrics since I'm reusing the same port to reduce the number of ports that we need to configure for Envoy.Signed-off-by: Steve Sloka steves@heptio.com