Add -a option to service/node ps #25983

jhorwit2 · 2016-08-24T14:43:27Z

Reopened #25251 because of weird github issue.

closes #25180
closes #25196

- What I did

I added an -a and --all flag to both commands to show all tasks.

- How I did it

Added a default filter to the CLI command when -a is false.

- How to verify it

Look at tests.

Run docker service ps & docker node ps on various nodes to ensure that only tasks are shown that are running or will be running w/o the -a flag. Also, make sure that --filter desired-state=<state> works with -a true/false.

- Description for the changelog

Add -a option to service/node ps

Signed-off-by: Josh Horwitz horwitzja@gmail.com

jhorwit2 · 2016-08-24T14:44:14Z

cc @thaJeztah @stevvooe @aluzzardi

Also, I'll try to remove the self code later today in favor of the other PR mentioned.

jhorwit2 · 2016-08-24T14:46:50Z

Whoops, I guess Gordon + Update Branch button = not friends.

dnephin · 2016-08-24T16:14:01Z

api/client/node/inspect.go

-		Args:  cli.RequiresMinArgs(1),
+		Use:   "inspect [OPTIONS] [NODE...]",
+		Short: "Display detailed information on one or more nodes (default to current node)",
+		Args:  nil,


Instead of nil could you use cobra.ArbitraryArgs so it's more explicit about what args are accepted?

I'm gonna be removing this shortly as it's covered in another PR by someone else. The only logic that will be in this one is the -a flag stuff.

thaJeztah · 2016-09-20T13:36:15Z

design LGTM. Moving to code review

thaJeztah · 2016-09-20T13:38:55Z

for the API - should this change be versioned? (i.e., for API 1.24 ignore the "all" option, and default to "all", for API 1.25 the opposite?)

stevvooe · 2016-09-23T23:34:48Z

api/types/client.go

@@ -292,6 +292,7 @@ type ServiceListOptions struct {
 // TaskListOptions holds parameters to list  tasks with.
 type TaskListOptions struct {
 	Filter filters.Args
+	All    bool


This shouldn't require such a literal API change. Shouldn't an empty filter send all?

I was following the same pattern in docker ps to keep it consistent. I agree though 👍

stevvooe · 2016-09-23T23:43:28Z

We need to be careful about the meaning of "all" here. In a cluster, this is very tricky. Most of the filtering here should be done on the client-side.

Also, right now, we mostly already show everything, so this PR would restrict to only running tasks (or preparing/starting, as well). Let's make sure that change is what we want.

cc @aluzzardi

jhorwit2 · 2016-09-23T23:53:20Z

@stevvooe Why do you think filtering should be done client-side? The logic is already in swarm to filter tasks, which reduces bytes over the wire. The result could be pretty large in a bigger cluster. Also, costly if you require quorum on each task returned.

What exactly are you worried about with "all" here? In this case, it really is all because the daemon is requesting a list from a manager. The UX here seems very similar to that of docker ps.

dnephin · 2016-09-26T18:31:17Z

docs/reference/api/docker_remote_api_v1.24.md

@@ -5088,6 +5088,9 @@ List tasks

 **Query parameters**:

+- **all** – 1/True/true or 0/False/false, Show all containers.
+    Only tasks that will be running or are currently running are shown by default(i.e., this defaults to false)
+


I think v1.24 is already released, so it shouldn't be added here.

dnephin · 2016-09-26T18:33:16Z

integration-cli/docker_cli_swarm_test.go

+	c.Assert(out, checker.Count, name, 2)
+	c.Assert(out, checker.Contains, "Running")
+	c.Assert(out, checker.Contains, "Shutdown")
+}


This looks like it's at least a couple different cases. I think this should be split up into different tests with a descriptive name of each case.

stevvooe · 2016-10-12T19:43:16Z

What exactly are you worried about with "all" here?

Does "all" really mean all or will it come to mean the output that is shown with -a?

Don't bind UX to API. We keep making this mistake and it makes this project impossible to work on because I keep finding UX buried deep in the backend.

thaJeztah · 2016-10-25T21:52:14Z

ping @stevvooe what do you suggest for changes? Is there some way to unblock this PR?

stevvooe · 2016-11-03T01:23:59Z

@thaJeztah By default, without a filter, the tasks endpoint should already return all. The standard mode in CLI should filter by state (all tasks <=running) and -a should be without a filter.

jhorwit2 · 2016-11-03T01:26:36Z

Sounds good to me.

jhorwit2 · 2016-11-03T14:05:17Z

@stevvooe PTAL.

thaJeztah · 2016-11-04T23:50:19Z

One nit in docs, but LGTM otherwise

Also, am I missing autocomplete files?

Yeah, this needs changes to those https://github.com/docker/docker/tree/master/contrib/completion, but we can also do those in a follow up if you prefer

Signed-off-by: Josh Horwitz <horwitzja@gmail.com>

jhorwit2 · 2016-11-06T12:45:01Z

@thaJeztah follow up PR sounds good to me. I made a ticket #28108

thaJeztah

LGTM

/cc @vdemeester

vdemeester

LGTM 🐸

aaronlehmann · 2016-11-16T21:50:37Z

TBH I'm not a fan of this change. It's a big UI change from 1.12.x and I think it will be confusing to users.

I just encountered it while using 1.13.0-rc1. I was trying to understand why a service wasn't starting, and docker service ps showed me nothing:

# docker service ps my_wordpress_db
NAME  IMAGE  NODE  DESIRED STATE  CURRENT STATE  ERROR  PORTS

I eventually discovered docker service ps -a, and then I could see the failed tasks.

aaronlehmann · 2016-11-16T21:56:07Z

I would suggest changing this so that we do show failed tasks by default, if there's no currently running tasks.

jhorwit2 · 2016-11-16T21:56:22Z

@aaronlehmann this is no different from how you would look into a container in docker ps that failed. You think it's good to have different UX across ps commands? As a user, that inconsistency across subcommands has always been confusing to me.

…ps -a` In moby#28507 and moby#28885, `docker service/node ps -a` has been removed so that information about slots are show up even without `-a` flag. The output of `docker stack ps` reused the same output as `docker service/node ps`. However, the `-a` was still there. It might make sense to remove `docker stack ps -a` as well to bring consistency with `docker service/node ps`. This fix is related to moby#28507, moby#28885, and moby#25983. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

…ps -a` In moby#28507 and moby#28885, `docker service/node ps -a` has been removed so that information about slots are show up even without `-a` flag. The output of `docker stack ps` reused the same output as `docker service/node ps`. However, the `-a` was still there. It might make sense to remove `docker stack ps -a` as well to bring consistency with `docker service/node ps`. This fix is related to moby#28507, moby#28885, and moby#25983. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> (cherry picked from commit 9155e14) Signed-off-by: Victor Vieux <vieux@docker.com> Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Add -a option to service/node ps

…ps -a` In moby#28507 and moby#28885, `docker service/node ps -a` has been removed so that information about slots are show up even without `-a` flag. The output of `docker stack ps` reused the same output as `docker service/node ps`. However, the `-a` was still there. It might make sense to remove `docker stack ps -a` as well to bring consistency with `docker service/node ps`. This fix is related to moby#28507, moby#28885, and moby#25983. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 24, 2016

jhorwit2 force-pushed the jah/ps-refactor branch from 1830083 to 53fd929 Compare August 24, 2016 14:48

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 24, 2016

dnephin reviewed Aug 24, 2016
View reviewed changes

jhorwit2 force-pushed the jah/ps-refactor branch 2 times, most recently from 2abf69b to 25c368b Compare August 26, 2016 13:51

tonistiigi added status/1-design-review status/needs-vendoring labels Aug 26, 2016

jhorwit2 force-pushed the jah/ps-refactor branch from 25c368b to 0f1cc69 Compare September 10, 2016 14:46

thaJeztah added status/2-code-review impact/api impact/changelog impact/cli and removed status/1-design-review labels Sep 20, 2016

stevvooe requested changes Sep 23, 2016

View reviewed changes

dnephin reviewed Sep 26, 2016

View reviewed changes

jhorwit2 force-pushed the jah/ps-refactor branch from 0f1cc69 to d88cee5 Compare November 3, 2016 14:04

jhorwit2 force-pushed the jah/ps-refactor branch from d88cee5 to 33e4f5e Compare November 3, 2016 14:06

jhorwit2 changed the title ~~Add -a option to service/node ps to show not running tasks~~ Add -a option to service/node ps Nov 3, 2016

Add -a option to service/node ps

139fff2

Signed-off-by: Josh Horwitz <horwitzja@gmail.com>

jhorwit2 force-pushed the jah/ps-refactor branch from 33e4f5e to 139fff2 Compare November 6, 2016 12:40

jhorwit2 mentioned this pull request Nov 6, 2016

update autocompletion docs for node/service ps -a flag #28108

Closed

jhorwit2 added status/3-docs-review and removed status/2-code-review labels Nov 7, 2016

thaJeztah approved these changes Nov 7, 2016

View reviewed changes

vdemeester approved these changes Nov 7, 2016

View reviewed changes

vdemeester merged commit 89ef0f2 into moby:master Nov 7, 2016

jhorwit2 added this to the 1.13.0 milestone Nov 7, 2016

albers mentioned this pull request Nov 13, 2016

Add bash completion for docker service ps --all #28360

Merged

jhorwit2 deleted the jah/ps-refactor branch November 16, 2016 21:57

aaronlehmann mentioned this pull request Nov 16, 2016

[1.13 UX] docker service ps -a #28507

Closed

This was referenced Dec 18, 2016

Remove bash completion for docker node ps --all|-a #29509

Merged

Remove --all|-a from docker node ps reference #29510

Merged

yongtang mentioned this pull request Dec 26, 2016

Remove docker stack ps -a to match removal of docker service/node ps -a #29716

Merged

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

Merge pull request moby#25983 from jhorwit2/jah/ps-refactor

dd33c28

Add -a option to service/node ps

ghost mentioned this pull request Sep 11, 2023

Provide option to hide task history on docker service ps #25196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -a option to service/node ps #25983

Add -a option to service/node ps #25983

jhorwit2 commented Aug 24, 2016 •

edited

Loading

jhorwit2 commented Aug 24, 2016

jhorwit2 commented Aug 24, 2016

dnephin Aug 24, 2016

jhorwit2 Aug 24, 2016

thaJeztah commented Sep 20, 2016

thaJeztah commented Sep 20, 2016

stevvooe Sep 23, 2016

jhorwit2 Sep 23, 2016

stevvooe commented Sep 23, 2016

jhorwit2 commented Sep 23, 2016

dnephin Sep 26, 2016 •

edited

Loading

dnephin Sep 26, 2016

stevvooe commented Oct 12, 2016

thaJeztah commented Oct 25, 2016

stevvooe commented Nov 3, 2016

jhorwit2 commented Nov 3, 2016

jhorwit2 commented Nov 3, 2016

thaJeztah commented Nov 4, 2016

jhorwit2 commented Nov 6, 2016

thaJeztah left a comment

vdemeester left a comment

aaronlehmann commented Nov 16, 2016

aaronlehmann commented Nov 16, 2016

jhorwit2 commented Nov 16, 2016 •

edited

Loading

Add -a option to service/node ps #25983

Add -a option to service/node ps #25983

Conversation

jhorwit2 commented Aug 24, 2016 • edited Loading

jhorwit2 commented Aug 24, 2016

jhorwit2 commented Aug 24, 2016

dnephin Aug 24, 2016

Choose a reason for hiding this comment

jhorwit2 Aug 24, 2016

Choose a reason for hiding this comment

thaJeztah commented Sep 20, 2016

thaJeztah commented Sep 20, 2016

stevvooe Sep 23, 2016

Choose a reason for hiding this comment

jhorwit2 Sep 23, 2016

Choose a reason for hiding this comment

stevvooe commented Sep 23, 2016

jhorwit2 commented Sep 23, 2016

dnephin Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

dnephin Sep 26, 2016

Choose a reason for hiding this comment

stevvooe commented Oct 12, 2016

thaJeztah commented Oct 25, 2016

stevvooe commented Nov 3, 2016

jhorwit2 commented Nov 3, 2016

jhorwit2 commented Nov 3, 2016

thaJeztah commented Nov 4, 2016

jhorwit2 commented Nov 6, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

aaronlehmann commented Nov 16, 2016

aaronlehmann commented Nov 16, 2016

jhorwit2 commented Nov 16, 2016 • edited Loading

jhorwit2 commented Aug 24, 2016 •

edited

Loading

dnephin Sep 26, 2016 •

edited

Loading

jhorwit2 commented Nov 16, 2016 •

edited

Loading