Filter out duplicated container stats #54606

miaoyq · 2017-10-26T06:53:04Z

What this PR does / why we need it:

Which issue this PR fixes *
fixes #53514

Special notes for your reviewer:

Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn

k8s-ci-robot · 2017-10-26T06:53:05Z

@miaoyq: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

xiangpengzhao · 2017-10-26T07:17:09Z

/release-note-none
/ok-to-test

dashpole · 2017-10-26T16:35:20Z

based on #53514 (comment), we only want to exclude terminated containers if they are duplicated. I.E. if we have stats for another container in that pod with the same name.

miaoyq · 2017-10-26T23:48:02Z

@dashpole Got it. Thank you for reminding me. : )

miaoyq · 2017-11-01T14:51:52Z

@dashpole I have updated the code and add unit test for this, PTAL, Thanks. :)

dashpole · 2017-11-01T16:56:12Z

pkg/kubelet/stats/cri_stats_provider.go

+		}
+		i := 0
+		for ; i < len(refs); i++ {
+			if containerStatsMap[refs[i].Id].Cpu != nil || containerStatsMap[refs[i].Id].Memory != nil {


Is it ever possible to have more than one container in refs running at once? Why cant we just take the last element from the list?

@dashpole Thanks for review. I have the same doubts, but cadvisor stats do like this.
Is there any different between CRI stats and cadvisor stats? ping @yguo0905

ping @yguo0905
Could you please help us solve our doubts, thanks so much. :-)

It would allow us to detect the error case where multiple containers with the same name are active at the same time. See #47853 (comment).

miaoyq · 2017-11-07T05:55:23Z

/assign @Random-Liu

yguo0905 · 2017-11-07T17:28:51Z

Let's wait for the answer to #53514 (comment).

If we'd like to do the filtering for cadvisor and cri stats, is it possible to have a single removeTerminatedContainer works for both cases?

miaoyq · 2017-11-09T02:36:58Z

Thanks @yguo0905 , I find it's hard to have a single removeTerminatedContainer works for both cases, because the objects filtered are different about cadvisor and cri.
If you have good ideas, please let me know. Thanks :-)

Random-Liu

LGTM other than the comments.

Random-Liu · 2017-11-17T01:53:29Z

pkg/kubelet/stats/cri_stats_provider.go

+	})
+	for _, container := range containers {
+		// Filter out the container that isn't managed by a Pod
+		if container.PodSandboxId == "" {


I think we can assume that all container stats returned by CRI container runtime are for kubelet. If not, it is an error.

Do we delete this logic? or return an error here?

I think delete the logic is fine. :)

Random-Liu · 2017-11-17T02:20:39Z

pkg/kubelet/stats/cri_stats_provider.go

+			continue
+		}
+		i := 0
+		for ; i < len(refs); i++ {


I prefer check container state and only keep running containers, since we've already got container state.

It seems that this is not addressed.

I have not got what you mean clearly, done.

miaoyq · 2017-11-17T06:55:54Z

/test pull-kubernetes-node-e2e

Random-Liu · 2017-11-17T22:52:24Z

pkg/kubelet/stats/cri_stats_provider.go

+			result = append(result, refs[0])
+			continue
+		}
+		for i := 0; i < len(refs); i++ {


Can we add one more logic? If no running container found, we should add the latest container.

Sorry for this, have added, PTAL, :-)

Random-Liu · 2017-11-17T22:52:41Z

LGTM with one last nit.

xiangpengzhao · 2017-11-17T23:09:34Z

/priority important-soon
/sig node
/retest

k8s-github-robot · 2017-11-17T23:10:25Z

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@Random-Liu @dashpole @miaoyq @tmrts @kubernetes/sig-node-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 6 days, the pull request will be moved out of the v1.9 milestone.

Pull Request Labels

sig/node: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>

Random-Liu · 2017-11-18T02:43:46Z

/lgtm

k8s-github-robot · 2017-11-18T02:44:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: miaoyq, Random-Liu

