add default resource requests to router creation #8495
Conversation
| return fmt.Errorf("limits or requests could not be interpreted: %v", err) | ||
| } | ||
| if cfg.ResourceLimits == "" || cfg.ResourceRequests == "" { | ||
| glog.Warningf("Running %s without resource requests or limits is bad practice in a production environment", cfg.Name) |
There was a problem hiding this comment.
@ramr @derekwaynecarr is this too verbose?
There was a problem hiding this comment.
will this mess up piped output?
There was a problem hiding this comment.
The reality is we need to check that both memory and cpu have made an explicit request. Absent either resource, the pod is still 'BestEffort' qos and will be first to get the boot if a node finds itself out of resource and has eviction enabled (kube 1.3 feature). I think the warning can be updated to state: "Running %s with BestEffort quality of service is not recommended in a production environment. A resource request for cpu and memory is strongly recommended."
|
I thought @smarterclayton wanted to do limits/requests as a post-creation step, similar to volumes |
|
I was responding to the git issue, which asks for |
|
For persistent volumes, we recommend people create the registry, then use |
|
@liggitt I think jacob is right, we can't/shouldn't do this after and forgetting to do this up front leaves you in a really bad place. With storage the pod won't deploy because the mount can't be satisfied. But pods deploy just fine without a limit/request. I almost feel like a request should be mandatory when deploying an infra container like this... (limit should be optional) I'll wait for @smarterclayton to comment, but my opinion is this PR doesn't go far enough... |
|
(but that doesn't mean I think clayton's generic limit/request setting cli is a bad idea) |
|
I did request that we add these flags since it would allow the deployment to be right from the start rather than requiring multiple commands. @smarterclayton discussed potentially having a |
|
I think the |
|
My vote is that this PR goes in as it stands. (And we do the same for the registry) |
|
@eparis - in case my previous comment was ambiguous, I would like to see --requests and --limits supported on this command. I also like the warning, but wonder if its better The only thing I wonder is if other things set requests or limits in admission path that it would maybe be better to warn after the deployment is created if we see the resultant object has no requests. I think it should be fine to run a router without limits, and requests will default to limits if not specified on the server. |
|
@smarterclayton has also pointed the oadm router -o yaml | oc set | oc create -f - flow for having |
|
We can't add infinite complexity to router commands, because we have
to draw the line. Eventually these commands will be removed and
replaced with templates - when they do, users will still have to know
how to change resources.
Adding those flags here solves one small problem. Adding set limits
solves a lot of small problems. The end goal is not all commands do
all things, but simple commands are composable.
|
|
These commands are at the limit of complexity that is reasonable. I am not in favor of adding to them. Solve the general problem and enable everyone, don't patch up the corners. |
|
@smarterclayton agreed. Fix the general problem. But what you are saying is the command we provide to deploy a registry or router is incapable of deploying a supported registry or router. That seems a little nutty... I think we should either default in some requests and not expose the flag or we expose the flags. I'm all for defaulting actually.... |
|
@JacobTanenbaum can you take on the task of creating a generic command to set requests and limits? |
|
Routers and registries don't have to have resource limits or requests - Where's the justification that router and registry must have |
|
I wasn't saying we should default limits/upper bounds. I said requests/minimums. A pod without a request is the first pod to be cpu starved, to be oom killed, to be kicked off a node, and to be unschedulable. So basically we only provide commands to run the most important infrastructure pieces in the least reliable completely unsupportable way :-( |
|
I'd be fine with defaulting a request based on our predicted use. On Mon, Apr 18, 2016 at 2:02 PM, Eric Paris [email protected]
|
|
So, to summarize:
Is that about right? |
|
Yes - but the oc set work should start upstream with Kube unless we have On Wed, Apr 20, 2016 at 10:20 AM, Ben Bennett [email protected]
|
|
Is there something I can do to help with the oc set migration? or ensuring that support for limits and requests are supported there? |
|
@JacobTanenbaum I think you can best help by writing a |
|
For this PR repurpose it to instead just set the defaults Ben indicated. |
|
@JacobTanenbaum also if you can do a 3rd PR to mention in the router (and registry) docs that it is important for users to set requests/limits as appropriate. ... |
|
@JacobTanenbaum please add your description of what you're going to do to On Thu, Apr 21, 2016 at 12:41 PM, Eric Paris [email protected]
|
|
@eparis is there a default for memory that we can agree on? |
|
Stand up a router, look at how much it is using? Just pick that :) |
|
For router CPU we should request at least half a core (i don't want to On Thu, Apr 21, 2016 at 3:08 PM, Eric Paris [email protected]
|
|
@JacobTanenbaum scratch what @smarterclayton just said. Go will 100 millicores (1/10 CPU) and however much ram you need from inspection. |
JacobTanenbaum
changed the title
|
[test] |
1 similar comment
|
[test] |
| @@ -666,6 +667,11 @@ func RunCmdRouter(f *clientcmd.Factory, cmd *cobra.Command, out io.Writer, cfg * | |||
| ReadinessProbe: readinessProbe, | |||
| ImagePullPolicy: kapi.PullIfNotPresent, | |||
| VolumeMounts: mounts, | |||
| Resources: kapi.ResourceRequirements{ | |||
| Requests: kapi.ResourceList{kapi.ResourceCPU: resource.MustParse("100m"), | |||
There was a problem hiding this comment.
can you put a \n between the { and the kapi.... doesn't this fail gofmt?
There was a problem hiding this comment.
It didn't fail gofmt but it does look better with that newline
This is a repurposing of my original pull request to add a default resource request to router creation instead of settable resource requests and limits. 100 millicores was an agreed upon default for CPU and I observed that the basic router with no routes uses about 86MB of RAM and decided 256MB was a good starting point for a request that allows ample room for growth.
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3362/) (Image: devenv-rhel7_4053) |
|
Evaluated for origin merge up to 97195e3 |
|
Evaluated for origin test up to 97195e3 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3362/) |
|
@miminar we should do something similar for the registry |
Pull request openshift/origin#8495 added default resource request values for a router pod and that should be mentioned in the documentation.
Pull request openshift/origin#8495 added default resource request values for a router pod and that should be mentioned in the documentation.
Pull request openshift/origin#8495 added default resource request values for a router pod and that should be mentioned in the documentation.
Add the ability to specify resource quotas on a router when it is
created using the "oadm router" command. When creating a router
without requests or limits oadm returns a warning that it is not
recommended to run a router without resource quotas.
if the limits or requests are not not passed the the router is still
built but a warning is returned that it is not advised to run a router
without requests and limits in a production environment
Also adds the flags help text to oadm help router
addresses: #8328