Add 'docker service rollback' subcommand #205

redpanda · 2017-06-19T19:09:11Z

Signed-off-by: Jimmy Leger jimmy.leger@gmail.com

fixes #142
PS: This bug was fixed during a Docker Hackergarten.

- What I did

I add the docker service rollback subcommand

- How I did it

I extracted the rollback process from runUpdate

- How to verify it

Create a service with a single replica:

$ docker service create --name my-service -p 8080:80 nginx:alpine

Confirm that the service is running with a single replica:

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          1/1                 nginx:alpine        *:8080->80/tcp

Update the service to use three replicas:

$ docker service update --replicas=3 my-service

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          3/3                 nginx:alpine        *:8080->80/tcp

Now roll back the service to its previous version, and confirm it is
running a single replica again:

$ docker service rollback my-service

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          1/1                 nginx:alpine        *:8080->80/tcp

- Description for the changelog

Add docker service rollback subcommand

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

dnephin

Thanks!

dnephin · 2017-06-19T19:12:51Z

cli/command/service/rollback.go

+		Aliases: []string{"rollback"},
+		Short:   "Rollback a service",
+		Args:    cli.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {


The pattern we use in these packages is to keep the inline function separate from adding flag.

The flags can be added at the end of newRollbackCommand() before return cmd

dnephin · 2017-06-19T19:13:41Z

cli/command/service/rollback.go

+		Args:    cli.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			flags := cmd.Flags()
+			flags.Bool("rollback", true, "")


Since the command is rollback, we don't need a rollback flag, right?

I guess the calling code expects this? We should refactor runUpdate() so that we have a function we can call directly that doesn't require such a flag.

dnephin · 2017-06-19T19:13:58Z

cli/command/service/rollback.go

+			flags := cmd.Flags()
+			flags.Bool("rollback", true, "")
+			flags.Bool("force", false, "Force update even if no changes require it")
+			addServiceFlags(flags, options, nil)


I don't think we want to add all the service flags. We only want the force flag added.

codecov-io · 2017-06-19T19:20:59Z

Codecov Report

❗ No coverage uploaded for pull request base (master@17adcbd). Click here to learn what that means.
The diff coverage is 76.36%.

@@            Coverage Diff            @@
##             master     #205   +/-   ##
=========================================
  Coverage          ?   46.33%           
=========================================
  Files             ?      194           
  Lines             ?    16134           
  Branches          ?        0           
=========================================
  Hits              ?     7475           
  Misses            ?     8272           
  Partials          ?      387

redpanda · 2017-06-25T22:13:26Z

Hello @dnephin!

First thank for your feedbacks.

I don't make the choice to refactor the runUpdate function but just implement runRollback which only call ServiceUpdate endpoint with the needed parameters.

BTW I have some code duplication between runUpdate and runRollback see: https://github.com/docker/cli/pull/205/files#diff-09b79b8a3ba1feabc2d859a294bb1ac5R43
Should I create a function to initialize types.ServiceUpdateOptions ?

Thanks by advance for your feedbacks.

dnephin

Thanks! This is looking better

I would suggest rebasing on docker/master to pick the new CI changes. It will help your build run faster.

dnephin · 2017-06-29T17:07:30Z

cli/command/service/rollback.go

+
+	cmd := &cobra.Command{
+		Use:     "rollback [OPTIONS]",
+		Aliases: []string{"rollback"},


This alias is not necessary because it's the same as the first word in Use:, so this line can be removed.

dnephin · 2017-06-29T17:12:46Z

cli/command/service/rollback.go

+	updateOpts := types.ServiceUpdateOptions{}
+	spec := &service.Spec
+
+	if versions.LessThan(dockerCli.Client().ClientVersion(), "1.28") {


I think we should just hide this command on earlier versions, so we don't need to check version here. You can do that by setting Tags on the cobra.Command above

Tags: map[string]string{"version": "1.31"}

That way we can always set updateOpts.Rollback to "previous"

dnephin

Thanks!

A unit test would be great as well.

There are examples of unit tests in other cli/command packages.

redpanda · 2017-07-04T17:17:01Z

Yeah I'm working on it! 👍

dnephin · 2017-07-05T15:06:33Z

cli/command/service/helpers.go

-func warnDetachDefault(err io.Writer, clientVersion string, flags *pflag.FlagSet, msg string) {
-	if !flags.Changed("detach") && versions.GreaterThanOrEqualTo(clientVersion, "1.29") {
+func warnDetachDefault(err io.Writer, clientVersion string, msg string) {
+	if versions.GreaterThanOrEqualTo(clientVersion, "1.29") {


Why this change? This doesn't look right

You right! I'm gonna fix that

redpanda · 2017-07-18T07:24:32Z

ping @vdemeester

thaJeztah

left some comments / suggestions

thaJeztah · 2017-08-01T09:09:50Z

docs/reference/commandline/service_rollback.md

+```
+```bash
+docker service update --image=redis:3.0.7 redis
+ID                  NAME                IMAGE               NODE                DESIRED STATE       CURRENT STATE                     ERROR                           PORTS


this example output is missing docker service ps (which is needed to show this output)

thaJeztah · 2017-08-01T09:11:08Z

docs/reference/commandline/service_rollback.md

+ID                  NAME                IMAGE               NODE                DESIRED STATE       CURRENT STATE                     ERROR                           PORTS
+3duazds0yne3        redis.1             3.0.7               moby                Ready               Rejected less than a second ago   "No such image: 3.0.7:latest"
+w08xdhdc5hjv         \_ redis.1         3.0.7               moby                Shutdown            Rejected 2 seconds ago            "No such image: 3.0.7:latest"
+x9x7gl374ih6         \_ redis.1         redis:3.0.6         moby                Shutdown            Shutdown 1 second ago


The "rejected" states here are probably only distracting from what we're trying to show here; I suggest updating the output to only show what's needed.

thaJeztah · 2017-08-01T09:14:26Z

docs/reference/commandline/service_rollback.md

+
+## Examples
+
+Rollback the `redis` service:


When using a custom name, it may be good to use something that's clearly "custom". Naming the service redis for a service that's using redis as an image, could lead to people thinking the name must be the same as the image.

thaJeztah · 2017-08-01T10:32:47Z

docs/reference/commandline/service_rollback.md

+Rollback the `redis` service:
+
+```bash
+$ docker service create --name redis redis:3.0.6


I wonder if "updating" the image from 3.0.6 to 3.0.7, then rolling back could make people think the "rollback" is rolling back an image update, not the service definition. Although it makes a nice example to update the image version, and roll back that change, would it be better to use something else for illustration? e.g.

### Roll back a service to its previous version This example creates a service with a single replica, updates the service to use three replicas, and then rolls back the service to the previous version, having one replica. Create a service with a single replica: ```bash $ docker service create --name my-service -p 8080:80 nginx:alpine ``` Confirm that the service is running with a single replica: ```bash $ docker service ls ID NAME MODE REPLICAS IMAGE PORTS xbw728mf6q0d my-service replicated 1/1 nginx:alpine *:8080->80/tcp ``` Update the service to use three replicas: ```bash $ docker service update --replicas=3 my-service $ docker service ls ID NAME MODE REPLICAS IMAGE PORTS xbw728mf6q0d my-service replicated 3/3 nginx:alpine *:8080->80/tcp ``` Now rollback the service to its previous version, and confirm it is running a single replica again: ```bash $ docker service rollback my-service $ docker service ls ID NAME MODE REPLICAS IMAGE PORTS xbw728mf6q0d my-service replicated 1/1 nginx:alpine *:8080->80/tcp ```

@mstanleyjones wdyt? ^^

Yep, agreed.

Thanks! I am going to use this example instead of the other one.

thaJeztah · 2017-08-01T10:54:33Z

docs/reference/commandline/service_rollback.md

+## Examples
+
+Rollback the `redis`
6D38
 service:
+


Agreed, this could use some additional information (also strip down the output to show just the relevant information, as mentioned in my other comments), something like;

### Roll back a service to a previous version In this example, a service is created using the `redis:3.0.6` image, updated to use the `redis:3.0.7` image, and then rolled back to its previous version. Create a service using the `redis:3.0.6` image: ```bash $ docker service create --name my-service redis:3.0.6 ``` Confirm that the service is running: ```bash $ docker service ps my-service ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS s2xak85rftfc my-service.1 redis:3.0.6 moby Running Running 4 seconds ago ``` Update the service to use the `redis:3.0.7` image: ```bash $ docker service update --image=redis:3.0.7 my-service ``` The service is now updated to use the `redis:3.0.7` image: ```bash $ docker service ps my-service ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS 7jtl9bxnxnx1 my-service.1 redis:3.0.7 moby Running Running less than a second ago s2xak85rftfc \_ my-service.1 redis:3.0.6 moby Shutdown Shutdown 10 seconds ago ``` Roll back the service to its previous version. Rolling back the service reverts all changes made to the service in the last update. In this example, rolling back the service changes the service's image back to `redis:3.0.6`. ```bash $ docker service rollback my-service $ docker service ps my-service ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS 8gwl1xoi0xh3 my-service.1 redis:3.0.6 moby Running Running 3 seconds ago 7jtl9bxnxnx1 \_ my-service.1 redis:3.0.7 moby Shutdown Shutdown 4 seconds ago s2xak85rftfc \_ my-service.1 redis:3.0.6 moby Shutdown Shutdown 2 minutes ago ```

mdlinville · 2017-08-01T17:29:07Z

docs/reference/commandline/service_update.md

 ```

-### Rolling back to the previous version of a service 
+### Rolling back to the previous version of a service


Don't use gerunds (-ing words) in titles. How about "Roll back to the previous version of a service"? By the way, when you are using the verb form, it is "roll back" rather than "rollback".

@redpanda I realise this wasn't added in your pull request, but can you address this comment as well?

redpanda · 2017-08-14T11:17:55Z

Should I implement the flag --update-delay.
This flag can be used with the --rolback flag docker service update command.
See: https://docs.docker.com/engine/reference/commandline/service_update/#rolling-back-to-the-previous-version-of-a-service

thaJeztah · 2017-08-14T11:57:55Z

@redpanda I think it's best to do so in a follow up. This pull request is already in docs review, and provides the core functionality needed; we can enhance the functionality in a follow up 👍

thaJeztah

Two minor comments, but LGTM after those are addressed 👍

thaJeztah · 2017-08-14T12:02:01Z

docs/reference/commandline/service_update.md

 ```

-### Rolling back to the previous version of a service 
+### Rolling back to the previous version of a service


@redpanda I realise this wasn't added in your pull request, but can you address this comment as well?

thaJeztah · 2017-08-14T12:07:44Z

docs/reference/commandline/service_rollback.md

+
+### Roll back to the previous version of a service
+
+This will revert the service to the configuration that was in place before the most recent docker service update command.


"This" is a bit unclear, because here it doesn't have the introduction sentence that is present in the section it's copied from.

How about;

Use the `docker service rollback` command to roll back to the previous version of a service. After executing this command, the service is reverted to the configuration that was in place before the most recent `docker service update` command.

mdlinville

Looking good. Just a couple more nitpicky changes please. :) 👼

mdlinville · 2017-08-15T18:48:53Z

docs/reference/commandline/service_rollback.md

+
+# service rollback
+
+```Markdown


This probably works just fine but the hint is usually markdown.

Good catch! I'm gonna fix it for all service_*.md files

mdlinville · 2017-08-15T18:49:26Z

docs/reference/commandline/service_rollback.md

@@ -0,0 +1,94 @@
+---
+title: "service rollback"


quotes not required in any of the front-matter

Should I replaced that line by: title: service rollback ?

I just read some information about the front-matter.
Should I changed it like that ?

--- title: service rollback description: The service rollback command description and usage keywords: ["service", "rollback"] ---

PS: I just keep the same front-matter used in other command line documentation but I can change it. ^^

It doesn't seem to hurt anything. We can probably do a bulk fix later on.

mdlinville · 2017-08-15T18:49:58Z

docs/reference/commandline/service_rollback.md

+```Markdown
+Usage:	docker service rollback SERVICE
+
+Rollback a service


Maybe "Revert changes to a service's configuration" so we are not using the word to describe the word. I realize this is in the --help output. Can you please change it there too?

mdlinville · 2017-08-15T18:50:51Z

docs/reference/commandline/service_rollback.md

+
+## Description
+
+Roll back a specified service to its previous version from the swarm. This command has to be run


s/has to be/must be

mdlinville · 2017-08-16T17:16:19Z

I feel like I confused you. Don't change the Markdown hint to markdown on files unrelated to the original intent of this commit. We can fix the other ones later.

redpanda · 2017-08-16T20:12:34Z

You right! I will revert these changes. ^^

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Implement runRollback to not use runUpdate Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Add version tag and add flag quiet to suppress progress output Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Removed flags from warnDetachDefault Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Used command.Cli interface Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Add detach flag on rollback command Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Create a fakeClient for service commands Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Added unit test for rollback command Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Used command.Cli interface instead of *command.DockerCli in service commands Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Revert "Removed flags from warnDetachDefault" This reverts commit 3e4f601. Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Fixed test.NewFakeCli instanciation Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Removed unused receiver Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Replaced cli by dockerCli Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Revert "Removed unused receiver" This reverts commit 604ef7c. Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com> Fixed last typo Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

mdlinville · 2017-08-17T17:34:55Z

Ping @thaJeztah

thaJeztah

LGTM, thanks!

[17.06] update changelog for 17.06.2-ce-rc1 release

GordonTheTurtle added the status/0-triage label Jun 19, 2017

dnephin added status/1-design-review and removed status/0-triage labels Jun 19, 2017

dnephin reviewed Jun 19, 2017

View reviewed changes

GordonTheTurtle added the dco/no label Jun 25, 2017

redpanda force-pushed the rollback branch from 79c4b2c to a9de770 Compare June 25, 2017 22:05

GordonTheTurtle removed the dco/no label Jun 25, 2017

dnephin reviewed Jun 29, 2017

View reviewed changes

redpanda force-pushed the rollback branch from a9de770 to 10a00bc Compare July 3, 2017 09:38

GordonTheTurtle added the dco/no label Jul 3, 2017

redpanda force-pushed the rollback branch from 5300f95 to ba205af Compare July 3, 2017 11:49

GordonTheTurtle removed the dco/no label Jul 3, 2017

vdemeester added status/2-code-review and removed status/1-design-review labels Jul 3, 2017

dnephin reviewed Jul 4, 2017

View reviewed changes

GordonTheTurtle added the dco/no label Jul 4, 2017

redpanda force-pushed the rollback branch from d5fd87c to 6a0e94c Compare July 4, 2017 18:18

GordonTheTurtle removed the dco/no label Jul 4, 2017

redpanda force-pushed the rollback branch 5 times, most recently from 5c6572d to 4440bc9 Compare July 4, 2017 20:22

dnephin reviewed Jul 5, 2017

View reviewed changes

redpanda force-pushed the rollback branch from d691059 to 0754df6 Compare July 17, 2017 17:56

docker deleted a comment from GordonTheTurtle Jul 18, 2017

thaJeztah added impact/changelog status/3-docs-review and removed status/2-code-review labels Aug 1, 2017

thaJeztah requested changes Aug 1, 2017

View reviewed changes

mdlinville suggested changes Aug 1, 2017

View reviewed changes

redpanda force-pushed the rollback branch 2 times, most recently from 326cb65 to 155835c Compare August 14, 2017 11:13

thaJeztah requested changes Aug 14, 2017

View reviewed changes

redpanda force-pushed the rollback branch 2 times, most recently from ac02d67 to 611c1d5 Compare August 15, 2017 09:35

mdlinville suggested changes Aug 15, 2017

View reviewed changes

redpanda force-pushed the rollback branch 2 times, most recently from 924fe65 to c14bc76 Compare August 16, 2017 10:12

redpanda force-pushed the rollback branch 3 times, most recently from ec7976e to 146baa4 Compare August 16, 2017 20:17

redpanda force-pushed the rollback branch from 146baa4 to 11d471d Compare August 16, 2017 20:18

mdlinville approved these changes Aug 17, 2017

View reviewed changes

thaJeztah approved these changes Aug 19, 2017

View reviewed changes

thaJeztah merged commit 3c7ede6 into docker:master Aug 19, 2017

GordonTheTurtle added this to the 17.08.0 milestone Aug 19, 2017

jphuynh mentioned this pull request Aug 20, 2017

Add zsh completion for service rollback #461

Merged

albers mentioned this pull request Aug 21, 2017

Add bash completion for service rollback #462

Merged

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#205 from andrewhsu/cl-rc1

e31c8d8

[17.06] update changelog for 17.06.2-ce-rc1 release


		### Roll back to the previous version of a service

		This will revert the service to the configuration that was in place before the most recent docker service update command.


		## Description

		Roll back a specified service to its previous version from the swarm. This command has to be run

Add 'docker service rollback' subcommand #205

Add 'docker service rollback' subcommand #205

Uh oh!

Co 8000 nversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment