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

docker: use streaming stats collection to correct CPU stats #24229

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

tgross
Copy link
Member

@tgross tgross commented Oct 16, 2024

In #23966 we switched to the official Docker SDK for the docker driver. In the process we refactored code around stats collection to use the "one shot" version of stats. Unfortunately this "one shot" stats collection does not include the PreCPU stats, which are the stats from the previous read. This breaks the calculation we use to determine CPU ticks, because now we're subtracting 0 from the current value to get the delta.

Switch back to using the streaming stats collection. Add a test that fully exercises the TaskStats API.

Fixes: #24224
Ref: https://hashicorp.atlassian.net/browse/NET-11348


In addition to new tests here, I've tested manually with the setup described here #24224 (comment) and we get the expected results now.

In #23966 we switched to the official Docker SDK for the `docker` driver. In the
process we refactored code around stats collection to use the "one shot" version
of stats. Unfortunately this "one shot" stats collection does not include the
`PreCPU` stats, which are the stats from the previous read. This breaks the
calculation we use to determine CPU ticks, because now we're subtracting 0 from
the current value to get the delta.

Switch back to using the streaming stats collection. Add a test that fully
exercises the `TaskStats` API.

Fixes: #24224
Ref: https://hashicorp.atlassian.net/browse/NET-11348
@tgross tgross force-pushed the docker-stats-cpu-ticks branch from fee1cc1 to 565b511 Compare October 16, 2024 19:53
@tgross tgross marked this pull request as ready for review October 16, 2024 20:09
@tgross tgross added the backport/1.9.x backport to 1.9.x release line label Oct 16, 2024
Copy link
Contributor

@pkazmierczak pkazmierczak left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Docker task driver CPU metrics broken in Nomad 1.9.0
2 participants