1216152 - l10n jobs should attach artifacts to their BBB tasks

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P1)

Description

[Rail Aliiev [:rail]]

This way we won't be relying on "insecure" :) index routes.

Comment 1

[Rail Aliiev [:rail]]

Attachment: bbb_artifacts.diff

Testing this on date. I changed task['status']['runs'][0] task['status']['runs'][-1] to make sure we use the latest runId (important for reruns). It wasn't important in case when you create a new task every time.

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8675694 [details] [diff] [review]
bbb_artifacts.diff

>+            bbb_task = Taskcluster(
>+                branch=None, rank=None, client_id=client_id,
>+                access_token=access_token, log_obj=self.log_obj,
>+                task_id=bbb_task_id).get_task(bbb_task_id)

It seems like we might want to break up the Taskcluster() class into something that just interfaces with the taskcluster module, and something that handles the branch/buildbot/task_id stuff? This should work fine, but it looks like I made Taskcluster() have too many responsibilities :). I wouldn't block you on this, but if it makes things easier...

>             for upload_file in files:
>                 # Create an S3 artifact for each file that gets uploaded. We also
>                 # check the uploaded file against the property conditions so that we
>                 # can set the buildbot config with the correct URLs for package
>                 # locations.
>                 tc.create_artifact(task, upload_file)
>+                if bbb_task:
>+                    # Also attach the artifact to BBB task
>+                    tc.create_artifact(bbb_task, upload_file)

So are we actually uploading the file twice? Is there a way we can upload it once and reference it from the BBB task or something?

Comment 3

[Rail Aliiev [:rail]]

(In reply to Michael Shal [:mshal] from comment #2)
> It seems like we might want to break up the Taskcluster() class into
> something that just interfaces with the taskcluster module, and something
> that handles the branch/buildbot/task_id stuff? This should work fine, but
> it looks like I made Taskcluster() have too many responsibilities :). I
> wouldn't block you on this, but if it makes things easier...

Yeah, maybe. I"m not quite sure what would be the API though... Needs 

> So are we actually uploading the file twice? Is there a way we can upload it
> once and reference it from the BBB task or something?

Twice, yeah. :/

Probably using references would be better, great idea.

Thank you for the fast feedback!

Comment 4

[Rail Aliiev [:rail]]

(In reply to Michael Shal [:mshal] from comment #2)
> So are we actually uploading the file twice? Is there a way we can upload it
> once and reference it from the BBB task or something?

Trying this: https://gist.github.com/rail/f6350161f707088b47eb

Comment 5

[Rail Aliiev [:rail]]

Looks like 0.0.15 doesn't support Queue().task(task_id)

testing https://hg.mozilla.org/projects/date/rev/5b17e462b6e6 now..

Comment 6

[Rail Aliiev [:rail]]

Attachment: bbb.diff

This is the current patch. ATM it hits "Authorization failed" because clientIds of BBB and the task are different. Probably we need some auth magic here to make it work.

Comment 7

[Rail Aliiev [:rail]]

11:32 <rail> task is created by some client id
11:32 <rail> BBB finds it
11:32 <rail> creates a BB job
11:32 <rail> a slave (which uses a different client id) wants to create an artifact
11:33 <rail> and fails :)
11:33 <dustin> ok
11:33 <rail> the task is claimed by BB at that point
11:33 <dustin> yeah
11:33 <rail> a slave uses createArtifact API
11:33 <dustin> so really the slave is working on behalf of BBB at that point
11:33 <rail> yup
11:34 <dustin> so maybe BBB should issue a temporary credential to the slave for that push/
11:34 <dustin> or do you want to use the same clientId as for non-BBB-related uploads?
11:35 <dustin> if the latter, then that clientId will need a scope  assume:worker-id:BBB-group/BBB-id
11:35 <rail> not sure...
11:35 <dustin> along with queue:create-artifact:<name>
11:35 <dustin> but the temp creds would be best, as that could be made to mirror the creds used for docker-worker's artifact creation
11:36 <rail> yeah...
11:36 <rail> it's safer this way
11:36 <rail> and we keep the scopes in a single place (BBB client id)
11:37 <rail> this is bit more complicated that I thought :)
11:37 <dustin> yeah, it keeps the BBB abstraction from leaking
11:37 <dustin> yeah, getting those credentials to the task securely might be tricky
11:38 <dustin> to the BB job I mean
11:38 <rail> and we don't probably want to generate temp credentials for every BBB job....
11:38 <dustin> no, there's no reason not to
11:39 <dustin> temp creds are very cheap and easy
11:39 <dustin> and you get to limit the scopes as precisely as you want

Comment 8

[Armen [:armenzg]]

Hi rail,
I have applied your changes to taskcluster_helper.py to my code.

This is for bug 1203552 and the wip is in here:
https://hg.mozilla.org/try/rev/bdbf0f15f167

Comment 9

[Morgan Phillips [:mrrrgn]]

