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

service tasks: Improve error reporting #24917

Merged
merged 1 commit into from Jul 25, 2016

Conversation

Projects
None yet
9 participants
@aluzzardi
Copy link
Member

aluzzardi commented Jul 22, 2016

  • Tasks will display all tasks (-a is the default and was removed)
  • Nest tasks to help display history
  • Display task errors inline

Example:

# docker service tasks vote
ID                         NAME        IMAGE           NODE          DESIRED STATE  CURRENT STATE            ERROR
bvojmk7wiwjswyb0ww1o8by9e  vote.1      instavote/vote  ce5a79de6642  Running        Running 15 minutes ago
amwhb0xwateq040lqwzu0qado   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Complete 30 minutes ago
14hd2jrgta2378ndoya2ddgug   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Complete 45 minutes ago
5kb6oq07oydo6i8oz50wtt7ir   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Failed 45 minutes ago    "starting container failed: oc…"
613cgiw1ajc4bwxbwa5h033it   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Shutdown 52 minutes ago
3vgj9c3ab7fphoughj041r144   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Failed 54 minutes ago    "task: non-zero exit (137)"
612ik4x1sgkgwmoed9riwc3je   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Failed 56 minutes ago    "task: non-zero exit (137)"
00oz0yso8xcjveycxpobh8afm   \_ vote.1  instavote/vote  ce5a79de6642  Shutdown       Failed 56 minutes ago    "task: non-zero exit (137)"
0hung96bc208fazmg19kuze6z  vote.2      instavote/vote  ce5a79de6642  Running        Running 15 minutes ago
d53beglz6wgp9rx4gz91sjr7c   \_ vote.2  instavote/vote  ce5a79de6642  Shutdown       Complete 30 minutes ago
e0sfqy2zrxi06xjizhhfr405h   \_ vote.2  instavote/vote  ce5a79de6642  Shutdown       Complete 45 minutes ago
ap2xcl5x2op6txl95zepntqno   \_ vote.2  instavote/vote  ce5a79de6642  Shutdown       Failed 45 minutes ago    "starting container failed: oc…"
8oiok9dfowzf2urglzggpzz9a   \_ vote.2  instavote/vote  ce5a79de6642  Shutdown       Shutdown 52 minutes ago

/cc @vieux @icecrime @stevvooe @aaronlehmann @mgoelzer

service tasks: Improve error reporting
- Tasks will display all tasks (`-a` is the default and was removed)
- Nest tasks to help display history
- Display task errors inline

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
@aaronlehmann

This comment has been minimized.

Copy link
Contributor

aaronlehmann commented Jul 22, 2016

LGTM

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 22, 2016

wondering if this should be the default, or hidden behind a (e.g.) --history or --whatever flag. I can see this format being troublesome for people that want to parse it (in a script, grep, awk), or if we want to add a --format flag later on.

I like the idea though 👍

@tiborvass tiborvass added this to the 1.12.0 milestone Jul 22, 2016

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 22, 2016

@aluzzardi so if a service continually fails to converge, how many tasks will there be?

@aluzzardi

This comment has been minimized.

Copy link
Member

aluzzardi commented Jul 22, 2016

@tiborvass Right now the "history" is capped at 15 (it's a SwarmKit flag - docker swarm update --task-history=N AFAIK).

I'm making a case with @aaronlehmann to move it 5 by default.

@thaJeztah: I think we should display it by default. I've seen lots of confusion about grasping the model and troubleshooting a Swarm. Showing all tasks makes users understand the model (not many people were aware of -a and even less knew what it actually meant).

Example: Run docker service create bad_image and then tasks would always show a single task in PREPARING state which looks like it's stuck. In reality, the task goes from PREPARING to REJECTED (bad image), it gets hidden by tasks (because of no -a) and then a replacement is created and displayed as PREPARING.

With this PR, tasks would show an increasing list of stuff (up to 15 - maybe 5) with a REJECTED state and an error column complaining about bad_imagewhich is a much better UX.

Regarding scripts - you can do what "no -a" was doing by using a filter (I believe -f desired-state=running?)

@vdemeester

This comment has been minimized.

Copy link
Member

vdemeester commented Jul 22, 2016

I like the it and do agree with @aluzzardi 👼
Design LGTM for me

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 22, 2016

@aluzzardi what happens if i do --replicas 100 i still get only the last 5 that were scheduled?

@aluzzardi

This comment has been minimized.

Copy link
Member

aluzzardi commented Jul 22, 2016

@tiborvass it's 5 per replica, so 500 total

@stevvooe

This comment has been minimized.

Copy link
Contributor

stevvooe commented Jul 22, 2016

LGTM with docker service ps PR.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 22, 2016

@aluzzardi so i would see all 500 in service tasks, correct?

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

aaronlehmann commented Jul 22, 2016

There would only be 500 if each replica had been restarted at least 5 times. But yes, in that case, you would see a total of 500.

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Jul 22, 2016

I like this idea LGTM

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 23, 2016

+1 , moving to code review

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 23, 2016

@aluzzardi can you change this so that it only returns the last 5 tasks per replica?

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 23, 2016

Otherwise this looks good

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 25, 2016

LGTM

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 25, 2016

LGTM we can change default number of tasks in other PR.

@tiborvass tiborvass merged commit 087ca1b into moby:master Jul 25, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 21503 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 8150 has succeeded
Details
janky Jenkins build Docker-PRs 30177 has succeeded
Details
userns Jenkins build Docker-PRs-userns 12233 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 28746 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 738 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment