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

check if Memory is not nil for container stats #77656

Merged
merged 1 commit into from May 10, 2019

Conversation

@yastij
Copy link
Member

commented May 9, 2019

/kind bug
/sig node
What this PR does / why we need it:

Which issue(s) this PR fixes: Fixes #77655

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Check if container memory stats are available before accessing it
@yastij

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

/priority critical-urgent

@yastij

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@k8s-ci-robot k8s-ci-robot requested a review from tallclair May 9, 2019

@mattjmcnaughton
Copy link
Contributor

left a comment

/lgtm

Nice work here :)

I'm curious. do you think its worth adding a unit test ensuring that we don't panic when cstat.Memory is nil? That'd also give me slightly more confidence that this lack of nil check was the cause of the error this pr claims to fix.

Additionally, for more own understanding, can you share more about why cstat.Memory would sometimes be nil? Do we want to address the times when cstat.Memory is nil?

@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

/assign @dchen1107

@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Also, we can talk about this once its merged, but this seems like a good candidate for cherry picking to earlier releases.

@neolit123
Copy link
Member

left a comment

thanks @yastij

/lgtm
/assign @yujuhong

@yastij

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I’ll also raise an issue to increase test coverage for some parts related to cadvisor in the kubelet.

@tallclair

This comment has been minimized.

Copy link
Member

commented May 9, 2019

/approve
/cc @dashpole

@k8s-ci-robot k8s-ci-robot requested a review from dashpole May 9, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tallclair, yastij

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dashpole

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

/lgtm

@yastij

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@mattjmcnaughton - when getting stats for docker for example (or libcontainer in general) cadvisor talks to a cgroupManager IIRC (e.g. cgroupfs).
When opening the memory.stat file if it dosent exist we return just nil with no err, leading the cgroupManager to not error, and thus having stat with a nil memory.
On my cluster this was the issue, but another logic can lead to something like this I think

@k8s-ci-robot k8s-ci-robot merged commit ef9e794 into kubernetes:master May 10, 2019

20 checks passed

cla/linuxfoundation yastij authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@yastij yastij deleted the yastij:check-cstat-mem branch May 10, 2019

k8s-ci-robot added a commit that referenced this pull request May 10, 2019
Merge pull request #77729 from yastij/automated-cherry-pick-of-#77656…
…-upstream-release-1.14

Automated cherry pick of #77656: check if Memory is not nil for container stats
k8s-ci-robot added a commit that referenced this pull request May 14, 2019
Merge pull request #77731 from yastij/automated-cherry-pick-of-#77656…
…-upstream-release-1.12

Automated cherry pick of #77656: check if Memory is not nil for container stats
k8s-ci-robot added a commit that referenced this pull request May 21, 2019
Merge pull request #77730 from yastij/automated-cherry-pick-of-#77656…
…-upstream-release-1.13

Automated cherry pick of #77656: check if Memory is not nil for container stats
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.