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

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

Merged
merged 1 commit into from
Jun 29, 2021

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
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 <[email protected]>
@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