1305096 - schedule fennec beetmover tasks in mach taskgraph

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

[Jordan Lund (:jlund)]

for expected task.json and payload, see beetmoverscript readme.md: https://github.com/mozilla-releng/beetmoverscript/blob/master/README.md

beetmoverscript should be scheduled in a very similar way as signingscript. For Fennec, it will need a beetmover task that depends on the Nightly build task and a beetmover task that depends on the signing task.

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

Assigning this for the moment but I might redirect to Kim.

Comment 3

[Kim Moir [:kmoir] ET]

from #tcmigration irc
mtabara> I had a conversation with rail on this on Friday and I'm not 100% sure if/how beetmoverscript should actually have an output (manifest with url locations) that's to be used by balrogworker 
11:50 AM but I'll double check on the Jordan's code again, maybe he already thought of that 
kmoir> mtabara: is this still accurate in terms of the json that the beetmover task requires https://github.com/mozilla-releng/beetmoverscript/blob/master/README.md
1:40 PM there json here /app/beetmoverworker/work_dir/task.json
1:40 PM the json rather
1:42 PM also, why does it need to depend on the signing task and the nightly task https://bugzilla.mozilla.org/show_bug.cgi?id=1305096
1:42 PM 
<mtabara> kmoir: yes, it is; you can also look at https://bugzilla.mozilla.org/show_bug.cgi?id=1308042#c3, a bit more verbose - the task I tested the staging with 
1:42 PM 
<kmoir> i would have guessed that it would just depend on the signing task
1:42 PM oh okay will look, thanks!
1:46 PM 
<mtabara> kmoir: as to the above question, I presume Jordan meant he wants two separate beetmovertasks tasks scheduled in the graph: 1) one to transport the signing bits consuming signing task artifacts 2) one to transport unsigned bits consuming nightly build task 
1:46 PM s,signing,signed
1:47 PM 
<kmoir> okay that makes sense, thanks
1:49 PM 
<mtabara> np

Comment 4

[Kim Moir [:kmoir] ET]

So from your example here
https://bugzilla.mozilla.org/show_bug.cgi?id=1308042#c3

"version": "52.0a1",
"template_key": "fennec_nightly_unsigned"

are in the payload

these values were created manually I assume in this example

I looked in https://github.com/mozilla-releng/beetmoverscript

How would we know this version to use and inject into the payload?  Also, are there standard template keys that we can use based on a per platform basis?

Comment 5

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Kim Moir [:kmoir] from comment #4)
> So from your example here
> https://bugzilla.mozilla.org/show_bug.cgi?id=1308042#c3
> 
> "version": "52.0a1",
> "template_key": "fennec_nightly_unsigned"
> 
> are in the payload
> 
> these values were created manually I assume in this example
> 
> I looked in https://github.com/mozilla-releng/beetmoverscript
> 
> How would we know this version to use and inject into the payload?  Also,
> are there standard template keys that we can use based on a per platform
> basis?

1. re: template keys 

I see this somehow following the promotion model. Supposedly we'd have everything implemented and we're done with all steps in nightly migration. I'd say we would have standard template keys based on a product(?)/builder_type basis. That is, [1] would probably have a section something like:

"template_files": {
        "fennec_nightly_unsigned": "/path/to/beetmoverscript/beetmoverscript/templates/fennec_nightly_en_us_multi_unsigned.yml",
        "fennec_nightly_signed": "/path/to/beetmoverscript/beetmoverscript/templates/fennec_nightly_en_us_multi_signed.yml",
        "firefox_nightly_enUS_unsigned": "/path/to/beetmoverscript/beetmoverscript/templates/firefox_nightly_en_us_multi_unsigned.yml",            
        "firefox_nightly_enUS_signed": "/path/to/beetmoverscript/beetmoverscript/templates/firefox_nightly_en_us_multi_signed.yml",
        "firefox_nightly_l10n_signed": "/path/to/beetmoverscript/beetmoverscript/templates/firefox_nightly_l10n_signed.yml",
        ....
},

