1411427 - beetmover-cdns kind

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, enhancement)

Description

[Aki Sasaki (not active)]

Attachment: beetmover-cdns.diff

We need a push-to-mirrors task in-tree.

- add beetmover-cdns kind
- add it to the publish_fennec action; remove the other beetmover kinds
- make sure we use the correct push-to-releases scope

TODO:
- add notifications. Our notification code is currently in the buildbot job code; let's pull it out and reuse
- add dependencies. I think this should depend on a dummy task (or the notification task) at the end of the promote action.
- run and debug

Comment 1

[Rail Aliiev [:rail]]

Comment on attachment 8921660 [details] [diff] [review]
beetmover-cdns.diff

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

LGTM!

There is one trick that we use for desktop releases. We have 2 push to cdns jobs. The first one pushes everything partner repacks (including EME), so we don't block on partner repacks to finish. The second one pushes the partner repacks only. Maybe we should reserve something that would pass --exclude/include to the worker? or we can define those in the worker itself maybe?

::: taskcluster/ci/beetmover-cdns/kind.yml
@@ +10,5 @@
> +
> +job-defaults:
> +   worker-type:
> +      by-project:
> +         release: scriptworker-prov-v1/beetmoverworker-v1

probably we need to add beta as well.

@@ +24,5 @@
> +            - index.releases.v1.{branch}.latest.fennec.latest.beetmover_cdns
> +            - index.releases.v1.{branch}.{revision}.fennec.{underscore_version}.build{build_number}.beetmover_cdns
> +      treeherder-platform: Android/opt
> +
> +# We probably want a dummy task after the beetmover checksums, or the publish notify task?

Maybe we need another transform to add notifications?

::: taskcluster/taskgraph/transforms/beetmover_cdns.py
@@ +98,2 @@
>          job["worker"] = worker
> +        del(job['product'])

Not sure why this is needed...

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8921660 [details] [diff] [review]
beetmover-cdns.diff

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

