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

Remove cpu limits for dns pod to avoid CPU starvation #33227

Merged
merged 1 commit into from
Sep 22, 2016
Merged

Remove cpu limits for dns pod to avoid CPU starvation #33227

merged 1 commit into from
Sep 22, 2016
+0 −21

Conversation

vishh
Copy link
Contributor

@vishh vishh commented Sep 21, 2016 •
edited by jessfraz
Loading

The current limits are not based on usage profiles
Fixes #33222

Remove cpu limits for dns pod to avoid CPU starvation

This change is Reviewable

bprashanth
bprashanth reviewed Sep 21, 2016
View reviewed changes
cluster/addons/dns/skydns-rc.yaml.base
limits:
cpu: 10m
Copy link
Contributor

Choose a reason for hiding this comment

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

this is guaranteed qos, so what happens now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of v1.4 this pod was in the Burstable QoS class even before this patch. QoS is per pod and per container.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, hmm, ok thinking

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment saying that the pod is anyway in "burstable" because of memory, so cpu limits don't matter. Also include that we don't want to set cpu until we've profiled the sub components thoroughly, because on debian systems the enforcement is more strict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding more TODO seems to be making the yaml outputs incompatible. Given the time constraint, I chose to not do that. I'm happy to post another PR tomorrow once I have more time to figure out why the yaml outputs were broken when TODOs are being added.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you confirm comments are what broke the make-ing, please file a bug

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Sep 21, 2016
@vishh vishh assigned bprashanth and unassigned zmerlynn Sep 21, 2016
@vishh vishh added this to the v1.4 milestone Sep 21, 2016
@vishh vishh added release-note Denotes a PR that will be considered when it comes time to generate release notes. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed release-note-label-needed labels Sep 21, 2016
@vishh vishh changed the title Remove cpu limits for dns pod. Remove cpu limits for dns pod to avoid CPU starvation Sep 21, 2016
@pwittrock
Copy link
Member

@k8s-bot smoke test this

@pwittrock
Copy link
Member

@k8s-bot gke test this

@vishh
Copy link
Contributor Author

vishh commented Sep 22, 2016

@k8s-bot gce e2e test this

@vishh
Copy link
Contributor Author

vishh commented Sep 22, 2016

@k8s-bot gke smoke test this

@vishh
Copy link
Contributor Author

vishh commented Sep 22, 2016

@k8s-bot kubemark test this

@vishh
Copy link
Contributor Author

vishh commented Sep 22, 2016

@k8s-bot gke e2e test this

@bprashanth
Copy link
Contributor

LGTM when you fix the nits, I assume you used the makefile to generate the yamls from the one base yaml. Please also answer #33222 (comment) so we have a reference of the impact of the change, if we need to debug a user issue.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 28c7632df46395de32dd1ab7f1fb2c1ac7f9aaf2.

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

@vishh
remove cpu limits for dns pod. The current limits are not based on us…
7631b09 
…age profiles

Signed-off-by: Vishnu kannan <vishnuk@google.com>
@vishh
Copy link
Contributor Author

vishh commented Sep 22, 2016

I think I fixed the issues with the config files. Turns out the TODO line that I added was confusing the yaml format. I removed it for now.
If tests pass this time around, this should be good to go @pwittrock

@vishh vishh mentioned this pull request Sep 22, 2016
Merged
@pwittrock pwittrock added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 22, 2016
@vishh vishh added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 22, 2016
pwittrock pushed a commit that referenced this pull request Sep 22, 2016
Merge pull request #33243 from vishh/automated-cherry-pick-of-#33227-…
1f85627 
…upstream-release-1.4

Automated cherry pick of #33227
@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 e69c8f1 into kubernetes:master Sep 22, 2016
@jessfraz jessfraz mentioned this pull request Oct 6, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 6, 2016
Merge pull request #34266 from jessfraz/automated-cherry-pick-of-#32914-
f579ae7 
#33163-#33227-#33359-#33605-#33967-#33977-#34158-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #32914 #33163 #33227 #33359 #33605 #33967 #33977 #34158 origin release 1.4

Cherry pick of #32914 #33163 #33227 #33359 #33605 #33967 #33977 #34158 on release-1.4.

#32914: Limit the number of names per image reported in the node
#33163: fix the appending bug
#33227: remove cpu limits for dns pod. The current limits are not
#33359: Fix goroutine leak in federation service controller
#33605: Add periodic ingress reconciliations.
#33967: scheduler: cache.delete deletes the pod from node specified
#33977: Heal the namespaceless ingresses in federation e2e.
#34158: Add missing argument to log message in federated ingress
@jessfraz
Copy link
Contributor

jessfraz commented Oct 7, 2016

was cherry-picked in #34266

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#33243 from vishh/automated-cherry-pick-…
7694018 
…of-#33227-upstream-release-1.4

Automated cherry pick of kubernetes#33227
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#34266 from jessfraz/automated-cherry-pi…
cc6193b 
…ck-of-#32914-kubernetes#33163-kubernetes#33227-kubernetes#33359-kubernetes#33605-kubernetes#33967-kubernetes#33977-kubernetes#34158-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of kubernetes#32914 kubernetes#33163 kubernetes#33227 kubernetes#33359 kubernetes#33605 kubernetes#33967 kubernetes#33977 kubernetes#34158 origin release 1.4

Cherry pick of kubernetes#32914 kubernetes#33163 kubernetes#33227 kubernetes#33359 kubernetes#33605 kubernetes#33967 kubernetes#33977 kubernetes#34158 on release-1.4.

kubernetes#32914: Limit the number of names per image reported in the node
kubernetes#33163: fix the appending bug
kubernetes#33227: remove cpu limits for dns pod. The current limits are not
kubernetes#33359: Fix goroutine leak in federation service controller
kubernetes#33605: Add periodic ingress reconciliations.
kubernetes#33967: scheduler: cache.delete deletes the pod from node specified
kubernetes#33977: Heal the namespaceless ingresses in federation e2e.
kubernetes#34158: Add missing argument to log message in federated ingress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bprashanth bprashanth bprashanth left review comments

Assignees

@bprashanth bprashanth

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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@vishh @pwittrock @bprashanth @k8s-ci-robot @k8s-github-robot @jessfraz @zmerlynn @googlebot