Support multiple service IDs on "docker service ps" #25234

yongtang · 2016-07-29T15:25:46Z

- What I did

This fix tries to address the issue raised in #25228 to support multiple service IDs on docker service ps.

- How I did it

Multiple IDs are allowed with docker service ps ..., and related documentation has been updated.

- How to verify it

A test has been added to cover the changes.

- Description for the changelog

Support multiple service IDs on "docker service ps"

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

This fix fixes #25228.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

thaJeztah · 2016-08-01T10:50:26Z

functionally, it makes sense to have an option to show tasks for multiple services; I'm a bit in doubt what the best approach is. For example, docker ps by default shows everything, but requires a --filter to just show a single (container).

I realize docker service ps is slightly different, hence my doubt

ping @stevvooe @aluzzardi @tiborvass

ezrasilvera · 2016-08-01T12:52:55Z

@thaJeztah I'm not sure I would compare it to docker ps (on single engine). Maybe it is closer indeed to "ps" on the previous swarm where you got all containers and indeed used filter if needed.
In general I would love to see a more "old swarm" related options where one can see all the resources and filer out.
BTW, specific for "docker service" it's not reasonable to allow multiple IDs for "inspect" but not for other subcommands

thaJeztah · 2016-08-01T12:59:47Z

Yes, perhaps this is the best approach; I was thinking out loud, and thought I'd better write it up on GitHub, before I forget to do so

cpuguy83 · 2016-08-01T14:05:37Z

multiple ID's on service ps is a bit strange, tbh. I'm not sure the UX holds up to it.

cpuguy83 · 2016-08-01T14:07:05Z

Maybe with multiple it can change format so tasks are nested underneath a service... but docker service ls pretty much shows this same information, just without the individual task details.

yongtang · 2016-08-01T15:59:28Z

@cpuguy83 Maybe we could add an option (e.g., --detail or --tasks) to docker service ls, so that it will shown individual task details together with the services?

thaJeztah · 2016-08-18T18:31:55Z

We discussed this, and since there's no other option to show multiple services on multiple nodes, this is probably the best solution after all, so moving to code review

cpuguy83 · 2016-08-18T18:32:38Z

p.s.... I think this might be nice: docker service ps $(docker serivce ls -q --filter ...)

stevvooe · 2016-08-18T20:28:10Z

api/client/service/ps.go

-	serviceID string
-	noResolve bool
-	filter    opts.FilterOpt
+	serviceIDs []string


These aren't just service ids. These can also be the names of services. Make sure to model that correctly.

stevvooe · 2016-08-18T20:30:15Z

This should really support name or id for service lookup.

yongtang · 2016-08-20T02:20:27Z

Thanks @thaJeztah @stevvooe @cpuguy83. I have updated the pull request and now with both name prefix and ID searched for services.

One thing I am not sure is the return code when some (or all of the) terms searched does not found service with matched ID or matched name prefix. The currently PR will return empty result (but no error). Should we return error on it? Would like to hear suggestions on this scenario.

cpuguy83 · 2016-08-25T17:12:39Z

@yongtang Probably best to look at docker inspect output in this scenario.

cpuguy83 · 2016-08-25T17:35:26Z

Name prefix is a bit wierd, particularly when you have services with similar names, e.g. test and test2

stevvooe · 2016-08-27T00:01:16Z

@cpuguy83

Name prefix is a bit wierd, particularly when you have services with similar names, e.g. test and test2

Yes, but we've specified an algorithm that should work in most cases.

It must be modified here, since ambiguous lookups are no longer an error but a merge.

yongtang · 2016-08-29T00:53:28Z

Thanks @cpuguy83 @stevvooe. The pull request has been updated. Now service ps will return error in case any one of the name/ID can not be found, similar to docker inspect. Please let me know if there are any issues.

stevvooe · 2016-08-30T02:37:34Z

api/client/service/ps.go

+				for _, serviceEntry := range serviceList {
+					filter.Add("service", serviceEntry.ID)
+				}
+				continue service_search


