Pod limit range #2080
Pod limit range #2080
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @bskiba |
k8s-ci-robot
added
the
size/L
|
@bskiba I'll sync & ping you after I finish |
jbartosik
force-pushed
the
pod-limit-range2
branch
from
b1d075b to
7095cc4
Compare
|
@bskiba rebased, PTAL |
jbartosik
force-pushed
the
pod-limit-range2
branch
from
7095cc4 to
8f62c6b
Compare
| }, | ||
| }, | ||
| }, | ||
| limitRange: apiv1.LimitRangeItem{ |
There was a problem hiding this comment.
No, it doesn't matter for this test but I see how this could be confusing. Done.
| expect []vpa_types.RecommendedContainerResources | ||
| }{ | ||
| { | ||
| name: "cap target cpu to max", |
There was a problem hiding this comment.
Test is now redundant. I'll change limit ranges to have both min and max.
|
I was thinking how pod limit and container limit will work together. Currently, if we trim due to podLimit, we give each of the containers limits proportionally to the current recommended resources, right? But then it might turn out that we have to trim to the containerLimit anyway and we end up with some unused podLimit and another container will have unnecessarily trimmed resources. But I guess it is sort of an edgecase anyway. |
|
Actually those comments are not so important |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bskiba The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
There was a problem hiding this comment.
I was thinking how pod limit and container limit will work together. Currently, if we trim due to podLimit, we give each of the containers limits proportionally to the current recommended resources, right? But then it might turn out that we have to trim to the containerLimit anyway and we end up with some unused podLimit and another container will have unnecessarily trimmed resources. But I guess it is sort of an edgecase anyway.
As decided offline we're going to do the simple thing for now and try to do something smarter if we see need to.
| }, | ||
| }, | ||
| }, | ||
| limitRange: apiv1.LimitRangeItem{ |
There was a problem hiding this comment.
No, it doesn't matter for this test but I see how this could be confusing. Done.
| expect []vpa_types.RecommendedContainerResources | ||
| }{ | ||
| { | ||
| name: "cap target cpu to max", |
There was a problem hiding this comment.
Test is now redundant. I'll change limit ranges to have both min and max.
No description provided.