1250700 - Artifact builds should use the gecko.v2 index instead of buildbot.revisions.X

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

I'd like to remove the buildbot.revisions.X namespace at some point, since it was originally made to just hold those builds that are done by buildbot and uploaded to Taskcluster. Can we use the gecko.v2 index when querying artifact builds instead?

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Shal [:mshal] from comment #0)
> I'd like to remove the buildbot.revisions.X namespace at some point, since
> it was originally made to just hold those builds that are done by buildbot
> and uploaded to Taskcluster. Can we use the gecko.v2 index when querying
> artifact builds instead?

I see no reason not to, just need to update:

https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=%22buildbot.%22%20path%3Amozbuild&redirect=true

Comment 2

[Michael Shal [:mshal]]

Do you see any reasons we can't use an approach like https://bugzilla.mozilla.org/show_bug.cgi?id=1276037#c3 here? Specifically, the part about iterating through m-c / m-i / fx-team to find builds. Or, we could try to add a separate gecko.v2.revisions without the branch name to support this use-case.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

We only actually accept artifacts from a handful of trees, so from a correctness standpoint this is fine. But the existing approach is convenient, because we don't waste the effort of asking TC about each tree.

Checking for new artifacts happens pretty often in the current set up, it would be good to keep it as fast as possible.

Comment 4

[Michael Shal [:mshal]]

gps and dustin had some comments in an email chain that I wanted to capture here...

From gps:
>>> I would absolutely love if we had an index "namespace" that had data grouped by revision without the
>>> {project} bit in the URL. That would allow any client-side operation walking commit ancestry to find
>>> the "nearest available" commit from automation without having to check state from multiple
>>> repositories. This would make said operations (including artifact builds) faster and simpler.
>>>
>>> A downside is different repos have different configurations. So we e.g. don't want results from Try
>>> written to the same "revisions" "namespace" as mozilla-central. And we don't want mozilla-beta and
>>> mozilla-aurora to exist within the same namespace either. We ideally want the revisions namespace to
>>> have some other "key" that isn't the repository name but is instead the type of job. e.g. "central
>>> configuration," "try configuration," etc.

From dustin:
>> Like, say, "project"? :)

From gps:
> Yes... but {mozilla-central, mozilla-inbound, fx-team, autoland} would need to be shared because
> they all share the same configuration. AFAIK the only thing that is different is which repo URL is
> reported in build info, symbols, etc.

Given this, should we add a gecko.v2 route that is something like the following?

gecko.v2.artifacts.{configuration}.revision.{head_rev}

This would put an "artifacts" namespace at the same level as branch names (mozilla-central, mozilla-inbound, etc), which could then have multiple configuration namespaces underneath.

Is there value in having configurations other than the {m-c/m-i/fx-team/autoland} one used by artifact builds? Is putting the name "artifacts" in the route name too specific to the one use-case?

Comment 5

[Michael Shal [:mshal]]

Marking this as depending on bug 1301720, mostly as a reminder that until that lands any new routes would have to be added in both mozharness and Taskcluster.

Comment 6

[Gregory Szorc [:gps]]

In terms of hierarchy, {revision} should come before {configuration}. This is because a single revision can have N configurations and sometimes it is *very* useful to iterate and compare configurations for a single revision. I think that's much easier if {configuration} is a child of {revision}.

Something else we need to consider is the security angle. We likely don't want Try builds in the same namespace as pretty much everything else for security reasons. I suppose if {configuration} allowed you to distinguish Try builds (OK - "level 1" builds), that would be fine.

Anyway, my comments in the email thread were generally applicable, not just to "artifacts." Although adopting this strategy for the artifacts we use to drive artifact builds would be a step forward!

Comment 7

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1250700-Use-the-gecko.v2-instead-of-buildbot-for.patch

