1341214 - Avoid multiple definitions of GECKO, ARTIFACTS_URL and INDEX_URL

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1341214 - Define GECKO in a single location.

Instead of every file trying to get the top source directory having an
ad-hoc definition that gets wrong if the files gets moved around for
some reason, define it in a more central location.

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

Various modules under taskcluster are doing ad-hoc url formatting or
requests to taskcluster services. While we could use the taskcluster
client python module, it's kind of overkill for the simple requests done
here. So instead of vendoring that module, create a smaller one with
a limited set of functions we need.

At the same time, make the function that does HTTP requests more
error-prone, using urllib3's Retry with a backoff factor.

Also add a function that retrieves the list of artifacts, that while
currently unused, will be used by `mach artifact` shortly.

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

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

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

Comment 4

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

Comment on attachment 8839353 [details]
Bug 1341214 - Define GECKO in a single location.

https://reviewboard.mozilla.org/r/114030/#review115722

Comment 5

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

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

https://reviewboard.mozilla.org/r/114032/#review115724

::: taskcluster/taskgraph/util/taskcluster.py:28
(Diff revision 2)
> +    INDEX_URL = 'https://index.taskcluster.net/v1/task/{}'
> +    ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}'
> +
> +
> +def _do_request(url):
> +    session = requests.Session()

Why not use a single, global session?

::: taskcluster/taskgraph/util/taskcluster.py:39
(Diff revision 2)
> +
> +def get_artifact_url(task_id, path):
> +    return ARTIFACT_URL.format(task_id, path)
> +
> +
> +def get_artifact(task_id, path):

Please add a docstring here explaining the content-or-file thing.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> Comment on attachment 8839354 [details]
> Bug 1341214 - Add a small API to handle taskcluster queue and index requests.
> 
> https://reviewboard.mozilla.org/r/114032/#review115724
> 
> ::: taskcluster/taskgraph/util/taskcluster.py:28
> (Diff revision 2)
> > +    INDEX_URL = 'https://index.taskcluster.net/v1/task/{}'
> > +    ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}'
> > +
> > +
> > +def _do_request(url):
> > +    session = requests.Session()
> 
> Why not use a single, global session?

No particular reason, other than not wanting to create a session object just by importing the module. I guess we could have a memoized function that returns a session object.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8122277e9fd
Define GECKO in a single location. r=dustin
https://hg.mozilla.org/integration/autoland/rev/5ffd6c7264ce
Add a small API to handle taskcluster queue and index requests. r=dustin

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8122277e9fd
https://hg.mozilla.org/mozilla-central/rev/5ffd6c7264ce

Comment 10

[Carsten Book [:Tomcat]]

backed out for causing on m-c trouble in vp-tc tasks like https://treeherder.mozilla.org/logviewer.html#?job_id=79361310&repo=mozilla-central

Comment 11

[Pete Moore [:pmoore][:pete]]

Currently no taskcluster-proxy on our Windows worker types, but for downloading public artifacts, you should be able to hit regular queue endpoint, rather than needing to go through the proxy.

Note, if you avoid using the proxy and hit queue directly, you also don't need to enable the taskcluster-proxy feature in the task payload, which might also free up some resources on the worker (e.g. one less docker container to run and less calls to auth overhead server-side).

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/a7b465111b2c
Backed out changeset 5ffd6c7264ce 
https://hg.mozilla.org/mozilla-central/rev/b06968288cff
Backed out changeset d8122277e9fd for causing on m-c problems in vp tc tasks

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8839353 [details]
Bug 1341214 - Define GECKO in a single location.

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

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

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

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114032/diff/4-5/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8839353 [details]
Bug 1341214 - Define GECKO in a single location.

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

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114032/diff/5-6/

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

This needs a re-review.

Note that a followup I have in mind is to change use_proxy to default to None, in which case TASKCLUSTER_PROXY would then be used to choose whether to use the proxy or not, and the things that currently fill e.g. installer-url for other tasks would pass an explicit use_proxy=False. Not sure it's worth doing, but it's an open possibility.

Comment 19

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

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

https://reviewboard.mozilla.org/r/114032/#review116768

I like the refactor of taskcluster API calls and URL generation into a single module.  I think that should continue to use $TASK_ID, rather than introducing a new $TASKCLUSTER_PROXY, but otherwise I'm onboard.

A further optimization I would like to see -- perhaps it's too much for this bug? -- is to generate proxy URLs for things that run in workers where it is supported, and non-proxy URLs for others.

::: taskcluster/taskgraph/task/base.py:106
(Diff revision 6)
>          """
>          for index_path in self.index_paths:
>              try:
> -                url = INDEX_URL.format(index_path)
> -                existing_task = json.load(urllib2.urlopen(url))
> +                task_id = find_task_id(
> +                    index_path,
> +                    use_proxy=bool(os.environ.get('TASKCLUSTER_PROXY')))

This code runs in `./mach taskgraph`, which is only ever run (a) in a decision task (on docker-worker) or (b) on a developer's desktop while debugging taskgraph generation.  It never runs on Windows.

In case (a) you want to use a proxy; in case (b) you do not.  Those two cases are already sufficiently distinguished by the presence (a) or absence (b) of $TASK_ID.

Comment 20

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

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

https://reviewboard.mozilla.org/r/114032/#review116782

This may be overlapping with bug 1341934 enough that the two should be merged?  I think I have the same objection to both..

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

(In reply to Dustin J. Mitchell [:dustin] from comment #20)
> Comment on attachment 8839354 [details]
> Bug 1341214 - Add a small API to handle taskcluster queue and index requests.
> 
> https://reviewboard.mozilla.org/r/114032/#review116782
> 
> This may be overlapping with bug 1341934 enough that the two should be
> merged?  I think I have the same objection to both..

The patch here is just on top of bug 1341934. If bug 1341934 doesn't go in, then the patch here can be trivially changed with s/TASKCLUSTER_PROXY/TASK_ID/ without a need for another round of reviews.

Comment 22

[Mike Hommey [:glandium]]

(In reply to Dustin J. Mitchell [:dustin] from comment #19)
> A further optimization I would like to see -- perhaps it's too much for this
> bug? -- is to generate proxy URLs for things that run in workers where it is
> supported, and non-proxy URLs for others.

That feels like scope bloat.

Comment 23

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

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

https://reviewboard.mozilla.org/r/114032/#review116914

Fair enough.  Let's land this one first, with s/TASKCLUSTER_PROXY/TASK_ID/g.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8839353 [details]
Bug 1341214 - Define GECKO in a single location.

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

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8839354 [details]
Bug 1341214 - Add a small API to handle taskcluster queue and index requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114032/diff/6-7/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16391584c12e
Define GECKO in a single location. r=dustin
https://hg.mozilla.org/integration/autoland/rev/c9dabe672fd8
Add a small API to handle taskcluster queue and index requests. r=dustin

Comment 27

[Mike Hommey [:glandium]]

(In reply to Dustin J. Mitchell [:dustin] from comment #23)
> Fair enough.  Let's land this one first, with s/TASKCLUSTER_PROXY/TASK_ID/g.

FWIW, I literally did that: extracted the patch, did a sed on it and reapplied.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16391584c12e
https://hg.mozilla.org/mozilla-central/rev/c9dabe672fd8