Associated issue: 53514

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [Random-Liu]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Random-Liu · 2017-11-18T02:45:39Z

@miaoyq Thanks for working on this!

xiangpengzhao · 2017-11-18T06:50:24Z

/retest

k8s-github-robot · 2017-11-18T07:20:35Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested a review from Random-Liu October 26, 2017 06:53

k8s-github-robot assigned dashpole and tmrts Oct 26, 2017

miaoyq force-pushed the filter-out-duplicated-container-stats branch from 9f1fcca to 365c2e7 Compare October 26, 2017 06:54

miaoyq force-pushed the filter-out-duplicated-container-stats branch 2 times, most recently from 1e210f6 to d4125b5 Compare October 30, 2017 12:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 1, 2017

dashpole reviewed Nov 1, 2017

View reviewed changes

Random-Liu self-assigned this Nov 7, 2017

Random-Liu reviewed Nov 17, 2017

View reviewed changes

miaoyq force-pushed the filter-out-duplicated-container-stats branch 3 times, most recently from 23c193a to 1b9b2f2 Compare November 17, 2017 05:54

Random-Liu reviewed Nov 17, 2017

View reviewed changes

Random-Liu added this to the v1.9 milestone Nov 17, 2017

Random-Liu added the kind/bug Categorizes issue or PR as related to a bug. label Nov 17, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 17, 2017

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 17, 2017

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels Nov 17, 2017

miaoyq force-pushed the filter-out-duplicated-container-stats branch from 1b9b2f2 to 11cf390 Compare November 18, 2017 01:44

Yanqiang Miao added 2 commits November 18, 2017 09:51

Filter out duplicated container stats

3bd315d

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>

Add unit test for "filter out duplicated container stats"

11cf390

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2017

k8s-github-robot merged commit 5e508b3 into kubernetes:master Nov 18, 2017

steven-sheehy mentioned this pull request Feb 21, 2019

cri_stats_provider: Implement removeTerminatedContainer correctly #74336

Merged

rphillips mentioned this pull request Sep 4, 2019

kubelet: fix removeTerminatedContainers and removeTerminatedPods #82294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out duplicated container stats #54606

Filter out duplicated container stats #54606

miaoyq commented Oct 26, 2017

k8s-ci-robot commented Oct 26, 2017

xiangpengzhao commented Oct 26, 2017

dashpole commented Oct 26, 2017

miaoyq commented Oct 26, 2017

miaoyq commented Nov 1, 2017

dashpole Nov 1, 2017

miaoyq Nov 1, 2017

miaoyq Nov 7, 2017

yguo0905 Nov 7, 2017

miaoyq commented Nov 7, 2017

yguo0905 commented Nov 7, 2017

miaoyq commented Nov 9, 2017

Random-Liu left a comment

Random-Liu Nov 17, 2017

miaoyq Nov 17, 2017 •

edited

Loading

Random-Liu Nov 17, 2017

Random-Liu Nov 17, 2017

miaoyq Nov 17, 2017

Random-Liu Nov 17, 2017

miaoyq Nov 17, 2017

miaoyq commented Nov 17, 2017

Random-Liu Nov 17, 2017

miaoyq Nov 18, 2017

Random-Liu commented Nov 17, 2017

xiangpengzhao commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

Random-Liu commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

Random-Liu commented Nov 18, 2017

xiangpengzhao commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

Filter out duplicated container stats #54606

Filter out duplicated container stats #54606

Conversation

miaoyq commented Oct 26, 2017

k8s-ci-robot commented Oct 26, 2017

xiangpengzhao commented Oct 26, 2017

dashpole commented Oct 26, 2017

miaoyq commented Oct 26, 2017

miaoyq commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miaoyq commented Nov 7, 2017

yguo0905 commented Nov 7, 2017

miaoyq commented Nov 9, 2017

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miaoyq Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miaoyq commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 17, 2017

xiangpengzhao commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

Random-Liu commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

Random-Liu commented Nov 18, 2017

xiangpengzhao commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

miaoyq Nov 17, 2017 •

edited

Loading