1302590 - remove nightly-fennec kind. use build kind for all nightly build tasks

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Jordan Lund (:jlund)]

now that "Bug 1286075 - Factor builds et al. out of legacy kind" is done, we should switch over to use that :)

Comment 1

[Jordan Lund (:jlund)]

Attachment: Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

looking for an overview feedback first. If it looks okay, we can dive into the
finer details like routes, index, etc

this patch:
1) removes all references to nightly-fennec kind
2) the nightly fennec build is now built using 'build' kind and TransformTask impl
3) renames signing kind to build-signing so we can have multiple kinds using
    same SigningTask impl
3) makes SigningTask impl more generic and configurable from the build-kind jobs-from
  this allows desktop to reuse signingtask pretty easily.


I doubt you are going to like the way I made the android nightlies their own
stanza and basically, a new variant.

fwiw - I tried a few other ways prior to this patch:

1) first I tried doing something similar to pgo where the 'nightly' went after the '/' instead of the 'opt'.
But then I needed hacks here[1].
2) I also tried to add a 'enable-nightly' flag to the opt equivalent within this block[2] so that it acted as
a 'do an opt task and a nightly task based on this opt job'. Though I found I needed to replace
many of these few lines in the opt copy anyway. Further, I had to tell taskgraph to do two tasks for
this one line as it was special. This would require some glue probably before we start any of the
transforms like around here[3]. But that looked pretty ugly when I did it.

now that you have context, you may tear it apart :)

[1] http://hg.mozilla.org/mozilla-central/file/edfff7a9e4c4/taskcluster/taskgraph/transforms/build_attrs.py#l23
[2] http://hg.mozilla.org/mozilla-central/file/edfff7a9e4c4/taskcluster/ci/build/android.yml#l51
[3] http://hg.mozilla.org/mozilla-central/file/edfff7a9e4c4/taskcluster/taskgraph/task/transform.py#l58

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

Comment 2

[Jordan Lund (:jlund)]

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

kmoir, callek: first draft is up for new approach using the new build kind and all of the other changes that came with it. Going with something like this should be an improvement from before and signing should be easier for desktop.

aki: you mentioned you were interested in what state the graph was in. Once this is finalized and stamped, it will be the new way of generating nightly builds and tasks. Again, let me know if you have questions for testing COT

Comment 3

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

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

https://reviewboard.mozilla.org/r/79178/#review77780

This is fine to land as-is, and I'm happy to re-review with the `nightly` attribute added (or some variation on that theme).

::: taskcluster/ci/build/android.yml:76
(Diff revision 1)
>          script: "mozharness/scripts/fx_desktop_build.py"
>          custom-build-variant-cfg: api-15
>          tooltool-downloads: internal
>  
> +android-api-15-nightly/opt:
> +    description: "Android 4.0 API15+ Nightly"

If you add `attributes: nightly: true` here that would make the filtering easier.  Please also add the attribute to `taskcluster/docs/attributes.rst`.

::: taskcluster/ci/build-signing/android-signing.yml:19
(Diff revision 1)
> -    maxRunTime: 600
> +      maxRunTime: 600
> -  metadata:
> +    metadata:
> -    name: "Signing Scriptworker Task"
> +      name: "Signing Scriptworker Task"
> -    description: "Testing the signing scriptworker"
> -    owner: "[e-mail]"
> +      description: "Sign Android Build Tasks"
> +      owner: "[e-mail]"
> -    source: "https://tools.taskcluster.net/task-creator/"
> +      source: "https://tools.taskcluster.net/task-creator/"

Ideally this metadata would come from a transform or from the kind implementation (but it's fine to add that later)

Comment 4

[Kim Moir [:kmoir] ET]

Thanks Jordan for your work on this, I'll refactor my desktop patches to reflect these changes.

Comment 5

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

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

https://reviewboard.mozilla.org/r/79178/#review77828

Ah, I missed the commit comments.

1) I don't know if nightly counts as a platform variant or whatever it is we call the things after the slash (opt, debug, asan, tsan, msan, pgo, ccov, jscov, etc.).  Probably better to leave the platform stuff alone for now.

2) I'm not sure what you mean by "many of these few lines".  That said, the transforms can easily "clone" a task.  We do it with e10s [here](https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests/desktop_test.py#59) for example.

I'll definitely look for an implementation of the latter when the second nightly build stanza lands, but for this patch I'm so delighted to see nightly-fennec go away that I'm happy to look the other way :)

Comment 6

[Justin Wood (:Callek)]

Looks good to me, dustin's points apply to my own mindset as well.

Comment 7

[Aki Sasaki (not active)]

Thanks!
I'm mainly going to have to:

- set task.payload.features.chainOfTrust to true in the decision task (always), artifact image builder task (always), all build tasks that need signing (probably always, but possibly only for nightly), and l10n tasks that need signing (probably always, but possibly only for nightly).  Later on I may have to turn this on in other tasks that need signing, like update mar creation steps.

- add the dependency task id to the signing task's dependency list.

- if we know the artifact image builder task's task id, add it to the signing task's dependency list.  This is likely in a different graph, so figuring this out might be tricky.

Comment 8

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

I'm not sure what the "artifact image builder task" is -- a docker-image task?  If I understand correctly, it's likely to be in a different graph because it's already been built -- that's what optimization does.  Basically, you put a new task in the full task graph, and then during the optimization phase you replace that with an existing task (usually from another graph).

Comment 9

[Aki Sasaki (not active)]

Yes, a docker image task.  Does that mean the optimization phase will insert the task id of the previous graph's docker image task into the signing task's dependency list?  If so, awesome.

Comment 10

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #7)
> - add the dependency task id to the signing task's dependency list.

This should read the 'decision' task id.

Comment 11

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

Yes, that's exactly what it means :)

Comment 12

[Jordan Lund (:jlund)]

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

https://reviewboard.mozilla.org/r/79178/#review77828

oh okay. I'll go with the clone approach in follow up bug

Comment 13

[Jordan Lund (:jlund)]

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

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

Comment 14

[Jordan Lund (:jlund)]

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

https://reviewboard.mozilla.org/r/79178/#review77780

