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 usage metrics collection for static pods #3079

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Conversation

CharlyF
Copy link
Contributor

@CharlyF CharlyF commented Feb 6, 2019

What does this PR do?

This PR fixed the support for OpenShift 3.9 & containerd.
Though, it introduced a regression as usage metrics (memory.usage and filesystem.usage) for static pods are not collected anymore.

Because of kubernetes/kubernetes#61717 the /pods does not contain the ContainerStatus section for static pods. Therefore, the current logic was not able to get the container id of the static pod.

With this new logic, if the pod is static we use the pod uid (which is the only uid available).
Note though:

The UID of the static pod from the kubelet's API is not the same as the one from the APIServer. This is not impacting the logic here, just a behaviour to be aware of.

Motivation

Fix collection of usage mertrics for static pods.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If PR adds a configuration option, it has been added to the configuration file.

Additional Notes

Anything else we should know when reviewing?

@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #3079 into master will increase coverage by 5.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3079      +/-   ##
==========================================
+ Coverage   84.43%   89.56%   +5.13%     
==========================================
  Files         677       10     -667     
  Lines       36580     1160   -35420     
  Branches     4263      157    -4106     
==========================================
- Hits        30885     1039   -29846     
+ Misses       4493       81    -4412     
+ Partials     1202       40    -1162

mfpierre
mfpierre previously approved these changes Feb 6, 2019
Copy link
Contributor

@mfpierre mfpierre left a comment

Choose a reason for hiding this comment

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

LGTM, just wondering about the perf impact on the pod lookup for all the container metrics?

@CharlyF
Copy link
Contributor Author

CharlyF commented Feb 6, 2019

It should not be too greedy, I'll make a custom build to try on a big cluster to confirm.

@CharlyF CharlyF merged commit ff540b0 into master Feb 11, 2019
@CharlyF CharlyF deleted the fix-static-pods-usage branch February 11, 2019 14: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.

2 participants