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

Validate minimum CPU limits to be >= 10m #23143

Merged
merged 1 commit into from
Mar 18, 2016
Merged

Validate minimum CPU limits to be >= 10m #23143

merged 1 commit into from
Mar 18, 2016

Conversation

vishh
Copy link
Contributor

@vishh vishh commented Mar 17, 2016

  • Make kubelet default to 10ms for CPU quota if limit < 10m for
    backwards compat.
  • Update documentation

Fixes #23113

cc @bgrant0607

@vishh vishh added sig/node Categorizes an issue or PR as relevant to SIG Node. cherrypick-candidate labels Mar 17, 2016
@vishh vishh added this to the v1.2 milestone Mar 17, 2016
@vishh vishh added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 17, 2016
@vishh
Copy link
Contributor Author

vishh commented Mar 17, 2016

@k8s-bot test node e2e experimental

@vishh
Copy link
Contributor Author

vishh commented Mar 17, 2016

cc @derekwaynecarr @mrunalp

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2016
@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@vishh Not sure if you are allowing users to modify the period as it needs the same check, too.

@k8s-bot
Copy link

k8s-bot commented Mar 17, 2016

GCE e2e build/test passed for commit 1baa3cd136db5c5e8fa375d2d5cf3bc98cf41870.

derekwaynecarr
derekwaynecarr reviewed Mar 17, 2016
View reviewed changes
pkg/api/validation/validation.go
@@ -2346,6 +2356,7 @@ func ValidateResourceRequirements(requirements *api.ResourceRequirements, fldPat
if api.IsStandardResourceName(string(resourceName)) {
allErrs = append(allErrs, validateBasicResource(quantity, fldPath.Key(string(resourceName)))...)
}
allErrs = append(allErrs, validateContainerResourceLimits(string(resourceName), quantity, fldPath)...)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is right.

I agree we should check that you can't have a quota on limits.cpu that is < 10m, but this is not that...
I also think we should check in LimitRange validation that max[cpu] and default[cpu] cannot be < 10m.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@derekwaynecarr: Are you suggesting disabling quota if limits < 10m?
I can add validation to limit ranges in this PR. Good point.

@derekwaynecarr derekwaynecarr mentioned this pull request Mar 17, 2016
Closed
85 tasks
@derekwaynecarr
Copy link
Member

@mrunalp the period is fixed.

@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@derekwaynecarr Ack. The only thing that we really need to ensure is that neither are set to less than 1000 as final values.

@vishh
1. Make kubelet default to 10ms for CPU quota if limit < 10m for
5165590 
backwards compat.
2. Update documentation to reflect minimum CPU limits.

Signed-off-by: Vishnu kannan <vishnuk@google.com>
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/S

@vishh
Copy link
Contributor Author

vishh commented Mar 17, 2016

@derekwaynecarr: Chatted with @bgrant0607 offline. He suggested skipping the validation part since it is not worth the pain that validation will bring in - existing RCs might fail to work. The values we are dealing with is practically so small that just defaulting at the node is good enough.

PTAL.!

@k8s-bot
Copy link

k8s-bot commented Mar 17, 2016

GCE e2e build/test passed for commit 5165590.

@vishh
Copy link
Contributor Author

vishh commented Mar 17, 2016

@k8s-bot test node e2e experimental

@pwittrock pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2016
@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 Mar 17, 2016

GCE e2e build/test passed for commit 5165590.

@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 Mar 18, 2016

GCE e2e build/test passed for commit 5165590.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 18, 2016
Merge pull request #23143 from vishh/23113
78adb88 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 78adb88 into kubernetes:master Mar 18, 2016
@derekwaynecarr
Copy link
Member

This is LGTM to me as well

On Thursday, March 17, 2016, k8s-merge-robot notifications@github.com
wrote:

Merged #23143 #23143.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23143 (comment)

@liggitt liggitt mentioned this pull request Mar 18, 2016
Closed
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 23, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 24, 2016
@eparis
Merge pull request kubernetes#23143 from vishh/23113
a428e51 
Auto commit by PR queue bot
(cherry picked from commit 78adb88)
@eparis eparis mentioned this pull request Mar 24, 2016
Merged
@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 24, 2016
@k8s-cherrypick-bot
Copy link

Commit a428e51 found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this s an error find help to get your PR picked.

AlainRoy pushed a commit to vmware-archive/kubernetes-archived that referenced this pull request Mar 29, 2016
Merge pull request kubernetes#23143 from vishh/23113
e90a439 
Auto commit by PR queue bot
(cherry picked from commit 78adb88)
alena1108 pushed a commit to rancher/kubernetes that referenced this pull request May 20, 2016
Merge pull request kubernetes#23143 from vishh/23113
c702507 
Auto commit by PR queue bot
(cherry picked from commit 78adb88)
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#23143 from vishh/23113
52d2794 
Auto commit by PR queue bot
(cherry picked from commit 78adb88)
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#23143 from vishh/23113
f237d0c 
Auto commit by PR queue bot
(cherry picked from commit 78adb88)
@vinaykul vinaykul mentioned this pull request Jul 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pwittrock pwittrock

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. kind/design Categorizes issue or PR as related to design. 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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@vishh @k8s-github-robot @mrunalp @k8s-bot @derekwaynecarr @k8s-cherrypick-bot @k8s-teamcity-mesosphere @googlebot @bgrant0607 @pwittrock