::: taskcluster/taskgraph/util/scriptworker.py
@@ +117,5 @@
>  
>  """Map the beetmover scope aliases to the actual scopes.
>  """
>  BEETMOVER_BUCKET_SCOPES = {
> +    'all-candidates-tasks': {

You need an entry here as well for `all-published-tasks' that looks like this one:

{
     'all-release-branches': 'project:releng:beetmover:bucket:release',
},	

Otherwise, for a `mozilla-beta`, Fennec publish job (hence the trigger task `publish_fennec`), it would fail to set the proper bucket scope here https://hg.mozilla.org/projects/maple/file/tip/taskcluster/taskgraph/util/scriptworker.py#l332 because of missing entry in this dictionary. It'll default to `dep` otherwise.

Comment 3

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #1)
> Comment on attachment 8921660 [details] [diff] [review]
> beetmover-cdns.diff
> 
> Review of attachment 8921660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM!
> 
> There is one trick that we use for desktop releases. We have 2 push to cdns
> jobs. The first one pushes everything partner repacks (including EME), so we
> don't block on partner repacks to finish. The second one pushes the partner
> repacks only. Maybe we should reserve something that would pass
> --exclude/include to the worker? or we can define those in the worker itself
> maybe?
> 
> ::: taskcluster/ci/beetmover-cdns/kind.yml
> @@ +10,5 @@
> > +
> > +job-defaults:
> > +   worker-type:
> > +      by-project:
> > +         release: scriptworker-prov-v1/beetmoverworker-v1
> 
> probably we need to add beta as well.

I was hoping `release` would cover all release branches, but I haven't verified.
Switching to explicit mozilla-beta and mozilla-release. We'll have to add each esr to the list when we branch them.

> @@ +24,5 @@
> > +            - index.releases.v1.{branch}.latest.fennec.latest.beetmover_cdns
> > +            - index.releases.v1.{branch}.{revision}.fennec.{underscore_version}.build{build_number}.beetmover_cdns
> > +      treeherder-platform: Android/opt
> > +
> > +# We probably want a dummy task after the beetmover checksums, or the publish notify task?
> 
> Maybe we need another transform to add notifications?

I think so. But we want those dummy tasks to have a single task to depend on.

> ::: taskcluster/taskgraph/transforms/beetmover_cdns.py
> @@ +98,2 @@
> >          job["worker"] = worker
> > +        del(job['product'])
> 
> Not sure why this is needed...

Transforms later on complain that 'product' is an unknown key in the job.

Comment 4

[Aki Sasaki (not active)]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #2)
> Comment on attachment 8921660 [details] [diff] [review]
> beetmover-cdns.diff
> 
> Review of attachment 8921660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/taskgraph/util/scriptworker.py
> @@ +117,5 @@
> >  
> >  """Map the beetmover scope aliases to the actual scopes.
> >  """
> >  BEETMOVER_BUCKET_SCOPES = {
> > +    'all-candidates-tasks': {
> 
> You need an entry here as well for `all-published-tasks' that looks like
> this one:
> 
> {
>      'all-release-branches': 'project:releng:beetmover:bucket:release',
> },	
> 
> Otherwise, for a `mozilla-beta`, Fennec publish job (hence the trigger task
> `publish_fennec`), it would fail to set the proper bucket scope here
> https://hg.mozilla.org/projects/maple/file/tip/taskcluster/taskgraph/util/
> scriptworker.py#l332 because of missing entry in this dictionary. It'll
> default to `dep` otherwise.

Thanks for the catch!

Comment 5

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/maple/rev/e68481303bd3aeebcf882396ac8bb728b0d7f9fc

Comment 6

[Aki Sasaki (not active)]

Attachment: notifications

:garbas - I'm trying to make the task.extra.notifications generic and not tie them to buildbot. This seems to work, but it doesn't actually add task.extra.notifications to the beetmover-cdns task. Do you know what I'm missing?

:jlorenzo - Will task.extra.notifications even work outside of buildbot-bridge? if not, this might not be worth the effort.

Comment 7

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8922123 [details] [diff] [review]
notifications

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

Looks good to me! I love the refactor done. There's one light improvement we can to reduce the duplication. It's not blocking, though.

::: taskcluster/taskgraph/util/release_notifications.py
@@ +45,5 @@
> +
> +        completed = notifications.get('completed')
> +        if completed:
> +            if isinstance(completed, list):
> +                worker['notifications']['task-completed'] = {

Nit: We can probably shorten this function, thanks to a function that crafts a notification dict.

```py

def _craft_notification(short_status, long_status, ids):
    return {
        "subject": "{}: {}".format(short_status.capitalize(), FULL_TASK_NAME),
        "message": "Uh-oh! {} {}.".format(FULL_TASK_NAME, long_status),
        "ids": ids,
    }

ALL_STATUSES = (
    ('completed', 'has completed successfully! Yay!'),
    ('failed', 'failed'),
    ('exception', 'resulted in an exception'),
)


def add_worker_notifications(config, worker, run):
    notifications = run.get('notifications')

    if not notifications:
         return

    worker.setdefault('notifications', {})

    for short_status, long_status in ALL_STATUSES:
        ids_or_notification_def = notifications.get(short_status)
        if ids_or_notification_def:
            if isinstance(ids_or_notification_def, list):
                ids = ids_or_notification_def
                notification_def = _craft_notification(short_status, long_status, ids)
            else:
                notification_def = ids_or_notification_def
            worker['notifications']['task-{}'.format(short_status)] = notification_def
```

Comment 8

[Johan Lorenzo [:jlorenzo]]

(In reply to Aki Sasaki [:aki] from comment #6)
> :jlorenzo - Will task.extra.notifications even work outside of
> buildbot-bridge? if not, this might not be worth the effort.

Yes. Pulse-notify just listens to some pulse routes, which currently are:
* route.index.releases.v1.mozilla-beta.#
* route.index.releases.v1.mozilla-esr45.#
* route.index.releases.v1.mozilla-esr52.#
* route.index.releases.v1.mozilla-release.#

At some point, we may want to use taskcluster's notifications instead of pulse-notify. For more details, see bug 1361228 comment 1.

Comment 9

[Rok Garbas [:garbas]]

Comment on attachment 8922123 [details] [diff] [review]
notifications

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

::: taskcluster/ci/beetmover-cdns/kind.yml
@@ +26,1 @@
>  

:aki to make "notifications" part more generic I think we should

(1) move notification assigigment out of `run:` and move it to the "root" of the kind. example:


> jobs:
>    fennec:
>       name: ...
>       notifications:
>          completed: ...
>          failed: ...
>          exception: ...

(2) and then handle the notifications option in either job tranform or even in task transform. that would make it completly generic and not coupled with specific worker implementation.

if you want I can make this generic in ticket i'm currently working on "per-task notifications for desktop".

Comment 10

[Aki Sasaki (not active)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #8)
> (In reply to Aki Sasaki [:aki] from comment #6)
> > :jlorenzo - Will task.extra.notifications even work outside of
> > buildbot-bridge? if not, this might not be worth the effort.
> 
> Yes. Pulse-notify just listens to some pulse routes, which currently are:
> * route.index.releases.v1.mozilla-beta.#
> * route.index.releases.v1.mozilla-esr45.#
> * route.index.releases.v1.mozilla-esr52.#
> * route.index.releases.v1.mozilla-release.#

Awesome.

> At some point, we may want to use taskcluster's notifications instead of
> pulse-notify. For more details, see bug 1361228 comment 1.

Yeah, we decided it would be faster to go with our notifications, and move to taskcluster notifications when we're not faced with both a change freeze deadline and an ESR59-off-buildbot deadline.

Comment 11

[Aki Sasaki (not active)]

(In reply to Rok Garbas [:garbas] from comment #9)
> Comment on attachment 8922123 [details] [diff] [review]
> notifications
> 
> Review of attachment 8922123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/ci/beetmover-cdns/kind.yml
> @@ +26,1 @@
> >  
> 
> :aki to make "notifications" part more generic I think we should
> 
> (1) move notification assigigment out of `run:` and move it to the "root" of
> the kind. example:
> 
> 
> > jobs:
> >    fennec:
> >       name: ...
> >       notifications:
> >          completed: ...
> >          failed: ...
> >          exception: ...
> 
> (2) and then handle the notifications option in either job tranform or even
> in task transform. that would make it completly generic and not coupled with
> specific worker implementation.
> 
> if you want I can make this generic in ticket i'm currently working on
> "per-task notifications for desktop".

Yes please!

Comment 12

[Aki Sasaki (not active)]

Fixed on maple; bug 1412690 tracks landing on m-c.