X Tutup
Skip to content

UX: don't reverse progress-bars when rolling back#2940

Merged
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:rollback_progress_bars
Jun 29, 2021
Merged

UX: don't reverse progress-bars when rolling back#2940
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:rollback_progress_bars

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 19, 2021

Commit https://github.com/moby/moby//commit/330a0035334871d92207b583c1c36d52a244753f (moby/moby#31144) introduced "synchronous" service update and rollback, using progress bars to show current status for each task.

As part of that change, progress bars were "reversed" when doing a rollback, to indicate that status was rolled back to a previous state.

Reversing direction is somewhat confusing, as progress bars now return to their "initial" state to indicate it was "completed"; for an "automatic" rollback, this may be somewhat clear (progress bars "move to the right", then "roll back" if the update failed), but when doing a manual rollback, it feels counter-intuitive (rolling back is the expected outcome).

This patch removes the code to reverse the direction of progress-bars, and makes progress-bars always move from left ("start") to right ("finished").

Before this patch

  1. create a service with automatic rollback on failure

     $ docker service create --update-failure-action=rollback --name foo --tty --replicas=5 nginx:alpine
     9xi1w3mv5sqtyexsuh78qg0cb
     overall progress: 5 out of 5 tasks
     1/5: running   [==================================================>]
     2/5: running   [==================================================>]
     3/5: running   [==================================================>]
     4/5: running   [==================================================>]
     5/5: running   [==================================================>]
     verify: Waiting 2 seconds to verify that tasks are stable...
    
  2. update the service, making it fail after 3 seconds

     $ docker service update --entrypoint="/bin/sh -c 'sleep 3; exit 1'" foo
     overall progress: rolling back update: 2 out of 5 tasks
     1/5: running   [==================================================>]
     2/5: running   [==================================================>]
     3/5: starting  [============================================>      ]
     4/5: running   [==================================================>]
     5/5: running   [==================================================>]
    
  3. Once the service starts failing, automatic rollback is started; progress-bars now move in the reverse direction;

     overall progress: rolling back update: 3 out of 5 tasks
     1/5: ready     [===========>                                       ]
     2/5: ready     [===========>                                       ]
     3/5: running   [>                                                  ]
     4/5: running   [>                                                  ]
     5/5: running   [>                                                  ]
    
  4. When the rollback is completed, the progressbars are at the "start" to indicate they completed;

     overall progress: rolling back update: 5 out of 5 tasks
     1/5: running   [>                                                  ]
     2/5: running   [>                                                  ]
     3/5: running   [>                                                  ]
     4/5: running   [>                                                  ]
     5/5: running   [>                                                  ]
     rollback: update rolled back due to failure or early termination of task bndiu8a998agr8s6sjlg9tnrw
     verify: Service converged
    

After this patch

Progress bars always go from left to right; also in a rollback situation;

After updating to the "faulty" entrypoint, task are deployed:

    $ docker service update --entrypoint="/bin/sh -c 'sleep 3; exit 1'" foo
    foo
    overall progress: 1 out of 5 tasks
    1/5:
    2/5: running   [==================================================>]
    3/5: ready     [======================================>            ]
    4/5:
    5/5:

Once tasks start failing, rollback is started, and presented the same as a regular
update; progress bars go from left to right;

    overall progress: rolling back update: 3 out of 5 tasks
    1/5: ready     [======================================>            ]
    2/5: starting  [============================================>      ]
    3/5: running   [==================================================>]
    4/5: running   [==================================================>]
    5/5: running   [==================================================>]
    rollback: update rolled back due to failure or early termination of task c11dxd7ud3d5pq8g45qkb4rjx

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

@silvin-lubecki
Copy link
Contributor

@thaJeztah linter is failing:

