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

UPSTREAM: google/cadvisor: 2979: container/libcontainer: fix schedulerStatsFromProcs hogging memory and wrong stats #13

Merged

Conversation

kolyshkin
Copy link

Instead of passing a few parameters, make this function a method,
to simplify its calling as well as further development.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 2b607f0)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The logic of the existing code of schedulerStatsFromProcs is to provide
a cumulative stats for all the processes inside a container. Once the
process is dead, its stat entry is no longer updated, but still used in
totals calculation. This creates two problems:

 - pidsMetricsCache map is ever growing -- in case of many short-lived
   processes in containers this can impact kubelet memory usage a lot;

 - in case a new process with the same PID appears (as a result of PID
   reuse), the stats from the old one are overwritten, resulting in
   wrong totals (e.g. they can be less than previous, which should not
   ever be the case).

To kill these two birds with one stone, let's accumulate stats from dead
processes in pidsMetricsSaved, and remove them from the pidsMetricsCache.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 9a22e3e)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@rphillips
Copy link

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Oct 29, 2021

@rphillips: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mrunalp
Copy link
Member

mrunalp commented Oct 29, 2021

/lgtm
/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 29, 2021

@mrunalp: changing LGTM is restricted to collaborators

In response to this:

/lgtm
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sjenning
Copy link

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2021
@sjenning sjenning added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2021
@sjenning sjenning merged commit ace0aee into openshift:openshift-4.8-cdavisor-v0.39.2 Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
4 participants