1275774 - missing test failure emails from try

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0fa70104f55
dispatched failure emails for many of the failures, including the Linux(32) test failures, but none of the linux64 test failures.

Comment 1

[Geoff Brown [:gbrown]]

I had the same experience in https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e35bb7f807: I received failure emails for the 2 buildbot failures, but none for the 4 taskcluster failures (same failure, same test).

--

[e-mail]
	
5:37 PM (17 hours ago)
		
to me
Your try Server test (46e35bb7f807) completed with warnings on builder try_ubuntu64_vm_test-mochitest-1.

Summary of test results:

The full log for this test run is available at http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-46e35bb7f807b2b0f908f6d654f7874e82f6a3c5/try-linux64/try_ubuntu64_vm_test-mochitest-1-bm122-tests1-linux64-build313.txt.gz.

For an overview of all results see Treeherder.
[e-mail]
	
6:05 PM (16 hours ago)
		
to me
Your try Server test (46e35bb7f807) completed with warnings on builder try_ubuntu64_vm_test-mochitest-1.

Summary of test results:

The full log for this test run is available at http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-46e35bb7f807b2b0f908f6d654f7874e82f6a3c5/try-linux64/try_ubuntu64_vm_test-mochitest-1-bm122-tests1-linux64-build314.txt.gz.

For an overview of all results see Treeherder.

Comment 2

[Armen [:armenzg]]

Do you get an email per failure on a try push?
Is the current experience for Buildbot good enough?

Comment 3

[Geoff Brown [:gbrown]]

I actually haven't used failure emails much, historically. But in my limited experience, yes, one email is sent for each buildbot test job failure on a try push, and yes, I think that's good enough.

I found https://dxr.mozilla.org/build-central/source/buildbotcustom/bin/try_mailer.py for buildbot support.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #2)
> Do you get an email per failure on a try push?

Yes, when it works, which I assume is when buildbot is used.

> Is the current experience for Buildbot good enough?

Yes.

Comment 5

[Armen [:armenzg]]

This feature activates when someone uses -f (for email failures) in their try syntax:
> try: -b o -f -p linux64 -u reftest-1 -t none

This feature was used 78 times in the last 2 months (May 8th). That's about 8-9 times a week:
https://hg.mozilla.org/try/pushloghtml?startdate=2+months+ago&enddate=now

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Brian, maybe this is something we can add to the decision task now that we have tc-notify?

Comment 7

[Brian Stack [:bstack]]

So long as we know who to email, then it's just a matter of adding a route that is 'notify.email.<email>.on-failed' and 'notify.email.<email>.on-exception' to every task we want to know the status of (which sounds like all of them?).

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Yeah, sounds perfect!

I think that could best be implemented within the task description transforms in taskcluster/taskgraph/transforms/task.py

Comment 9

[Brian Stack [:bstack]]

Attachment: Bug 1275774 - Fix taskcluster how-to docs example command

Review commit: https://reviewboard.mozilla.org/r/80592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80592/

Comment 10

[Brian Stack [:bstack]]

Attachment: Bug 1275774 - Add try-syntax parsing of email notifications to taskcluster

Review commit: https://reviewboard.mozilla.org/r/80594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80594/

Comment 11

[Brian Stack [:bstack]]

Attachment: Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

Review commit: https://reviewboard.mozilla.org/r/80596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80596/

Comment 12

[Brian Stack [:bstack]]

Attachment: Bug 1275774 - Improve notification finding logic

Review commit: https://reviewboard.mozilla.org/r/80598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80598/

Comment 13

[Brian Stack [:bstack]]

Comment on attachment 8794908 [details]
Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80596/diff/1-2/

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8794906 [details]
Bug 1275774 - Fix taskcluster how-to docs example command

https://reviewboard.mozilla.org/r/80592/#review79924

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8794908 [details]
Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

https://reviewboard.mozilla.org/r/80596/#review79932

Just some minor fixes!

::: taskcluster/taskgraph/target_tasks.py:63
(Diff revision 2)
>                  task.attributes['task_duplicates'] = options.trigger_tests
>  
> +    # Add notifications here as well
> +    if options.notifications:
> +        for task in full_task_graph:
> +            owner = task.task.get('tags', {}).get('createdForUser')

I'd prefer to get this from the source (`parameters['owner']`) rather than depend on this tag.  TBH I have no idea what tags are or how they are used, and I suspect only jlal really does, so that tag may disappear someday.

::: taskcluster/taskgraph/target_tasks.py:65
(Diff revision 2)
> +    # Add notifications here as well
> +    if options.notifications:
> +        for task in full_task_graph:
> +            owner = task.task.get('tags', {}).get('createdForUser')
> +            routes = task.task.get('routes')
> +            if not (owner or routes):

If `routes` is `[]`, we still want to add the routes, I think.  Although I can hardly see a case where this would occur, since we have index and TH routes.  Perhaps the line before should read `routes = task.task.setdefault('routes', [])` and `routes` should be removed from this conditional.

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8794907 [details]
Bug 1275774 - Add try-syntax parsing of email notifications to taskcluster

https://reviewboard.mozilla.org/r/80594/#review79938

Comment 17

[Brian Stack [:bstack]]

Comment on attachment 8794908 [details]
Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

https://reviewboard.mozilla.org/r/80596/#review79932

> I'd prefer to get this from the source (`parameters['owner']`) rather than depend on this tag.  TBH I have no idea what tags are or how they are used, and I suspect only jlal really does, so that tag may disappear someday.

I initially grabbed the owner from that field as well, but that resulted in some of them having an owner of aki iirc. Specifically the signer builds or whatever. If that's desired functionality that aki gets emails on those tasks finishing, then that makes sense, but using this field does appear to do The Right Thing™. Is it possible that the bug here is with aki's task generation stuff? Does it really want him to be the owner of the task every time no matter whether he runs it or not?

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

I suspect you saw jlund, from the android build-signing task.  Yes, that's a bug in the task.  However, I'm suggesting using *parameters*, which contains the inputs to the decision task (including the pushing user), rather than task['metadata']['owner'] (which, for all tasks except the build-signing tasks, is copied from parameters['owner'])

Comment 19

[Brian Stack [:bstack]]

Attachment: Bug 1275774 - Add tests for new taskcluster try flag parsing

Review commit: https://reviewboard.mozilla.org/r/81710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81710/

Comment 20

[Brian Stack [:bstack]]

Comment on attachment 8794908 [details]
Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80596/diff/2-3/

Comment 21

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8794908 [details]
Bug 1275774 - Add notify routes to taskcluster tasks for certain try flags

https://reviewboard.mozilla.org/r/80596/#review80340

Comment 22

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8795768 [details]
Bug 1275774 - Add tests for new taskcluster try flag parsing

https://reviewboard.mozilla.org/r/81710/#review80342

yay, tests!

Comment 23

[Brian Stack [:bstack]]

Great. Now that there are a bunch of little green boxes in mozreview. The next thing we need to figure out is assigning scopes to allow this to be used. 

https://github.com/taskcluster/mozilla-taskcluster/pull/101 is how I think we can do this for try. If that is the correct way of doing it then we can land that and then land this and we're set I believe. 

For moz-tree:level:2 and moz-tree:level:3, I figure we can give `route.notify.*` to those roles. Does that make sense?

Comment 24

[Dustin J. Mitchell [:dustin] (he/him)]

Try doesn't run on levels 2 or 3, so this patch won't ever result in task graphs at those levels sending notifications.  That said, I think adding those scopes in case someone wants to use them someday (via some landed patch in the taskgraph generation) is a good idea.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d596d23a7efe
Fix taskcluster how-to docs example command r=dustin
https://hg.mozilla.org/integration/autoland/rev/5202c2328850
Add try-syntax parsing of email notifications to taskcluster r=dustin
https://hg.mozilla.org/integration/autoland/rev/229984f2cc93
Add notify routes to taskcluster tasks for certain try flags r=dustin
https://hg.mozilla.org/integration/autoland/rev/afffe391d026
Add tests for new taskcluster try flag parsing r=dustin

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d596d23a7efe
https://hg.mozilla.org/mozilla-central/rev/5202c2328850
https://hg.mozilla.org/mozilla-central/rev/229984f2cc93
https://hg.mozilla.org/mozilla-central/rev/afffe391d026