1283879 - ASan builds shouldn't try to download symbols (since they don't produce symbols zips)

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[(not currently active) Ted Mielczarek]

jmaher noticed some error spew in this log:
https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=69788#L586

05:27:14 WARNING - Can't figure out symbols_url from installer_url: https://queue.taskcluster.net/v1/task/U1qDiQjCQQ6xcvLlJo7GLA/artifacts/public/build/target.tar.bz2! 

(and then a Python traceback)

This is an ASan build, and those builds --disable-crashreporter so they don't build and upload a symbol package. They shouldn't try to download symbols at all.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So we should only check if download_symbols is equal to ondemand and True?

Comment 3

[Chris Cooper [:coop] (he/him)]

kmoir: this isn't really blocking ASan as tier-1 in TC since it's just a warning, but can I ask you to get this finished up?

Comment 4

[Kim Moir [:kmoir] ET]

sure, I'm looking at the mozconfigs and mozharness scripts now to see where this is set

Comment 5

[Kim Moir [:kmoir] ET]

Notes from Callek if dustin's stuff lands

0:52 AM <Callek> kmoir: regarding your asan symbols stuff, if its in TC, this should be relatively easy: https://reviewboard.mozilla.org/r/61884/diff/6#42 -- see line 60 in that file...  (yes this is predecated on dustins stuff landing finally)
10:53 AM <Callek> kmoir: you'd probably add it to https://reviewboard.mozilla.org/r/61884/diff/6#44 though
10:53 AM <kmoir> ok
10:54 AM <Callek> (easier than having to edit how ever many files and untangle the yaml inherits I mean :-)

Comment 6

[Kim Moir [:kmoir] ET]

The asan builds run the same tests as the regular linux ones which specify symbols-path=%(symbols_path)s in the mozharness configs.  I have a try run ready to go once the trees reopen to test a fix.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

From the log in comment 0 I can see that we specify "--download-symbols=ondemand" for ASAN builds. Why are we doing that if there are no symbols available anyway? 

On taskcluster I can see that we use the opt test task definition for such builds which exactly specify this setting:

https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/legacy/tasks/branches/base_jobs.yml#264
https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/legacy/tasks/tests/fx_linux64_crashtest_opt.yml#7

Shouldn't we have separate task definitions for ASAN tests?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

:ted, :gbrown - thoughts ^ ?

Comment 9

[(not currently active) Ted Mielczarek]

I don't really have any opinion on the specifics of how things are defined.

Comment 10

[Geoff Brown [:gbrown]]

When I was setting up the asan tests in taskcluster, I did not know about this issue. Buildbot  specifies "--download-symbols=ondemand" for asan and opt tests, so it looked to me like the tc asan tests could re-use the opt task definitions. If we need different options for asan, we need separate task definitions. There may be an easier way to do that once bug 1281004 lands.

Comment 11

[Kim Moir [:kmoir] ET]

I'm going to mark this as dependent on bug 1281004.  I tried a few try runs but couldn't get it working and since it will all be refactored soon, it makes sense to just do it once.

Comment 12

[Chris Cooper [:coop] (he/him)]

Bug 1281004 is resolved - can we wrap this up soon?

Comment 13

[Kim Moir [:kmoir] ET]

Yes, I am looking at this but I don't know how to solve it yet.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm fairly sure that Dustin can point us to the right direction. 

I had a quick look and the only place I could find where we customize tasks for ASAN tasks is:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests/desktop_test.py?q=file%3Adesktop_test.py&redirect_type=single#40

Maybe we have to also overwrite the mozharness settings for download_symbols?

Comment 15

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

Yes, I'd suggest adding another transform there which recognizes test jobs on asan platforms and changes the download-symbols property.

Comment 16

[Kim Moir [:kmoir] ET]

thanks dustin, I'll try that

Comment 17

[Kim Moir [:kmoir] ET]

I tried a bunch of try runs to try to fix this via transforms for the mh values

https://hg.mozilla.org/try/rev/563d1abc5e2b833384dde526979c9b7b19273ca0

but it still shows up in the payload

for instance search for 

desktop-test-linux64-asan/opt-jsreftest-e10s-2

in this task graph
https://public-artifacts.taskcluster.net/PeE7lGqYRhCXT0IHKA9CMg/0/public/full-task-graph.json

and you can see that  "--download-symbols=ondemand" is still in the command in the payload.  I'm trying now to change the payload via transforms but not sure if this is advisable.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe the transform here has a higher priority?

https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests/all_kinds.py#60

Wouldn't we be able to directly add the ASAN case there without the need for another transform?

Comment 19

[Kim Moir [:kmoir] ET]

Attachment: bug1283879.patch

Comment 20

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

The desktop_test.py transforms run before the all_kinds.py transforms:
  https://hg.mozilla.org/try/file/563d1abc5e2b/taskcluster/ci/desktop-test/kind.yml#l8

Kim's solution in comment 19 looks good.  It would be great to have the docstring updated as to why, and a comment pointing to this bug, for the next person (I realize there's no r? yet..)  Also, if this is generic to all asan builds then the filter should probably use some kind of pattern matching on the platform (platforms will get more structure in bug 1286086, and we can adapt the pattern at that point)

Comment 21

[Kim Moir [:kmoir] ET]

Attachment: bug1283879-2.patch

The last patch didn't work.  The value of download-symbols as false is not defined in mh and therefore invalid.  I'm trying this patch now.  If it works I will update patch to indicate bug # and why.  Looking at treeherder there are only asan builds on Linux64 and we only run tests on opt.

Comment 22

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

That looks good.  You could also change the schema so that False is allowed and means the option is omitted.

Comment 23

[Kim Moir [:kmoir] ET]

Attachment: bug1283879-3.patch

Comment 24

[Kim Moir [:kmoir] ET]

Attachment: bug1283879symbols.patch

I have this patch too since False is not defined as a valid option in mozharness for download-symbols

Comment 25

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

Comment on attachment 8779002 [details] [diff] [review]
bug1283879-3.patch

Review of attachment 8779002 [details] [diff] [review]:
-----------------------------------------------------------------

::: taskcluster/taskgraph/transforms/tests/all_kinds.py
@@ +57,5 @@
>  @transforms.add
>  def set_download_symbols(config, tests):
>      """In general, we download symbols immediately for debug builds, but only
> +    on demand for everything else. ASAN builds shouldn't download
> +    symbols since they don't product symbol zips see bug 1283879"""

*produce

@@ +62,4 @@
>      for test in tests:
>          if test['test-platform'].split('/')[-1] == 'debug':
>              test['mozharness']['download-symbols'] = True
> +        elif test['build-platform'] == 'linux64-asan/opt':

Will there never be any other ASAN build than linux64 opt?

Comment 26

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

Comment on attachment 8779003 [details] [diff] [review]
bug1283879symbols.patch

Review of attachment 8779003 [details] [diff] [review]:
-----------------------------------------------------------------

::: taskcluster/taskgraph/transforms/tests/test_description.py
@@ -144,5 @@
>          Required('no-read-buildbot-config', default=False): bool,
>  
>          # The setting for --download-symbols (if omitted, the option will not
>          # be passed to mozharness)
> -        Optional('download-symbols'): Any(True, False, 'ondemand'),

Looks good -- I see that the "False" case is already handled in make_task_description.py:

     download_symbols = {True: 'true', False: 'false'}.get(download_symbols, download_symbols)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3cd5974457
ASan builds shouldn't try to download symbols (since they don't produce symbols zips) r=dustin DONTBUILD

Comment 28

[Kim Moir [:kmoir] ET]

no, I'm not aware of any ASAN builds in the works, they will stay on Linux64.

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing "all_kinds.py:68:15 | multiple spaces after keyword (E271)" as requested by dustin:

https://hg.mozilla.org/integration/mozilla-inbound/rev/189b59814c4d34a93a731d2c2c97574aadd6e78e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33517073&repo=mozilla-inbound
[task 2016-08-08T21:09:16.074168Z] + ./mach lint -l flake8 -f treeherder
[task 2016-08-08T21:09:23.972966Z] TEST-UNEXPECTED-ERROR | taskcluster/taskgraph/transforms/tests/all_kinds.py:68:15 | multiple spaces after keyword (E271)

Comment 30

[Kim Moir [:kmoir] ET]

Attachment: bug1283879-4.patch

Comment 31

[Kim Moir [:kmoir] ET]

Comment on attachment 8779109 [details] [diff] [review]
bug1283879-4.patch

flake8 job passed this time with extra space removed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfaf24918cbcb4e2eb5cf870ee0ab40567ff4cd6

Comment 32

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

Comment on attachment 8779109 [details] [diff] [review]
bug1283879-4.patch

Review of attachment 8779109 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/210edbef152a
ASan builds shouldn't try to download symbols (since they don't produce symbols zips) r=dustin

Comment 34

[Kim Moir [:kmoir] ET]

fixed, verified on treeherder

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's not fixed yet, given that the code has not been landed on mozilla-central. Once that has been done it will be automatically marked as such.

Thank you for fixing it, Kim!

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/210edbef152a

Comment 37

[Armen [:armenzg]]

I think there was a typo on this.
Patch incoming.

Comment 38

[Armen [:armenzg]]

Nevermind me. This is probably right.