I took another look at this today - one thing I noticed is that we sort of already do a tree iteration in _pushheads_from_rev(), but we discard that information and just return a list of revisions. If instead we keep the tree:revision mapping around through to find_pushhead_artifacts(), we don't have to do any back & forth with the index, since we can calculate the gecko.v2 route directly (and the whole TreeCache goes away).

Does this seem reasonable? Or am I missing something here?

There is still a TODO, so I'm just asking f? for the general approach for now. Presumably we could add a MOZ_ARTIFACT_TREE to go along with MOZ_ARTIFACT_REVISION (perhaps defaulting to mozilla-central if unspecified) to take care of the other _install_from_hg_pushheads() entrypoint.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8796739 [details] [diff] [review]
0001-Bug-1250700-Use-the-gecko.v2-instead-of-buildbot-for.patch

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

::: python/mozbuild/mozbuild/artifacts.py
@@ +803,5 @@
>              'mozilla-central',
>              'integration/fx-team',
>              'integration/mozilla-inbound',
>              'releases/mozilla-aurora'
>          )

The only issue I see is that these names don't always correspond to their naming in the index route. This happens to work for central, but if I pull inbound and try to build with this patch it fails. Maybe we can find another way to make this mapping.

Comment 9

[Michael Shal [:mshal]]

(In reply to Chris Manchester (:chmanchester) from comment #8)
> ::: python/mozbuild/mozbuild/artifacts.py
> @@ +803,5 @@
> >              'mozilla-central',
> >              'integration/fx-team',
> >              'integration/mozilla-inbound',
> >              'releases/mozilla-aurora'
> >          )
> 
> The only issue I see is that these names don't always correspond to their
> naming in the index route. This happens to work for central, but if I pull
> inbound and try to build with this patch it fails. Maybe we can find another
> way to make this mapping.

Good catch. At least for the trees we have so far, the part after the '/' maps to what we have in the index, so we can just convert this tree name into the index name pretty easily.

I also added support for the MOZ_ARTIFACT_REVISION case - here it just tries the specified revision on each tree. This ends up a little slower when you specify MOZ_ARTIFACT_REVISION, but a little faster in the default case since it doesn't have the back&forth with the index. So performance-wise it's about even, but this should allow us to remove the buildbot.* namespace.

Comment 10

[Michael Shal [:mshal]]

Attachment: Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

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

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

https://reviewboard.mozilla.org/r/82892/#review81546

::: python/mozbuild/mozbuild/artifacts.py:829
(Diff revision 1)
> -            candidate_pushheads = []
> +            candidate_pushheads = OrderedDefaultDict(list)
>              for tree, pushid in found_pushids.iteritems():
>                  end = pushid
>                  start = pushid - NUM_PUSHHEADS_TO_QUERY_PER_PARENT
>  
>                  self.log(logging.INFO, 'artifact',
>                           {'tree': tree,
>                            'pushid': pushid,
>                            'num': NUM_PUSHHEADS_TO_QUERY_PER_PARENT},
>                           'Retrieving the last {num} pushheads starting with id {pushid} on {tree}')
> -                candidate_pushheads.extend(pushhead_cache.pushid_range(tree, start, end))
> +                candidate_pushheads[tree].extend(pushhead_cache.pushid_range(tree, start, end))

This might be a little clearer if we use a regular DefaultDict here, we're not going to see a tree more than once going through this loop, so extending the list isn't really meaningful.

::: python/mozbuild/mozbuild/artifacts.py:889
(Diff revision 1)
> +        for tree in candidate_pushheads:
> +            candidate_revs = set(candidate_pushheads[tree])
> -        for rev in last_revs:
> +            for rev in last_revs:
> -            rev = rev.rstrip()
> +                rev = rev.rstrip()
> -            if not rev:
> +                if not rev:
> -                continue
> +                    continue
> -            if rev not in candidate_pushheads:
> +                if rev not in candidate_revs:
> -                continue
> +                    continue
> -            count += 1
> +                count += 1

We want to be iterating revisions, then trees here, right? We want the most recent available artifacts.

