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 constraints func for quota validates resources #25487

Merged
merged 1 commit into from
May 13, 2016

Conversation

derekwaynecarr
Copy link
Member

Fixes #25484

This prevents quota from prematurely charging for quota usage around a pod when the pod had invalid resources. For example, if a container limit < container request, prior to this a quota would charge the usage, but the pod failed validation.

We do not do full validation on the pod because at this point, the pod does not have all of ObjectMeta filled out.

/cc @smarterclayton

@derekwaynecarr
Copy link
Member Author

/cc @huang195 - this fixes the issue you reported.

@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 May 11, 2016
@smarterclayton smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels May 11, 2016
@k8s-github-robot k8s-github-robot added release-note-label-needed and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 11, 2016
@k8s-bot
Copy link

k8s-bot commented May 11, 2016

GCE e2e build/test passed for commit 8b8a22b.

@derekwaynecarr derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels May 11, 2016
@lavalamp
Copy link
Member

Is this the only way to fix the reported issue? I feel like there's probably N ways to make an invalid pod and by the time you fix them all you'll have reimplemented pod validation. Why not just call the actual pod validation method? (also ugh, but better than code duplication)

@smarterclayton
Copy link
Contributor

smarterclayton commented May 11, 2016 via email

@derekwaynecarr
Copy link
Member Author

@lavalamp - the object lacks a UID and may not have Name at this point, so I cannot call the pod validate directly. agree it's not ideal.

@derekwaynecarr
Copy link
Member Author

alternative is that quota could do name and uid assignment, and call validate pod directly.

@smarterclayton
Copy link
Contributor

Veto.

On May 12, 2016, at 7:30 PM, Derek Carr notifications@github.com wrote:

alternative is that quota could do name and uid assignment, and call
validate pod directly.


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

@smarterclayton
Copy link
Contributor

Can we call the resource validation directly somehow?

On May 12, 2016, at 7:30 PM, Derek Carr notifications@github.com wrote:

alternative is that quota could do name and uid assignment, and call
validate pod directly.


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

@derekwaynecarr
Copy link
Member Author

thats what I am doing:

https://github.com/kubernetes/kubernetes/pull/25487/files#diff-9b60439f29cb51347a55472508f51f60R83

Overall, I am fine keeping this as-is for now.

@j3ffml j3ffml merged commit 1661df4 into kubernetes:master May 13, 2016
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. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants