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

NEW
Unassigned

Status

a year ago
8 months ago

People

(Reporter: dustin, Unassigned)

Tracking

52 Branch

Firefox Tracking Flags

(firefox57 wontfix)

Details

Attachments

(6 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
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 12

a year ago
mozreview-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 13

a year ago
mozreview-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 14

a year ago
mozreview-review
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 15

a year ago
mozreview-review
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 16

a year ago
mozreview-review
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+
(Reporter)

Comment 17

a year ago
mozreview-review-reply
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.

Updated

a year ago
status-firefox57: --- → wontfix
Assignee: dustin → nobody

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.