What is this loop doing?

@stevvooe The outmost loop for _, service := range opts.services is to iterate through the input term(ID/Name) of the service ps.

The inner loop for _, filterType := range filterTypes is to search with id then with name.

During the search process, if either id or name search returns anything, it will add the serviceEntry.ID to the filter (for process later), and continue with the next term(ID/Name) of the service search (continue service_search).

If both id and name search returns nothing, then it will be added to the unknownServices.

Previously I adds all the search term (Name/ID) of service ps to id filter first, do one search, and then to name filter, do another search.

However, that will not give us the information on which search term (Name/ID) is a hit and which is not.

So I just search each term (Name/ID) one by one, first through id, then through name. This will allows us to show unfound terms like docker inspect.

@yongtang I can read the code. I mean "What is the purpose of this loop?". It's fairly obtuse.

@stevvooe Yes the loops might be unnecessarily redundant. Let me take a further look and see if it could be improved.

thaJeztah · 2016-09-21T21:27:57Z

ping @stevvooe PTAL #25234 (comment)

yongtang · 2016-10-05T23:38:32Z

@stevvooe the PR has been updated. Please take a look and let me know if there are any issues.

thaJeztah · 2016-10-21T20:06:13Z

ping @dnephin could you give this a review?

thaJeztah · 2016-10-21T20:06:31Z

also /cc @albers @sdurrheimer as this need changes to the completion scripts

dnephin · 2016-10-21T21:28:09Z

cli/command/service/ps.go

