Added support for maximum replicas per node #2758

olljanat · 2018-09-29T20:41:17Z

- What I did
Added support for maximum replicas per node

First step to be able solve moby/moby#26259

- How I did it
Added maxreplicas settings under type Placement and --replicas-max-per-node to cli.

- How to test it
Create test service on one node environment using command:

swarmctl service create --image nginx --replicas 2 --replicas-max-per-node 1 --name web

codecov · 2018-09-30T06:35:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7d5d33b). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2758   +/-   ##
=========================================
  Coverage          ?   61.85%           
=========================================
  Files             ?      134           
  Lines             ?    21874           
  Branches          ?        0           
=========================================
  Hits              ?    13531           
  Misses            ?     6884           
  Partials          ?     1459

olljanat · 2018-09-30T09:17:53Z

Ready for review.

Note that I needed switch to Protoc 3.6.1 because it was failing to error:

protoc --decode google.protobuf.FileDescriptorSet /usr/local/include/google/protobuf/descriptor.proto
/usr/local/include/google/protobuf/descriptor.proto: File does not reside within any path specified using --proto_path (or -I).  You must specify a --proto_path which encompasses this file.  Note that the proto_path must be an exact prefix of the .proto file names -- protoc is too dumb to figure out when two paths (e.g. absolute and relative) are equivalent (it's harder than you think).
2018/09/27 15:41:47 exit status 1
make: *** [protos] Error 1
Exited with code 2

Same way than example #2757 does

dperny

The feature looks fine to me.

The code mostly looks good, but has some small issues.

Here's a big question for you, though, that we'll need an answer to before we proceed: If the user is trying to do a rolling update, and they have the update set to Start First, but the maximum number of tasks is already present on every node, then the update will be unable to proceed. The user will have to first scale the service down, then do the rolling update. Is this expected and acceptable behavior?

dperny · 2018-10-01T16:03:59Z

api/types.proto

@@ -862,6 +862,9 @@ message Placement {
 	// This field is used in the platform filter for scheduling. If empty,
 	// then the platform filter is off, meaning there are no scheduling restrictions.
 	repeated Platform platforms = 3;
+
+	// Max replicas specifies limit for maximum number of replicas running on one node.
+	uint64 maxreplicas = 4;


nit: calling this max_replicas should get it to compile to go as MaxReplicas

Additionally, the comment should then be of the form

// MaxReplicas specifies the limit . . .

dperny · 2018-10-01T16:07:58Z

manager/scheduler/filter.go

+
+// Check returns true if there is free slots for task in a given node.
+func (f *MaxReplicasFilter) Check(n *NodeInfo) bool {
+	if uint64(n.ActiveTasksCountByService[f.t.ServiceID]) < f.t.Spec.Placement.Maxreplicas {


is this concurrency safe? if so, can you explain why it's concurrency safe in a comment?

As far I understand swarmkit code then scheduler will process only one task at time?
and code logic on https://github.com/docker/swarmkit/blob/master/manager/scheduler/nodeinfo.go#L114 updates ActiveTasksCountByService counter immediately when task is scheduled to host without waiting that it actually starts. That should make it concurrency safe.

But to be honest I'm not sure how to describe it nicely.

dperny · 2018-10-01T16:08:18Z

manager/scheduler/scheduler_test.go

+
+	assert.Len(t, t1Assignments, 2)
+	assert.Equal(t, 2, t1Assignments["id1"])
+	assert.Equal(t, 2, t1Assignments["id2"])


need to add a check to the test to see that the Explain step works correctly.

cpuguy83 · 2018-10-01T17:49:50Z

@dperny My first thought, this should be taken literally with a separate option to override.

But I see the dilemma here in that allowing this (more than N-max on a node) to happen may be very bad in some cases, and having a separate option is cumbersome and confusing... and at what level is this option set at considering that you can potentially be updating more than one replica on a node in parallel.

olljanat · 2018-10-01T19:53:05Z

Good point. I think that correct logic would be that if start first setting is enabled then:
max replicas => max replicas value + update parallelism value

I can try look about it.

Btw. I also setup voting about it to original issue: moby/moby#26259 (comment)

olljanat · 2018-10-03T14:53:19Z

Here's a big question for you, though, that we'll need an answer to before we proceed: If the user is trying to do a rolling update, and they have the update set to Start First, but the maximum number of tasks is already present on every node, then the update will be unable to proceed. The user will have to first scale the service down, then do the rolling update. Is this expected and acceptable behavior?

Actually more I think this more sure I came that it is excepted and acceptable behavior as it would very hard to define good logic when scheduler is allowed to exceed that limit. So I would say that it just need to be explained on documentation that user need to make sure that there is always room of update tasks on that combination. Simplest solution is actually have one more nodes than is needed to run application (example 3 nodes with settings --replicas 2 are --replicas-max-per-node 1 )

@dperny I also made changes you requested.

olljanat · 2018-10-05T04:46:15Z

@wk8 I can see that you have been reviewing many PRs here so can you take a look also this one?

dperny · 2018-10-08T15:33:46Z

LGTM

@anshulpundir and @wk8, can one of y'all sign off and merge?

wk8

Minor nitpicks :)

wk8 · 2018-10-11T20:32:47Z

manager/scheduler/filter.go

+// SetTask returns true when max replicas per node filter > 0 for a given task.
+func (f *MaxReplicasFilter) SetTask(t *api.Task) bool {
+	if t.Spec.Placement != nil {
+		if t.Spec.Placement.MaxReplicas > 0 {


No need for the 2 ifs, just use &&?

wk8 · 2018-10-11T20:34:03Z

manager/scheduler/filter.go

+		return true
+	}
+
+	return false


Maybe more readable as return uint64(n.ActiveTasksCountByService[f.t.ServiceID]) < f.t.Spec.Placement.MaxReplicas ?

manager/scheduler/scheduler_test.go

Signed-off-by: Olli Janatuinen <[email protected]>

olljanat · 2018-10-14T18:03:55Z

@wk8 Changes which you requested are now in-place :)

wk8

Thank you! :)

cpuguy83

LGTM

olljanat force-pushed the replicas-max-per-node branch 8 times, most recently from 9cf49b9 to b1c85ca Compare September 29, 2018 22:21

olljanat mentioned this pull request Sep 29, 2018

Dockerfile, circleci: update protoc to 3.6.1, simplify install #2756

Merged

olljanat force-pushed the replicas-max-per-node branch from b1c85ca to 9563529 Compare September 30, 2018 06:20

olljanat force-pushed the replicas-max-per-node branch 3 times, most recently from 17745e1 to d49d639 Compare September 30, 2018 09:09

olljanat changed the title ~~WIP: Added support for maximum replicas per node~~ Added support for maximum replicas per node Sep 30, 2018

olljanat force-pushed the replicas-max-per-node branch from d49d639 to 51f1d01 Compare September 30, 2018 09:19

olljanat mentioned this pull request Sep 30, 2018

Added support for maximum replicas per node moby/moby#37940

Merged

2 tasks

dperny requested changes Oct 1, 2018

View reviewed changes

olljanat force-pushed the replicas-max-per-node branch 2 times, most recently from 9d470c2 to 6673107 Compare October 2, 2018 18:35

This was referenced Oct 2, 2018

Add maximum replicas per node support to stack version 3.8 docker/cli#1410

Merged

is it possible to create my own Docker swarm scheduler? #2569

Open

olljanat mentioned this pull request Oct 4, 2018

Fix typo removable #2760

Closed

olljanat force-pushed the replicas-max-per-node branch 2 times, most recently from f3919d1 to 6bc6b12 Compare October 4, 2018 21:10

dperny approved these changes Oct 8, 2018

View reviewed changes

wk8 reviewed Oct 11, 2018

View reviewed changes

olljanat force-pushed the replicas-max-per-node branch 2 times, most recently from f58495f to 1c93a4b Compare October 14, 2018 17:22

Added support for --replicas-max-per-node switch

6553bbe

Signed-off-by: Olli Janatuinen <[email protected]>

olljanat force-pushed the replicas-max-per-node branch from 1c93a4b to 6553bbe Compare October 14, 2018 17:42

wk8 approved these changes Oct 17, 2018

View reviewed changes

cpuguy83 approved these changes Oct 17, 2018

View reviewed changes

anshulpundir approved these changes Oct 17, 2018

View reviewed changes

anshulpundir merged commit 7777b15 into moby:master Oct 17, 2018

anshulpundir mentioned this pull request Oct 18, 2018

Bump swarmkit to a84c01f49091167dd086c26b45dc18b38d52e4d9 moby/moby#38056

Merged

thaJeztah mentioned this pull request Oct 18, 2018

WIP [18.09 backport] Bump swarmkit to a84c01f docker-archive/engine#89

Closed

olljanat mentioned this pull request Oct 29, 2018

Add --max-replicas-per-node for docker service create moby/moby#26259

Closed

olljanat mentioned this pull request Apr 7, 2020

max_replicas_per_node in paused/drained nodes drives to -> no suitable node when updating service docker-archive/classicswarm#2979

Closed

Alvise88 mentioned this pull request Oct 15, 2020

Usage of max-replicas-per-node not compatible with start-first update_config moby/moby#40797

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for maximum replicas per node #2758

Added support for maximum replicas per node #2758

olljanat commented Sep 29, 2018 •

edited

Loading

codecov bot commented Sep 30, 2018 •

edited

Loading

olljanat commented Sep 30, 2018

dperny left a comment

dperny Oct 1, 2018

dperny Oct 1, 2018

olljanat Oct 1, 2018

dperny Oct 1, 2018

cpuguy83 commented Oct 1, 2018

olljanat commented Oct 1, 2018

olljanat commented Oct 3, 2018 •

edited

Loading

olljanat commented Oct 5, 2018

dperny commented Oct 8, 2018

wk8 left a comment

wk8 Oct 11, 2018

wk8 Oct 11, 2018

olljanat commented Oct 14, 2018

wk8 left a comment

cpuguy83 left a comment

Added support for maximum replicas per node #2758

Added support for maximum replicas per node #2758

Conversation

olljanat commented Sep 29, 2018 • edited Loading

codecov bot commented Sep 30, 2018 • edited Loading

Codecov Report

olljanat commented Sep 30, 2018

dperny left a comment

Choose a reason for hiding this comment

dperny Oct 1, 2018

Choose a reason for hiding this comment

dperny Oct 1, 2018

Choose a reason for hiding this comment

olljanat Oct 1, 2018

Choose a reason for hiding this comment

dperny Oct 1, 2018

Choose a reason for hiding this comment

cpuguy83 commented Oct 1, 2018

olljanat commented Oct 1, 2018

olljanat commented Oct 3, 2018 • edited Loading

olljanat commented Oct 5, 2018

dperny commented Oct 8, 2018

wk8 left a comment

Choose a reason for hiding this comment

wk8 Oct 11, 2018

Choose a reason for hiding this comment

wk8 Oct 11, 2018

Choose a reason for hiding this comment

olljanat commented Oct 14, 2018

wk8 left a comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

olljanat commented Sep 29, 2018 •

edited

Loading

codecov bot commented Sep 30, 2018 •

edited

Loading

olljanat commented Oct 3, 2018 •

edited

Loading