> Ideally this metadata would come from a transform or from the kind implementation (but it's fine to add that later)

I'll fix this in follow up along with the e10s task.clone() style to create nightly tasks based on existing stanzas.

Comment 15

[Jordan Lund (:jlund)]

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

don't think I can re-r? you in mozreview so setting it here with the inter-diff patch: https://reviewboard.mozilla.org/r/79178/diff/1-2/

Comment 16

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

Comment on attachment 8791892 [details]
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,

https://reviewboard.mozilla.org/r/79178/#review78250

A bunch of comments here, but none are serious issues.  Assuming that you're going to continue to hack on the build-signing kind and its implementation, I think this can land and the comments can be handled in subsequent work.

::: taskcluster/ci/build-signing/android-signing.yml:26
(Diff revision 2)
> +    nightly: true
> +  unsigned-task:
> +    label: "build-android-api-15-nightly/opt"
> +    artifacts:
> +      - "public/build/target.apk"
> +      - "public/build/en-US/target.apk"

This seems weird -- the "task" part of this YAML looks like it would be about the same for all signing operations, but then this section is specific to a particular build, named by label.

I'm happy to wait to see what happens when you add a second signing thing in here, though.  I'd hate to see that "task" but repeated in dozens of files.

::: taskcluster/taskgraph/task/signing.py:34
(Diff revision 2)
> -        fennec_tasks = [t for t in loaded_tasks if t.attributes.get('kind') == 'nightly-fennec']
>  
>          tasks = []
> -        for fennec_task in fennec_tasks:
> +        for filename in config.get('jobs-from', []):
>              templates = Templates(root)
> -            task = templates.load('signing.yml', {})
> +            jobs = templates.load(filename, {})

This is a terminology thing, but I think it can mess people up: these are task definitions you're working with here, while "job" in this context suggests a "job description" -- a totally different thing.  It would probably help the next person to come along if you name the config `tasks-from`, and use `tasks` as the variable here.

::: taskcluster/taskgraph/task/signing.py:38
(Diff revision 2)
>              templates = Templates(root)
> -            task = templates.load('signing.yml', {})
> +            jobs = templates.load(filename, {})
>  
> -            artifacts = ['public/build/target.apk',
> -                         'public/build/en-US/target.apk']
> -            for artifact in artifacts:
> +            for name, job in jobs.iteritems():
> +                for artifact in job['unsigned-task']['artifacts']:
> +                    url = ARTIFACT_URL.format('<{}>'.format('unsigned-artifact'), artifact)

or just, `ARTIFACT_URL.format('<{unsigned-artifact}>', artifact)`

::: taskcluster/taskgraph/task/signing.py:39
(Diff revision 2)
> -            task = templates.load('signing.yml', {})
> +            jobs = templates.load(filename, {})
>  
> -            artifacts = ['public/build/target.apk',
> -                         'public/build/en-US/target.apk']
> -            for artifact in artifacts:
> -                url = ARTIFACT_URL.format('<build-nightly-fennec>', artifact)
> +            for name, job in jobs.iteritems():
> +                for artifact in job['unsigned-task']['artifacts']:
> +                    url = ARTIFACT_URL.format('<{}>'.format('unsigned-artifact'), artifact)
> +                    job['task']['payload']['unsignedArtifacts'].append({

This is getting on toward something that could be accomplished with a transform: the YAML would specify a list of artifact names on the upstream task, and the transform would set up the payload like this.  This is fine for now, but next time you return to this bit of code and add another tweak, think of transforms :)

::: taskcluster/taskgraph/task/signing.py:49
(Diff revision 2)
> -                             attributes=attributes))
> -
>          return tasks
>  
>      def get_dependencies(self, taskgraph):
> -        return [('build-nightly-fennec', 'build-nightly-fennec')]
> +        return [(self.unsigned_artifact_label, 'unsigned-artifact')]

Another minor terminology issue here: this is referring to the task, not the artifacts, so the dependency name "unsigned-artifact" feels a bit weird.  How about "unsigned-build-task"?  That still seems a little clunky but at least it's more descriptive..

Comment 17

[Jordan Lund (:jlund)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b269fb8a704a1a1fd2fe67543b9a083ce155563
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies, r=dustin

Comment 18

[Jordan Lund (:jlund)]

(In reply to Dustin J. Mitchell [:dustin] from comment #16)
> Comment on attachment 8791892 [details]
> Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies,
> 
> https://reviewboard.mozilla.org/r/79178/#review78250
> 
> A bunch of comments here, but none are serious issues.  Assuming that you're
> going to continue to hack on the build-signing kind and its implementation,
> I think this can land and the comments can be handled in subsequent work.
> 

thanks!

tracking all follow-ups here:
"Bug 1303894 - clean up build-signing kind, create nightly build based on opt equivalent"

Comment 19

[Iris Hsiao [:ihsiao]]

Backed out for Gecko Decision Task bustage, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=36104306&repo=mozilla-inbound#L94

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82fb090d690
Backed out changeset 1b269fb8a704 for Gecko Decision Task bustage

Comment 21

[Kim Moir [:kmoir] ET]

Attachment: whitelist.patch

patch to fix decision tree breakage

Comment 22

[Jordan Lund (:jlund)]

Comment on attachment 8792899 [details] [diff] [review]
whitelist.patch

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

thanks! I should have known I'd bitrot in 1 day :|

guess this whitelist did its job though :)

I've combined your patch into mine for a relanding

Comment 23

[Jordan Lund (:jlund)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33d031e3454e1bdd1bb94ada0b918c48d7935067
Bug 1302590 - remove nightly-fennec kind. use build kind for nightlies, r=dustin

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33d031e3454e