Skip to content
New issue

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

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

Already on GitHub? # to your account

Add ulimits support to docker service and docker stack deploy #2660

Closed
wants to merge 3 commits into from

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented Jul 28, 2020

- What I did

  1. Bump docker/docker to my own fork to have API changes regarding ulimits support on service endpoints ;
  2. Add --ulimit to docker service create ;
  3. Add --ulimit-add and --ulimit-rm options to docker service update ;
  4. Add Ulimits to docker service inspect --pretty ;
  5. Support ulimits in docker-compose files, to make them work with docker stack deploy ;

This is related to moby/moby#40639.

- How I did it

- How to verify it

Given the following docker-compose.yaml:

version: "3"

services:
  test:
    image: debian
    command: /bin/bash -c "ulimit -a && sleep 15"
    ulimits:
      nofile: 100
    deploy:
      mode: replicated
docker service create
$ docker service create --name t2 --ulimit=nofile=100 debian /bin/bash -c "ulimit -a && sleep 30"
$ docker service logs -f t2
...
t2.1.dkm9duj5i4rq@aker    | open files                      (-n) 100
t2.1.dkm9duj5i4rq@aker    | real-time priority              (-r) 0
...
docker service update
$ docker service update --ulimit-rm nofile --ulimit-add rtprio=1 t2
$ docker service logs -f t2
...
t2.1.dkm9duj5i4rq@aker    | open files                      (-n) 1048576
t2.1.dkm9duj5i4rq@aker    | real-time priority              (-r) 1
...
docker service inspect
$ docker service update --ulimit-add nofile=100:200 t2
$ docker service inspect --pretty t2
...
Ulimits:
 nofile: 100:200
 nproc: -1:-1
...

$ docker service inspect t2
...
        "Ulimits": [
            {
                "Name": "nproc",
                "Hard": -1,
                "Soft": -1
            },
            {
                "Name": "nofile",
                "Hard": 200,
                "Soft": 100
            }
        ]
...
docker stack deploy
$ cat docker-compose.yaml
version: "3"

services:
  test:
    image: debian
    command: /bin/bash -c "ulimit -a && sleep 15"
    ulimits:
      nofile: 100
    deploy:
      mode: replicated

$ docker stack deploy --compose-file docker-compose.yaml test
$ docker service logs -f test_test | grep "open files"
test_test.1.jezlw4wt08rz@aker    | open files                      (-n) 100
$ docker inspect $(docker ps --filter=name=test_test -q)
...
        "Ulimits": [
            {
                "Name": "nofile",
                "Hard": 100,
                "Soft": 100
            }
        ],
...

- Description for the changelog

Add ulimits support to docker service create|update|inspect and docker stack deploy

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

@akerouanton akerouanton changed the title Ulimits Add ulimits support to docker service and docker stack deploy Jul 28, 2020
@akerouanton akerouanton force-pushed the ulimits branch 2 times, most recently from ccf5e75 to b9c148c Compare July 28, 2020 20:44
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #2660 into master will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2660      +/-   ##
==========================================
- Coverage   58.14%   58.12%   -0.02%     
==========================================
  Files         295      295              
  Lines       21198    21238      +40     
==========================================
+ Hits        12325    12345      +20     
- Misses       7966     7982      +16     
- Partials      907      911       +4     

@akerouanton akerouanton marked this pull request as ready for review July 30, 2020 18:37
@akerouanton
Copy link
Member Author

@thaJeztah This PR is ready for review 😉

…cd07

Includes the API changes regarding ulimits support on service endpoints.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
This is related to moby/moby#40639.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
This is related to moby/moby#40639.

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

Thanks!

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Thank you @akerouanton for this PR 👍 The code looks good, but I think this PR lacks some tests. Do you think you could add some to the formater part and to the service update 🙏 ?

@cpuguy83
Copy link
Collaborator

Just a couple of comments:

Need to make sure the final list of ulimits removes duplicates and is sorted.
Other than this seems good.

@thaJeztah
Copy link
Member

carried in #2712, thanks!

@akerouanton akerouanton deleted the ulimits branch October 27, 2021 20:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants