support show numbers of global service in service ls command #27710

allencloud · 2016-10-25T03:52:47Z

make command docker service ls show numbers for global services, like :

ID            NAME              MODE        REPLICAS  IMAGE         COMMAND
50va1ha64xhi  nauseous_beaver   replicated  1/1       nginx
a430fxzr07iq  nauseous_hodgkin  global      1/1       ubuntu:14.04  sleep 1000

if a sevice's mode is global, actually we can not get the total number of task in this service. Here what I did is that we collect all the tasks, and classify via task.ServiceID. Then we can know that in a service how many tasks are in this service. In addition, we can not simply get the number of nodes to make it the total number of tasks in a service, since user can use constraints in creating global service, then task number in a service may not equal to the number of nodes.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

Signed-off-by: allencloud allen.sun@daocloud.io

AkihiroSuda · 2016-10-25T04:37:11Z

I think MODE column should be introduced because (I feel) it looks pretty

ID            NAME              REPLICAS    MODE           IMAGE         COMMAND
50va1ha64xhi  nauseous_beaver   1/1         replicated     nginx
a430fxzr07iq  nauseous_hodgkin  1/1         global         ubuntu:14.04  sleep 1000

allencloud · 2016-10-25T05:25:05Z

Oh, to be honest, your example is better. @AkihiroSuda
How about moving MODE before REPLICAS?

AkihiroSuda · 2016-10-25T05:39:16Z

@allencloud SGTM

allencloud · 2016-10-25T09:30:26Z

It will add a new column for the output maybe need more discussion. WDYT? @thaJeztah @AkihiroSuda @vdemeester

vdemeester · 2016-10-25T16:38:03Z

/cc @thaJeztah @aluzzardi @stevvooe any reason why we did not add MODE in the first place ?
If we where to add this, I agree with @AkihiroSuda that we should add a new column.

thaJeztah · 2016-10-25T16:41:19Z

I agree with having a separate column as well (#27670 (comment)) 😄

thaJeztah · 2016-10-25T16:42:44Z

cli/command/service/list.go

@@ -116,7 +118,7 @@ func printTable(out io.Writer, services []swarm.Service, running map[string]int)
 		if service.Spec.Mode.Replicated != nil && service.Spec.Mode.Replicated.Replicas != nil {
 			replicas = fmt.Sprintf("%d/%d", running[service.ID], *service.Spec.Mode.Replicated.Replicas)
 		} else if service.Spec.Mode.Global != nil {
-			replicas = "global"
+			replicas = fmt.Sprintf("%d/%d global", running[service.ID], tasksPerService[service.ID])


Is the "total" here always using the total node count? If so, that should probably be changed to take constraints into account (which is supported for global services)

No, actually it is not the total node count. Here is scene:

5 nodes, 2 managers, 3 workers.

docker service create --constraint node.role==manager ubuntu:14.04 sleep 100000

before 1.13.0, global service will be every node even if the node does not meet the constraint. While since PR Only create global service tasks on nodes that satisfy the constraints swarmkit#1570, task will only be on nodes which satisfies the constraints. However this PR counts all tasks of a global service, and I think it will be OK for docker version both before 1.13- and 1.13+

Since 1.13.0, task of global service will only on node which satisfies constraints. So this PR counts node which only has the corresponding task. PTAL @thaJeztah

allencloud · 2016-10-26T18:04:23Z

Added a column already. PTAL @AkihiroSuda @vdemeester @thaJeztah

thaJeztah · 2016-10-26T21:55:12Z

design LGTM, but ping @aluzzardi PTAL

aluzzardi · 2016-10-26T23:28:25Z

LGTM

/cc @dongluochen

AkihiroSuda · 2016-10-27T06:22:01Z

cli/command/service/list.go

+
+	// first key is service id, the value of first key is a map
+	// In the second map, key is node ID, value is task number of this service on this node.
+	tasksPerService := map[string]map[string]int{}


can we avoid 2D map because we are just interested in the number of tasks?

Yeah, what we are interested in is number of tasks in every Service. I think we should classify tasks via different service.
If you have any better ways to implement this, please feel free to tell me.

You can implement the same thing with a struct-keyed map but I don't see this being accessed by taskID, so I don't quite understand what is being attempted here.

This should be called tasksByService or taskCountByService?

allencloud · 2016-10-31T09:29:59Z

ping @dongluochen . PTAL, thanks. 😃

aaronlehmann · 2016-11-02T17:37:46Z

cli/command/service/list.go

@@ -95,34 +95,51 @@ func PrintNotQuiet(out io.Writer, services []swarm.Service, nodes []swarm.Node,
 	}

 	running := map[string]int{}
+
+	// first key is service id, the value of first key is a map
+	// In the second map, key is task ID, if the task's desired status is not shutdown, it has a value.


Why is the map by task ID necessary? Does TaskList return duplicate tasks?

Oh, Yes, It is unnecessary now. Thanks a lot for your review. PTAL

aaronlehmann · 2016-11-03T16:43:02Z

docs/reference/commandline/service_ls.md

 ```
-
+  


Extra whitespace here. @thaJeztah does this matter in markdown?

Don't think it's a problem here, but @allencloud would be nice to clean up 😄

done, Thanks. Now cleaned up this. 😄

stevvooe · 2016-11-03T19:52:13Z

LGTM

aaronlehmann · 2016-11-04T22:20:54Z

LGTM

thaJeztah

LGTM

docs changes are going to conflict with #28029

/cc @mstanleyjones

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud · 2016-11-08T08:19:37Z

Solve conflict because merging of #28029
PTAL, my friends.

vdemeester

LGTM 🐯

mdlinville

LGTM. Leaving the merge for @thaJeztah

thaJeztah · 2016-11-08T17:53:29Z

oops, late to the party, thanks!

yank1 · 2016-11-09T01:52:40Z

LGTM

…vice-in-service-ls support show numbers of global service in service ls command

GordonTheTurtle added the status/0-triage label Oct 25, 2016

allencloud force-pushed the show-num-for-global-service-in-service-ls branch 2 times, most recently from 65c9aea to de687d7 Compare October 25, 2016 03:57

vdemeester added status/1-design-review and removed status/0-triage labels Oct 25, 2016

thaJeztah reviewed Oct 25, 2016

View reviewed changes

allencloud force-pushed the show-num-for-global-service-in-service-ls branch 8 times, most recently from 1d94a47 to 5c69224 Compare October 26, 2016 09:15

thaJeztah added status/2-code-review impact/changelog impact/cli impact/documentation and removed status/1-design-review labels Oct 27, 2016

AkihiroSuda reviewed Oct 27, 2016

View reviewed changes

allencloud force-pushed the show-num-for-global-service-in-service-ls branch from 1c1fc64 to d166eb8 Compare November 2, 2016 10:20

aaronlehmann reviewed Nov 2, 2016

View reviewed changes

allencloud force-pushed the show-num-for-global-service-in-service-ls branch 5 times, most recently from 036671a to 6b2d8d0 Compare November 3, 2016 10:25

aaronlehmann reviewed Nov 3, 2016

View reviewed changes

allencloud force-pushed the show-num-for-global-service-in-service-ls branch from 6b2d8d0 to 5021c03 Compare November 3, 2016 23:26

aaronlehmann added status/3-docs-review and removed status/2-code-review labels Nov 4, 2016

thaJeztah approved these changes Nov 4, 2016

View reviewed changes

mdlinville mentioned this pull request Nov 7, 2016

remove COMMAND column from service ls output. closes #27994 #28029

Merged

allencloud force-pushed the show-num-for-global-service-in-service-ls branch from 5021c03 to 5a6c967 Compare November 8, 2016 07:48

support show numbers of global service in service ls command

ea03f09

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud force-pushed the show-num-for-global-service-in-service-ls branch from 5a6c967 to ea03f09 Compare November 8, 2016 08:17

vdemeester approved these changes Nov 8, 2016

View reviewed changes

mdlinville approved these changes Nov 8, 2016

View reviewed changes

vdemeester merged commit a70b1d7 into moby:master Nov 8, 2016

GordonTheTurtle added this to the 1.13.0 milestone Nov 8, 2016

allencloud deleted the show-num-for-global-service-in-service-ls branch November 9, 2016 02:12

aaronlehmann mentioned this pull request Feb 2, 2017

MODE column not listed moby/swarmkit#1917

Closed

thaJeztah mentioned this pull request Feb 9, 2017

Global mode target replicas keep increasing #30854

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#27710 from allencloud/show-num-for-global-ser…

b04bb19

…vice-in-service-ls support show numbers of global service in service ls command

thaJeztah mentioned this pull request May 16, 2017

swarm creates more service replicas than desired #33218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support show numbers of global service in service ls command #27710

support show numbers of global service in service ls command #27710

allencloud commented Oct 25, 2016 •

edited

Loading

AkihiroSuda commented Oct 25, 2016

allencloud commented Oct 25, 2016

AkihiroSuda commented Oct 25, 2016

allencloud commented Oct 25, 2016

vdemeester commented Oct 25, 2016

thaJeztah commented Oct 25, 2016

thaJeztah Oct 25, 2016

allencloud Oct 26, 2016

allencloud Oct 26, 2016

allencloud commented Oct 26, 2016

thaJeztah commented Oct 26, 2016

aluzzardi commented Oct 26, 2016

AkihiroSuda Oct 27, 2016

allencloud Oct 27, 2016

stevvooe Nov 1, 2016

allencloud commented Oct 31, 2016

aaronlehmann Nov 2, 2016

allencloud Nov 3, 2016

aaronlehmann Nov 3, 2016

thaJeztah Nov 3, 2016

allencloud Nov 3, 2016

stevvooe commented Nov 3, 2016

aaronlehmann commented Nov 4, 2016

thaJeztah left a comment

allencloud commented Nov 8, 2016

vdemeester left a comment

mdlinville left a comment

thaJeztah commented Nov 8, 2016

yank1 commented Nov 9, 2016

support show numbers of global service in service ls command #27710

support show numbers of global service in service ls command #27710

Conversation

allencloud commented Oct 25, 2016 • edited Loading

AkihiroSuda commented Oct 25, 2016

allencloud commented Oct 25, 2016

AkihiroSuda commented Oct 25, 2016

allencloud commented Oct 25, 2016

vdemeester commented Oct 25, 2016

thaJeztah commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Oct 26, 2016

thaJeztah commented Oct 26, 2016

aluzzardi commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 3, 2016

aaronlehmann commented Nov 4, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

allencloud commented Nov 8, 2016

vdemeester left a comment

Choose a reason for hiding this comment

mdlinville left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 8, 2016

yank1 commented Nov 9, 2016

allencloud commented Oct 25, 2016 •

edited

Loading