cli/command/service/progress/progress.go:381:105: `(*replicatedProgressUpdater).writeTaskProgress` - `rollback` is unused (unparam)
func (u *replicatedProgressUpdater) writeTaskProgress(task swarm.Task, mappedSlot int, replicas uint64, rollback bool) {

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

linter is failing:

Ah, dang. I separated this from the other PR, probably forgot to remove some things; I'll have a look

@thaJeztah thaJeztah force-pushed the rollback_progress_bars branch from 11d9491 to 8039b6d Compare January 19, 2021 14:40
@thaJeztah
Copy link
Member Author

Think it should be fixed now

@codecov-io
Copy link

Codecov Report

Merging #2940 (8039b6d) into master (1e54c5d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
- Coverage   57.11%   57.11%   -0.01%     
==========================================
  Files         299      299              
  Lines       18666    18663       -3     
==========================================
- Hits        10661    10659       -2     
+ Misses       7145     7144       -1     
  Partials      860      860              

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

@tiborvass @cpuguy83 ptal

@thaJeztah thaJeztah added this to the 21.xx milestone Jun 22, 2021
Commit https://github.com/moby/moby//commit/330a0035334871d92207b583c1c36d52a244753f
introduced "synchronous" service update and rollback, using progress bars to show
current status for each task.

As part of that change, progress bars were "reversed" when doing a rollback, to
indicate that status was rolled back to a previous state.

Reversing direction is somewhat confusing, as progress bars now return to their
"initial" state to indicate it was "completed"; for an "automatic" rollback, this
may be somewhat clear (progress bars "move to the right", then "roll back" if the
update failed), but when doing a manual rollback, it feels counter-intuitive
(rolling back is the _expected_ outcome).

This patch removes the code to reverse the direction of progress-bars, and makes
progress-bars always move from left ("start") to right ("finished").

Before this patch
----------------------------------------

1. create a service with automatic rollback on failure

    $ docker service create --update-failure-action=rollback --name foo --tty --replicas=5 nginx:alpine
    9xi1w3mv5sqtyexsuh78qg0cb
    overall progress: 5 out of 5 tasks
    1/5: running   [==================================================>]
    2/5: running   [==================================================>]
    3/5: running   [==================================================>]
    4/5: running   [==================================================>]
    5/5: running   [==================================================>]
    verify: Waiting 2 seconds to verify that tasks are stable...

2. update the service, making it fail after 3 seconds

    $ docker service update --entrypoint="/bin/sh -c 'sleep 3; exit 1'" foo
    overall progress: rolling back update: 2 out of 5 tasks
    1/5: running   [==================================================>]
    2/5: running   [==================================================>]
    3/5: starting  [============================================>      ]
    4/5: running   [==================================================>]
    5/5: running   [==================================================>]

3. Once the service starts failing, automatic rollback is started; progress-bars now move in the reverse direction;

    overall progress: rolling back update: 3 out of 5 tasks
    1/5: ready     [===========>                                       ]
    2/5: ready     [===========>                                       ]
    3/5: running   [>                                                  ]
    4/5: running   [>                                                  ]
    5/5: running   [>                                                  ]

4. When the rollback is completed, the progressbars are at the "start" to indicate they completed;

    overall progress: rolling back update: 5 out of 5 tasks
    1/5: running   [>                                                  ]
    2/5: running   [>                                                  ]
    3/5: running   [>                                                  ]
    4/5: running   [>                                                  ]
    5/5: running   [>                                                  ]
    rollback: update rolled back due to failure or early termination of task bndiu8a998agr8s6sjlg9tnrw
    verify: Service converged

After this patch
----------------------------------------

Progress bars always go from left to right; also in a rollback situation;

After updating to the "faulty" entrypoint, task are deployed:

    $ docker service update --entrypoint="/bin/sh -c 'sleep 3; exit 1'" foo
    foo
    overall progress: 1 out of 5 tasks
    1/5:
    2/5: running   [==================================================>]
    3/5: ready     [======================================>            ]
    4/5:
    5/5:

Once tasks start failing, rollback is started, and presented the same as a regular
update; progress bars go from left to right;

    overall progress: rolling back update: 3 out of 5 tasks
    1/5: ready     [======================================>            ]
    2/5: starting  [============================================>      ]
    3/5: running   [==================================================>]
    4/5: running   [==================================================>]
    5/5: running   [==================================================>]
    rollback: update rolled back due to failure or early termination of task c11dxd7ud3d5pq8g45qkb4rjx

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the rollback_progress_bars branch from 8039b6d to 678c2fd Compare June 22, 2021 08:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2021

Codecov Report

Merging #2940 (678c2fd) into master (12e8782) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
- Coverage   57.09%   57.09%   -0.01%     
==========================================
  Files         299      299              
  Lines       18731    18728       -3     
==========================================
- Hits        10695    10693       -2     
+ Misses       7166     7165       -1     
  Partials      870      870              

@thaJeztah
Copy link
Member Author

Rebased to get a fresh run of CI

@thaJeztah thaJeztah merged commit 4a6fe51 into docker:master Jun 29, 2021
@thaJeztah thaJeztah deleted the rollback_progress_bars branch June 29, 2021 14:02
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.

5 participants

X Tutup