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 progress of replicated tasks before they are assigned #237

Merged
merged 1 commit into from Jun 27, 2017

Conversation

Projects
None yet
5 participants
@aaronlehmann
Collaborator

aaronlehmann commented Jun 24, 2017

This was only showing tasks that belong to nodes that are currently up,
so that tasks on down nodes don't appear to be stuck. But this
unintentionally excludes tasks that haven't been assigned yet, so if a
task is stuck before assignment, for example because no nodes meet its
constraints, a progress bar won't even be shown. The check should only
apply to tasks that have a node assignment.

Related to moby/moby#33807

cc @thaJeztah

progress: Show progress of replicated tasks before they are assigned
This was only showing tasks that belong to nodes that are currently up,
so that tasks on down nodes don't appear to be stuck. But this
unintentionally excludes tasks that haven't been assigned yet, so if a
task is stuck before assignment, for example because no nodes meet its
constraints, a progress bar won't even be shown. The check should only
apply to tasks that have a node assignment.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 24, 2017

Codecov Report

Merging #237 into master will decrease coverage by 1.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage    47.2%   45.67%   -1.53%     
==========================================
  Files         171      171              
  Lines       11556    11513      -43     
==========================================
- Hits         5455     5259     -196     
- Misses       5790     5947     +157     
+ Partials      311      307       -4

codecov-io commented Jun 24, 2017

Codecov Report

Merging #237 into master will decrease coverage by 1.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage    47.2%   45.67%   -1.53%     
==========================================
  Files         171      171              
  Lines       11556    11513      -43     
==========================================
- Hits         5455     5259     -196     
- Misses       5790     5947     +157     
+ Partials      311      307       -4
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 24, 2017

Member

Without this patch;

$ docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
0wx8h7rp50tqg0xnykwbxy4n6
overall progress: 0 out of 1 tasks
1/1:

With this patch applied;

./docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
ve5pnzf0swqtapeb0uqnla2cz
overall progress: 0 out of 1 tasks
1/1: pending   [================>                                  ]
Member

thaJeztah commented Jun 24, 2017

Without this patch;

$ docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
0wx8h7rp50tqg0xnykwbxy4n6
overall progress: 0 out of 1 tasks
1/1:

With this patch applied;

./docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
ve5pnzf0swqtapeb0uqnla2cz
overall progress: 0 out of 1 tasks
1/1: pending   [================>                                  ]
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 24, 2017

Member

After applying the label;

$ docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
ve5pnzf0swqtapeb0uqnla2cz
overall progress: 1 out of 1 tasks
1/1: running   [==================================================>]
verify: Waiting 1 seconds to verify that tasks are stable...

guess I found another issue (verify: Waiting 1 seconds to verify that tasks are stable... isn't remove after it finishes), let me open an issue for that 😄

Member

thaJeztah commented Jun 24, 2017

After applying the label;

$ docker service create --name donotdeploy --constraint "node.labels.somelabel==foobar" --detach=false nginx:alpine
ve5pnzf0swqtapeb0uqnla2cz
overall progress: 1 out of 1 tasks
1/1: running   [==================================================>]
verify: Waiting 1 seconds to verify that tasks are stable...

guess I found another issue (verify: Waiting 1 seconds to verify that tasks are stable... isn't remove after it finishes), let me open an issue for that 😄

@thaJeztah

LGTM

@thaJeztah thaJeztah added this to the 17.06.1 milestone Jun 24, 2017

@vdemeester

LGTM 🐯

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 27, 2017

Member

All green 👍

Member

thaJeztah commented Jun 27, 2017

All green 👍

@thaJeztah thaJeztah merged commit 2f58992 into docker:master Jun 27, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 8b6196d...d3d09f6
Details
codecov/project 45.67% (-1.53%) compared to 8b6196d
Details
dco-signed All commits are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment