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

Add Ulimits to ContainerSpec #2967

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Add Ulimits to ContainerSpec #2967

merged 1 commit into from
Jul 28, 2020

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented Jul 26, 2020

- What I did

Unlike Docker and docker-compose, Swarm never supported ulimits.

This change introduces a new Ulimits field on the ContainerSpec type. It
can be used by API clients to set desired ulimits.

This is related to moby/moby#40639.

- How to test it

term1$ sudo ./bin/swarmd
term2$ sudo ./bin/swarmctl service create --ulimit=nofile=100 --image debian --command /bin/bash --args="-c" --args="ulimit -n" --name test
term2$ sudo ./bin/swarmctl service logs -f test
test.1@aker❯ 100

- Description for the changelog

Add Ulimits field to the API

@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #2967 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2967      +/-   ##
==========================================
+ Coverage   61.75%   61.84%   +0.08%     
==========================================
  Files         142      142              
  Lines       22999    23005       +6     
==========================================
+ Hits        14204    14228      +24     
+ Misses       7295     7278      -17     
+ Partials     1500     1499       -1     

api/specs.proto Outdated
@@ -358,8 +358,18 @@ message ContainerSpec {

// CapabilityAdd sets the list of capabilities to add to the default capability list
repeated string capability_add = 27;
// CapabilityAdd sets the list of capabilities to drop from the default capability list
// CapabilityDrop sets the list of capabilities to drop from the default capability list
Copy link
Member

Choose a reason for hiding this comment

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

Whoops! looks like we missed this in #2965)

I guess the linter did not complain about this because it skips the generated .pb.go files 🤔

Could you perhaps create a separate PR for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted the change here and created #2968 😉

@thaJeztah
Copy link
Member

@cpuguy83 @dperny PTAL

Unlike Docker and docker-compose, Swarm never supported ulimits.

This change introduces a new Ulimits field on the ContainerSpec type. It
can be used by API clients to set desired ulimits.

This is related to moby/moby#40639.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
@dperny
Copy link
Collaborator

dperny commented Jul 28, 2020

This looks fine. Swarm is just plumbing this field down, and the hard work is all handled in the Engine.

Nota bene, @akerouanton, that the github.com/swarmkit/agent/exec/dockerapi package is only used for internally testing swarmkit, and you'll need to make identical changes to the github.com/docker/docker/daemon/cluster/executor package.

@dperny dperny merged commit d6592dd into moby:master Jul 28, 2020
akerouanton added a commit to akerouanton/docker that referenced this pull request Jul 28, 2020
Includes moby/swarmkit#2967, which adds Ulimits to ContainerSpec.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 30, 2020
Includes moby/swarmkit#2967, which adds Ulimits to ContainerSpec.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
Upstream-commit: 1fdb1033c4cecbd589fa6afa5b7a975bbaf712e4
Component: engine
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.

3 participants