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

Synchronous service create and service update #31144

Merged
merged 2 commits into from Apr 4, 2017

Conversation

@aaronlehmann
Contributor

aaronlehmann commented Feb 18, 2017

Change "service create" and "service update" to wait until the creation or update finishes, when --detach=false is specified. Show progress bars for the overall operation and for each individual task (when there are a small enough number of tasks), unless -q / --quiet is specified.

Internals:

  • The current implementation is polling. It would be much better to use the Swarm events API once it's ready (@dongluochen).
  • The task endpoint spec is not included when evaluating if a task is up to date. This structure isn't exposed through the REST API, so it would need to be added (or we need to come up with a better way of checking if a task is up to date).
  • The method of checking for global service convergence is a terrible hack. The constraint evaluator was ported to the client so it can tell which nodes match the constraints. This needs to be redone in a better way.
  • Handle rollback update states (once #31108 is merged)

Cosmetic/UX:

  • The progress bars show numbers like "9B/9B", assuming the units are bytes. This should be changed to drop the "B" suffix.
  • Upon ^C, the command should print a notice that the creation or update is still continuing in the background.
  • Possible visual improvements: line up progress bars, etc.

cc @docker/core-swarmkit-maintainers @tiborvass @anusha-ragunathan

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
Contributor

aaronlehmann commented Feb 18, 2017

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 22, 2017

Member

@aaronlehmann what happens if the service created can't be scheduled, because of constraint that cannot be fullfilled for example ? How should it behave ?

Member

vdemeester commented Feb 22, 2017

@aaronlehmann what happens if the service created can't be scheduled, because of constraint that cannot be fullfilled for example ? How should it behave ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 22, 2017

Member

I like the feature but it would be a huge breaking change. It feels a bit like the docker-compose up vs docker-compose up -d debate.

I can see use-cases for this, but should we also attach to the service logs after the service was deployed (like docker-compose up)?

Let me add this to the next maintainers meeting to get more eyes / open the discussion

Member

thaJeztah commented Feb 22, 2017

I like the feature but it would be a huge breaking change. It feels a bit like the docker-compose up vs docker-compose up -d debate.

I can see use-cases for this, but should we also attach to the service logs after the service was deployed (like docker-compose up)?

Let me add this to the next maintainers meeting to get more eyes / open the discussion

@thaJeztah thaJeztah added this to backlog in maintainers-session Feb 22, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 22, 2017

Contributor

what happens if the service created can't be scheduled, because of constraint that cannot be fullfilled for example ? How should it behave ?

You would see one or more of the tasks stuck at "pending". This synchronous mode gives visibility into the problem, which would otherwise require a "service ps" command to notice.

Contributor

aaronlehmann commented Feb 22, 2017

what happens if the service created can't be scheduled, because of constraint that cannot be fullfilled for example ? How should it behave ?

You would see one or more of the tasks stuck at "pending". This synchronous mode gives visibility into the problem, which would otherwise require a "service ps" command to notice.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 22, 2017

Member

You would see one or more of the tasks stuck at "pending".

Would it output the error messages / reason for being unable to deploy the task?

Member

thaJeztah commented Feb 22, 2017

You would see one or more of the tasks stuck at "pending".

Would it output the error messages / reason for being unable to deploy the task?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 22, 2017

Contributor

Not at present, but that's definitely possible.

Contributor

aaronlehmann commented Feb 22, 2017

Not at present, but that's definitely possible.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 23, 2017

Member

+1 for the feature, -1 for changing the default immediately.

I think we need to put it behind a flag for now. We can add a warning when no flag is used, and change the default in a release or two.

Changing the default is going to break people who use this command in scripts.

Member

dnephin commented Feb 23, 2017

+1 for the feature, -1 for changing the default immediately.

I think we need to put it behind a flag for now. We can add a warning when no flag is used, and change the default in a release or two.

Changing the default is going to break people who use this command in scripts.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 23, 2017

Member

We should also look at this for docker stack deploy, to show progress, and ideally show logs as well for a stack (to be more like docker-compose)

We discussed this in the maintainers meeting, and like the feature, but (for now) to not be the default.

Member

thaJeztah commented Feb 23, 2017

We should also look at this for docker stack deploy, to show progress, and ideally show logs as well for a stack (to be more like docker-compose)

We discussed this in the maintainers meeting, and like the feature, but (for now) to not be the default.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Feb 23, 2017

Member

Huge +1

It's also going to "fix" the workflow for many people that currently do a service create then just a bunch of service ps and then have to inspect tasks to figure out what happened.

Or worse - people that just do service create. Contrary to docker run, a service is not immediately ready and service creation is fully asynchronous. So the service is not responding, the port doesn't work, and the rollout may have failed.

UX Feedback:

  • Everything should be aligned. I think the state should be padded to the longest possible value so the progress bars don't "shift" to the left/right when the state changes
  • What's the XB/XB at the end?
  • Waiting to verify that tasks are stable - this is a bit non obvious, some rewording might help. Also, could we have a countdown? e.g. Waiting X seconds
  • Not sure if overall needs its own progress bar. By definition it has a slightly different meaning than the others. On first look it seems like a slow task. Maybe plan text would be better, e.g. Updating service <foo> (2 out of 12 completed)
  • What's interesting is error handling here. Could you link to a demo? I think the interesting bits of information are the overall status (the one in UpdateStatus - which I think should be part of the first line), the task error (can we show it somewhere?) and the rollback/pause status
  • When the user Ctrl-C's the deployment, we should say that it's continuing in the background
  • When we rollback, we should print the error message etc
  • When we pause, we should say why and how to continue

Looks amazing so far!

Member

aluzzardi commented Feb 23, 2017

Huge +1

It's also going to "fix" the workflow for many people that currently do a service create then just a bunch of service ps and then have to inspect tasks to figure out what happened.

Or worse - people that just do service create. Contrary to docker run, a service is not immediately ready and service creation is fully asynchronous. So the service is not responding, the port doesn't work, and the rollout may have failed.

UX Feedback:

  • Everything should be aligned. I think the state should be padded to the longest possible value so the progress bars don't "shift" to the left/right when the state changes
  • What's the XB/XB at the end?
  • Waiting to verify that tasks are stable - this is a bit non obvious, some rewording might help. Also, could we have a countdown? e.g. Waiting X seconds
  • Not sure if overall needs its own progress bar. By definition it has a slightly different meaning than the others. On first look it seems like a slow task. Maybe plan text would be better, e.g. Updating service <foo> (2 out of 12 completed)
  • What's interesting is error handling here. Could you link to a demo? I think the interesting bits of information are the overall status (the one in UpdateStatus - which I think should be part of the first line), the task error (can we show it somewhere?) and the rollback/pause status
  • When the user Ctrl-C's the deployment, we should say that it's continuing in the background
  • When we rollback, we should print the error message etc
  • When we pause, we should say why and how to continue

Looks amazing so far!

@thaJeztah thaJeztah removed this from backlog in maintainers-session Feb 23, 2017

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Feb 23, 2017

Contributor

Looks awesome.

  • Should Ctrl-C rollback the deployment?
  • I think this going on by default might be too much, but an --interactive flag sounds good.
Contributor

diogomonica commented Feb 23, 2017

Looks awesome.

  • Should Ctrl-C rollback the deployment?
  • I think this going on by default might be too much, but an --interactive flag sounds good.
@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Feb 24, 2017

Contributor

I'm with @dnephin: huge 👍, but I wouldn't change the default.

Contributor

icecrime commented Feb 24, 2017

I'm with @dnephin: huge 👍, but I wouldn't change the default.

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Feb 24, 2017

Contributor

Nice work!

Regarding re-usability of the service progressbar for other clients that potentially want to display service updates (eg, plugin install), it is better to extract cli/command/service/progress.go into a separate package.

Contributor

anusha-ragunathan commented Feb 24, 2017

Nice work!

Regarding re-usability of the service progressbar for other clients that potentially want to display service updates (eg, plugin install), it is better to extract cli/command/service/progress.go into a separate package.

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Feb 25, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 25, 2017

Contributor

Thanks for all the comments. I've updated this to address the feedback. Details below, and new demo here: https://asciinema.org/a/9f9rgk0e7fczkxi5vfu7yofmw

I think we need to put it behind a flag for now. We can add a warning when no flag is used, and change the default in a release or two.

I've changed the behavior so the default is unchanged, and the -i / --interactive flag is required to see progress info. One thought I had is that if we only enable this by default when the command is running on a TTY, it's less likely we would break scripts. But I'm also okay with putting this behind an option, for now. It also means there's less urgency in fixing the technical implementation issues described in the PR description (though I would like to fix at least some of them before merging).

Everything should be aligned. I think the state should be padded to the longest possible value so the progress bars don't "shift" to the left/right when the state changes

Done.

What's the XB/XB at the end?

This was an artifact of the progress bar code. I've now added an option that suppresses it.

Waiting to verify that tasks are stable - this is a bit non obvious, some rewording might help. Also, could we have a countdown? e.g. Waiting X seconds

Changed this so it has a countdown now.

Not sure if overall needs its own progress bar. By definition it has a slightly different meaning than the others. On first look it seems like a slow task. Maybe plan text would be better, e.g. Updating service (2 out of 12 completed)

Changed to plain text

What's interesting is error handling here. Could you link to a demo? I think the interesting bits of information are the overall status (the one in UpdateStatus - which I think should be part of the first line), the task error (can we show it somewhere?) and the rollback/pause status

Demo linked above. I've done some more testing and refinement of this now. Unfortunately it's not especially straightforward to show the task error, because the ID of the task that paused an update is inside a textual message. But we can figure something out if this would be a big usability win.

When the user Ctrl-C's the deployment, we should say that it's continuing in the background

Done

When we rollback, we should print the error message etc

Blocked on #31108

When we pause, we should say why and how to continue

It says why. I'm not sure about the "how to continue" part. Any suggestions for what the message should say?

Regarding re-usability of the service progressbar for other clients that potentially want to display service updates (eg, plugin install), it is better to extract cli/command/service/progress.go into a separate package.

Moved to github.com/docker/docker/cli/command/service/progress.

Contributor

aaronlehmann commented Feb 25, 2017

Thanks for all the comments. I've updated this to address the feedback. Details below, and new demo here: https://asciinema.org/a/9f9rgk0e7fczkxi5vfu7yofmw

I think we need to put it behind a flag for now. We can add a warning when no flag is used, and change the default in a release or two.

I've changed the behavior so the default is unchanged, and the -i / --interactive flag is required to see progress info. One thought I had is that if we only enable this by default when the command is running on a TTY, it's less likely we would break scripts. But I'm also okay with putting this behind an option, for now. It also means there's less urgency in fixing the technical implementation issues described in the PR description (though I would like to fix at least some of them before merging).

Everything should be aligned. I think the state should be padded to the longest possible value so the progress bars don't "shift" to the left/right when the state changes

Done.

What's the XB/XB at the end?

This was an artifact of the progress bar code. I've now added an option that suppresses it.

Waiting to verify that tasks are stable - this is a bit non obvious, some rewording might help. Also, could we have a countdown? e.g. Waiting X seconds

Changed this so it has a countdown now.

Not sure if overall needs its own progress bar. By definition it has a slightly different meaning than the others. On first look it seems like a slow task. Maybe plan text would be better, e.g. Updating service (2 out of 12 completed)

Changed to plain text

What's interesting is error handling here. Could you link to a demo? I think the interesting bits of information are the overall status (the one in UpdateStatus - which I think should be part of the first line), the task error (can we show it somewhere?) and the rollback/pause status

Demo linked above. I've done some more testing and refinement of this now. Unfortunately it's not especially straightforward to show the task error, because the ID of the task that paused an update is inside a textual message. But we can figure something out if this would be a big usability win.

When the user Ctrl-C's the deployment, we should say that it's continuing in the background

Done

When we rollback, we should print the error message etc

Blocked on #31108

When we pause, we should say why and how to continue

It says why. I'm not sure about the "how to continue" part. Any suggestions for what the message should say?

Regarding re-usability of the service progressbar for other clients that potentially want to display service updates (eg, plugin install), it is better to extract cli/command/service/progress.go into a separate package.

Moved to github.com/docker/docker/cli/command/service/progress.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Feb 25, 2017

Member

It says why. I'm not sure about the "how to continue" part. Any suggestions for what the message should say?

docker service update <foo> should continue the paused deployment right?

Regarding the verification time - how are we picking the 45 seconds?

Also, is this looking at task status or update status? As in, do we take into account the rollback threshold?

Member

aluzzardi commented Feb 25, 2017

It says why. I'm not sure about the "how to continue" part. Any suggestions for what the message should say?

docker service update <foo> should continue the paused deployment right?

Regarding the verification time - how are we picking the 45 seconds?

Also, is this looking at task status or update status? As in, do we take into account the rollback threshold?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 25, 2017

Contributor

docker service update <foo> should continue the paused deployment right?

Yeah, but I'm not sure we want to recommend that, because usually there's an underlying problem that should be fixed.

Regarding the verification time - how are we picking the 45 seconds?

I set --update-monitor 45s when I created the service, to demonstrate failures (by manually stopping some containers). The default is 5s.

Also, is this looking at task status or update status? As in, do we take into account the rollback threshold?

Sort of a mix of both. Basically, if the update pauses or rolls back, we'll see that in the update status. But the progress bars and countdown are driven by task status.

Contributor

aaronlehmann commented Feb 25, 2017

docker service update <foo> should continue the paused deployment right?

Yeah, but I'm not sure we want to recommend that, because usually there's an underlying problem that should be fixed.

Regarding the verification time - how are we picking the 45 seconds?

I set --update-monitor 45s when I created the service, to demonstrate failures (by manually stopping some containers). The default is 5s.

Also, is this looking at task status or update status? As in, do we take into account the rollback threshold?

Sort of a mix of both. Basically, if the update pauses or rolls back, we'll see that in the update status. But the progress bars and countdown are driven by task status.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Feb 25, 2017

Member

Looks great :)

Do we have some CLI designers in the house? @dnephin feedback?

Member

aluzzardi commented Feb 25, 2017

Looks great :)

Do we have some CLI designers in the house? @dnephin feedback?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 27, 2017

Member

Design LGTM

Member

dnephin commented Feb 27, 2017

Design LGTM

@WTFKr0

This comment has been minimized.

Show comment
Hide comment
@WTFKr0

WTFKr0 Feb 27, 2017

Hi
Is it possible to manage "docker service rm" action too ?
And what about "docker stack deploy/rm" command which launch multiple "docker service" actions ?
Thanx for reading

WTFKr0 commented Feb 27, 2017

Hi
Is it possible to manage "docker service rm" action too ?
And what about "docker stack deploy/rm" command which launch multiple "docker service" actions ?
Thanx for reading

@vieux vieux merged commit 21ec12b into moby:master Apr 4, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32471 has succeeded
Details
janky Jenkins build Docker-PRs 41058 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1224 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12158 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1062 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 4, 2017

@aaronlehmann aaronlehmann deleted the aaronlehmann:synchronous-service-commands branch Apr 4, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

Thanks everyone!

Contributor

aaronlehmann commented Apr 4, 2017

Thanks everyone!

@nathanleclaire

This comment has been minimized.

Show comment
Hide comment
@nathanleclaire

nathanleclaire Apr 4, 2017

Contributor

Is there a lil asciinema or GIF of this?

Contributor

nathanleclaire commented Apr 4, 2017

Is there a lil asciinema or GIF of this?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

@nathanleclaire: https://asciinema.org/a/9f9rgk0e7fczkxi5vfu7yofmw (slightly out of date, but mostly the same as what was merged)

Contributor

aaronlehmann commented Apr 4, 2017

@nathanleclaire: https://asciinema.org/a/9f9rgk0e7fczkxi5vfu7yofmw (slightly out of date, but mostly the same as what was merged)

@marcosnils

This comment has been minimized.

Show comment
Hide comment
@marcosnils

marcosnils Apr 4, 2017

Hey guys, per @vieux request Play With Docker has been updated with this patch. You can play with it there. Let me know in case you need me to change anything.

marcosnils commented Apr 4, 2017

Hey guys, per @vieux request Play With Docker has been updated with this patch. You can play with it there. Let me know in case you need me to change anything.

@sirlatrom

This comment has been minimized.

Show comment
Hide comment
@sirlatrom

sirlatrom Apr 4, 2017

@aaronlehmann Great to see this merged 👍 Is there going to be a follow-up for docker service scale? I know that I can do docker service update --detach=false --replicas=5, but docker service scale allows scaling multiple services more than one service at a time, which is potentially a more complex operation to show than updating a single service.

