(not currently active) Ted Mielczarek Reporter
	Description • 9 years ago

Currently our Firefox builders run "make uploadsymbols" as part of the build and POST the symbols zip to the Socorro API. To do so they need to have a Socorro authentication token available on the builder, which is suboptimal.

catlee suggested we should be having the builders put the full symbols.zip up as an artifact, and have a separate task do the uploading, so that only the workers for that task need to have the credentials available. This would help us solve bug 1138617, where try pushes can contain arbitrary code that could abuse the credentials.

	(not currently active) Ted Mielczarek Reporter
	Comment 1 • 9 years ago

wcosta: jlal suggested that you were already looking at this?

	Wander Lairson Costa
	Comment 2 • 9 years ago

We talked about this in IRC, I am not working on this, but can take it, np.

	Chris Cooper [:coop] (he/him)
	Comment 3 • 9 years ago

Anthony (releng intern) is going to investigate this.

	(not currently active) Ted Mielczarek Reporter
	Comment 4 • 9 years ago

The existing build jobs call `make buildsymbols` which lives here:
https://dxr.mozilla.org/mozilla-central/source/Makefile.in#254

That essentially just passes the path to the symbols-full.zip file to this Python script:
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/upload_symbols.py

The actual upload itself is pretty trivial, it just does a HTTP POST. Most of that script is retry/error printing logic.

For testing you can use Socorro's staging instance:
https://crash-stats.mocotoolsstaging.net/symbols/upload

The database gets synced from production regularly, so any auth tokens valid for production should work for staging as well.

If you have any questions about the build system side of things I can help you out. If you have questions about the crash-stats API side of things, peterbe knows the most about that.

	(not currently active) Ted Mielczarek Reporter
	Comment 5 • 9 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> The existing build jobs call `make buildsymbols` which lives here:

That's `make uploadsymbols`, but the link was correct at least. :)

	(not currently active) Ted Mielczarek Reporter
	Comment 6 • 9 years ago

We also might want to build this with the possibility of using a different target bucket (not currently supported by the upload API), which would be a prerequisite for bug 1138617. We would want to be able to upload symbols from Try builds, but send them to a separate s3 bucket.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 7 • 9 years ago

This patch allows us to run upload_symbols.py as a standalone script. Otherwise, setting up the dependencies for mozconfig for a single environmental variable on a new machine is relatively difficult.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 8 • 9 years ago

This patch adds a `[TC] Upload Symbols` task as a dependency in the build task-graph. This will run a docker image built using https://github.com/acmiyaguchi/upload-symbols

Example task graph can be found here:
https://tools.taskcluster.net/task-graph-inspector/#qopZ8a8HRYGF5R47DygOvw/TDmhsPruQmywFdh40MhZxw/

	Morgan Phillips [:mrrrgn]
	Comment 9 • 9 years ago

Comment on attachment 8630601 [details] [diff] [review]
upload_symbol_dependency.diff

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

This looks really nice to me. I think post-build is a fair general heading. If we wanted to do something after tests, we could also make a post-test imo.

	Morgan Phillips [:mrrrgn]
	Comment 10 • 9 years ago

Greg, I flagged you for feedback to see what you think of this "post-build" heading.

	Greg Arndt [:garndt]
	Comment 11 • 9 years ago

There were some talks at Whistler as to how to handle task dependencies in tree.  I think for now, something like post-build and post-test are a good stepping stone to what we should want to strive for.

Currently there is a common flow that's followed for the intree things:
build -> post-build -> test -> post-test

But that gets more complicated when there could be dependencies that might not flow the same way.  Anyways, I think this is good so far.

post-build is good for things like symbols upload, and we should have post-test for things like balrog updates for a particular build that we only want uploaded if both the build and tests are successful. 

I would imagine post-build tasks also would want to be specific for a particular build (similar to how we have "allowed_build_tasks" for tests.

Also these tasks should probably be in directories similar to how we have tasks/builds tasks/tests

	Greg Arndt [:garndt]
	Comment 12 • 9 years ago

Also, instead of using the artifact task Id in the upload symbols task, maybe we could use the extra.location field like we do for images and build urls.  This way it's very explicit what artifact is being uploaded for that task.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 13 • 9 years ago

(In reply to Greg Arndt [:garndt] from comment #11)
> I would imagine post-build tasks also would want to be specific for a
> particular build (similar to how we have "allowed_build_tasks" for tests.

Hrnn, I think its all the nightly b2g builds that would want to consider having their symbols uploaded.
Here is the b2g_build.py for reference: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/b2g_build.py#597
Do you think it would be acceptable to determine whether or not the tasks should be run based on matching against `tasks/tests/bg2_*` instead of an explicit white-list?

	Greg Arndt [:garndt]
	Comment 14 • 9 years ago

(In reply to Anthony Miyaguchi [:amiyaguchi] from comment #13)
> (In reply to Greg Arndt [:garndt] from comment #11)
> > I would imagine post-build tasks also would want to be specific for a
> > particular build (similar to how we have "allowed_build_tasks" for tests.
> 
> Hrnn, I think its all the nightly b2g builds that would want to consider
> having their symbols uploaded.
> Here is the b2g_build.py for reference:
> https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/
> b2g_build.py#597
> Do you think it would be acceptable to determine whether or not the tasks
> should be run based on matching against `tasks/tests/bg2_*` instead of an
> explicit white-list?

We could match based on something like that as well.  The methods for building out the list of jobs would probably just need slight modification along with the tests.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 15 • 9 years ago

Bug 1168979 - Add fallback for buildconfig in standalone use of upload_symbols script; r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 16 • 9 years ago

Bug 1168979 - Add symbol uploading as a build dependency; try: -b o -p linux64 -u none -t none

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 17 • 9 years ago

Bug 1168979 - Clean up upload_symbol template.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 18 • 9 years ago

Bug 1168979 - Factor out building dependent tasks as a function; Create a task/post-builds for upload_symbols to live in.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 19 • 9 years ago

Bug 1168979 - Add allowed_build_tasks to distinguish builds that require symbol uploading.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 20 • 9 years ago

Bug 1168979 - Bump version of upload_symbols container, with addition of new arguments.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 21 • 9 years ago

Since the last review, I've added an `allowed_build_tasks` list to white-list certain builds that are require uploading to the crash-stats site. Unfortunately, there is no distinguishing between nightly builds or otherwise, due to the early progress of porting builds over to TaskCluster. I've enabled symbol uploading for both linux64 [debug/opt] and android-api-11 for the meanwhile, to demonstrate proof of concept. Until there are more builds available, this is the best that can be done for the moment.

Work on the docker container can be found https://github.com/acmiyaguchi/upload-symbols. This contains the scripts to do the actual uploading to the crash-stats api. The corresponding image can be found https://quay.io/repository/amiyaguchi/upload_symbols. 

You can run `./mach taskcluster-graph --project='try' --owner='amiyaguchi@mozill.com' --message='try: -b d -p linux64 -u none -t none' --extend-graph` locally to view a sample taskgraph json. An example of a successful run can be found at https://tools.taskcluster.net/task-graph-inspector/#bJLg48_3S7ODSydbNzO-mA/X6pDvwizQSm8V_ezl8h8Nw/. A `[TC] Upload Symbols` task is successfully spawned as a dependent task of the parent build. 

For future reference, symbol uploading will be done if MOZ_AUTOMATION_UPLOAD_SYMBOLS is defined in the mozconfig, and is currently done in the macosx, linux, win, and android nightly builds. I didn't find the variable set for any of the b2g builds.

I'm also figuring out how to use mozreview, I figure it's easier to post patches this way. Hopefully those six commits don't look like spam.

	(not currently active) Ted Mielczarek Reporter
	Comment 22 • 9 years ago

https://reviewboard.mozilla.org/r/12969/#review11555

::: testing/taskcluster/tasks/branches/try/job_flags.yml:139
(Diff revision 1)
> +      - tasks/builds/android_api_11.yml

I'd like to see a follow-up filed for fixing this in a nicer way. We currently use MOZ_AUTOMATION_UPLOAD_SYMBOLS in the mozconfigs to determine this, although I'm failing to find where we set it. (I suspect it's in mozharness configs somewhere.)
https://dxr.mozilla.org/mozilla-central/source/build/mozconfig.automation#19

	(not currently active) Ted Mielczarek Reporter
	Comment 23 • 9 years ago

Oh, you mentioned MOZ_AUTOMATION_UPLOAD_SYMBOLS already. It would be great if the post-build task could be triggered by a task having something in its definition instead of having a whitelist, though.

	(not currently active) Ted Mielczarek Reporter
	Comment 24 • 9 years ago

https://reviewboard.mozilla.org/r/12963/#review11557

::: testing/taskcluster/tasks/upload_symbols.yml:29
(Diff revision 1)
> +      'public/build':

I don't think you actually need this, unless there's some other code expecting it. This task shouldn't product any artifacts.

	(not currently active) Ted Mielczarek Reporter
	Comment 25 • 9 years ago

https://reviewboard.mozilla.org/r/12961/#review11559

::: toolkit/crashreporter/tools/upload_symbols.py:25
(Diff revision 1)
> +    from os import environ as substs

A comment here explaining that this is for running in a standalone Taskcluster task would be useful.

::: toolkit/crashreporter/tools/upload_symbols.py:27
(Diff revision 1)
>  url = 'https://crash-stats.mozilla.com/symbols/upload'

While you're here, you should probably allow the upload URL to be overridden from the environment so you can easily point it at the staging server for testing.

	Morgan Phillips [:mrrrgn]
	Comment 26 • 9 years ago

Comment on attachment 8631911 [details]
MozReview Request: Bug 1168979 part 1 - Add fallback for buildconfig in standalone use of upload_symbols script; r=mrrrgn

http://media.giphy.com/media/ToMjGpvXu27Ebj3NHlS/giphy.gif

	Morgan Phillips [:mrrrgn]
	Comment 27 • 9 years ago

Comment on attachment 8631916 [details]
MozReview Request: Bug 1168979 part 6 - Bump version of upload_symbols container, with addition of new arguments; r=mrrrgn

http://www.recaption.com/uploads/181125060ac4dcf7a3.jpg

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 28 • 9 years ago

Comment on attachment 8631911 [details]
MozReview Request: Bug 1168979 part 1 - Add fallback for buildconfig in standalone use of upload_symbols script; r=mrrrgn

Bug 1168979 part 1 - Add fallback for buildconfig in standalone use of upload_symbols script; r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 29 • 9 years ago

Comment on attachment 8631912 [details]
MozReview Request: Bug 1168979 part 2 - Add symbol uploading as a build dependency; r=mrrrgn

Bug 1168979 part 2 - Add symbol uploading as a build dependency; r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 30 • 9 years ago

Comment on attachment 8631913 [details]
MozReview Request: Bug 1168979 part 3 - Clean up upload_symbol template. r=mrrrgn

Bug 1168979 part 3 - Clean up upload_symbol template. r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 31 • 9 years ago

Comment on attachment 8631914 [details]
MozReview Request: Bug 1168979 part 4 - Factor out building dependent tasks as a function; Create a task/post-builds for upload_symbols to live in; r=mrrrgn

Bug 1168979 part 4 - Factor out building dependent tasks as a function; Create a task/post-builds for upload_symbols to live in; r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 32 • 9 years ago

Comment on attachment 8631915 [details]
MozReview Request: Bug 1168979 part 5 - Add allowed_build_tasks to distinguish builds that require symbol uploading; r=mrrrgn

Bug 1168979 part 5 - Add allowed_build_tasks to distinguish builds that require symbol uploading; r=mrrrgn

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 33 • 9 years ago

Comment on attachment 8631916 [details]
MozReview Request: Bug 1168979 part 6 - Bump version of upload_symbols container, with addition of new arguments; r=mrrrgn

Bug 1168979 part 6 - Bump version of upload_symbols container, with addition of new arguments; r=mrrrgn

	Morgan Phillips [:mrrrgn]
	Comment 34 • 9 years ago

Comment on attachment 8631911 [details]
MozReview Request: Bug 1168979 part 1 - Add fallback for buildconfig in standalone use of upload_symbols script; r=mrrrgn

Already reviewed in bulk +

	Morgan Phillips [:mrrrgn]
	Comment 35 • 9 years ago

Comment on attachment 8631912 [details]
MozReview Request: Bug 1168979 part 2 - Add symbol uploading as a build dependency; r=mrrrgn

Already reviewed in bulk +

	Morgan Phillips [:mrrrgn]
	Comment 36 • 9 years ago

Comment on attachment 8631913 [details]
MozReview Request: Bug 1168979 part 3 - Clean up upload_symbol template. r=mrrrgn

Already reviewed in bulk +

	Morgan Phillips [:mrrrgn]
	Comment 37 • 9 years ago

Comment on attachment 8631914 [details]
MozReview Request: Bug 1168979 part 4 - Factor out building dependent tasks as a function; Create a task/post-builds for upload_symbols to live in; r=mrrrgn

Already reviewed in bulk +

	Morgan Phillips [:mrrrgn]
	Comment 38 • 9 years ago

Comment on attachment 8631915 [details]
MozReview Request: Bug 1168979 part 5 - Add allowed_build_tasks to distinguish builds that require symbol uploading; r=mrrrgn

Already reviewed in bulk +

	Morgan Phillips [:mrrrgn]
	Comment 39 • 9 years ago

Comment on attachment 8631916 [details]
MozReview Request: Bug 1168979 part 6 - Bump version of upload_symbols container, with addition of new arguments; r=mrrrgn

Already reviewed in bulk +

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 40 • 9 years ago

Hg formatted patch for uploading symbols as a separate task.

	Morgan Phillips [:mrrrgn]
	Comment 41 • 9 years ago

Comment on attachment 8633164 [details] [diff] [review]
upload_symbols.diff

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

This still looks good. Let's go ahead and get a private container in place with a production key; then I think we should be able to ship it. :)

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 42 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccd8a76211d2

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 43 • 9 years ago

Updates the task to pull from a private repository containing the production crash-stats token, with a worker-type of upload-symbol.

	Morgan Phillips [:mrrrgn]
	Comment 44 • 9 years ago

Comment on attachment 8634881 [details] [diff] [review]
upload_symbols1.diff

https://assets.rbl.ms/1514319/980x.jpg

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 45 • 9 years ago

Rebased patch on latest mozilla-inbound head.

	Pulsebot
	Comment 46 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d97c2ab4fe4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bfd2e72d330
https://hg.mozilla.org/integration/mozilla-inbound/rev/162318c4e626
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5644a30469
https://hg.mozilla.org/integration/mozilla-inbound/rev/18560f41c3fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/913594264a8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9496880482

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 9 years ago

Backed out for breaking the gecko decision task.
https://tools.taskcluster.net/task-inspector/#TUEE0yIvRH2MpFIUgvoUXw/3

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c526448011

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 48 • 9 years ago

Fixes decision-task building in non-try branches.

	Chris AtLee [:catlee]
	Comment 49 • 9 years ago

Comment on attachment 8634951 [details] [diff] [review]
upload_symbols_nontry_fix.diff

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

::: testing/taskcluster/taskcluster_graph/commit_parser.py
@@ +261,5 @@
>  
>              # Generate list of post build tasks that run on this build
>              post_build_jobs = []
> +            if 'post_build' in jobs:
> +                for job_flag in jobs['flags']['post-build']:

would this work?

for job_flag in jobs['flags'].get('post-build', []):
    ...

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 50 • 9 years ago

It's a little tricky, since `post-builds` is defined in the base_jobs_flags.yml, but not inside the base_jobs.yml. The inheritance tree for the yaml files goes as follows:

`base_job_flags -> base_jobs -> {project_name}/base_jobs`

...so actually, yes. Thanks for the neat little trick.

	Anthony Miyaguchi [:amiyaguchi] Assignee
	Comment 51 • 9 years ago

This puts the scoping of the job flag from the global config into the branch specific config. Also use .get with defaults.

	Morgan Phillips [:mrrrgn]
	Comment 52 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56a5165f6f80 a try push for the latest patches

	Pulsebot
	Comment 53 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f87d11f5cb1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/95aa87c9e492
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada9565323a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e16dbf99059c
https://hg.mozilla.org/integration/mozilla-inbound/rev/937a44398614
https://hg.mozilla.org/integration/mozilla-inbound/rev/964cebe92d43
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f36f94b285
https://hg.mozilla.org/integration/mozilla-inbound/rev/e58aba8b984b

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f87d11f5cb1c
https://hg.mozilla.org/mozilla-central/rev/95aa87c9e492
https://hg.mozilla.org/mozilla-central/rev/ada9565323a3
https://hg.mozilla.org/mozilla-central/rev/e16dbf99059c
https://hg.mozilla.org/mozilla-central/rev/937a44398614
https://hg.mozilla.org/mozilla-central/rev/964cebe92d43
https://hg.mozilla.org/mozilla-central/rev/f8f36f94b285
https://hg.mozilla.org/mozilla-central/rev/e58aba8b984b