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

cli: docker service|node|stack ps instead of tasks #24816

Merged
merged 1 commit into from Jul 27, 2016

Conversation

stevvooe
Copy link
Contributor

@stevvooe stevvooe commented Jul 19, 2016

Rather than conflict with the unexposed task model, change the names of
the object-oriented task display to docker <object> ps. The command
works identically to docker service tasks. This change is superficial.

This provides a more sensical docker experience while not trampling on
the task model that may be introduced as a top-level command at a later
date.

The following is an example of the display using docker service ps
with a service named condescending_cori:

$ docker service ps condescending_cori
ID                         NAME                  SERVICE             IMAGE   LAST STATE              DESIRED STATE  NODE
e2cd9vqb62qjk38lw65uoffd2  condescending_cori.1  condescending_cori  alpine  Running 13 minutes ago  Running        6c6d232a5d0e

The following shows the output for the node on which the command is
running:

$ docker node ps self
ID                         NAME                  SERVICE             IMAGE   LAST STATE              DESIRED STATE  NODE
b1tpbi43k1ibevg2e94bmqo0s  mad_kalam.1           mad_kalam           apline  Accepted 2 seconds ago  Accepted       6c6d232a5d0e
e2cd9vqb62qjk38lw65uoffd2  condescending_cori.1  condescending_cori  alpine  Running 12 minutes ago  Running        6c6d232a5d0e
4x609m5o0qyn0kgpzvf0ad8x5  furious_davinci.1     furious_davinci     redis   Running 32 minutes ago  Running        6c6d232a5d0e

cc @aluzzardi

Closes #24152

Signed-off-by: Stephen J Day stephen.day@docker.com

@aluzzardi
Copy link
Member

LGTM

/cc @icecrime @dnephin

@abronan
Copy link
Contributor

abronan commented Jul 19, 2016

Design LGTM fwiw, I always found the use of <...> tasks confusing and inconsistent with the existing cli.

@dnephin
Copy link
Member

dnephin commented Jul 20, 2016

#24152 (comment)

I don't see this as an improvement. Users need to understand the difference between a container and a task. See #24241 (comment) for an example of why. Using ps confuses the two concepts leading to confused users.

It's better that a user has to learn what about tasks and understand the difference than be confused about why containers in swarm mode seem to be hidden for some commands and not others.

I also think ps in general is not the best term. I think we will likely be moving docker ps to docker container ls at some point, and ps wont be anywhere in the usage text.

docker x tasks to me is just short for docker x ls-tasks. docker images is the precedent.

@stevvooe
Copy link
Contributor Author

stevvooe commented Jul 20, 2016

It's better that a user has to learn what about tasks and understand the difference than be confused about why containers in swarm mode seem to be hidden for some commands and not others.

This is what I've been saying from the start but it really needs to a be top-level concept for this to make sense.

We use docker service|node|stack ps to avoid future conflict with the design of a top-level tasks command. We can use documentation to clarify to people that these are tasks, rather than containers.

@cmingxu
Copy link

cmingxu commented Jul 22, 2016

From what I learn now that node stack service are all first-class concept from a swarm point of view, but container image are downgrade to under node, while task is a concept under service or node

So I would suggest the following command like

docker node ls
docker stack ls
docker service ls

docker service tasks ls
docker node tasks ls
docker node containers ls
docker node images ls
docker stack services ls

@stevvooe
Copy link
Contributor Author

@cmingxu The problem with docker service tasks ls is that it can't be added in a backwards compatible manner. One cannot tell the difference between a service named ls and the command ls.

We will probably introduce a top-level docker task command to list and manage tasks, but not with 1.12.

@aluzzardi
Copy link
Member

aluzzardi commented Jul 22, 2016

Can we move this forward?

I believe tasks is the wrong thing - it exposes the user to an unnecessary concept right away.

My vote goes to ps.

