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 env var to override introspection bind address #501

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

jacksontj
Copy link
Contributor

@jacksontj jacksontj commented Jun 5, 2019

2b08772 (inadvertently?) changed the
bind address from :61678 to localhost:61678 which is backwards
incompatible and for those actually scraping the metrics via prometheus
makes the metrics endpoint entirely unusable (since prometheus isn't
local to each node in the cluster).

This path simply adds an env var to override the bind address.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jacksontj
Copy link
Contributor Author

I have added another commit which changes the default back because the introspection endpoint is largely useless when bound to localhost.

@mogren
Copy link
Contributor

mogren commented Jun 5, 2019

@jacksontj Hi, the prometheus metrics port has not changed, it was just moved to another file: ipamd/metrics.go. You can still query :61678/metrics on the aws-node to get the metrics.

PR #433 moved the introspection endpoint that returns data in json format and that is used for debugging the CNI's internal state to another port and bound it to localhost: ipamd/introspect.go. The debug script was updated to handle this.

Are there some additional metrics that you are missing that can only be found through the introspection endpoints?

@jacksontj jacksontj force-pushed the introspection_bindaddr branch from 8b48c9d to 7756375 Compare June 5, 2019 18:12
@jacksontj
Copy link
Contributor Author

jacksontj commented Jun 5, 2019

@mogren OIC, it changed the / response code for the metrics endpoint as well (used to be 200, now 404 -- this is what we were using for our liveliness check). I was seeing failures connecting initially but those seem to be all due to #502 .

The bind interface for this introspection endpoint was still changed without mention in the release notes. I'll remove the patch to change the default back, but I still think it would be good to add the env variable override -- but it isn't a huge blocker for my specific use-case

2b08772 (inadvertently?) changed the
bind address from `:61678` to `localhost:61678` which is backwards
incompatible and for those actually scraping the metrics via prometheus
makes the metrics endpoint entirely unusable (since prometheus isn't
local to each node in the cluster).

This path simply adds an env var to override the bind address.
@jacksontj jacksontj force-pushed the introspection_bindaddr branch from 7756375 to bcc21ae Compare June 5, 2019 20:18
@jacksontj
Copy link
Contributor Author

I also just added the new env var to README (apparently I missed that in the previous commit).

Copy link
Contributor

@tiffanyfay tiffanyfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@mogren mogren added this to the v1.6 milestone Jun 13, 2019
@mogren mogren merged commit 6176e35 into aws:master Jun 13, 2019
@jacksontj jacksontj deleted the introspection_bindaddr branch June 14, 2019 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants