-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
progress: Show task error in place of progress bar #259
progress: Show task error in place of progress bar #259
Conversation
If a task encounters an error, the interactive "service create" and "service update" commands should show that error instead of showing a stuck progress bar. To validate: docker service create --detach=false --name broken --restart-condition=none --replicas 3 busybox asdf and docker service create --detach=false --name broken --mode global --restart-condition none busybox asdf Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
9abe8ad
to
1ef585f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we test these changes?
Full integration tests of the progress bars is going to be difficult, but we should be able to unit test a bunch of the internal logic.
running++ | ||
} | ||
|
||
if u.done || replicas > maxProgressBars || uint64(mappedSlot) > replicas { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the evaluation of replicas > maxProgressBars
will never change inside this for loop (one is a constant, the other is set one above). Is that right?
I guess the purpose of this check is to not display progress bars if there are too many replicas? Couldn't this be handled at the top of the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the purpose of this check is to not display progress bars if there are too many replicas? Couldn't this be handled at the top of the function?
We also have to count the number of running tasks, which this loop does as well.
if !u.done && replicas <= maxProgressBars && uint64(mappedSlot) <= replicas { | ||
if !terminalState(task.DesiredState) && task.Status.State == swarm.TaskStateRunning { | ||
running++ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the body of this for loop be a separate method which returns the running count?
I think that would improve the readability of this function, and would make it easier to test this new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the body of this for loop be a separate method which returns the running count?
Wouldn't it have the side effect of outputting progress information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would, but the current function has that side effect plus a bunch of other logic, so it's not any worse off, right? It actually moves almost all of the progress printing to that new function. With a couple other smaller changes we could probably remove all of the progress printing from update()
running++ | ||
} | ||
|
||
if u.done || nodeCount > maxProgressBars { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question about nodeCount > maxProgressBars
not changing value within this for loop.
And also splitting this out into a new function (which should remove the need for nolint, and help testing).
I am going to work on some tests for the internal logic. Any preferences on adding them to this PR or a separate one? |
My preference would be this PR |
bad435d
to
ce2aa0d
Compare
Added unit tests and did some light refactoring. |
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
4519d06
to
04c6299
Compare
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
=========================================
- Coverage 48.68% 46.9% -1.79%
=========================================
Files 185 173 -12
Lines 12168 11942 -226
=========================================
- Hits 5924 5601 -323
- Misses 5877 6019 +142
+ Partials 367 322 -45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the refactor and unit tests, looks great. One nit, but it's not a blocker
if task.NodeID != "" { | ||
if _, nodeActive := activeNodes[task.NodeID]; nodeActive { | ||
tasksBySlot[task.Slot] = task | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If task.NodeID
is not empty but the node is not active then we skip the task.
Minor nit, but this case seems like it would be more obvious like this:
if _, nodeActive := activeNodes[task.NodeID]; !nodeActive {
continue
}
tasksBySlot[task.Slot] = task
That way it matches the checks above and there is only one line which assigns to tasksBySlot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
04c6299
to
c9b92a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
If a task encounters an error, the interactive "service create" and "service update" commands should show that error instead of showing a stuck progress bar.
To validate:
and
Related to moby/moby#33807
Also related: moby/swarmkit#2287
cc @thaJeztah