Default resource limits for build pods #11281
Conversation
|
@bparees Please help review, when you can ,thanks. |
|
@wanghaoran1988 I appreciate the initiative but please start consulting with me before starting PRs for backlog features, especially ones that haven't even been groomed (ie are still in the new list). I don't want you to waste your time on something we aren't sure we want, or haven't thought through the direction on yet. This one is ok but it's going to cause merge conflicts with this PR #11144 which is for a feature that's committed for 3.4, so I don't want to merge your PR until that goes through. |
| }, | ||
| Requests: kapi.ResourceList{ | ||
| kapi.ResourceName(kapi.ResourceCPU): resource.MustParse("10"), | ||
| kapi.ResourceName(kapi.ResourceMemory): resource.MustParse("1G"), |
There was a problem hiding this comment.
for this and all the other tests, use different values for "requests" and "limits" to ensure the values aren't getting mixed up. As it is if you were assigning the limit value to the resource field, the test would still pass.
| }, | ||
| }, | ||
| }, | ||
| "BuildDefaults plugin defined nothings ,BUILD object defined resource limits": { |
There was a problem hiding this comment.
"BuildDefaults plugin defined nothing, Build object defined resource limits"
(nothing is singular, comma goes immediately after nothing, Build doesn't need to be all caps. Same changes in the other test descriptions)
| kapi.ResourceName(kapi.ResourceCPU): resource.MustParse("20"), | ||
| kapi.ResourceName(kapi.ResourceMemory): resource.MustParse("2G"), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Needs a test where the build Resources object is empty to test your length checking/list initializing logic in the defaulter.
| @@ -25,6 +25,9 @@ type BuildDefaultsConfig struct { | |||
| // SourceStrategyDefaults are default values that apply to builds using the | |||
| // source strategy. | |||
| SourceStrategyDefaults *SourceStrategyDefaultsConfig | |||
|
|
|||
| // Resources computes resource requirements to execute the build. | |||
| Resources kapi.ResourceRequirements | |||
There was a problem hiding this comment.
and then you will need to add a test where Resources is nil (in addition to the existing test where Resources is non-nil but empty)
wanghaoran1988
force-pushed
the
add_default_resources_to_builds
branch
from
0afa5ac to
620c919
Compare
openshift-bot
added
the
needs-rebase
wanghaoran1988
force-pushed
the
add_default_resources_to_builds
branch
from
620c919 to
7304454
Compare
openshift-bot
removed
the
needs-rebase
|
@wanghaoran1988 this can be resumed now that the 3.4 work has landed, but i'm afraid you're going to have to do a bit of a messy rebase first off. |
|
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
openshift-bot
added
the
needs-rebase
|
use this11575 instead |
Trello Card