Also, a minor detail: --replicas 5, --name my_service, etc. work, but --detach false does not; only --detach=false. Is this intentional?

sirlatrom commented Apr 4, 2017

@aaronlehmann Great to see this merged 👍 Is there going to be a follow-up for docker service scale? I know that I can do docker service update --detach=false --replicas=5, but docker service scale allows scaling multiple services more than one service at a time, which is potentially a more complex operation to show than updating a single service.

Also, a minor detail: --replicas 5, --name my_service, etc. work, but --detach false does not; only --detach=false. Is this intentional?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 4, 2017

Member

but --detach false does not

Could it be that (because false is a built-in / shell command), that the shell evaluates false if there's no = in between?

Member

thaJeztah commented Apr 4, 2017

but --detach false does not

Could it be that (because false is a built-in / shell command), that the shell evaluates false if there's no = in between?

@sirlatrom

This comment has been minimized.

Show comment
Hide comment
@sirlatrom

sirlatrom Apr 4, 2017

@thaJeztah No, that would require me putting false inside backticks or a subshell. I have checked, and the --disable-content-trust argument to e.g. docker run behaves the same. I guess the difference between boolean and string arguments is that boolean arguments will be interpreted as set to true if specified on the command line, requiring an explicit =false to set them to false.

sirlatrom commented Apr 4, 2017

