hook for off-cycle partner repacks

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: aki, Assigned: aki)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

a year ago
Current plan:

* move partner config generation back into taskcluster.taskgraph.util.partners, but generate it inside the release_promotion action rather than partner_repack transform
** only generate the partner config if we don't pass in a partner config
** still populate release_partner_config in params, so we can test locally easily
* create dev hook to trigger the action task on maple, with appropriate inputs
** release_partner_build_number, version, buildnum, revision at least? we may need the promote action task id, which will make this trickier to trigger outside of ship-it-v2
* remove partner config generation from ship-it-v2 and staging releaserunner3
* test on maple, debug
** we may end up altering the parameters as part of this, if it makes more sense
* review - in-tree, releng services, tools, puppet
* land
* uplift to fx60
* verify in production on the next request
* retire partner-repack1 in both scl3 and mdc1
(Assignee)

Comment 3

a year ago
(Assignee)

Updated

a year ago
Attachment #8972695 - Flags: review?(rail)
(Assignee)

Comment 4

a year ago
This works! https://tools.taskcluster.net/groups/REf-WA1nQcS9kHScg4r4TQ/tasks/H9OEmO4RT4-LuYfIdizOZA/runs/0/artifacts didn't have any partner config sent to it by rr3 / shipitv2. It polled github itself and populated the release_partner_config parameter.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
Nick - I'm aware I'm asking you to review your code, but I figure that a) you'll have more context about those TODOs than I have, and b) if you want another set of eyes here, please feel free to add another r?

Thanks!
Attachment #8972695 - Flags: review?(rail) → review+
Comment on attachment 8972731 [details]
bug 1457034 - re-add partner github code.

https://reviewboard.mozilla.org/r/241254/#review247422

::: taskcluster/taskgraph/util/partners.py:1
(Diff revision 1)
> +# TODO - redo errors to keep in style ??

I think I wrote this pretty early on and the logging seems OK in retrospect, so I'm happy to remove this now. Confined to the action log anyways.

::: taskcluster/taskgraph/util/partners.py:245
(Diff revision 1)
> +        name = sub_config['name']
> +        for file in sub_config['object'].get('entries', []):
> +            if file['name'] != 'repack.cfg':
> +                continue
> +            configs[name] = parse_config(file['object']['text'])
> +    ALLOWED_KEYS = ('locales', 'upload_to_candidates', 'platforms')

Maybe this filtering would be slightly better in parse_config(), to avoid writing a dict and culling it again. Not a big deal.

::: taskcluster/taskgraph/util/partners.py:272
(Diff revision 1)
> +
> +        partner_configs[kind] = {}
> +        for partner, partner_url in partners.items():
> +            partner_configs[kind][partner] = get_repack_configs(partner_url, token)
> +
> +        # if we're only interested in a subset of partners we remove the rest

Perhaps it would be cleaner/quicker to pass partner_subset to get_repack_configs(), and avoid retrieving+reading the repack.cfg if we're not interested. Again, not a huge deal.

::: taskcluster/taskgraph/util/partners.py:278
(Diff revision 1)
> +        if partner_subset:
> +            new_config = {}
> +            for partner in partner_subset:
> +                if partner not in partner_configs[kind]:
> +                    # TODO - should be fatal ?
> +                    log.warning('Partner config for %s not found, skipping', partner)

Seems like a good idea to check if we didn't get a config for each member of partner_subset.
Attachment #8972731 - Flags: review?(nthomas) → review+
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review247424

::: taskcluster/taskgraph/actions/release_promotion.py:109
(Diff revision 1)
>      'ship_firefox_rc',
>      'ship_devedition',
>  )
> -
> +PARTNER_CONFIG = {
> +    'release-eme-free-repack': 'https://github.com/mozilla-partners/mozilla-EME-free-manifest',
> +    'release-partner-repack': 'git@github.com:mozilla-partners/repack-manifests',

Any thoughts on this getting out of sync with REPACK_MANIFEST_URL in ci/release-partner-repack/kind.yml and so on ? Perhaps we centralise that logic here, somehow.

::: taskcluster/taskgraph/actions/release_promotion.py:122
(Diff revision 1)
>  
>  
> +def get_partner_config(github_token):
> +    partner_config = {}
> +    for kind, url in PARTNER_CONFIG.items():
> +        partner_config[kind] = get_partner_config_by_url(url, kind, github_token)

Do we have any support for partner subsets ? I think we talked about hook parameters at some point, so maybe this comes later. taskcluster/taskgraph/transforms/partner_repack.py#l44 would need to pass -p arguments too.

::: taskcluster/taskgraph/util/partners.py:156
(Diff revision 1)
> +    repositories. This is not fine grained and also grants r/w access, but is revoked at the repo
> +    level.
> +    """
> +
> +    # The 'usual' method - via taskClusterProxy for decision tasks
> +    # TODO use {level}? Or allow the token to level 1 and remove level from the path?

I don't have enough context here to know what the 'right' thing to do is. It's certainly not like other tokens which take a different value depending on level 1/2/3, but then we have other secrets which we shoehorn into that model. Open to suggestions.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8972731 [details]
bug 1457034 - re-add partner github code.

https://reviewboard.mozilla.org/r/241254/#review247422

> I think I wrote this pretty early on and the logging seems OK in retrospect, so I'm happy to remove this now. Confined to the action log anyways.

Removed.

> Maybe this filtering would be slightly better in parse_config(), to avoid writing a dict and culling it again. Not a big deal.

Done.

> Perhaps it would be cleaner/quicker to pass partner_subset to get_repack_configs(), and avoid retrieving+reading the repack.cfg if we're not interested. Again, not a huge deal.

Moved.

> Seems like a good idea to check if we didn't get a config for each member of partner_subset.

Hm, the new logic doesn't handle this. Enough of a concern to add another check?
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review247424

> Any thoughts on this getting out of sync with REPACK_MANIFEST_URL in ci/release-partner-repack/kind.yml and so on ? Perhaps we centralise that logic here, somehow.

I think this is centralized now. Untested so far; I'm thinking about landing on maple and running another staging release.

> Do we have any support for partner subsets ? I think we talked about hook parameters at some point, so maybe this comes later. taskcluster/taskgraph/transforms/partner_repack.py#l44 would need to pass -p arguments too.

Yes, the `release_partners` parameter is also an input. Added the `-p` arguments; let me know if that looks correct.

> I don't have enough context here to know what the 'right' thing to do is. It's certainly not like other tokens which take a different value depending on level 1/2/3, but then we have other secrets which we shoehorn into that model. Open to suggestions.

I'm thinking we remove it from the path, since this is a secret we can probably expose to level 1.
(Assignee)

Comment 14

a year ago
mozreview-review
Comment on attachment 8972731 [details]
bug 1457034 - re-add partner github code.

https://reviewboard.mozilla.org/r/241254/#review247712

::: taskcluster/taskgraph/util/partners.py:270
(Diff revisions 1 - 2)
>          partner_configs[kind] = {}
>          for partner, partner_url in partners.items():
> -            partner_configs[kind][partner] = get_repack_configs(partner_url, token)
> -
> -        # if we're only interested in a subset of partners we remove the rest
> -        if partner_subset:
> +            if partner_subset and partner not in partner_subset:
> +                continue
> +            partner_configs[kind][partner] = get_repack_configs(
> +                partner_url, token, partner_subset

I need to remove this `partner_subset`.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
The latest maple staging release is looking good so far. https://tools.taskcluster.net/groups/NaZqTklHT36FzjQ9i9CWDw

Comment 19

a year ago
mozreview-review-reply
Comment on attachment 8972731 [details]
bug 1457034 - re-add partner github code.

https://reviewboard.mozilla.org/r/241254/#review247422

> Hm, the new logic doesn't handle this. Enough of a concern to add another check?

No, let's not worry. Reiterating r+.
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review247864

::: taskcluster/ci/config.yml:139
(Diff revision 4)
> +    release:
> +        release-partner-repack: git@github.com:mozilla-partners/repack-manifests.git
> +        release-eme-free-repack: git@github.com:mozilla-partners/mozilla-EME-free-manifest
> +    staging:
> +        release-partner-repack: git@github.com:mozilla-releng/staging-repack-manifests.git
> +        release-eme-free-repack: git@github.com:mozilla-releng/staging-repack-manifests.git

I cheated when I first made this repo and it doesn't actually do anything, as no partners defined in the default.xml. I'd be happy to fix that up, and add a genuine EME-free for staging.

::: taskcluster/taskgraph/transforms/partner_repack.py:25
(Diff revision 4)
>  def resolve_properties(config, tasks):
>      for task in tasks:
> -        for property in ("REPACK_MANIFESTS_URL", ):
> -            property = "worker.env.{}".format(property)
> -            resolve_keyed_by(task, property, property, **config.params)
> +        for prop in ("environment", ):
> +            resolve_keyed_by(task, prop, prop, **config.params)
> +
> +        for k in config.graph_config['partner'][task['environment']]:

Could you call get_partner_url_config() here ? That might save having staging/release in both the kind and PARTNER_BRANCHES/EMEFREE_BRANCHES.

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review247424

> Yes, the `release_partners` parameter is also an input. Added the `-p` arguments; let me know if that looks correct.

That looks like it'll work.

> I'm thinking we remove it from the path, since this is a secret we can probably expose to level 1.

I feel a bit uncomfortable about that, if only because the token grants access to all of a partner repo and anything the API can yield. I'm not sure what the means for releases-on-try, maybe just public staging configs there via a level-1 specific token, which we don't need to give the 'repo' priv which grants write.
(In reply to Aki Sasaki [:aki] from comment #18)
> The latest maple staging release is looking good so far.
> https://tools.taskcluster.net/groups/NaZqTklHT36FzjQ9i9CWDw

Looks like you ran into some issues where Ben's --limit-locale arg isn't yet in the prod tc-partner-repacks.py script, and mozharness wasn't setup to fail when the script failed (so all the bustage shows up in downstreams). The latter should be fixed by 45b236695172 on maple.
(Assignee)

Updated

a year ago
Depends on: cotv3
(Assignee)

Comment 23

a year ago
(In reply to Nick Thomas [:nthomas] (UTC+12) from comment #21)
> > I'm thinking we remove it from the path, since this is a secret we can probably expose to level 1.
> 
> I feel a bit uncomfortable about that, if only because the token grants
> access to all of a partner repo and anything the API can yield. I'm not sure
> what the means for releases-on-try, maybe just public staging configs there
> via a level-1 specific token, which we don't need to give the 'repo' priv
> which grants write.

Why do we need write access with this token? I was under the impression this was a read-only token.
(Assignee)

Comment 24

a year ago
(In reply to Nick Thomas [:nthomas] (UTC+12) from comment #20)
> Comment on attachment 8972732 [details]
> bug 1457034 - generate `release_partner_config` in `release_promotion`
> 
> https://reviewboard.mozilla.org/r/241256/#review247864
> 
> ::: taskcluster/ci/config.yml:139
> (Diff revision 4)
> > +    release:
> > +        release-partner-repack: git@github.com:mozilla-partners/repack-manifests.git
> > +        release-eme-free-repack: git@github.com:mozilla-partners/mozilla-EME-free-manifest
> > +    staging:
> > +        release-partner-repack: git@github.com:mozilla-releng/staging-repack-manifests.git
> > +        release-eme-free-repack: git@github.com:mozilla-releng/staging-repack-manifests.git
> 
> I cheated when I first made this repo and it doesn't actually do anything,
> as no partners defined in the default.xml. I'd be happy to fix that up, and
> add a genuine EME-free for staging.
> 
> ::: taskcluster/taskgraph/transforms/partner_repack.py:25
> (Diff revision 4)
> >  def resolve_properties(config, tasks):
> >      for task in tasks:
> > -        for property in ("REPACK_MANIFESTS_URL", ):
> > -            property = "worker.env.{}".format(property)
> > -            resolve_keyed_by(task, property, property, **config.params)
> > +        for prop in ("environment", ):
> > +            resolve_keyed_by(task, prop, prop, **config.params)
> > +
> > +        for k in config.graph_config['partner'][task['environment']]:
> 
> Could you call get_partner_url_config() here ? That might save having
> staging/release in both the kind and PARTNER_BRANCHES/EMEFREE_BRANCHES.

Looks like we increased our "just get it working" requirement at some point?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

a year ago
mozreview-review
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review248004


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/transforms/partner_repack.py:11
(Diff revision 5)
>  """
>  
>  from __future__ import absolute_import, print_function, unicode_literals
>  
>  from taskgraph.transforms.base import TransformSequence
>  from taskgraph.util.schema import resolve_keyed_by

Error: 'taskgraph.util.schema.resolve_keyed_by' imported but unused [flake8: F401]
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
Comment on attachment 8972695 [details] [diff] [review]
rr3 partner hook patch

https://hg.mozilla.org/build/tools/rev/6cb67381ed9a . I think b3-b7 would have run partner repacks with {} release_partner_config otherwise.
Attachment #8972695 - Flags: checked-in+
(Assignee)

Comment 30

a year ago
emI7mlD0TRW1iTmVrnLtbg is the latest maple promote graph.
(In reply to Aki Sasaki [:aki] from comment #23)
> Why do we need write access with this token? I was under the impression this
> was a read-only token.

It's to do with the github permissions model, where access to private repos is grouped with other access. I'll volunteer to sort this out at a later time.

> Looks like we increased our "just get it working" requirement at some point?

I'm sorry, I've totally done that to you without thinking properly about it. No more cleanup suggestions from me.
(Assignee)

Comment 32

a year ago
(In reply to Nick Thomas [:nthomas] (UTC+12) from comment #31)
> (In reply to Aki Sasaki [:aki] from comment #23)
> > Why do we need write access with this token? I was under the impression this
> > was a read-only token.
> 
> It's to do with the github permissions model, where access to private repos
> is grouped with other access. I'll volunteer to sort this out at a later
> time.

Makes sense.

> > Looks like we increased our "just get it working" requirement at some point?
> 
> I'm sorry, I've totally done that to you without thinking properly about it.
> No more cleanup suggestions from me.

Sorry, frustrating day. Does that mean https://bugzilla.mozilla.org/attachment.cgi?id=8972732 is r+ed? Or is there something else?
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review248384

lgtm, thanks Aki.
Attachment #8972732 - Flags: review?(nthomas) → review+
(Assignee)

Comment 35

a year ago
Comment on attachment 8974214 [details] [diff] [review]
tools: trigger_partner_hook.diff

I added the promote_firefox_partners flavor for Ben to retrigger just partner repacks a while back. For the short term, I think releaseduty can use it once I add some releasewarrior docs. Once bug 1415868 and bug 1459705 land, we should be able to switch to action hooks instead of trigger_action.py.
Attachment #8974214 - Flags: review?(nthomas)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

a year ago
Almost there, thanks Nick. I imagine there will be more cleanup tasks after this, but we should be able to run partner repacks off-cycle without partner-repack1 after this lands, at least.

Comment 40

a year ago
mozreview-review
Comment on attachment 8972732 [details]
bug 1457034 - generate `release_partner_config` in `release_promotion`

https://reviewboard.mozilla.org/r/241256/#review248402


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/transforms/partner_repack.py:36
(Diff revision 7)
> +        else:
> +            raise Exception("Can't find partner REPACK_MANIFESTS_URL")
>  
>          for property in ("limit-locales", ):
>              property = "extra.{}".format(property)
>              resolve_keyed_by(task, property, property, **config.params)

Error: Undefined name 'resolve_keyed_by' [flake8: F821]
(Assignee)

Comment 41

a year ago
https://tools.taskcluster.net/groups/UjoDJRMcTUKgVsEAKp7doA is the latest "off-cycle" partner repack graph. It should push to v2/ subdirs.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

a year ago
(In reply to Code Review Bot [:reviewbot] from comment #40)
> Comment on attachment 8972732 [details]

Thanks reviewbot! Fixed.
Comment on attachment 8974235 [details]
bug 1457034 - populate release_partner_build_number.

https://reviewboard.mozilla.org/r/242538/#review248414
Attachment #8974235 - Flags: review?(nthomas) → review+
Comment on attachment 8974214 [details] [diff] [review]
tools: trigger_partner_hook.diff

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

::: buildfarm/release/trigger_action.py
@@ +131,5 @@
> +            "release_partner_build_number": args.partner_build_num,
> +        })
> +        if args.partner_subset:
> +            action_task_input['release_partners'] = [
> +                partner for partner in args.partner_subset.split(',')

Nit, don't think the list comprehension is necessary when split() returns a list.
Attachment #8974214 - Flags: review?(nthomas) → review+
(Assignee)

Comment 47

a year ago
Comment on attachment 8974214 [details] [diff] [review]
tools: trigger_partner_hook.diff

Removed the list comprehension, thanks!
https://hg.mozilla.org/build/tools/rev/c9c12ae86e180451cfdcbc84c5a1d6d00376cc78
Attachment #8974214 - Flags: checked-in+

Comment 48

a year ago
Pushed by asasaki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c1d508a6c25
re-add partner github code. r=nthomas
https://hg.mozilla.org/integration/autoland/rev/929020e53bd8
generate `release_partner_config` in `release_promotion` r=nthomas
https://hg.mozilla.org/integration/autoland/rev/b66f0fade2a9
populate release_partner_build_number. r=nthomas
(Assignee)

Comment 50

a year ago
Followup, since graph_config hasn't been set for actions on m-r:
https://hg.mozilla.org/releases/mozilla-release/rev/3bfc82b371ecec483e3336944c3e0f33e7d94397

Comment 52

a year ago
Could you please let me know how to get C-C builds back working again since the last merge busted them:
  Exception: Invalid graph configuration:

Log says:
[task 2018-05-09T23:14:07.549Z] Exception: Invalid graph configuration:
[task 2018-05-09T23:14:07.549Z] required key not provided @ data[u'partner']
[task 2018-05-09T23:14:07.550Z] {'index': {'products': ['thunderbird']},

Last time we had something similar (required key not provided @ data[u'release-promotion']), Tom landed this:
https://hg.mozilla.org/comm-central/rev/95c87ceae28de62af4929dd647a8ebd68283da00
Flags: needinfo?(nthomas)
Flags: needinfo?(mozilla)
Flags: needinfo?(aki)
(Assignee)

Comment 53

a year ago
I think adding a 

partner: {}

at the bottom of config.yml should fix.
Flags: needinfo?(nthomas)
Flags: needinfo?(aki)

Comment 54

a year ago
Pushed by acelists@atlas.sk:
https://hg.mozilla.org/comm-central/rev/7128a8ccb15d
Only use a stub 'partner' in taskcluster config, per bug 1457034 comment 53. rs=bustage-fix

Comment 55

a year ago
Thanks, it needed this:
partner:
    release: {}
    staging: {}

Updated

a year ago
Flags: needinfo?(mozilla)

Comment 56

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f298ca26667b
Port bug 1457034 - Use stub 'partner' configuration for TB. r=jorgk
You need to log in before you can comment on or make changes to this bug.