Closed Bug 1457034 Opened 7 years ago Closed 6 years ago

hook for off-cycle partner repacks

Categories

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

Product:
Release Engineering
Component:
Release Automation: Other
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

Attachments

(6 files)

shipit workflow partner hook patch
52 bytes, text/x-github-pull-request
Details | Review
rr3 partner hook patch
2.18 KB, patch
rail
: review+
mozilla
: checked-in+
Details | Diff | Splinter Review
bug 1457034 - re-add partner github code.
59 bytes, text/x-review-board-request
nthomas
: review+
Details
bug 1457034 - generate `release_partner_config` in `release_promotion`
59 bytes, text/x-review-board-request
nthomas
: review+
Details
tools: trigger_partner_hook.diff
3.12 KB, patch
nthomas
: review+
mozilla
: checked-in+
Details | Diff | Splinter Review
bug 1457034 - populate release_partner_build_number.
59 bytes, text/x-review-board-request
nthomas
: review+
Details
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
Attachment #8972695 - Flags: review?(rail)
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.
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 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?
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.
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`.
The latest maple staging release is looking good so far. https://tools.taskcluster.net/groups/NaZqTklHT36FzjQ9i9CWDw
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 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.
Depends on: cotv3
(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.
(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 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 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+
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.
(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+
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)
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 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]
https://tools.taskcluster.net/groups/UjoDJRMcTUKgVsEAKp7doA is the latest "off-cycle" partner repack graph. It should push to v2/ subdirs.
(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+
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+
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
Followup, since graph_config hasn't been set for actions on m-r: https://hg.mozilla.org/releases/mozilla-release/rev/3bfc82b371ecec483e3336944c3e0f33e7d94397
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)
I think adding a partner: {} at the bottom of config.yml should fix.
Flags: needinfo?(nthomas)
Flags: needinfo?(aki)
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
Thanks, it needed this: partner: release: {} staging: {}
Flags: needinfo?(mozilla)
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.

Attachment

General

Creator:
Created:
Updated:
Size: