1470621 - Convert all existing actions to use hooks

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

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

Attachment: Bug 1470621: make all actions use hooks

Everything but release-promotion, cancel-all, and purge-caches is generic. For
the moment, these all run with the default repo scopes; once this is landed, I
can start adjusting that and granting the necessary scopes only to these
actions.

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

Comment 2

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

Attachment: ci-configuration.patch

I can see two ways of cleaning this up:
 - allow `level` and `trust-domain` to be lists, and implicitly expand those using a cross-product
 - grant scopes with scope-grants.yml (bug 1465842)

Both can come later.

Comment 3

[Tom Prince [:tomprince]]

Comment on attachment 8987969 [details] [diff] [review]
ci-configuration.patch

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

::: actions.yml
@@ +75,5 @@
> +
> +- trust_domain: gecko
> +  level: 1
> +  action_perm: cancel-all
> +  groups: [releng, vpn_sheriff]

I think we probably want to give everyone with level-1 permissions permission the ability to cancel all jobs on level-1 trees. We want people able to cancel all jobs on a try push, for example. (And possibly level-2 as well).

Comment 4

[Tom Prince [:tomprince]]

Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

https://reviewboard.mozilla.org/r/253236/#review260082

This looks good. I think we want to handle rel-pro in a separate bug, given that two callers haven't been updated yet.

Also, we should test all the switched actions before landing this.

::: taskcluster/taskgraph/actions/release_promotion.py:58
(Diff revision 1)
>  
>  @register_callback_action(
>      name='release-promotion',
>      title='Release Promotion',
> +    kind='hook',
> +    generic=False,

Lets split this one into a separate bug. There are at least a couple of places that need to be updated to handle hooks before this bit can land:

- https://hg.mozilla.org/build/tools/file/61c5335d64e2/lib/python/kickoff/actions.py#l37
- https://github.com/mozilla-releng/services/blob/master/src/shipit_workflow/shipit_workflow/tasks.py

Comment 5

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

https://hg.mozilla.org/build/ci-configuration/rev/05567b74d2e3059dd02b31e11f6b710bffa2d448 (with the suggested group changes)

Comment 6

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

(applied with ci-admin)

Comment 7

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

Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

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

Comment 8

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

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

Comment 9

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

The hooks are erroring out because data.user.taskId doesn't exist..
  https://github.com/mozilla/treeherder/pull/3728

Comment 10

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

Attachment: action_perm_cb_name.patch

action_perm needs to be the cb_name for the specific callback, not the name (which might correspond to a few different callbacks, e.g., retrigger works differently for mochitests and non-mochitests).

Comment 11

[Tom Prince [:tomprince]]

Comment on attachment 8988544 [details]
action_perm_cb_name.patch

We discussed this in IRC, and are instead going to adjust the in-tree code to not expose the names of the python callbacks.

Comment 12

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

Attachment: Bug 1470621: actionPerm is the cb_name, not name;

The actionPerm is for access control, so it must limit access to a specific
callback function, not a name (which can apply to mulitple functions).

To make things nicer, we allow functions to specify their cb_name, rather than
assuming it matches the Python function name.

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

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=794647d9847acfa3959cc286ed81fd43294c4fac

Comment 14

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

Testing:

[DONE] add-new-jobs
BnMdsqUySWmXFU-PWzPQew

[DONE] add-talos (context: [])
buIm9ja5RJWt_IpfKdUhOA

[DONE] cancel-all (context: []; non-generic)
LTd1HfPeTP2tB9Qm5CjKOQ

[DONE] mochitest retrigger
Ega9P6msSjGanT5mYdVz0A

[DONE] purge-caches (non-generic)
LsD_XNBNRLOaMcs6M1VZhw

[DONE] rerun
bov0-7AkRQWtBFagdogq6w

[DONE] run-missing-tests (context: [])
GGnqqUEGSyq8v2TwZ4j7Gw

The actions with context: [] need https://github.com/mozilla/treeherder/pull/3728 to land first.

Comment 15

[Tom Prince [:tomprince]]

Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

https://reviewboard.mozilla.org/r/253790/#review260524

::: taskcluster/taskgraph/actions/registry.py:144
(Diff revision 1)
>              assert 1 <= len(symbol) <= 25, 'symbol must be between 1 and 25 characters'
>          assert isinstance(symbol, basestring), 'symbol must be a string'
>  
>          assert not mem['registered'], 'register_callback_action must be used as decorator'
> -        assert cb.__name__ not in callbacks, 'callback name {} is not unique'.format(cb.__name__)
> +        if not cb_name:
> +            cb_name = cb.__name__

I tried just setting this to `name`. It looks like the mochitest retrigger is called `retrigger-mochitest-reftest-with-options` so none of them conflict.

I think we should just go with the name (but with the check that they are unique) for now. If we later want to add identically named actions with different callbacks (that presumably have disjoint sets of tasks they work on), we can add a cb_name parameter that defaults to name.

Comment 16

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

Names are not required to be unique -- https://docs.taskcluster.net/docs/manual/using/actions/spec#action-metadata

I'm not sure why the mochitest retrigger isn't named "retrigger".  Likely because we couldn't figure out how to make those task sets disjoint (I've been thinking of maybe adding a "precedence" to allow non-disjoint sets).

So it doesn't seem like a good idea to remove that capability now, only to re-land it later.

Comment 17

[Tom Prince [:tomprince]]

Well, I'd be happy with just defaulting the callback name to the name (rather than the function name). That would mean there isn't anybody explicitly setting the callback name. But that would be fine.

Comment 18

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

Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

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

Comment 19

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

On reflection, I think we should make all actions generic for now (except relpromo).  It's just too much scope fiddling to try to do more than that.  So purge-cache, cancel, and cancel-all can all be generic.  We can follow up later to make those non-generic.

Comment 20

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

Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

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

Comment 21

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

Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

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

Comment 22

[Tom Prince [:tomprince]]

Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

https://reviewboard.mozilla.org/r/253236/#review261324

Comment 23

[Tom Prince [:tomprince]]

Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

https://reviewboard.mozilla.org/r/253790/#review261330

Comment 24

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32946dea16558e97261abefc65ca34b03db26dc9

Comment 25

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

Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253236/diff/3-4/

Comment 26

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

Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253790/diff/3-4/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceffafc6dee3
make all actions use hooks r=tomprince
https://hg.mozilla.org/integration/autoland/rev/3f4e1feb7c8e
actionPerm is the cb_name, not name; r=tomprince

Comment 28

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ceffafc6dee3
https://hg.mozilla.org/mozilla-central/rev/3f4e1feb7c8e

Comment 29

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/2127780cd6d6
https://hg.mozilla.org/releases/mozilla-release/rev/08804f27a9c5

Comment 30

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9928b1f58895
https://hg.mozilla.org/releases/mozilla-esr60/rev/40eccbaa189f