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

[Kubelet] Improving QOS in kubelet by introducing QoS level Cgroups - --cgroups-per-qos #27853

Merged
merged 1 commit into from
Jul 15, 2016
Merged

[Kubelet] Improving QOS in kubelet by introducing QoS level Cgroups - --cgroups-per-qos #27853

merged 1 commit into from
Jul 15, 2016

Conversation

dubstack
Copy link

@dubstack dubstack commented Jun 22, 2016 •
edited
Loading

This PR is tied to this upstream issue #27204
Please note that only the last commit is unique to this PR. The first two commits are from previous PR's.

It introduces a new flag in the Kubelet which can be used to specify if the user wants to use the QoS cgroup hierarchy.

cc @kubernetes/sig-node

@dubstack dubstack added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 22, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 22, 2016
@dubstack dubstack added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 22, 2016
vishh
vishh reviewed Jun 23, 2016
View reviewed changes
pkg/kubelet/cm/types.go
}

// QOSContainersInfo hold the names of containers per qos
type QOSContainersInfo struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why is this required?

Copy link
Author

Choose a reason for hiding this comment

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

I thought that this would be good have. Once the top level QOS containers are created they won't change. So we can store there names in the cm and it can be used for getting the absolute names of the pod containers.

@vishh
Copy link
Contributor

vishh commented Jun 23, 2016

I added a few comments. Ping me back once those comments are addressed

@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from 274b013 to 37093d9 Compare June 24, 2016 12:58
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2016
@dubstack
Copy link
Author

@vishh I added the e2e test, but I am not sure how to access the host node cgroup fs information from inside the container. Is there a hack to achieve that? I have addressed all your other comments except changing the default of cgroup-root to "/".

@vishh
Copy link
Contributor

vishh commented Jun 24, 2016

You can mount in /sys/fs/cgroup using a HostPath volume

On Fri, Jun 24, 2016 at 6:07 AM, Buddha Prakash notifications@github.com
wrote:

@vishh https://github.com/vishh I added the e2e test, but I am not sure
how to access the host node cgroup fs information from inside the
container. Is there a hack to achieve that? I have addressed all your other
comments except changing the default of cgroup-root to "/".

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27853 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGvIKOxjreoIiR_grw3xEBb9KRNspFWDks5qO9apgaJpZM4I7o6F
.

derekwaynecarr
derekwaynecarr reviewed Jun 27, 2016
View reviewed changes
pkg/apis/componentconfig/types.go
// Enable Qos based Cgroup hieracrchy: top level cgroups for Qos Classes
// And all pods are broguht up under a top level Qos cgroup
// based on the Qos class they belong to.
EnableQosCgroups bool `json:"enablePodCgroups,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

see mismatch between var name and json.

Copy link
Author

Choose a reason for hiding this comment

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

Ack! Thanks!

@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from 37093d9 to 7e6241f Compare June 27, 2016 23:33
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 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 Jun 27, 2016
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch 3 times, most recently from db4b9c6 to 107f2f5 Compare June 28, 2016 02:44
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch 2 times, most recently from cb4c129 to 227f9b9 Compare July 13, 2016 00:14
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2016
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from 227f9b9 to e7183f2 Compare July 13, 2016 03:35
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 13, 2016
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from e7183f2 to d0ee9ca Compare July 13, 2016 18:06
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2016
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from d0ee9ca to 012385f Compare July 13, 2016 21:36
vishh
vishh reviewed Jul 14, 2016
View reviewed changes
pkg/apis/componentconfig/deep_copy_generated.go
@@ -1,300 +0,0 @@
// +build !ignore_autogenerated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file being deleted?

Copy link
Author

Choose a reason for hiding this comment

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

I think a PR went in which included the auto code generation in the build system.

@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from 012385f to ee329aa Compare July 14, 2016 22:29
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 14, 2016
@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 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 Jul 15, 2016
@dubstack dubstack force-pushed the dubstack-inject-qos-creation2 branch from ee329aa to 5000e74 Compare July 15, 2016 17:02
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 15, 2016
@dubstack
Copy link
Author

Adding LGTM after Rebase.

@dubstack dubstack added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 15, 2016

GCE e2e build/test passed for commit 5000e74.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Jul 15, 2016

GCE e2e build/test passed for commit 5000e74.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b6c8790 into kubernetes:master Jul 15, 2016
@derekwaynecarr derekwaynecarr mentioned this pull request Aug 2, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Random-Liu Random-Liu

@vishh vishh

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dubstack @vishh @Random-Liu @k8s-github-robot @derekwaynecarr @euank @k8s-bot @ncdc @googlebot