@thaJeztah No, that would require me putting false inside backticks or a subshell. I have checked, and the --disable-content-trust argument to e.g. docker run behaves the same. I guess the difference between boolean and string arguments is that boolean arguments will be interpreted as set to true if specified on the command line, requiring an explicit =false to set them to false.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 4, 2017

Member

@sirlatrom interesting; could you open an issue for that, so that it can be looked into? It should be either fixed, or at least documented

Member

thaJeztah commented Apr 4, 2017

@sirlatrom interesting; could you open an issue for that, so that it can be looked into? It should be either fixed, or at least documented

@sirlatrom

This comment has been minimized.

Show comment
Hide comment
@sirlatrom

sirlatrom Apr 4, 2017

@thaJeztah I believe this is how the go flags package (or whatever is being used in the Docker CLI) works, so it's really just working as designed. The --interactive/-i and --tty/-t flags to docker run work the same way, and one can specify e.g. --interactive=false (which is the default in that case).

sirlatrom commented Apr 4, 2017

@thaJeztah I believe this is how the go flags package (or whatever is being used in the Docker CLI) works, so it's really just working as designed. The --interactive/-i and --tty/-t flags to docker run work the same way, and one can specify e.g. --interactive=false (which is the default in that case).

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

It's because boolean flags can take an optional argument. --name requires an argument, so in --name my_service, my_service can be assumed to be the argument. But --detach is valid on its own, so false could be an entirely separate argument.

Contributor

aaronlehmann commented Apr 4, 2017

It's because boolean flags can take an optional argument. --name requires an argument, so in --name my_service, my_service can be assumed to be the argument. But --detach is valid on its own, so false could be an entirely separate argument.

@nathanleclaire

This comment has been minimized.

Show comment
Hide comment
@nathanleclaire
Contributor

nathanleclaire commented Apr 4, 2017

@marcosnils Cool!

@ozlatkov

This comment has been minimized.

Show comment
Hide comment
@ozlatkov

ozlatkov May 16, 2017

This looks great, thank you.

Just a quick one if anyone is aware - are there any plans to integrate this with

docker stack deploy

which in a nutshell creates/updates a collection of services?

Thanks again!

ozlatkov commented May 16, 2017

This looks great, thank you.

Just a quick one if anyone is aware - are there any plans to integrate this with

docker stack deploy

which in a nutshell creates/updates a collection of services?

Thanks again!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented May 16, 2017

@ozlatkov

This comment has been minimized.

Show comment
Hide comment
@ozlatkov

ozlatkov commented May 17, 2017

@thaJeztah - Thank you.

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