An alternative approach I propose (but don't necessarily back) is behaving like ls and considering services directories and tasks files.

e.g.

docker service ls
[list of services]
docker service ls web
[list web tasks]

/cc @icecrime

@dnephin
Copy link
Member

dnephin commented Jul 25, 2016

it exposes the user to an unnecessary concept right away.

I don't think it's unnecessary. It's necessary to explain this (re-post from #24241 (comment)):

$ docker ps
# (no containers)

$ docker service ps
abcd123  nodeA...
abcd124  nodeB ...

$ docker inspect abcd123
# Shows the task

$ ssh nodeA inspect abcd123
# Shows the container

$ docker stop abcd123
Error: response from daemon: No such container: abcd123

User:

  • why does docker service ps show me docker "containers" that docker ps doesn't show?
  • why do I get different output for docker inspect from different nodes on the same swarm?
  • why can't I stop this container that I see in docker service ps ?

@stevvooe
Copy link
Contributor Author

@dnephin Much of these problems are from the early UX decision to avoid having a top-level tasks command to provide this functionality. We'll need to introduce this at some point, but now is too late for 1.12. docker xxx ps ensures that we have room to innovate on future versions and we don't need to match behavior of docker tasks with docker xxx ps.

Also, note that these will have different functions. docker xxx ps will show service tasks, while docker tasks may show plugins, unikernels or whatever. We need to tailor the output of these to the use case and, in this case, we want to show the "processes" that are implementing that service.

In general, most of the demonstrated problems come from confusing aspects of the swarmkit integration. For example, placing the inspection of tasks on docker inspect creates the confusion (not to mention the "multi-type" model being absurdly broken in practice) and ssh node inspect abcd123 won't show the container, since they have different ids (unless we overlap the names).

We are going to need to fill in these gaps with solid documentation and clarify that:

  1. docker xxx ps shows tasks not containers.
  2. How to get to a container for a particular service and stop it.
  3. If individual containers are being accessed, there is a problem with usage of swarm-mode.

@thaJeztah
Copy link
Member

/cc @sfsmithcha for your notes 😄 #24816 (comment)

@dnephin
Copy link
Member

dnephin commented Jul 25, 2016

Much of these problems are from the early UX decision to avoid having a top-level tasks command to provide this functionality. We'll need to introduce this at some point

I agree

docker xxx ps ensures that we have room to innovate on future versions and we don't need to match behavior of docker tasks with docker xxx ps

I disagree. I don't think renaming it makes it any easier or harder to introduce docker tasks.

while docker tasks may show plugins, unikernels or whatever

That would be very confusing. Why would docker tasks list anything but a swarm task? docker tasks should be identical to docker x tasks.

placing the inspection of tasks on docker inspect creates the confusion

I agree

ssh node inspect abcd123 won't show the container

True, that part of the example is no longer relevant after the other issue was closed.

@stevvooe
Copy link
Contributor Author

That would be very confusing. Why would docker tasks list anything but a swarm task? docker tasks should be identical to docker x tasks.

@dnephin We want to be able to schedule things other than containers as part of tasks.

@aluzzardi
Copy link
Member

why does docker service ps show me docker "containers" that docker ps doesn't show?
Because docker ps is local and docker service is cluster aware.

This is true for every docker service command

why do I get different output for docker inspect from different nodes on the same swarm?

That is true no matter what

why can't I stop this container that I see in docker service ps ?

Local vs Cluster

@crosbymichael
Copy link
Contributor

Looks like experimental build failures

18:35:10 api/client/stack/cmd.go:28: undefined: newTasksCommand
18:35:10 api/client/stack/ps.go:47: undefined: tasksOptions
18:35:11 Build step 'Execute shell' marked build as failure

@stevvooe
Copy link
Contributor Author

Looks like experimental build failures

Fixed.

@aaronlehmann
Copy link
Contributor

Design LGTM

@crosbymichael
Copy link
Contributor

i'll review after the tests complete

@dnephin
Copy link
Member

dnephin commented Jul 26, 2016

I expect ps to output containers

That's exactly the problem. docker service tasks doesn't list containers, and using ps is going to mislead users into thinking that it does.

@crosbymichael
Copy link
Contributor

crosbymichael commented Jul 26, 2016

LGTM

ps seems fine to use here. its docker's current term, for better or worse, for a "list" of something that is running, so this change makes it consistent.

@icecrime
Copy link
Contributor

LGTM

@@ -1,18 +1,18 @@
<!--[metadata]>
+++
title = "node tasks"
description = "The node tasks command description and usage"
title = "node ps"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add

aliases = ["/engine/reference/commandline/node_tasks/"]

to the metadata here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this?

aliases = ["/engine/reference/commandline/node_ps/"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, first example should be good; although it's not released yet, by now links can exist to the old location; the aliases adds a redirect-page for the old URL

@thaJeztah
Copy link
Member

This also needs updates to the completion scripts;
contrib/completion/bash/docker#L1576 (and three other locations) and contrib/completion/zsh/_docker

@thaJeztah
Copy link
Member

thanks @stevvooe, comments above, also if you need help with the completion scripts (the zsh looked trickier) we can do those in a follow-up

@stevvooe
Copy link
Contributor Author

@thaJeztah

  • Made updates to aliases
  • Updated bash completion, but it is broken in master:
bash-3.2$ docker serbash: _get_comp_words_by_ref: command not found
bash: [: 1: unary operator expected
bash: [: 1: unary operator expected
bash: [: -eq: unary operator expected
  • ZSH tested and working:
~/g/s/g/d/docker ❯❯❯ docker service                                                                                                                                                                                                                                                           service-ps-over-tasks ✭ ◼
 -- docker service command --
create   -- Create a new service
inspect  -- Display detailed information on one or more services
ls       -- List services
ps       -- List the tasks of a service
rm       -- Remove a service
scale    -- Scale one or multiple services
update   -- Update a service
~/g/s/g/d/docker ❯❯❯ docker service ps                                                                                                                                                                                                                                                        service-ps-over-tasks ✭ ◼
 -- service --
stoic_swanson  a6yjg74nab76  --   redis

@stevvooe
Copy link
Contributor Author

Looks like the problem with bash completion is #18140. I'm missing some bash completion helper.

Confirmed working after installing completion package:

bash-3.2$ docker service
create   inspect  list     ls       ps       remove   rm       scale    update
bash-3.2$ docker service ps
dreamy_leavitt  stoic_swanson

@thaJeztah
Copy link
Member

LGTM

ping @sfsmithcha ptal

@sfsmithcha
Copy link
Contributor

docs LGTM

@vdemeester
Copy link
Member

@stevvooe arf, all green 💚, but needs a rebase

Rather than conflict with the unexposed task model, change the names of
the object-oriented task display to `docker <object> ps`. The command
works identically to `docker service tasks`. This change is superficial.

This provides a more sensical docker experience while not trampling on
the task model that may be introduced as a top-level command at a later
date.

The following is an example of the display using `docker service ps`
with a service named `condescending_cori`:

```
$ docker service ps condescending_cori
ID                         NAME                  SERVICE             IMAGE   LAST STATE              DESIRED STATE  NODE
e2cd9vqb62qjk38lw65uoffd2  condescending_cori.1  condescending_cori  alpine  Running 13 minutes ago  Running        6c6d232a5d0e
```

The following shows the output for the node on which the command is
running:

```console
$ docker node ps self
ID                         NAME                  SERVICE             IMAGE   LAST STATE              DESIRED STATE  NODE
b1tpbi43k1ibevg2e94bmqo0s  mad_kalam.1           mad_kalam           apline  Accepted 2 seconds ago  Accepted       6c6d232a5d0e
e2cd9vqb62qjk38lw65uoffd2  condescending_cori.1  condescending_cori  alpine  Running 12 minutes ago  Running        6c6d232a5d0e
4x609m5o0qyn0kgpzvf0ad8x5  furious_davinci.1     furious_davinci     redis   Running 32 minutes ago  Running        6c6d232a5d0e
```

Signed-off-by: Stephen J Day <stephen.day@docker.com>
@thaJeztah
Copy link
Member

docs failures can be ignored

@stevvooe
Copy link
Contributor Author

@thaJeztah Looks like those are from master. Are there fixes we need in this branch?

Let's get this merged.

@thaJeztah
Copy link
Member

I discussed with @stevvooe; Windows CI is having issues (Jenkins), and the rebase only was due to documentation changes.

Given that it was green before those, I'll merge

@thaJeztah thaJeztah merged commit 1a7d339 into moby:master Jul 27, 2016
@stevvooe stevvooe deleted the service-ps-over-tasks branch July 27, 2016 21:46
@SvenDowideit
Copy link
Contributor

SvenDowideit commented Jul 28, 2016

@tiborvass @thaJeztah should this move to milestone 1.12.1 ?

mmm, confusing - so this is a merge from the 1.12.0 bump branch in #25140

I'm going to mark this as cherry-picked so the casual observer doesn't think that this was missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet