Skip to content

Add env var to override introspection bind address#501

Merged
mogren merged 1 commit intoaws:masterfrom
jacksontj:introspection_bindaddr
Jun 13, 2019
Merged

Add env var to override introspection bind address#501
mogren merged 1 commit intoaws:masterfrom
jacksontj:introspection_bindaddr

Conversation

@jacksontj
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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