Add ExtendedResourceToleration admission controller. #55839
Conversation
|
/assign @derekwaynecarr @liggitt |
|
/lgtm |
rohitagarwal003
force-pushed the
rohitagarwal003:extended-resource-toleration
branch
from
138e76c
to
6471801
|
Hey, release team reminder. If this PR is approved, please remember to add the 1.9 milestone and approve it for that milestone by Monday, Nov. 20th to make the Code Slush cutoff. Thanks! |
| } | ||
|
|
||
| for _, toleration := range pod.Spec.Tolerations { | ||
| if len(toleration.Key) == 0 && len(toleration.Value) == 0 && |
vishh
Nov 17, 2017
Member
nit: Can this be a function that is something like func podHasWildcardToleration(*api.pod) bool?
nit: Can this be a function that is something like func podHasWildcardToleration(*api.pod) bool?
davidopp
Nov 18, 2017
Member
I suspect people don't use wildcard toleration much, and also it might be easier from an understandability perspective (both of the code and someone looking at the pod) if you always add the toleration. So I would suggest to just remove this special case where the wildcard toleration already exists.
I suspect people don't use wildcard toleration much, and also it might be easier from an understandability perspective (both of the code and someone looking at the pod) if you always add the toleration. So I would suggest to just remove this special case where the wildcard toleration already exists.
rohitagarwal003
Nov 19, 2017
Author
Contributor
Done.
Done.
|
/lgtm |
davidopp
added
status/approved-for-milestone
and removed
milestone/needs-approval
labels
|
[MILESTONENOTIFIER] Milestone Pull Request Current @bsalamat @davidopp @derekwaynecarr @jiayingz @liggitt @mindprince @vishh Pull Request Labels
|
rohitagarwal003
force-pushed the
rohitagarwal003:extended-resource-toleration
branch
from
6471801
to
0b9f41f
rohitagarwal003
force-pushed the
rohitagarwal003:extended-resource-toleration
branch
from
0b9f41f
to
3c4c85f
|
/retest |
|
/lgtm |
|
/assign @liggitt |
|
A nit. Fix in follow-up PR is fine. |
|
|
||
| // Admit updates the toleration of a pod based on the resources requested by it. | ||
| // If an extended resource of name "example.com/device" is requested, it adds | ||
| // a toleration with key "example.com/device", operator "Exists" and effect "NoSchedule". |
caesarxuchao
Nov 20, 2017
Member
The result is that such pods can be scheduled to nodes with the "example.com/device" taint, right? Consider adding that in the comment and please also add this plugin to the doc.
The result is that such pods can be scheduled to nodes with the "example.com/device" taint, right? Consider adding that in the comment and please also add this plugin to the doc.
rohitagarwal003
Nov 20, 2017
Author
Contributor
Yes. I will send another PR with the comment fix. And another one to update the public documentation.
Yes. I will send another PR with the comment fix. And another one to update the public documentation.
liggitt
Nov 20, 2017
Member
are arbitrary resources allowed? does this give pod authors a way to cause arbitrary taints to be added? I expected a specific prefix/namespace to ensure resource-specific taints were tolerated, not arbitrary taints
are arbitrary resources allowed? does this give pod authors a way to cause arbitrary taints to be added? I expected a specific prefix/namespace to ensure resource-specific taints were tolerated, not arbitrary taints
rohitagarwal003
Nov 20, 2017
Author
Contributor
It only adds tolerations for the resource you requested and that resource cannot be in kubernetes.io namespace.
Now you can request an arbitrary string as the resource (and then this will add that arbitrary string as the toleration) but then your pod will not get scheduled because that arbitrary string doesn't exist as a resource. And if the arbitrary string exists as a resource, we do want this admission controller to add that as a toleration.
It only adds tolerations for the resource you requested and that resource cannot be in kubernetes.io namespace.
Now you can request an arbitrary string as the resource (and then this will add that arbitrary string as the toleration) but then your pod will not get scheduled because that arbitrary string doesn't exist as a resource. And if the arbitrary string exists as a resource, we do want this admission controller to add that as a toleration.
liggitt
Nov 20, 2017
Member
Now you can request an arbitrary string as the resource (and then this will add that arbitrary string as the toleration) but then your pod will not get scheduled because that arbitrary string doesn't exist as a resource.
Thinking through this some more:
- by default, users can already add arbitrary tolerations to their pod specs
- any cluster using taints/tolerations as a security measure to keep user pods off specific nodes already has to use something like PodTolerationRestriction to whitelist the tolerations a pod is allowed to have
- that whitelist would give control over allowing custom-resource tolerations while disallowing arbitrary security-related tolerations
So I think this still works as is
Now you can request an arbitrary string as the resource (and then this will add that arbitrary string as the toleration) but then your pod will not get scheduled because that arbitrary string doesn't exist as a resource.
Thinking through this some more:
- by default, users can already add arbitrary tolerations to their pod specs
- any cluster using taints/tolerations as a security measure to keep user pods off specific nodes already has to use something like PodTolerationRestriction to whitelist the tolerations a pod is allowed to have
- that whitelist would give control over allowing custom-resource tolerations while disallowing arbitrary security-related tolerations
So I think this still works as is
|
/approve no-issue |
|
Release note is user facing. Please add a brief description of what the plugin does and who needs it. |
|
Updated the release note with more details. When the final release notes doc will get generated, I would link this release note to the documentation in kubernetes.io |
| resources := sets.String{} | ||
| for _, container := range pod.Spec.Containers { | ||
| for resourceName := range container.Resources.Requests { | ||
| if helper.IsExtendedResourceName(resourceName) { |
caesarxuchao
Nov 20, 2017
Member
@mindprince, regarding @liggitt's question, this line ensures that the resourceName (and the toleration key) has a "kubernetes.io/" or a "pod.alpha.kubernetes.io/opaque-int-resource-" prefix. The "pod.alpha.kubernetes.io/opaque-int-resource-" prefix seems strong enough. But the "kubernetes.io/" seems very generic, will it collide with other taint key name?
@mindprince, regarding @liggitt's question, this line ensures that the resourceName (and the toleration key) has a "kubernetes.io/" or a "pod.alpha.kubernetes.io/opaque-int-resource-" prefix. The "pod.alpha.kubernetes.io/opaque-int-resource-" prefix seems strong enough. But the "kubernetes.io/" seems very generic, will it collide with other taint key name?
rohitagarwal003
Nov 20, 2017
Author
Contributor
It ensures the opposite. It ensures that the resource name doesn't have kubernetes.io as the prefix.
It ensures the opposite. It ensures that the resource name doesn't have kubernetes.io as the prefix.
caesarxuchao
Nov 20, 2017
Member
Right. I missed the !.
Right. I missed the !.
|
/hold for @liggitt's question |
|
/hold |
|
/hold cancel |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, caesarxuchao, davidopp, jiayingz, mindprince, vishh Associated issue: 55080 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
k8s-github-robot
merged commit b3f7ad7
into
kubernetes:master
|
Update this admission controller in doc kubernetes/website#6618. |
/kind feature
/sig scheduling
/area hw-accelerators
There's elaborate discussion on this in #55080. In short, we would like to enable cluster operators and/or cloud providers to create dedicated nodes with extended resources (like GPUs, FPGAs etc.) that are reserved for pods requesting such resources. Taints is the kubernetes concept to create dedicated nodes. If the cluster operator or cloud provider wants to create dedicated node pools, they are expected to taint the nodes containing extended resources with the key equal to the name of the resource and effect equal to NoSchedule. If they do that, only pods that have a toleration for such a taint can be scheduled there. To make it easy for the user, this admission controller when enabled, automatically adds a toleration with key
example.com/device, operatorExistsand effectNoScheduleif an extended resource of nameexample.com/deviceis requested.Release note: