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 support for maximum replicas per node without stack #1612

Merged
merged 1 commit into from Feb 22, 2019

Conversation

Projects
None yet
6 participants
@olljanat
Copy link
Contributor

commented Jan 14, 2019

- What I did
Added support for maximum replicas per node

Third step to be able solve:

- How I did it
Added new switch --replicas-max-per-node switch to docker service

- How to verify it
Create two services and specify --replicas-max-per-node one of them:

docker service create --detach=true --name web1 --replicas 2 nginx
docker service create --detach=true --name web2 --replicas 2 --replicas-max-per-node 1 nginx

See difference on command outputs:

$ docker service ls
ID                  NAME                MODE                REPLICAS               IMAGE               PORTS
0inbv7q148nn        web1                replicated          2/2                    nginx:latest        
9kry59rk4ecr        web2                replicated          1/2 (max 1 per node)   nginx:latest
        
$ docker service ps --no-trunc web2
ID                          NAME                IMAGE                                                                                  NODE                DESIRED STATE       CURRENT STATE            ERROR                                                     PORTS
bf90bhy72o2ry2pj50xh24cfp   web2.1              nginx:latest@sha256:b543f6d0983fbc25b9874e22f4fe257a567111da96fd1d8f1b44315f1236398c   limint              Running             Running 34 seconds ago                                                             
xedop9dwtilok0r56w4g7h5jm   web2.2              nginx:latest@sha256:b543f6d0983fbc25b9874e22f4fe257a567111da96fd1d8f1b44315f1236398c                       Running             Pending 35 seconds ago   "no suitable node (max replicas per node limit exceed)"   

- Description for the changelog

  • Split from #1410 to simplify review
@codecov-io

This comment has been minimized.

Copy link

commented Jan 14, 2019

Codecov Report

Merging #1612 into master will increase coverage by 0.08%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #1612      +/-   ##
==========================================
+ Coverage   56.05%   56.14%   +0.08%     
==========================================
  Files         306      306              
  Lines       20981    20980       -1     
==========================================
+ Hits        11761    11779      +18     
+ Misses       8371     8347      -24     
- Partials      849      854       +5
@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@thaJeztah FYI. I moved non-stack version to this PR as it is already fully working and it easier to debug stack version when there is less stuff.

Can you give first review? I assume that at least some kind of tests are wanted on but which kind of?

Logic is tested on swarmkit and API on Moby.

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2019

@thaJeztah reminder of this one. I really would like get feedback so I can finalize it.

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@cpuguy83
Copy link
Collaborator

left a comment

Looks good but could use some unit tests.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Ah, whoops, thought I reviewed; yes, changes look good, but if you can add a unit test, that'd be great

@olljanat olljanat force-pushed the olljanat:replicas-max-per-node-cli branch 2 times, most recently from 4a9cbcd to 79b5119 Feb 18, 2019

@silvin-lubecki
Copy link
Contributor

left a comment

Small nit but LGTM otherwise, thank you @olljanat 👍 !

@olljanat olljanat force-pushed the olljanat:replicas-max-per-node-cli branch from 79b5119 to d103bd4 Feb 18, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@silvin-lubecki corrected that one and rebased to one commit.

@thaJeztah
Copy link
Member

left a comment

apologies for the delay; found some minor issues, and a missing "version" annotation 😊

Show resolved Hide resolved cli/command/service/opts.go Outdated
Show resolved Hide resolved cli/command/service/opts.go
Show resolved Hide resolved cli/command/service/update_test.go Outdated
Show resolved Hide resolved docs/reference/commandline/service_create.md Outdated
Show resolved Hide resolved docs/reference/commandline/service_create.md Outdated
@GordonTheTurtle

This comment has been minimized.

Copy link

commented Feb 22, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "replicas-max-per-node-cli" git@github.com:olljanat/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842358453696
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Add support for maximum replicas per node without stack
Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

@olljanat olljanat force-pushed the olljanat:replicas-max-per-node-cli branch from bb7be90 to f7f4d3b Feb 22, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Feb 22, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@thaJeztah included changes you suggested and rebased to one commit.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Gave this a spin, and all looks good 😄

We could add some more tests based on the scenarios below, but I'm ok to keep that for a follow-up.

Create a service with max 2 replicas:

docker service create --replicas=2 --replicas-max-per-node=2 --name test nginx:alpine
docker service inspect --format '{{.Spec.TaskTemplate.Placement.MaxReplicas}}' test
2

Update the service (max replicas should keep its value) - this works ok;

docker service update --replicas=1 test
docker service inspect --format '{{.Spec.TaskTemplate.Placement.MaxReplicas}}' test
2

Update the max replicas to 1

docker service update --replicas-max-per-node=1 test
docker service inspect --format '{{.Spec.TaskTemplate.Placement.MaxReplicas}}' test
1

And reset to 0:

docker service update --replicas-max-per-node=0 test
docker service inspect --format '{{.Spec.TaskTemplate.Placement.MaxReplicas}}' test
0
@thaJeztah
Copy link
Member

left a comment

LGTM, thanks!

@thaJeztah thaJeztah merged commit f1de399 into docker:master Feb 22, 2019

7 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed
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.