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.

Sign up for GitHub

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

Jump to bottom

Fix getting cgroup pids #36551

Merged
merged 2 commits into from
Nov 10, 2016
Merged

Fix getting cgroup pids #36551

merged 2 commits into from
Nov 10, 2016

Conversation

timstclair
Copy link

@timstclair timstclair commented Nov 10, 2016 •
edited by k8s-oncall
Loading

Fixes #35214, #33232

Verified manually, but I didn't have time to run all the e2e's yet (will check it in the morning).

This should be cherry-picked into 1.4, and merged into 1.5 (/cc @saad-ali )

Fix fetching pids running in a cgroup, which caused problems with OOM score adjustments & setting the /system cgroup ("misc" in the summary API).

/cc @kubernetes/sig-node

This change is Reviewable

@timstclair timstclair added cherrypick-candidate priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 10, 2016
@timstclair timstclair added this to the v1.4 milestone Nov 10, 2016
@saad-ali
Copy link
Member

This should be cherry-picked into 1.4, and merged into 1.5 (/cc @saad-ali )

Ack. Bug fix ok for post-code freeze merge.

CC @jessfraz for 1.4 cherry pick

@jessfraz jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Nov 10, 2016
@jessfraz
Copy link
Contributor

awesome!!

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 10, 2016
feiskyer
feiskyer reviewed Nov 10, 2016
View reviewed changes
pkg/kubelet/cm/container_manager_linux.go
}
systemContainers = append(systemContainers, cont)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

remove this else block?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

Oops! Thanks, done.

feiskyer
feiskyer reviewed Nov 10, 2016
View reviewed changes
pkg/util/cgroups/cgroups_linux.go
return libcontainercgroups.GetPids(dir)
}

func getCgroupPath(cgroupPath string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: add documents for getCgroupPath and getCgroupParentPath

Copy link
Author

Choose a reason for hiding this comment

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

Done.

vishh
vishh reviewed Nov 10, 2016
View reviewed changes
pkg/util/cgroups/cgroups_linux.go
@@ -0,0 +1,73 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer cgroups logic to be contained inside pkg/kubelet/cm/

Copy link
Author

Choose a reason for hiding this comment

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

That creates a circular dependency, since pkg/kubelet/cm/ depends on pkg/util/oom

Copy link
Contributor

Choose a reason for hiding this comment

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

@timstclair will a sub-package of cm work? Having all cgroups logic in there will help with code structure.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I don't have a strong opinion on where the package is.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

pkg/kubelet/cm/container_manager_linux.go
}
systemContainers = append(systemContainers, cont)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 585673fb55b4522bcece6fce283876e8de2d393d. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit a2f18e6. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@timstclair
Copy link
Author

Verified OOM tests pass on all images with this fix.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit e58108b. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2016
@timstclair
Copy link
Author

Squashed commits. I kept the BUILD files in a separate commit for easier cherry-picking to 1.4. Reapplying LGTM.

@timstclair timstclair added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 10, 2016
@jessfraz
Copy link
Contributor

thanks!

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 89ebb2a into kubernetes:master Nov 10, 2016
This was referenced Nov 10, 2016
Merged
Closed
k8s-github-robot pushed a commit that referenced this pull request Nov 11, 2016
Merge pull request #36614 from timstclair/cvm-system
41e212e 
Automatic merge from submit-queue

Fix getting cgroup pids

Cherry-pick from #36551

Targetting v1.4.6

```release-note
Fix fetching pids running in a cgroup, which caused problems with OOM score adjustments & setting the /system cgroup ("misc" in the summary API).
```
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#36614 from timstclair/cvm-system
6fd723d 
Automatic merge from submit-queue

Fix getting cgroup pids

Cherry-pick from kubernetes#36551

Targetting v1.4.6

```release-note
Fix fetching pids running in a cgroup, which caused problems with OOM score adjustments & setting the /system cgroup ("misc" in the summary API).
```
@chentao1596 chentao1596 mentioned this pull request Dec 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@vishh vishh vishh approved these changes

Assignees

@jessfraz jessfraz

@vishh vishh

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
9 participants
@timstclair @saad-ali @jessfraz @k8s-ci-robot @k8s-github-robot @feiskyer @vishh @googlebot @tallclair