+		}
+		serviceNameFilter := filters.NewArgs()
+		serviceNameFilter.Add("name", service)
+		if serviceList, err := client.ServiceList(ctx, types.ServiceListOptions{Filter: serviceNameFilter}); err == nil {


Doing n * 2 service calls here is not great. Can this be done from the API?

@dnephin I tried to reduce the number of passes to list the services but cannot find a good way. The issue is that ServiceList will not indicate which search term is hit and which search term is a miss. We will not be able to know if any search term returns nothing if we combines search terms into less ServiceList calls.

I also thought about list all services in one call (without filtering), then do the filtering on the client side to find out which search term is a hit and which search term is a miss. This might be OK if the total number of services is small but I am not sure if it will be an issue if the total number of services is big.

Maybe that's a sign that we shouldn't support multiple service ids on this command?

@dnephin what's your suggestion? Any way to get this PR moving again, or are you not comfortable with this feature?

I don't have any ideas of how to make this work. I think it's a bit awkward to support multiple services from this command because you end up seeing a list of unrelated tasks.

@dnephin It already happens with docker node ps.

Not too worried about making extra calls, because the user will do it anyway. But if we can optimize it all the better.

yongtang · 2016-12-01T22:57:36Z

@dnephin I updated the PR and now there are only 2 service calls instead of n * 2 service calls. Please take a look and see if it fit the requirement.

dnephin · 2016-12-02T18:48:55Z

Thanks, this design works for me

thaJeztah · 2016-12-22T19:24:28Z

ping @yongtang looks like this needs a rebase

thaJeztah · 2016-12-22T19:25:30Z

ping @dnephin LGTY? If so we're ready to merge I think

This fix tries to address issue raised in 25228 to support multiple service IDs on `docker service ps`. Multiple IDs are allowed with `docker service ps ...`, and related documentation has been updated. A test has been added to cover the changes. This fix fixes 25228. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-12-22T19:34:22Z

@thaJeztah @dnephin The PR has been rebased. Thanks.

dnephin

LGTM

Support multiple service IDs on "docker service ps"

GordonTheTurtle added the status/0-triage label Jul 29, 2016

yongtang mentioned this pull request Jul 29, 2016

Support multiple service IDs on "docker service tasks" #25228

Closed

vdemeester added status/1-design-review and removed status/0-triage labels Jul 31, 2016

thaJeztah added status/2-code-review and removed status/1-design-review labels Aug 18, 2016

stevvooe reviewed Aug 18, 2016
View reviewed changes

yongtang force-pushed the 25228-service-ps-multiple-ID branch from 040e1f4 to f607e0e Compare August 20, 2016 02:14

yongtang force-pushed the 25228-service-ps-multiple-ID branch from f607e0e to 0890af7 Compare August 28, 2016 20:33

stevvooe reviewed Aug 30, 2016
View reviewed changes

yongtang force-pushed the 25228-service-ps-multiple-ID branch 2 times, most recently from aeeb76f to fb4edd6 Compare September 19, 2016 23:36

yongtang force-pushed the 25228-service-ps-multiple-ID branch from fb4edd6 to 3f0eaf3 Compare October 5, 2016 20:13

thaJeztah added impact/changelog impact/cli labels Oct 21, 2016

dnephin reviewed Oct 21, 2016

View reviewed changes

yongtang force-pushed the 25228-service-ps-multiple-ID branch 2 times, most recently from 6dd0e1f to 66d3ae3 Compare October 22, 2016 16:58

yongtang force-pushed the 25228-service-ps-multiple-ID branch from 66d3ae3 to eba9150 Compare November 8, 2016 04:07

yongtang force-pushed the 25228-service-ps-multiple-ID branch from eba9150 to d678736 Compare December 1, 2016 22:55

yongtang force-pushed the 25228-service-ps-multiple-ID branch from d678736 to 7094235 Compare December 22, 2016 19:34

dnephin approved these changes Dec 22, 2016

View reviewed changes

thaJeztah added this to the 1.14.0 milestone Jan 2, 2017

thaJeztah added status/4-merge and removed status/2-code-review labels Jan 2, 2017

vdemeester added the rebuild/windowsRS1 label Jan 2, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 2, 2017

vdemeester merged commit 1826559 into moby:master Jan 3, 2017

yongtang deleted the 25228-service-ps-multiple-ID branch January 3, 2017 12:51

albers mentioned this pull request Jan 3, 2017

Support multiple services on bash completion for docker service ps #29847

Merged

dperny mentioned this pull request Jan 9, 2017

how to know all the containers in the cluster moby/swarmkit#1781

Closed

sdurrheimer mentioned this pull request Jan 19, 2017

Add zsh completion for multiple service for 'docker service ps' #30269

Merged

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

Merge pull request moby#25234 from yongtang/25228-service-ps-multiple-ID

636e546

Support multiple service IDs on "docker service ps"

kinghuang mentioned this pull request Apr 20, 2017

docker servcie ps <service_name> lists all instances of services with names starting with service_name #32556

Closed

albers mentioned this pull request Aug 1, 2017

bash completion for service ps supports multiple services docker/cli#395

Merged

Support multiple service IDs on "docker service ps" #25234

Support multiple service IDs on "docker service ps" #25234

Conversation

yongtang commented Jul 29, 2016

thaJeztah commented Aug 1, 2016

ezrasilvera commented Aug 1, 2016

thaJeztah commented Aug 1, 2016

cpuguy83 commented Aug 1, 2016

cpuguy83 commented Aug 1, 2016

yongtang commented Aug 1, 2016

thaJeztah commented Aug 18, 2016

cpuguy83 commented Aug 18, 2016

Choose a reason for hiding this comment

stevvooe commented Aug 18, 2016

yongtang commented Aug 20, 2016

cpuguy83 commented Aug 25, 2016

cpuguy83 commented Aug 25, 2016

stevvooe commented Aug 27, 2016 • edited

yongtang commented Aug 29, 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

thaJeztah commented Sep 21, 2016

yongtang commented Oct 5, 2016

thaJeztah commented Oct 21, 2016

thaJeztah commented Oct 21, 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

Choose a reason for hiding this comment

yongtang commented Dec 1, 2016

dnephin commented Dec 2, 2016

thaJeztah commented Dec 22, 2016

thaJeztah commented Dec 22, 2016

yongtang commented Dec 22, 2016

dnephin left a comment

Choose a reason for hiding this comment

stevvooe commented Aug 27, 2016 •

edited