8000 Support ulimits on Swarm services. by akerouanton · Pull Request #41284 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support ulimits on Swarm services. #41284

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

Merged
merged 2 commits into from
Jul 30, 2020
Merged

Conversation

akerouanton
Copy link
Member
@akerouanton akerouanton commented Jul 28, 2020

- What I did

  1. Bump swarmkit to d6592ddefd8a5319aadff74c558b816b1a0b2590 to include Add Ulimits to ContainerSpec swarmkit#2967 ;
  2. Ulimits field has been added to the ContainerSpec API type, such that the Docker daemon API can now take ulimits when declaring new services, updating or inspecting them ;
  3. The cluster executor has been changed to convert from/to Swarmkit type ;

This is related to #40639.

- How I did it

- How to verify it

I tested my changes by updating docker/cli and using docker service create|update|inspect and docker stack deploy (I have to create that PR).

- Description for the changelog

Support ulimits on Swarm services.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member

#41282 was merged; this needs a rebase now

Includes moby/swarmkit#2967, which adds Ulimits to ContainerSpec.

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

Can you squash the last two commits?

Add Ulimits field to the ContainerSpec API type and wire it to Swarmkit.

This is related to moby#40639.

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

I rebased and squashed my commits 😉

Copy link
Member
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 double check; this is not something we want to be living in Resources.Limits ?

@thaJeztah
Copy link
Member

Discussing with @cpuguy83 and we should be ok merging as-is; we can update if we think it has to move

@thaJeztah thaJeztah merged commit 47b7c88 into moby:master Jul 30, 2020
@thaJeztah thaJeztah added this to the 20.03.0 milestone Jul 30, 2020
@cpuguy83
Copy link
Member

tldr; didn't want to hold this up and require the contributor to change, we can fix it if necessary. CapAdd/Drop would follow this same line of thinking as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0