Bug is fixed in production :) https://tools.taskcluster.net/task-graph-inspector/#NT19ZwpGQ_-h6JB99MMPYg/Iv4KseVGQNqtn4Xeo9qIOw/0

Comment 10

[Rail Aliiev [:rail]]

Let's bump it so I pay more attention. :)

Comment 11

[Rail Aliiev [:rail]]

Attachment: artifacts_task.diff

This is something that we talked about yesterday.

The idea is to create a separate task for every l10n task and use it to accumulate artifacts. Unlike the BBB task, this task is supposed to be not claimed by different clientId. It's claimed by MH, used by MH and resolved by MH. Something tells me that it should work. :)

I pushed it to https://treeherder.mozilla.org/#/jobs?repo=date&revision=882ad0da04db

releasetasks patch incoming

Comment 12

[Rail Aliiev [:rail]]

Attachment: releasetasks patch

Comment 13

[Jordan Lund (:jlund)]

Comment on attachment 8701841 [details] [diff] [review]
artifacts_task.diff

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

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py
@@ +77,5 @@
> +    def get_task(self, task_id):
> +        return self.taskcluster_queue.status(task_id)
> +
> +    @staticmethod
> +    def get_mime_type(ext, default='application/octet-stream'):

++

@@ +91,5 @@
>          }
> +        return mime_types.get(ext, default)
> +
> +    @property
> +    def expiration(self):

nice :)

::: testing/mozharness/scripts/desktop_l10n.py
@@ +1014,5 @@
> +                # Create an S3 artifact for each file that gets uploaded. We
> +                # also check the uploaded file against the property conditions
> +                # so that we can set the buildbot config with the correct URLs
> +                # for package locations.
> +                artifact_url = tc.create_artifact(task, upload_file)

hm, so we upload twice in two different tasks? why not just use the artifacts_tc if artifacts_task exists?

if we are going to do both, the naming of vars might be confusing. e.g. `tc` and artifacts_tc` are both artifact uploading tasks no? Similarly, `task` and `artifacts_task`.

Comment 14

[Rail Aliiev [:rail]]

(In reply to Jordan Lund (:jlund) from comment #13)

> hm, so we upload twice in two different tasks? why not just use the
> artifacts_tc if artifacts_task exists?

we use different index routes per locale in the original implementation. Can't assign artifact routes or more than 22 (iirc) routes per task. This is why we have multiple tasks originally.

> if we are going to do both, the naming of vars might be confusing. e.g. `tc`
> and artifacts_tc` are both artifact uploading tasks no? Similarly, `task`
> and `artifacts_task`.

Yeah, maybe I should use something like global_tc and locale_tc...

Comment 15

[Jordan Lund (:jlund)]

(In reply to Rail Aliiev [:rail] from comment #14)
> (In reply to Jordan Lund (:jlund) from comment #13)
> 
> > hm, so we upload twice in two different tasks? why not just use the
> > artifacts_tc if artifacts_task exists?
> 
> we use different index routes per locale in the original implementation.
> Can't assign artifact routes or more than 22 (iirc) routes per task. This is
> why we have multiple tasks originally.
> 

ahh makes sense

Comment 16

[Rail Aliiev [:rail]]

Attachment: l10n_artifacts.diff

This one worked fine, see https://treeherder.allizom.org/#/jobs?repo=date&revision=1f53104e1641

I bumped taskcluster-client.py version up to 0.0.26 to support reference artifacts API, what changed repostCompleted() signature.

Comment 17

[Rail Aliiev [:rail]]

Comment on attachment 8701845 [details] [review]
releasetasks patch

I merged this one.

Comment 18

[Jordan Lund (:jlund)]

Comment on attachment 8702072 [details] [diff] [review]
l10n_artifacts.diff

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

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py
@@ +131,5 @@
> +        task_id = task['status']['taskId']
> +        run_id = task['status']['runs'][-1]['runId']
> +        self.info("Resolving %s, run %s. Full task:" % (task_id, run_id))
> +        self.info(str(task))
> +        self.taskcluster_queue.reportCompleted(task_id, run_id)

oh. nice

::: testing/mozharness/scripts/desktop_l10n.py
@@ +1015,5 @@
> +                # also check the uploaded file against the property conditions
> +                # so that we can set the buildbot config with the correct URLs
> +                # for package locations.
> +                artifact_url = tc.create_artifact(task, upload_file)
> +                if artifacts_task:

still like expressing the difference between an upload task for this specific job (local) and the overall release task graph (global). But meh, I guess this works :)

::: testing/mozharness/scripts/fx_desktop_build.py
@@ +88,5 @@
>  
>                  'stage_username': 'ffxbld',
>                  'stage_ssh_key': 'ffxbld_rsa',
>                  'virtualenv_modules': [
> +                    'requests==2.7.0',

++

Comment 19

[Rail Aliiev [:rail]]

Comment on attachment 8702072 [details] [diff] [review]
l10n_artifacts.diff

Had to resolve some conflicts
https://hg.mozilla.org/integration/mozilla-inbound/rev/004972cea66a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/004972cea66a