l10n jobs should attach artifacts to their BBB tasks

RESOLVED FIXED

Status

Release Engineering
Applications: MozharnessCore
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: rail, Assigned: rail)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
This way we won't be relying on "insecure" :) index routes.
(Assignee)

Comment 1

3 years ago
Created attachment 8675694 [details] [diff] [review]
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.
Attachment #8675694 - Flags: feedback?(mshal)
(Assignee)

Updated

3 years ago
Blocks: 1214347
(Assignee)

Updated

3 years ago
Blocks: 1216160
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?
Attachment #8675694 - Flags: feedback?(mshal) → feedback+
(Assignee)

Comment 3

3 years ago
(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!
(Assignee)

Comment 4

3 years ago
(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
(Assignee)

Comment 5

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

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

Comment 6

3 years ago
Created attachment 8676784 [details] [diff] [review]
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.
Attachment #8675694 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1208182
(Assignee)

Comment 7

3 years ago
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
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
(Assignee)

Comment 10

3 years ago
Let's bump it so I pay more attention. :)
Severity: normal → critical
Priority: -- → P1
(Assignee)

Comment 11

2 years ago
Created attachment 8701841 [details] [diff] [review]
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
Attachment #8676784 - Attachment is obsolete: true
Attachment #8701841 - Flags: feedback?(jlund)
(Assignee)

Comment 12

2 years ago
Created attachment 8701845 [details] [review]
releasetasks patch
Attachment #8701845 - Flags: feedback?(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`.
Attachment #8701841 - Flags: feedback?(jlund) → feedback+
(Assignee)

Comment 14

2 years ago
(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...
(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
(Assignee)

Comment 16

2 years ago
Created attachment 8702072 [details] [diff] [review]
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.
Attachment #8701841 - Attachment is obsolete: true
Attachment #8702072 - Flags: review?(jlund)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8701845 [details] [review]
releasetasks patch

I merged this one.
Attachment #8701845 - Flags: feedback?(jlund)
Attachment #8701845 - Flags: feedback+
Attachment #8701845 - Flags: checked-in+
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',

++
Attachment #8702072 - Flags: review?(jlund) → review+
(Assignee)

Comment 19

2 years ago
Comment on attachment 8702072 [details] [diff] [review]
l10n_artifacts.diff

Had to resolve some conflicts
https://hg.mozilla.org/integration/mozilla-inbound/rev/004972cea66a
Attachment #8702072 - Flags: checked-in+

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/004972cea66a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.