Not sure though about the unsigned/signed bits. To be honest I thought we're only beetmoving signed artifacts but I guess we need to copy those somewhere too at some point. The mapping yml files would somehow resemble the relpro beetmover configs[2] which have all platforms within builder mappings (e.g. enUS, locales, snaps, partials, etc). So in your graph you'd indeed have to use one of the above standard keys. If we go down this path, [1] would also be deployed with puppet to [3].

My questions here: how do I add more mappings there? Where do I get the mapping information from?

2. re: version

Hm, in release promotion the version was being passed down to beetmover from releastasks via release runner. Not sure how the current workflow is in the current graph but I'm guessing we can procure that piece of information from somehwere, right?

3. re: task to beetmove - this should be straightforward if my understanding is right, should use the task id(s) that's/are dependency/ies for the current beetmover task to schedule. 

4. re: upload_date - I'm honestly confused of this one, not sure what this is.

I am bringing jlund in the conversation too - he's the expert on beetmover and might have the right answers ;)

Offtopic - question: just so that I don't forget, I think we need to tweak beetmoverscript to output all artifacts locations to some manifest (maybe overwrite the existing signing manifest with urls?) to have them served later on to balrogworker furthermore. 

[1]: https://github.com/mozilla-releng/beetmoverscript/blob/master/config_example.json
[2]: http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/beetmover
[3]: http://hg.mozilla.org/build/puppet/rev/7e5a2522f625#l7.9

Comment 6

[Jordan Lund (:jlund)]

(In reply to Kim Moir [:kmoir] from comment #4)
> So from your example here
> https://bugzilla.mozilla.org/show_bug.cgi?id=1308042#c3
> 
> "version": "52.0a1",
> "template_key": "fennec_nightly_unsigned"
> 

Mihai and I spoke about this today and we have come up with a few solutions. We also discussed what balrogworker requires from beetmoverscript. I believe that we don't need to include the version or even the template_key in the payload anymore. These can be determined from properties within certain artifacts beetmoverscript is beetmoving. Namely, the nightly build [unsigned] task's balrog_props.json[1]. Using this file kills two birds with one stone: 1) we can determine the version, branch, and app 2) we can update this props file (manifest) with things like CompleteMarUrl and pass it on to the balrogworker tasks that need it to call balrog-submitter.py with.

Given this, I think all we need in the payload is something like:

  "payload": {
    "maxRunTime": 600,
    "upload_date": 1472747174,
    "taskid_to_beetmove": "YVq4WkdlTmSz4on_FwuGIw",  # the taskid that contains artifacts we want to beetmove
    "taskid_of_manifest": "YVq4WkdlTmSz4on_FwuGIw"  # the taskid that contains the artifact of balrog_props.json
    "update_manifest": False,  # tells beetmoverscript whether or not to update the balrog_props.json manifest with CompleteMarUrl props and such.
  },

*note* in the above: these are both the same taskid. This would be the case for the 'unsigned build artifact beetmoverscript' task as it is that task that creates the manifest for balrogworker. taskid_of_manifest and taskid_to_beetmove values will differ in the 'signed build artifact beetmoverscript' task case. To be explicit, this is because the taskid_to_beetmove will represent the signingscript build taskid while the taskid_of_manifest will stay the same.

"update_manifest" should be False (default) in the 'unsigned beetmoverscript' (dep'd on build kind) task case and True in the 'signed beetmoverscript' (dep'd on signing kind) task case. I just realized that we will need someway to tell beetmoverscrpt whether or not to determine the size, hash, and url of the package/installer, update balrog_props.json with that info, and upload the newly updated props file to its artifacts. We could condition off of this payload value.

These are only suggestions. The benefit here is it keeps taskgraph logic minimal Happy to utilize another solution.


[1] https://public-artifacts.taskcluster.net/HrU5yKKGSVSiMdwbJeBVqw/0/public/build/balrog_props.json

