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

Fix docker memory and cpu reporting in Windows #3043

Merged
merged 5 commits into from Jul 27, 2017

Conversation

danielnelson
Copy link
Contributor

@danielnelson danielnelson commented Jul 22, 2017

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@danielnelson danielnelson added this to the 1.4.0 milestone Jul 22, 2017
@danielnelson
Copy link
Contributor Author

closes #2457

@danielnelson danielnelson changed the title Improve docker unittests Update docker plugin Jul 25, 2017
@danielnelson
Copy link
Contributor Author

@georgyturevich @eesprit I have done a few improvements to the docker plugin here, and decided to bring in the unix and windows helper functions in order for us to behave more like the docker cli. This should also fix the reporting of unset stats on Windows. It would be great if both of you could double check that everything is still working as expected.

@georgyturevich
Copy link

@danielnelson

Hi Daniel, the code looks good to me, but I am not a big expert in Golang. If you already have some compiled binaries, I can try to test it on my environment.

@eesprit
Copy link
Contributor

eesprit commented Jul 26, 2017

@danielnelson the new docker client code seems to break when using the regular Unix Socket :

juil. 26 13:08:40 vp909doc1003 telegraf[170236]: 2017-07-26T11:08:40Z E! Error in plugin [inputs.docker]: error during connect: Get http://%2Fvar%2Frun%2Fdocker.sock/containers/json?limit=0: dial tcp: lookup /var/run/docker.sock: invalid domain name
juil. 26 13:08:40 vp909doc1003 telegraf[170236]: 2017-07-26T11:08:40Z E! Error in plugin [inputs.docker]: error during connect: Get http://%2Fvar%2Frun%2Fdocker.sock/info: dial tcp: lookup /var/run/docker.sock: invalid domain name

@eesprit
Copy link
Contributor

eesprit commented Jul 26, 2017

Might be due to creating an httpClient and passing it to NewClient().
For Unix Socket, the httpClient should be nil so the socket type is correctly configured (just a guess though) : https://github.com/moby/moby/blob/master/client/client.go#L173 which leads to https://github.com/docker/go-connections/blob/master/sockets/sockets.go#L21

That was the case when it was working (httpClient was nil) so I think that is the actual problem with the TLS patch.

A solution would be to parse the host string like it is done here : https://github.com/moby/moby/blob/master/client/client.go#L162
And then check the proto to test if unix or tcp and create le Client depending on that.

@danielnelson
Copy link
Contributor Author

@eesprit Thanks for catching that, would be pretty bad if I totally broke the docker plugin. I don't see a good way to pass just the tls.Config in without a client and transport, so I pulled the call to sockets.ConfigureTransport up into Telegraf.

@danielnelson
Copy link
Contributor Author

@georgyturevich Here is a Windows amd64 build for testing with.

@eesprit
Copy link
Contributor

eesprit commented Jul 27, 2017

@danielnelson Works fine here, and metrics are corrects even with offline CPUs.
I didn't test test in tcp mode and TLS, as I don't have a docker daemon configured in that mode on hand. I will see if I can take the time to setup one for testing though.

@danielnelson
Copy link
Contributor Author

@eesprit I'm splitting the tls off into it's own pull request, I'll mention you when I open it.

@danielnelson danielnelson merged commit d6cf9f4 into master Jul 27, 2017
@danielnelson danielnelson deleted the improve-docker-testing branch July 27, 2017 22:12
@danielnelson danielnelson changed the title Update docker plugin Fix docker memory and cpu reporting in Windows Jul 27, 2017
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.

None yet

3 participants