Find toolchains in `./mach artifact toolchain --from-build` using indexes

RESOLVED FIXED in Firefox -esr60

Status

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: dustin, Assigned: tomprince)

Tracking

52 Branch
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox57 wontfix, firefox65 fixed)

Details

Attachments

(8 attachments)

In bug 1383880 I came across some code that seems to be trying to fake out parts of the taskgraph generation to determine the taskId of an artifact build.  This is fragile as it relies on implementation details of the task-graph generation code.

A slightly easier option would be to download the most recent decision task's artifacts and use those to find the desired task.  That's what I'll try to implement in bug 1383880.

A *much* easier option would be to just index those tasks at a route that can be reproduced from the given inputs, and look it up at that index path.
Blocks: 1383880
Assignee: nobody → dustin
> A *much* easier option would be to just index those tasks at a route that can be reproduced from the given inputs, and look it up at that index path.

That's exactly what the code *currently* does, by "reusing" the taskcluster code. Except the taskcluster API doesn't have an API for that, so there's some abuse going on.
tl;dr is this:
 1. pull toolchain definitions out of the taskgraph config
 2. standardize hashing toolchains
 3. use that in taskgraph and in ./mach artifact toolchain
While your approach works for this usecase, it's not sustainable for other similar things. Such as bug 1356529, which would then require to pull definitions out of toolchains/ci/build/ which doesn't seem reasonable. Or a helper command to download docker images.
Bug 1356529 hasn't seen any action for five months. I'm going to push back on scope-creeping this bug to cover that work. My work here is to maintain current functionality while fixing bustage caused by the "abuse" you referenced in comment 1.

By the way, there is a helper command to download docker images, already.
Comment on attachment 8908409 [details]
Bug 1397847: allow jobs-from to be topsrcdir-relative, move toolchains;

https://reviewboard.mozilla.org/r/180030/#review186354

I don't think this is desirable. TC job definitions shouldn't spread outside the taskcluster/ directory.
Attachment #8908409 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8908410 [details]
Bug 1397847: rename 'toolchain' transform to 'use_toolchains';

https://reviewboard.mozilla.org/r/180032/#review186356

This is nice but unrelated. It should go in its own bug.
Attachment #8908410 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8908411 [details]
Bug 1397847: make toolchain definitions a bit more generic;

https://reviewboard.mozilla.org/r/180034/#review186358

Nice simplifications, but they should go in their own bug (well, maybe they could share a separate bug with the previous patch).

::: taskcluster/taskgraph/transforms/job/toolchain.py:46
(Diff revision 1)
> -    Optional('toolchain-alias'): basestring,
>  })
>  
>  
>  def add_optimizations(config, run, taskdesc):
> -    files = list(run.get('resources', []))
> +    files = []  # fixed in later commits..

huh

::: taskcluster/taskgraph/transforms/job/toolchain.py:165
(Diff revision 1)
>      attributes = taskdesc.setdefault('attributes', {})
> -    attributes['toolchain-artifact'] = run['toolchain-artifact']
>      if 'toolchain-alias' in run:
>          attributes['toolchain-alias'] = run['toolchain-alias']

Seems like this should all go, like fot docker-worker.

::: taskcluster/taskgraph/transforms/toolchain.py:101
(Diff revision 1)
> +        run_on = tcd['run-on']
> +        if run_on == 'linux':
> +            job['worker-type'] = 'aws-provisioner-v1/gecko-{level}-b-linux'
> +            job['worker'] = {
> +                'docker-image': {'in-tree': 'desktop-build'},
> +                'max-run-time': 36000,

It would be good to be able to override this. Particularly, these values are cargo culted and wrong, and we should change them, possibly to different values depending on the job. Bug 1391427 defines some values that are not 36000 for instance.
Attachment #8908411 - Flags: review?(mh+mozilla)
Comment on attachment 8908412 [details]
Bug 1397847: build common code for finding and hashing toolchains;

https://reviewboard.mozilla.org/r/180036/#review186360

I'm not particularly convinced by this. I still think we should be able to get useful partial task graphs through mach taskgraph or some direct API.

::: taskcluster/taskgraph/transforms/job/toolchain.py:63
(Diff revision 1)
> +    if 'toolchain-alias' in toolchain:
> +        subs['name'] = toolchain['toolchain-alias']
> +        routes.append('index.{}'.format(TOOLCHAIN_INDEX.format(**subs)))

I can see why you're adding this, but this is a separate matter, it should be in a separate patch or even bug.
Attachment #8908412 - Flags: review?(mh+mozilla)
Comment on attachment 8908413 [details]
Bug 1397847: use hashing and the index to download toolchains;

https://reviewboard.mozilla.org/r/180038/#review186362

::: python/mozbuild/mozbuild/mach_commands.py:1899
(Diff revision 1)
>                               'Could not find a toolchain build named `{build}`')
>                      return 1
> +                toolchain = toolchains[b]
> +                digest = hash_toolchain(toolchains, b)
>  
> -                task_id = optimize_task(task, {})
> +                index_path = 'gecko.cache.level-{level}.toolchains.v1.{name}.{digest}'

This is the kind of thing that I didn't want to have duplicated between mach_commands and the taskcluster code. Plus, this doesn't try all levels above MOZ_SCM_LEVEL like the toolchain optimization code does.

::: taskcluster/taskgraph/util/taskcluster.py:85
(Diff revision 1)
> +    except requests.exceptions.HTTPError as e:
> +        if e.response.status_code == 404:
> +            raise KeyError
> +        raise

Seems like a nice api change, that could be done separately.
Attachment #8908413 - Flags: review?(mh+mozilla)
Comment on attachment 8908609 [details]
Bug 1397847: --from-builds is a dev tool, not for automation;

https://reviewboard.mozilla.org/r/180272/#review186364

Fair enough

::: commit-message-19b51:1
(Diff revision 1)
> +Bug 1397847: --from-builds is a dev tool, not for automation; r?glandium

--from-build (no s)
Attachment #8908609 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8908412 [details]
Bug 1397847: build common code for finding and hashing toolchains;

https://reviewboard.mozilla.org/r/180036/#review186360

I think ahal's work on tryselect for `mach try` is the right way to do this -- generate and cache a full taskgraph.  That might need a slight bit of generalization to make it usable here, but it's *very* close to what you need.  Callek was interested in such a thing, too.  You're right that moving task configuration out of taskcluster/ci starts to set a bad precedent, especially if we move build definitions out.
Assignee: dustin → nobody
Product: Core → Firefox Build System
`mach artifact toolchain` gets task definitions from taskgraph, to get the
index path to find the artifacts at. Now that these index paths depend on the
digests of fetch tasks, those kinds need to be loaded as well. This adds a
supported API to get task definitions for a given kind, which loads all the
kind dependencies.
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/393f02fb1ad7
Add supported API to get tasks of a given kind from a taskgraph; r=dustin
https://hg.mozilla.org/mozilla-central/rev/393f02fb1ad7
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee: nobody → mozilla
See Also: → 1511048
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/mozilla-central/rev/19480c535117
Pass the taskcluster root to `load_tasks_for_kind` in `mach artifact toolchain`; r=dustin a=tomprince
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.