Comment 7

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

That sounds like a great solution!  Is it worth thinking about adding some in-tree documentation for it, too?

Comment 8

[Kim Moir [:kmoir] ET]

Attachment: wip patches

So I am stuck and I need a second pair of eyes to figure what is wrong with my patches.

The problem is that it fails with the validation of the task like this

Traceback (most recent call last):
3981   File "/Users/kmoir/hg/date/taskcluster/mach_commands.py", line 228, in show_taskgraph
3982     tg = getattr(tgg, graph_attr)
3983   File "/Users/kmoir/hg/date/taskcluster/taskgraph/generator.py", line 103, in target_task_set
3984     return self._run_until('target_task_set')
3985   File "/Users/kmoir/hg/date/taskcluster/taskgraph/generator.py", line 214, in _run_until
3986     k, v = self._run.next()
3987   File "/Users/kmoir/hg/date/taskcluster/taskgraph/generator.py", line 168, in _run
3988     new_tasks = kind.load_tasks(self.parameters, list(all_tasks.values()))
3989   File "/Users/kmoir/hg/date/taskcluster/taskgraph/generator.py", line 36, in load_tasks
3990     parameters, loaded_tasks)
3991   File "/Users/kmoir/hg/date/taskcluster/taskgraph/task/transform.py", line 76, in load_tasks
3992     tasks = [cls(kind, t) for t in transforms(trans_config, inputs)]
3993   File "/Users/kmoir/hg/date/taskcluster/taskgraph/transforms/task.py", line 548, in build_task
3994     for task in tasks:
3995   File "/Users/kmoir/hg/date/taskcluster/taskgraph/transforms/task.py", line 493, in add_index_routes
3996     for task in tasks:
3997   File "/Users/kmoir/hg/date/taskcluster/taskgraph/transforms/task.py", line 488, in validate
3998     "In task {!r}:".format(task.get('label', '?no-label?')))
3999   File "/Users/kmoir/hg/date/taskcluster/taskgraph/transforms/base.py", line 73, in validate_schema
4000     raise Exception('\n'.join(msg) + '\n' + pprint.pformat(obj))
4001 Exception: In task u'beetmover-build-android-api-15-nightly/opt':
4002 not a valid value for dictionary value @ data[u'worker'][u'implementation']
4003 required key not provided @ data[u'worker'][u'docker-image']
4004 {u'attributes': {u'build_platform': u'android-api-15-nightly',
4005                  u'build_type': u'opt',
4006                  u'nightly': True},
4007  u'dependencies': {u'build': u'build-android-api-15-nightly/opt'},
4008  u'description': u'Android 4.0 API15+ Nightly Beetmover',
4009  u'label': u'beetmover-build-android-api-15-nightly/opt',
4010  u'run-on-projects': [],
4011  u'scopes': [],
4012  u'treeherder': {u'kind': u'build',
4013                  u'platform': u'android-4-0-armv7-api15/opt',
4014                  u'symbol': u'tc(BM)',
4015                  u'tier': 2},
4016  u'worker': {u'implementation': u'beetmover'},
4017  u'worker-type': u'beetmover/beetmover-linux-v1'}

I don't understand why it is failing on u'worker': {u'implementation': u'beetmover'}, and assuming we need a value for docker_image

Comment 9

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

This is voluptuous (the schema tool) getting a bit confused.

4002 not a valid value for dictionary value @ data[u'worker'][u'implementation']

it didn't find a section of the worker scheme with implementation = 'beetmover', so it gave up and picked the first option -- docker-worker/docker-engine.  Then it didn't see a docker image, so it gave the second error,

4003 required key not provided @ data[u'worker'][u'docker-image']

I'm guessing your patch doesn't have a beetmover section in task.py:

    # information specific to the worker implementation that will run this task
    'worker': Any({
        Required('implementation'): Any('docker-worker', 'docker-engine'),
        ...
    }, {
        ...
    }, {
        Required('implementation'): 'beetmover,
        ...                                         # this part is missing?
    })

