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

pod and qos level cgroup support #31546

Merged

Conversation

derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Aug 26, 2016

[Kubelet] Add alpha support for `--cgroups-per-qos` using the configured `--cgroup-driver`. Disabled by default.

This change is Reviewable

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@derekwaynecarr derekwaynecarr added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 26, 2016
@derekwaynecarr derekwaynecarr added this to the v1.5 milestone Aug 26, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 26, 2016
@bgrant0607 bgrant0607 assigned dchen1107 and unassigned bgrant0607 Aug 30, 2016
@derekwaynecarr
Copy link
Member Author

Picking this back up now for 1.5 release. Will start sending discrete PRs.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 26, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2016
@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Sep 27, 2016

This PR is gated on #29049

This PR is replacing the referenced item as it abstracts across cgroup drivers which required significant changes in the interim.

@derekwaynecarr
Copy link
Member Author

opencontainers/runc pr changes here: opencontainers/runc#1084

@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Sep 27, 2016

Recording so I do not forget:

There are two issues that need resolution around naming.

  • When/how to convert the "internal" cgroup name (BestEffort/pod-xyz) to the "concrete" cgroup name that may be modified by the driver (i.e. "/BestEffort.slice/BestEffort-pod_xyz.slice")?

When constructing the sandbox config, we need to have a way to convert internal names easily to concrete names, but always keeping the linux cgroupfs style syntax. Exposing GetConcreteContainerName on PodContainerManager that does that conversion seems practical to me.

  • How individual shims need to handle conversion of the concrete names to their driver specific syntax?

In this case, the dockershim would need to run docker info to see its configured cgroup driver, and then call a utility to convert that to the proper slice form in the case of systemd (i.e /BestEffort.slice/BestEffort-pod_xyz.slice) becomes BestEffort-pod_xyz.slice. That conversion can be shared in libcontainer utility.

/cc @vishh as I think this captures what we agreed upon.

@derekwaynecarr derekwaynecarr force-pushed the systemd-pod-cgroups branch 2 times, most recently from 24d320a to babd430 Compare October 3, 2016 21:11
@derekwaynecarr
Copy link
Member Author

@dubstack @vishh -- i can manually verify pod level cgroup creation AND deletion works as expected on systemd driver now, need to do a lot more testing and cleanup, but this will be the basis for breaking this uber PR into a number of smaller pulls. either way, i was happy to see everything is getting cleaned up on the cgroupfs filesystem as expected.

@derekwaynecarr derekwaynecarr force-pushed the systemd-pod-cgroups branch 2 times, most recently from 99cbd72 to a645ce2 Compare October 4, 2016 16:25
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 4, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2016
@derekwaynecarr derekwaynecarr force-pushed the systemd-pod-cgroups branch 7 times, most recently from 0a65fa8 to 1d0846b Compare November 2, 2016 03:31
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2016
@derekwaynecarr derekwaynecarr removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2016
@derekwaynecarr
Copy link
Member Author

@vishh -- this should be good.

@vishh
Copy link
Contributor

vishh commented Nov 2, 2016

:lgtm:


Reviewed 9 of 30 files at r13, 1 of 10 files at r14, 4 of 11 files at r15, 20 of 20 files at r18.
Review status: all files reviewed at latest revision, 31 unresolved discussions.


Comments from Reviewable

@vishh vishh added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Nov 2, 2016
@vishh
Copy link
Contributor

vishh commented Nov 2, 2016

Bumping priority since @derekwaynecarr cannot shepherd this PR beyond today until the code freeze and this PR is necessary for v1.5.

@vishh vishh added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 2, 2016
@vishh
Copy link
Contributor

vishh commented Nov 2, 2016

@derekwaynecarr Given that this feature is alpha, why not use the feature gates flag instead of adding a separate flag for cgroups-per-qos?

@vishh vishh removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 2, 2016
@vishh
Copy link
Contributor

vishh commented Nov 2, 2016

Chatted with @derekwaynecarr offline. We decided to prefix the cgroup related flags with an experimental string to better convey the support for this feature to users.

@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 41b5fe8 into kubernetes:master Nov 3, 2016
mtaufen added a commit to mtaufen/kubernetes that referenced this pull request Nov 3, 2016
…md-pod-cgroups"

This reverts commit 41b5fe8, reversing
changes made to db68b90.
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
…groups

Automatic merge from submit-queue

pod and qos level cgroup support

```release-note
[Kubelet] Add alpha support for `--cgroups-per-qos` using the configured `--cgroup-driver`. Disabled by default.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants