Closed Bug 1415619 Opened 7 years ago Closed 6 years ago

Add separate index space for comm-central builds for cached artifacts for toolchains and docker images

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: tomprince, Assigned: tomprince)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1415619: Factor out toolchain cache index calculations.
59 bytes, text/x-review-board-request
dustin
: review+
mshal
: review+
Details
Bug 1415619: Add features for legacy taskcluster routes.
59 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1415619: Add some routes that humans can use to cached tasks.
59 bytes, text/x-review-board-request
dustin
: review+
mshal
: review+
Details
Bug 1415619: Use `gecko.caches` for indexing docker tasks.
59 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1415619: Add a separate trust-domain to cached tasks.
59 bytes, text/x-review-board-request
dustin
: review+
gps
: review+
Details
Bug 1415619: Add some sphinx documentation pointing at taskgraph configuration.
59 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1415619: Specify trust domains for production branches.
59 bytes, text/x-review-board-request
dustin
: review+
Details 
      No description provided.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8927106 - Attachment is obsolete: true
Comment on attachment 8926577 [details]
Bug 1415619: Factor out toolchain cache index calculations.

https://reviewboard.mozilla.org/r/197810/#review204242
Attachment #8926577 - Flags: review?(dustin) → review+
Comment on attachment 8927105 [details]
Bug 1415619: Add a separate trust-domain to cached tasks.

https://reviewboard.mozilla.org/r/198316/#review204244

