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 support to docker service and docker stack deploy (carry 2660) #2712

Merged
merged 3 commits into from
Sep 10, 2020

Conversation

thaJeztah
Copy link
Member

carries #2660
closes #2660

- 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)

thaJeztah and others added 2 commits September 10, 2020 11:56
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This is related to moby/moby 40639.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Comment on lines +3810 to +3811
--ulimit-add
--ulimit-rm
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to update the zsh script as well, but might do so in a follow-up

Comment on lines +94 to +95
--ulimit-add ulimit Add or update a ulimit option (default [])
--ulimit-rm list Remove a ulimit option
Copy link
Member Author

Choose a reason for hiding this comment

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

Because one uses a regular "list" and the other uses a ulimit option, the output for these flags is inconsistent (ulimit vs list); probably something we should fix somehow; not sure what's best

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #2712 into master will increase coverage by 0.02%.
The diff coverage is 66.10%.

@@            Coverage Diff             @@
##           master    #2712      +/-   ##
==========================================
+ Coverage   58.54%   58.56%   +0.02%     
==========================================
  Files         296      296              
  Lines       21286    21345      +59     
==========================================
+ Hits        12462    12501      +39     
- Misses       7915     7931      +16     
- Partials      909      913       +4     

This is related to moby/moby 40639.

Signed-off-by: Albin Kerouanton <albin@akerouanton.name>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Collaborator

@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
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

Bash completion LGM.
I'll create a follow-up PR that adds completion for the limits.

@vindhya2g
Copy link

Hi team,
docker service update throws error saying --ulimit-add is not a valid option. I do not see ulimits when I run inspect as well. My docker version is Docker version 19.03.9, build 9d988398e7

@cpuguy83
Copy link
Collaborator

@vindhya2g You need to upgrade docker to 20.10.

@vindhya2g
Copy link

Thank you

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.

None yet

7 participants