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

Missing ulimits on docker.running / dockerng.running #30802

Closed
kjelle opened this issue Feb 2, 2016 · 9 comments

Comments

@kjelle
Copy link

commented Feb 2, 2016

Hello.

A feature request; allow setting Ulimits on HostConfig when starting a docker container.

@jfindlay

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

@kjelle, thanks for the report.

@vitalyisaev2

This comment has been minimized.

Copy link

commented Apr 15, 2016

Upvoting this. Cannot configure core dumps properly for the Docker containers deployed via SaltStack.

@rickh563 rickh563 added the ZD label Sep 16, 2016

@rickh563

This comment has been minimized.

Copy link

commented Sep 16, 2016

ZD-954

@mgardiner

This comment has been minimized.

Copy link

commented Sep 16, 2016

Besides the missing ulimits there are also other missing options that we would like to have supported.
See https://docs.docker.com/engine/reference/commandline/run/

Also related is other functions (like create) that have their own set of supported options as listed below:
https://docs.docker.com/engine/reference/commandline/create/

One thought I had was instead of the dockerng module needing to implement each of these options for each supported function you could add an "arg" property or whatever name makes sense that would allow us to pass through whatever options Docker supports. That way as Docker adds or removes options for each of the functions in subsequent releases, SaltStack will not have to worry about keeping these in sync and releasing updates so frequently while allowing us to use whichever options we deem necessary.

@christhomas-msm

This comment has been minimized.

Copy link

commented Oct 26, 2016

👍 Yep, this would be very handy for setting ulimits inside the containers.

@simon-pirschel-vouchercube

This comment has been minimized.

Copy link

commented Dec 23, 2016

+1 need it to properly configure my ElasticSearch node's memlock and nofile.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

Working on this right now.

I'm also working on a more elegant solution moving forward so that we can more gracefully handle new arguments as they are added to docker-py.

The reason for the inflexible code for comparing the container to the desired configuration has to do with a couple issues: First, when I wrote dockerng, docker-py had no means of taking arguments and returning a configuration that could be compared against the return data from inspecting a container. So, all API arguments had to be mapped to their location in the inspect return. Second, I was trying to come up with a CLI-friendly means of passing arguments to the execution module, since docker-py accepts complex structures which are not easy to represent on the CLI. The state needed to support accepting input using the CLI format as well, so we needed to translate the input into what it would end up looking like in the inspect results.

The plan for the future is to take the arguments passed to the docker.running state and use the API to build a config dictionary which can be compared to the running container. This solution would inspect the functions in docker-py themselves to figure out which arguments are valid, so this would give us the benefit of being able to support new arguments as they are added to docker-py.

The one downside is that if there is a more sane CLI representation of the argument, the CLI usage would not always immediately be supported. We would in some cases need to add code to translate CLI usage to the data structure the API expects. However, one could still use newly-added arguments by formatting them in the data structure that docker-py expects.

#39530 has been opened to track the progress of that task.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

Done in #39562.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

#39562 has been merged, ulimit support will be in 2016.3.6, 2016.11.4, and the upcoming Nitrogen feature release.

I'm also efforting to get the more elegant method of container management described in my earlier comment in for Nitrogen, but it may need to wait until the following feature release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.