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 GPU resource validation #28743

Merged
merged 1 commit into from
Jul 16, 2016
Merged

Fix GPU resource validation #28743

merged 1 commit into from
Jul 16, 2016

Conversation

therc
Copy link
Member

@therc therc commented Jul 10, 2016 •
edited by erictune
Loading

This fixes scheduling of pods with GPU resources. The change was never upstreamed during the 1.3 beta period, as it got lost in the noise of other changes in our fork. Ooops. I'll submit a cherry-pick request for 1.3.1 as soon as this lands in master.

Because of defaulting, requests are always set if limits are. Thus, the check can never succeed. Instead, make sure that the two values are equal.

Also, remove a few other error messages and remove unnecessary Sprintf calls.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 10, 2016
@therc therc mentioned this pull request Jul 12, 2016
Closed
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

erictune
erictune reviewed Jul 13, 2016
View reviewed changes
pkg/api/validation/validation.go
// For GPUs, require that no request be set.
if resourceName == api.ResourceNvidiaGPU {
allErrs = append(allErrs, field.Invalid(reqPath, requestQuantity.String(), "cannot be set"))
// For GPUs, require that `requests` never be less than `limits`.
Copy link
Member

Choose a reason for hiding this comment

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

Does comment above match code below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, for all resources it's already the case that a request can't exceed the limit. In this case, we also require for it to never be less than the limit, either. Perhaps I should reword the comment to make that clearer.

@erictune erictune assigned erictune and unassigned smarterclayton Jul 13, 2016
Fix GPU resource validation
b86dfcf 
Because of defaulting, requests are always set if limits are. Thus, the check can never succeed. Instead, make sure that the two values are equal.

Also, remove a few other error messages and remove unnecessary Sprintf calls.
@therc
Copy link
Member Author

therc commented Jul 13, 2016

Fixed comment and rebased

@therc
Copy link
Member Author

therc commented Jul 15, 2016

@erictune anything else that should be taken care of? I want to pester the 1.3 release shepard for a 1.3.1 cherry-pick, but first I need to get a fix in. Thanks!

@erictune
Copy link
Member

LGTM

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2016
@erictune erictune added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Jul 16, 2016
@therc
Copy link
Member Author

therc commented Jul 16, 2016

@k8s-bot test this: github issue #IGNORE

@therc
Copy link
Member Author

therc commented Jul 16, 2016

@k8s-bot test this: github issue #26760

@therc
Copy link
Member Author

therc commented Jul 16, 2016

@k8s-bot test this: github issue #27462

1 similar comment
@therc
Copy link
Member Author

therc commented Jul 16, 2016

@k8s-bot test this: github issue #27462

@therc therc mentioned this pull request Jul 16, 2016
Closed
@therc
Copy link
Member Author

therc commented Jul 16, 2016

@k8s-bot test this: github issue #27462

@k8s-bot
Copy link

k8s-bot commented Jul 16, 2016

GCE e2e build/test passed for commit b86dfcf.

@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 16, 2016

GCE e2e build/test passed for commit b86dfcf.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8677b0c into kubernetes:master Jul 16, 2016
@therc
Copy link
Member Author

therc commented Jul 16, 2016

Yay. @erictune would you be able to re-add the cherrypick-candidate label? I think it also requires the 1.3 milestone to be set.

@mikedanese mikedanese added this to the v1.3 milestone Jul 16, 2016
@therc therc mentioned this pull request Jul 18, 2016
Merged
@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 19, 2016
@fabioy
Copy link
Contributor

fabioy commented Jul 19, 2016

@therc Cherrypick approved. Please create a cherrypick PR. Thanks.

@fabioy
Copy link
Contributor

fabioy commented Jul 19, 2016

@therc Oops, sorry, just noticed the PR. Please ignore last request. Thanks.

k8s-github-robot pushed a commit that referenced this pull request Jul 19, 2016
Merge pull request #29142 from Clarifai/automated-cherry-pick-of-#287…
c8ce0d6 
…43-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #28743

Cherry pick of #28743 on release-1.3.
@k8s-cherrypick-bot
Copy link

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

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#29142 from Clarifai/automated-cherry-pi…
c335a2a 
…ck-of-#28743-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#28743

Cherry pick of kubernetes#28743 on release-1.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
10 participants
@therc @k8s-cherrypick-bot @erictune @k8s-bot @k8s-github-robot @fabioy @smarterclayton @mikedanese @googlebot @bgrant0607