Comment 10

[Kim Moir [:kmoir] ET]

I do have this in my attached patch, but I will debug some more

+    }, {
+        Required('implementation'): 'beetmover',
+
+        # the maximum time to spend signing, in seconds
+        Required('max-run-time', default=600): int,
+
+        # date when artifacts were beetmoved
+        Required('upload_date'): int,
+
+        # taskid of task with artifacts to beetmove
+        Required('taskid_to_beetmove'): taskref_or_string,
+
+        # taskid of task with artifacts to beetmove
+        Required('taskid_of_manifest'): taskref_or_string,
+
+        # beetmover template key
+        Required('update_manifest'): bool,
     }),

Comment 11

[Justin Wood (:Callek)]

(In reply to Kim Moir [:kmoir] from comment #10)
> I do have this in my attached patch, but I will debug some more
> 

So 4016 has the actual value of `worker` when it reaches here. Which wouldn't match the Any() for what you added, since it has nothing in the Required's (where Required has no default)

Comment 12

[Kim Moir [:kmoir] ET]

Attachment: wip patches

have to fix a few things still

Comment 13

[Kim Moir [:kmoir] ET]

Attachment: beet.patch

Updated patches

Try run here
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2a31c53cad736af9d86f25cce2eb12bda1eecc9&selectedJob=30244184

task graph here
https://public-artifacts.taskcluster.net/Rnd-cV90TK2Ings5trsJag/0/public/full-task-graph.json

One thing I'm not sure about is how to get the task id of the unsigned build task in the task for the signing beetmover task

Comment 14

[Kim Moir [:kmoir] ET]

Attachment: beet.patch

this patch adds the build-signing dependency to the beetmover task that depends on signing

I left the logging imports in case it needs to be refactored again

Comment 15

[Kim Moir [:kmoir] ET]

Comment on attachment 8806430 [details] [diff] [review]
beet.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d32540b6fa0

and taskgraph
https://public-artifacts.taskcluster.net/Ffe1EbAuQJetpyKfl1sf5Q/0/public/full-task-graph.json

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 8806430 [details] [diff] [review]
beet.patch

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

Some code cleanliness nits, only code-correctness nit is in kind.yml, but since it works "today" I'm not strictly worried about it before landing on date.

To be clear the issue I thought I saw and mentioned during our meeting today, was not actually an issue.

::: taskcluster/ci/beetmover/kind.yml
@@ +8,5 @@
> +   - taskgraph.transforms.beetmover:transforms
> +   - taskgraph.transforms.task:transforms
> +
> +kind-dependencies:
> +  - build

don't you want build-signing as well?  (failing that you're not guaranteed to get generated for signing jobs, since they won't necessarily have generated prior to this kind running)

::: taskcluster/taskgraph/task/beetmover.py
@@ +27,5 @@
> +                config.get('kind-dependencies', []) != ["build-signing"]):
> +            raise Exception("Beetmover kinds must depend on builds or signing builds")
> +        for task in loaded_tasks:
> +            if task.attributes.get('build_platform') != "android-api-15-nightly":
> +                continue

future-improvement-fodder:  This might be better as a config (kind.yml) entry similar to how upload symbols is "only for build platforms..."

::: taskcluster/taskgraph/transforms/beetmover.py
@@ +53,5 @@
> +    for job in jobs:
> +        label = job.get('dependent-task', object).__dict__.get('label', '?no-label?')
> +        yield validate_schema(
> +            beetmover_description_schema, job,
> +            "In beetmover ({!r} kind) task for {!r}:".format(config.kind, label))

The above schema and this validate is not really that useful if we're not passing anything else to this transform at present (since the only thing that would get here would be the `dependant-task` piece which is from the get-inputs on the kind implementation.

That said, I'm not eager to strip this out at present since I will need this (or a slightly modified version of this) for l10n beetmoving anyway, so why get rid of it :-)

@@ +74,5 @@
> +        label = job.get('label', "beetmover-{}".format(dep_job.label))
> +        # if dependent task is build, taskid_to_beetmove == taskid_of_manifest
> +        # and update_manifest = False
> +        # if dependent task is signed, the taskid_to_beetmove is the signed
> +        # build and update_manifest = True

for final landing on m-c, I'd prefer a better comment (something about why we update the manifest in signed builds, and why the two taskid's). But since I know this reasoning it's far from blocking.

@@ +86,5 @@
> +        if "signing" in dependent_kind:
> +            taskid_of_manifest = "<build>"
> +            update_manifest = True
> +            signing_dependencies = dep_job.dependencies
> +            dependencies.update(signing_dependencies)

this logic will have to be adjusted for other consumers (e.g. l10n) because dep_job.dependencies could have a different dep label than this.  But it works here at least.

Comment 17

[Kim Moir [:kmoir] ET]

Attachment: beet.patch

updated patch to reflect updates to beetmover kind.yml and other changes to accommodate it.  All other comments were for future work so we'll leave that for now.

If my try run is okay, I'll land it on date.

Comment 18

[Kim Moir [:kmoir] ET]

Comment on attachment 8806495 [details] [diff] [review]
beet.patch

Try run looked good 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0793fa276761&selectedJob=30257418

https://public-artifacts.taskcluster.net/QTCHGCZqTs-uGnjRIdYQpA/0/public/full-task-graph.json

so landed it on date
https://hg.mozilla.org/projects/date/rev/2287a2568038

Comment 19

[Kim Moir [:kmoir] ET]

Attachment: fixprov.patch

mtabara> and, if my understanding is right, you should be using
5:42 PM 	"provisionerId": "scriptworker-prov-v1",
5:42 PM 	"workerType": "beetmoverworker-v1",
5:42 PM 
<kmoir> okay will fix that
5:42 PM thanks
5:42 PM 
<mtabara> np

Comment 20

[Kim Moir [:kmoir] ET]

Okay, with the above patch and fixing the roles for the hook to include queue:create-task:scriptworker-prov-v1/beetmoverworker-v1


the beetmover task is included in the taskgraph


 https://tools.taskcluster.net/push-inspector/#/TDmPvDjeSNqqqixUNLidzA?_k=pb8blk

Comment 21

[Kim Moir [:kmoir] ET]

Looks like all tasks are green except for the beetmover tasks with have exceptions for the new payload schema.  Mihai will land changes to the beetmover worker payload schema tomorrow which will fix this and we should be good to proceed.

Comment 22

[Kim Moir [:kmoir] ET]

Attachment: int.patch

patch to change upload_date to int

looks good on try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=530eff99929cedb683179e1e869777a9cd60ac4f&selectedJob=30319236

https://public-artifacts.taskcluster.net/XUYC5BRJS6qpNMWHB_ydDw/0/public/full-task-graph.json

and in taskgraph

https://public-artifacts.taskcluster.net/XUYC5BRJS6qpNMWHB_ydDw/0/public/full-task-graph.json

landed on date

https://hg.mozilla.org/projects/date/rev/2c75d35a2fb7

Comment 23

[Kim Moir [:kmoir] ET]

Attachment: linux.patch

patch to enable beetmover tasks on other nightlies

try run here
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c7437a0733d14daec77eecdc2fd7997a3cb5010&selectedJob=30322038

https://public-artifacts.taskcluster.net/HIfH4LdlQuezQ7WHA_FmVw/0/public/full-task-graph.json

I fixed the lint error since the try run

Comment 24

[Kim Moir [:kmoir] ET]

I added it to the taskgraph for linux too, not just fennec.

Comment 25

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Kim Moir [:kmoir] from comment #24)
> I added it to the taskgraph for linux too, not just fennec.

Awesome work Kim! Was just about to file a separate bug for that :P