Enforce --max-pods in kubelet admission; previously was only enforced in scheduler #24674
Conversation
gmarek
added
the
priority/critical-urgent
k8s-github-robot
added
size/XS
|
GCE e2e build/test passed for commit 983a5962f57e8f05539253f107cbb15745a16f22. |
| return false, | ||
| newInsufficientResourceError(podCountResourceName, 1, int64(len(nodeInfo.Pods())), allowedPodNumber) | ||
| } | ||
|
|
There was a problem hiding this comment.
I do't like it.
What we should do is to insted of calling podFitsResourcesInternal above, call just PodFitsResources and remove it from here.
Then we can even remove podFitsResourcesInternal completely since it will not be used.
There was a problem hiding this comment.
It's not that easy. PodFitsResources is a NodeStatus method, which needs predicates.NodeInfo. I can probably create one from the things I have here, but it'd be as ugly as this.
There was a problem hiding this comment.
aah - ok. let's leave as it - I will fix that in my PR where I'm changing it :)
With #24598 - it should be easy to do :)
|
cc @yujuhong |
k8s-github-robot
added
size/M
|
Yup - that's what I did:) I added you, as I'm modifying Kubelet tests. |
|
(Travis just doesn't work today) |
Cool. The tests look good. I think we'll deprecate these tests soon (since they are tested in the predicate library) and replace them with one generic admission test. |
|
GCE e2e build/test passed for commit b35864593c8b9075a4b9a41a21dbbf693f162c25. |
| @@ -775,6 +775,14 @@ func RunGeneralPredicates(pod *api.Pod, nodeName string, nodeInfo *schedulercach | |||
| if !fit { | |||
| return fit, err | |||
| } | |||
| // TODO(HaiyangDING): Make a proper fix for it. | |||
There was a problem hiding this comment.
I think the comment here should instead be exactly the same comment as the other place where we check max pods, namely
// TODO: move the following podNumber check to podFitsResourcesInternal when Kubelet allows podNumber check (See #20263).
davidopp
added
release-note
|
Since the release notes are generated from the PR title, I would suggest re-titling this PR to something like "enforce --max-pods in kubelet admission; previously was only enforced in scheduler" Also, noting here that this PR fixes #20263. |
gmarek
changed the title
davidopp
added
the
lgtm
|
LGTM @yujuhong said change to have Kubelet enforce --max-pods is OK |
|
GCE e2e build/test passed for commit e0712f7. |
|
GCE e2e build/test passed for commit e0712f7. |
|
GCE e2e build/test passed for commit e0712f7. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit e0712f7. |
|
Automatic merge from submit-queue |
This is an ugly hack - I spent some time trying to understand what one NodeInfo has in common with the other one, but at some point decided that I just don't have time to do that.
Fixes #24262
Fixes #20263
cc @HaiyangDING @lavalamp