Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support setting nproc limit #761

Merged
merged 1 commit into from Mar 10, 2016
Merged

Support setting nproc limit #761

merged 1 commit into from Mar 10, 2016

Conversation

Flygsand
Copy link
Contributor

@Flygsand Flygsand commented Mar 2, 2016

This PR introduces the ability to set the nproc limit on containers. There are at least two use cases for this. The first being compatibility with Heroku's buildpacks, which use ulimit -u (example 1, example 2) to detect dyno size and adjust parameters accordingly. The second being that nproc control 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 nproc for 1X and 2X dynos, but not PX. This is because buildpack-jvm-common sets 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 custom nproc can be set by appending a third value to the constraints spec.

@@ -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"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, can we make the first argument to ulimit take an ecs.Ulimit instead?

ulimit(*container.Ulimit, "nproc")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for existing Empire installations, I think there will be a possibility that container.Ulimits will be nil, which we'd want to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Added a nil check and changed the type signature.

@ejholmes
Copy link
Contributor

ejholmes commented Mar 2, 2016

This looks fantastic. Any comments @phobologic?

@phobologic
Copy link
Contributor

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 ?

@Flygsand
Copy link
Contributor Author

Flygsand commented Mar 3, 2016

@phobologic Having a separate command for this would be great. Something like:

$ emp container web ulimits set nproc=256 nofile=65536
$ emp container web ulimits
  nproc: 256
  nofile: 65536
$ emp container web ulimits unset nofile
$ emp container worker set readonlyRootFilesystem=true
$ emp container web security-opts add type:docker_apache_t

What's up with the tests timing out by the way?

@Flygsand
Copy link
Contributor Author

Flygsand commented Mar 8, 2016

Alright, so apparently there are some subtle differences in how the SQL package treats int64 compared to ordinary int types. This caused a bunch of reflect-related errors when we put this into our staging environment. This is most likely what happened with the tests as well. Nproc is now an uint.

@@ -0,0 +1,2 @@
ALTER TABLE processes ADD COLUMN nproc bigint;
UPDATE processes SET nproc = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this break existing applications by setting the ulimit in the task definition to 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed the Nproc != 0 below. Carry on!

@ejholmes
Copy link
Contributor

ejholmes commented Mar 8, 2016

Cool! This looks great to me. @phobologic brought up that it might be nice the make the third component of the scale (cpu:mem:xxx) a comma separated list of key=value pairs, so you could do:

emp scale web=1:256:30mb:nproc=100

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!

@Flygsand
Copy link
Contributor Author

Flygsand commented Mar 9, 2016

The key-value thing definitely seems a bit nicer, so added.

@phobologic
Copy link
Contributor

sorry - commented in the commit itself, rather than here - other than that, I think this looks great. Thanks for following through @protomouse !

@Flygsand
Copy link
Contributor Author

@phobologic Saw your comment. I've added a check for unsupported keys.

@phobologic
Copy link
Contributor

Awesome - this is great, thanks a ton @protomouse

phobologic added a commit that referenced this pull request Mar 10, 2016
@phobologic phobologic merged commit 85e9c5a into remind101:master Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants