Support setting nproc limit #761
Conversation
| @@ -597,9 +609,22 @@ func taskDefinitionToProcess(td *ecs.TaskDefinition) (*scheduler.Process, error) | |||
| Env: env, | |||
| CPUShares: uint(*container.Cpu), | |||
| MemoryLimit: uint(*container.Memory) * MB, | |||
| Nproc: ulimit(td, "nproc"), | |||
There was a problem hiding this comment.
Minor, can we make the first argument to ulimit take an ecs.Ulimit instead?
ulimit(*container.Ulimit, "nproc")There was a problem hiding this comment.
Also, for existing Empire installations, I think there will be a possibility that container.Ulimits will be nil, which we'd want to handle.
There was a problem hiding this comment.
Good catch. Added a nil check and changed the type signature.
|
This looks fantastic. Any comments @phobologic? |
|
I think this is great - but I have one concern, and that's largely the syntax of the command. It's getting a little unwieldy to keep appending new values on the end of the scale command, and this one is a bit arbitrary - there are 15 other Ulimits that ECS accepts, and I could see folks following this as an example when they add their favorite ulimit. Not sure what the best way to fix this is - I think in the long run we just have to start getting away from the heroku CLI and come up with something that handles all the features available to us. For now, I'm cool with accepting this (because having nproc would be great), but I'd be curious if we can come up with a more generic, extensible syntax for modifying these things. @protomouse ? |
|
@phobologic Having a separate command for this would be great. Something like: What's up with the tests timing out by the way? |
|
Alright, so apparently there are some subtle differences in how the SQL package treats |
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE processes ADD COLUMN nproc bigint; | |||
| UPDATE processes SET nproc = 0; | |||
There was a problem hiding this comment.
Will this break existing applications by setting the ulimit in the task definition to 0?
There was a problem hiding this comment.
Oh, I missed the Nproc != 0 below. Carry on!
|
Cool! This looks great to me. @phobologic brought up that it might be nice the make the third component of the scale ( It's a little more readable and opens it up for adding additional flags that you can set. In the long run, a separate command is better imo, so up to you if you want to change it in this PR, otherwise we'll merge! |
|
The key-value thing definitely seems a bit nicer, so added. |
|
sorry - commented in the commit itself, rather than here - other than that, I think this looks great. Thanks for following through @protomouse ! |
|
@phobologic Saw your comment. I've added a check for unsupported keys. |
|
Awesome - this is great, thanks a ton @protomouse |
Support setting nproc limit
This PR introduces the ability to set the
nproclimit on containers. There are at least two use cases for this. The first being compatibility with Heroku's buildpacks, which useulimit -u(example 1, example 2) to detect dyno size and adjust parameters accordingly. The second being thatnproccontrol is great for security purposes.It is meant to be an optional part of the constraints spec, and not enabled by default for anything but the fixed dyno sizes. Note that this PR sets
nprocfor1Xand2Xdynos, but notPX. This is becausebuildpack-jvm-commonsets too high of a heap size limit for the legacy dyno spec that Empire is using, and I fear that might lead to bad things. A customnproccan be set by appending a third value to the constraints spec.