::: python/mozbuild/mozbuild/artifacts.py:898
(Diff revision 1)
> -            if not rev:
> +                if not rev:
> -                continue
> +                    continue
> -            if rev not in candidate_pushheads:
> +                if rev not in candidate_revs:
> -                continue
> +                    continue
> -            count += 1
> +                count += 1
> -            yield rev
> +                yield (tree, rev)

Nit: parens not necessary here.

::: python/mozbuild/mozbuild/artifacts.py:986
(Diff revision 1)
>  
>          urls = None
>          count = 0
>          # with blocks handle handle persistence.
> -        with self._task_cache as task_cache, self._tree_cache as tree_cache:
> -            for hg_hash in hg_pushheads:
> +        with self._task_cache as task_cache:
> +            for (tree, hg_hash) in hg_pushheads:

Nit: parens not necessary here.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

https://reviewboard.mozilla.org/r/82892/#review81552

Thanks for taking this on. A few nits and an issue here. I'll want to find time to test this a bit locally before landing as well.

If you're struggling to find the comments that go along with the issues in bugzilla, it's because I accidentally published the review issues before finishing the review. Sorry about that.

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

https://reviewboard.mozilla.org/r/82892/#review81546

> This might be a little clearer if we use a regular DefaultDict here, we're not going to see a tree more than once going through this loop, so extending the list isn't really meaningful.

As a result of the next issue, I ended up changing this to a regular dict as a mapping from revision -> tree.

> We want to be iterating revisions, then trees here, right? We want the most recent available artifacts.

Ahh, you're right - I mixed up what we're actually iterating through. By making candidate_pushheads a dict mapping revision to tree, we don't need to create the set and can just grab the tree name from candidate_pushheads. We shouldn't be looping over trees here.

Comment 14

[Michael Shal [:mshal]]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

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

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

https://reviewboard.mozilla.org/r/82892/#review82052

Looks good and works well testing locally. One note about an edge case that should be addressed before landing.

::: python/mozbuild/mozbuild/artifacts.py:837
(Diff revisions 1 - 2)
> -                candidate_pushheads[tree].extend(pushhead_cache.pushid_range(tree, start, end))
> +                for pushhead in pushhead_cache.pushid_range(tree, start, end):
> +                    candidate_pushheads[pushhead] = tree

Iterating over trees makes some sense: it's probably quite rare, but occasionally we'll find that a revision is a push head on multiple trees, and if we do, we don't want to assume builds have finished for both pushes.

Comment 16

[Michael Shal [:mshal]]

(In reply to Chris Manchester (:chmanchester) from comment #15)
> ::: python/mozbuild/mozbuild/artifacts.py:837
> (Diff revisions 1 - 2)
> > -                candidate_pushheads[tree].extend(pushhead_cache.pushid_range(tree, start, end))
> > +                for pushhead in pushhead_cache.pushid_range(tree, start, end):
> > +                    candidate_pushheads[pushhead] = tree
> 
> Iterating over trees makes some sense: it's probably quite rare, but
> occasionally we'll find that a revision is a push head on multiple trees,
> and if we do, we don't want to assume builds have finished for both pushes.

Ahh, that makes sense. With this I had just been overriding the tree if we found the rev on multiple trees. Changing candidate_pushheads to a dict of revision: list-of-trees seems to handle this case.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

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

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

Oops, meant to ask for re-review for the tree looping changes.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

https://reviewboard.mozilla.org/r/82892/#review82874

Looks good! Sorry I missed this update.

Comment 20

[Michael Shal [:mshal]]

Comment on attachment 8797295 [details]
Bug 1250700 - Use the gecko.v2 instead of buildbot for artifact builds;

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

Comment 21

[Michael Shal [:mshal]]

Rebased against bug 1305502.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69a218996577
Use the gecko.v2 instead of buildbot for artifact builds; r=chmanchester

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/69a218996577