::: taskcluster/taskgraph/config.py:15
(Diff revision 3)
>  graph_config_schema = Schema({
>      Required('treeherder'): {
>          # Mapping of treeherder group symbols to descriptive names
>          Required('group-names'): {basestring: basestring}
>      },
> +    # XXX

XXX!

Maybe it would be good to write a section somewhere in taskcluster/docs about this concept?  I'd like to have a few people look at it, including someone from FoxSec.
Attachment #8927105 - Flags: review?(dustin) → review+
Comment on attachment 8927104 [details]
Bug 1415619: Use `gecko.caches` for indexing docker tasks.

https://reviewboard.mozilla.org/r/198314/#review204246
Attachment #8927104 - Flags: review?(dustin) → review+
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review204248
Attachment #8927103 - Flags: review?(dustin) → review+
I like this, but let's make sure "trust-domain" is a useful thing before landing.
Comment on attachment 8929066 [details]
Bug 1415619: Add some sphinx documentation pointing at taskgraph configuration.

https://reviewboard.mozilla.org/r/200368/#review205488
Attachment #8929066 - Flags: review?(dustin) → review+
Comment on attachment 8927105 [details]
Bug 1415619: Add a separate trust-domain to cached tasks.

https://reviewboard.mozilla.org/r/198316/#review204244

> XXX!
> 
> Maybe it would be good to write a section somewhere in taskcluster/docs about this concept?  I'd like to have a few people look at it, including someone from FoxSec.

Now this can link to the docs..
Attachment #8929101 - Flags: review?(dustin)
Attachment #8926580 - Flags: review?(dustin)
Keywords: leave-open
Comment on attachment 8927105 [details]
Bug 1415619: Add a separate trust-domain to cached tasks.

https://reviewboard.mozilla.org/r/198316/#review205568

This seems pretty straightforward. It looks to be backwards compatible for Firefox and it just paving the road for using a separate index prefix for non-Firefox CI. Seems like a good approach!
Attachment #8927105 - Flags: review?(gps) → review+
Comment on attachment 8929101 [details]
Bug 1415619: Specify trust domains for production branches.

https://reviewboard.mozilla.org/r/200396/#review205578
Attachment #8929101 - Flags: review?(dustin) → review+
Comment on attachment 8927105 [details]
Bug 1415619: Add a separate trust-domain to cached tasks.

https://reviewboard.mozilla.org/r/198316/#review205584

I didn't see the other changes in this series.

The changes to the index paths scares me. I think we should get a few more build peers to look at this to assess implications to toolchain paths, artifact builds, and code signing. I'm specifically worried about references to index paths that aren't in taskgraph itself. glandium and mshal should have most bases covered.
Attachment #8926577 - Flags: review?(mshal)
Attachment #8926577 - Flags: review?(mh+mozilla)
Attachment #8927103 - Flags: review?(mshal)
Attachment #8927103 - Flags: review?(mh+mozilla)
Attachment #8926577 - Flags: review?(mh+mozilla)
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review205714

::: taskcluster/taskgraph/util/cached_tasks.py:14
(Diff revision 5)
>  
>  TARGET_CACHE_INDEX = (
> -    'gecko.cache.level-{level}.{type}.{name}.{digest}'
> +    'gecko.cache.level-{level}.{type}.{name}.hash.{digest}'
>  )
> +EXTRA_CACHE_INDEXES = [
> +    'gecko.cache.level-{level}.{type}.{name}.latest',

I'm opposed to any "latest" in TC indexes. Retrigger an old job, and it becomes the latest. There's also an order issue between branches.

::: taskcluster/taskgraph/util/cached_tasks.py:15
(Diff revision 5)
>  TARGET_CACHE_INDEX = (
> -    'gecko.cache.level-{level}.{type}.{name}.{digest}'
> +    'gecko.cache.level-{level}.{type}.{name}.hash.{digest}'
>  )
> +EXTRA_CACHE_INDEXES = [
> +    'gecko.cache.level-{level}.{type}.{name}.latest',
> +    'gecko.cache.level-{level}.{type}.{name}.pushdate.{year}.{month}-{day}-{pushtime}',

I'm not convinced this is useful, and the commit message doesn't tell much about what use case this is supposed to help with.
Attachment #8927103 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review205714

> I'm not convinced this is useful, and the commit message doesn't tell much about what use case this is supposed to help with.

These routes currently exist for docker images, and I am unifying the logic for caching docker images and toolchains, and wanted to preserve the existing routes. I don't have a strong preference on whether these routes stick around.
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review205714

> These routes currently exist for docker images, and I am unifying the logic for caching docker images and toolchains, and wanted to preserve the existing routes. I don't have a strong preference on whether these routes stick around.

Specifically, the route [here](https://reviewboard.mozilla.org/r/198314/diff/5#3). It seemed easier to add new routes to toolchains, than to remove routes from docker images.
Comment on attachment 8926580 [details]
Bug 1415619: Add features for legacy taskcluster routes.

https://reviewboard.mozilla.org/r/197814/#review206002
Attachment #8926580 - Flags: review?(dustin) → review+
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review205714

> I'm opposed to any "latest" in TC indexes. Retrigger an old job, and it becomes the latest. There's also an order issue between branches.

<dustin> so latest is probably useful for the "I need an ___ and I don't especially care which one"
<tomprince> I know I was using it, before I decided to switch to building in c-c.
<dustin> that seems like a decent argument -- especially if it's definitely not used in automation
(In reply to Mike Hommey [:glandium] from comment #41)
> > +    'gecko.cache.level-{level}.{type}.{name}.latest',
> 
> I'm opposed to any "latest" in TC indexes. Retrigger an old job, and it
> becomes the latest. There's also an order issue between branches.

This depends on how the rank field is set. If the retrigger uses the same rank as an old job, it will not overwrite a newer job with a higher rank.
Comment on attachment 8926577 [details]
Bug 1415619: Factor out toolchain cache index calculations.

https://reviewboard.mozilla.org/r/197810/#review206116
Attachment #8926577 - Flags: review?(mshal) → review+
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review206124

::: taskcluster/taskgraph/util/cached_tasks.py:15
(Diff revision 5)
>  TARGET_CACHE_INDEX = (
> -    'gecko.cache.level-{level}.{type}.{name}.{digest}'
> +    'gecko.cache.level-{level}.{type}.{name}.hash.{digest}'
>  )
> +EXTRA_CACHE_INDEXES = [
> +    'gecko.cache.level-{level}.{type}.{name}.latest',
> +    'gecko.cache.level-{level}.{type}.{name}.pushdate.{year}.{month}-{day}-{pushtime}',

It would've been nice if these matched the gecko.v2 routes of {year}.{month}.{day}.{pushdate} when they were introduced, but since the toolchain tasks have always used this format I suppose we should keep it.
Attachment #8927103 - Flags: review?(mshal) → review+
Comment on attachment 8927103 [details]
Bug 1415619: Add some routes that humans can use to cached tasks.

https://reviewboard.mozilla.org/r/198312/#review206124

> It would've been nice if these matched the gecko.v2 routes of {year}.{month}.{day}.{pushdate} when they were introduced, but since the toolchain tasks have always used this format I suppose we should keep it.

The toolchain tasks didn't have these routes, and the docker tasks were indexed elsewhere anyway. I'd be happy to make this match the gecko.v2 routes.
(In reply to Tom Prince [:tomprince] from comment #50)
> Comment on attachment 8927103 [details]
> Bug 1415619: Add some routes that humans can use to cached tasks.
> 
> https://reviewboard.mozilla.org/r/198312/#review206124
> 
> > It would've been nice if these matched the gecko.v2 routes of {year}.{month}.{day}.{pushdate} when they were introduced, but since the toolchain tasks have always used this format I suppose we should keep it.
> 
> The toolchain tasks didn't have these routes, and the docker tasks were
> indexed elsewhere anyway. I'd be happy to make this match the gecko.v2
> routes.

Since they're both under gecko.cache, I think they should match the existing docker tasks like you have in the patch. I don't think it's worth changing the docker ones as well. I didn't mark it as an issue to fix, just whining :)
All mozilla-central derived trees can use the new routes with the current scopes.
I've add more scopes to the project roles, so that eventually they can be
removed from moz-tree:level:N, which will remove them from the comm-central
derived trees.

My plan for landing this is

1) land to m-c and tools.
2) run new tc-admin against comm (and merge?)
3) after infra freeze run rest of tc-admin and delete roles from moz-tree:level:N
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/44579aec5f27
Factor out toolchain cache index calculations. r=dustin,mshal
https://hg.mozilla.org/integration/autoland/rev/4a3811a038dc
Add some routes that humans can use to cached tasks. r=dustin,mshal
https://hg.mozilla.org/integration/autoland/rev/2e35d7731c81
Use `gecko.caches` for indexing docker tasks. r=dustin
https://hg.mozilla.org/integration/autoland/rev/4116e89e495c
Add a separate trust-domain to cached tasks. r=dustin,gps
https://hg.mozilla.org/integration/autoland/rev/588e1170b413
Add some sphinx documentation pointing at taskgraph configuration. r=dustin
Attachment #8927103 - Flags: review?(mh+mozilla)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: TaskCluster → Firefox Build System
status-firefox59: --- → fixed
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: