Closed
Bug 1340609
Opened 8 years ago
Closed 8 years ago
Enable signing for linux CI builds on jamun
Categories
(Release Engineering :: Release Automation: Other, defect, P2)
Release Engineering
Release Automation: Other
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rail, Assigned: mozilla)
References
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
dustin
:
review+
|
Details |
(deleted),
patch
|
dustin
:
feedback-
|
Details | Diff | Splinter Review |
(deleted),
text/x-review-board-request
|
dustin
:
review+
|
Details |
(deleted),
patch
|
rail
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mtabara
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mtabara
:
review+
|
Details | Diff | Splinter Review |
We need these promotable, so they should be signed.
Assignee | ||
Comment 1•8 years ago
|
||
This is essentially making the default CI graph for linux the release graph on Jamun, including scopes changes.
Assignee: nobody → aki
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/projects/jamun/rev/937886931383e449e02923cf2bede436b536590f
bug 1340609 - scopes on demand.
https://hg.mozilla.org/projects/jamun/rev/69d820ddffd2b136a664002ff8dc2d18c3e7b9af
bug 1340609 - trigger correct graph?
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
Dustin:
1) I asked for review (via mozreview) on the scopes-on-demand patch, which I think is ready. This elevates our beta+release scopes for release promotion. We probably want this to be even more customizable later, but this works for now. This also makes things like nightly graphs on try and project branches possible without breaking chain of trust verification, though we still have some ways to go to green up nightly-graphs-on-try. Once balrog and beetmover can handle different scope levels, we can update the various scopes in taskcluster.taskgraph.utils.scriptworker. Let me know if this approach works for you?
2) For on-push graphs on jamun (later mozilla-beta), we need to a) trigger the linux nightly graph minus beetmover and balrog, and b) trigger the android CI graph. Attached is my attempt at it, but it's not working so far. Do you have any suggestions?
Attachment #8838966 -
Flags: feedback?(dustin)
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8838965 [details]
bug 1340609 - toggle nightly scopes on-demand. a=release
https://reviewboard.mozilla.org/r/113722/#review115598
::: taskcluster/taskgraph/transforms/balrog.py:105
(Diff revision 1)
> 'worker-type': 'scriptworker-prov-v1/balrogworker-v1',
> 'worker': {
> 'implementation': 'balrog',
> 'upstream-artifacts': upstream_artifacts,
> },
> # bump this to nightly / release when applicable+permitted
is this comment still relevant?
::: taskcluster/taskgraph/transforms/checksums_signing.py:88
(Diff revision 1)
> 'worker-type': "scriptworker-prov-v1/signing-linux-v1",
> 'worker': {'implementation': 'scriptworker-signing',
> 'upstream-artifacts': upstream_artifacts,
> 'max-run-time': 3600},
> 'scopes': [
> - "project:releng:signing:cert:nightly-signing",
> + signing_cert_scope,
Is this how the signing worker figures out which cert to use, by examining task.scopes? What would happen if a task had `project:releng:signing:cert:*`, or multiple scopes?
From a design perspective, I try to avoid pattern-matching on scopes, preferring to specify something explicitly elsewhere, and then just verify that the corresponding scope exists. Here that would mean something like `task.payload.cert = "nightly-signing"`, with a scope-satisfaction check in the scriptworker for `project:releng:signing:cert:<cert>`.
Not blocking this patch, just something I noticed here.
::: taskcluster/taskgraph/util/scriptworker.py:66
(Diff revision 1)
> +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> + for alias, projects in level_to_project_map:
I'm confused by the terminology here, and in particular `for alias, projects in level_to_project_map`. Is it an alias or a level? In general, a bit more comments in this file would be good, as I'm not even sure I understand it after staring at it for a while, and a logic error here might lead to using the wrong key or bucket.
::: taskcluster/taskgraph/util/scriptworker.py:66
(Diff revision 1)
> +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> + for alias, projects in level_to_project_map:
Attachment #8838965 -
Flags: review?(dustin) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8838966 [details] [diff] [review]
beta-hack.diff
Review of attachment 8838966 [details] [diff] [review]:
-----------------------------------------------------------------
::: .taskcluster.yml
@@ +110,5 @@
> --base-repository='https://hg.mozilla.org/mozilla-central'
> --head-repository='{{{url}}}'
> --head-ref='{{revision}}'
> --head-rev='{{revision}}'
> + --target-tasks-method='beta_tasks'
This line will mean that every push is handled with `beta_tasks`, regardless of tree -- that doesn't seem like what you want?
I suspect that instead you want to set the default parameter for the `mozilla-beta` project (in PER_PROJECT_PARAMETERS in taskcluster/taskgraph/decision.py)
::: taskcluster/taskgraph/target_tasks.py
@@ +213,5 @@
> + 'linux-nightly'):
> + return False
> + if platform in ('linux64-nightly', 'linux-nightly'):
> + for skip in ('beetmover', 'checksum', 'balrog'):
> + if skip in task.label:
Please use an an attribute (maybe just attributes["kind"]) to identify these tasks. Substring matching on labels is risky. In this case, I can imagine scenarios where someone adds an unrelated task that includes the string "checksum", which then causes issues when it hits beta.
Attachment #8838966 -
Flags: feedback?(dustin) → feedback-
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Dustin J. Mitchell [:dustin] from comment #9)
> Comment on attachment 8838966 [details] [diff] [review]
> beta-hack.diff
>
> Review of attachment 8838966 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: .taskcluster.yml
> @@ +110,5 @@
> > --base-repository='https://hg.mozilla.org/mozilla-central'
> > --head-repository='{{{url}}}'
> > --head-ref='{{revision}}'
> > --head-rev='{{revision}}'
> > + --target-tasks-method='beta_tasks'
>
> This line will mean that every push is handled with `beta_tasks`, regardless
> of tree -- that doesn't seem like what you want?
I was going to land it on m-b, so m-c and m-a would never see this line. I can certainly do this in a more train-friendly way; landing on m-b was going to be the quick fix for fx53.
> I suspect that instead you want to set the default parameter for the
> `mozilla-beta` project (in PER_PROJECT_PARAMETERS in
> taskcluster/taskgraph/decision.py)
I'll try that, thanks!
> ::: taskcluster/taskgraph/target_tasks.py
> @@ +213,5 @@
> > + 'linux-nightly'):
> > + return False
> > + if platform in ('linux64-nightly', 'linux-nightly'):
> > + for skip in ('beetmover', 'checksum', 'balrog'):
> > + if skip in task.label:
>
> Please use an an attribute (maybe just attributes["kind"]) to identify these
> tasks. Substring matching on labels is risky. In this case, I can imagine
> scenarios where someone adds an unrelated task that includes the string
> "checksum", which then causes issues when it hits beta.
Will do.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Dustin J. Mitchell [:dustin] from comment #8)
> ::: taskcluster/taskgraph/transforms/checksums_signing.py:88
> (Diff revision 1)
> > 'worker-type': "scriptworker-prov-v1/signing-linux-v1",
> > 'worker': {'implementation': 'scriptworker-signing',
> > 'upstream-artifacts': upstream_artifacts,
> > 'max-run-time': 3600},
> > 'scopes': [
> > - "project:releng:signing:cert:nightly-signing",
> > + signing_cert_scope,
>
> Is this how the signing worker figures out which cert to use, by examining
> task.scopes? What would happen if a task had
> `project:releng:signing:cert:*`, or multiple scopes?
Yes, this specifies which level of certs to use (essentially which signing server to talk to).
Multiple project:releng:signing:cert: scopes will die: https://github.com/mozilla-releng/signingscript/blob/7519e23645f89933ba81eda8b2c0407563bc39dc/signingscript/task.py#L27
project:releng:signing:cert:* won't match anything in passwords.json and will die: https://hg.mozilla.org/build/puppet/file/94bb8a3a6c7d/modules/signing_scriptworker/templates/passwords.json.erb#l2
https://github.com/mozilla-releng/signingscript/blob/7519e23645f89933ba81eda8b2c0407563bc39dc/signingscript/task.py#L45
Specifying nightly-signing or release-signing on non-allowlisted branches will die in cot verification:
https://github.com/mozilla-releng/scriptworker/blob/dbb8ae3dc3b970f10162cc4f219937ed9522a890/scriptworker/constants.py#L182-L187
https://github.com/mozilla-releng/scriptworker/blob/dbb8ae3dc3b970f10162cc4f219937ed9522a890/scriptworker/cot/verify.py#L1136-L1144
> From a design perspective, I try to avoid pattern-matching on scopes,
> preferring to specify something explicitly elsewhere, and then just verify
> that the corresponding scope exists. Here that would mean something like
> `task.payload.cert = "nightly-signing"`, with a scope-satisfaction check in
> the scriptworker for `project:releng:signing:cert:<cert>`.
>
> Not blocking this patch, just something I noticed here.
I'm following the funsize signingworker model:
https://github.com/mozilla-releng/signingworker/blob/master/signingworker/task.py#L24
We're also following this model for beetmover and balrog scriptworkers for fx53b1.
We can certainly adjust, but I think our current scope checking is sufficient. By hardcoding behavior to the scope, we make sure we restrict the ability to request certain behaviors to scopes, and by only allowing certain scopes on certain branches, and by dynamically switching those scopes in the decision task, I think it's user friendly. The only thing that isn't user friendly is trying to hack the graph without the decision task, which chain of trust is trying to discourage anyway.
If we think this isn't the ideal approach, let's make a followup fix after we get 53b1 squared away. I'm not convinced avoiding scope pattern matching is the right approach here, but we can discuss further if this still bothers you.
> ::: taskcluster/taskgraph/util/scriptworker.py:66
> (Diff revision 1)
> > +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> > + for alias, projects in level_to_project_map:
>
> I'm confused by the terminology here, and in particular `for alias, projects
> in level_to_project_map`. Is it an alias or a level? In general, a bit
> more comments in this file would be good, as I'm not even sure I understand
> it after staring at it for a while, and a logic error here might lead to
> using the wrong key or bucket.
I did call it level earlier, and renamed it to alias because a) it's not necessarily 1-2-3, since aliases like "all-nightly-branches" are a superset of branches that are allowlisted to run nightly signing rather than just m-c, and b) we use level for commit level, which is also something different.
I can certainly rename it level, and add more comments.
Comment 12•8 years ago
|
||
The scope thing definitely doesn't have to be fixed here. The idea is that scopes shouldn't communicate information about what the task should do; they should just represent permission to do it.
I agree with your reasons to call it "alias", so maybe use that throughout?
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/projects/jamun/rev/68dcff3c10dd69b3c7af2a3d6df33568c4000ee6
bug 1340609 - promotable beta linux builds on push. r?dustin a=release
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8840047 [details]
bug 1340609 - promotable beta linux builds on push. a=release
https://reviewboard.mozilla.org/r/114598/#review116102
Attachment #8840047 -
Flags: review?(dustin) → review+
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ea30263e2f4a0c2339944c394fbf9138b099fe
bug 1340609 - toggle nightly scopes on-demand. r=dustin a=release
https://hg.mozilla.org/integration/mozilla-inbound/rev/edab1b9f2a7ee081e08ff7f2ddbd653c6cede3c1
bug 1340609 - promotable beta linux builds on push. r=dustin a=release
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8840160 -
Flags: review?(rail)
Assignee | ||
Comment 19•8 years ago
|
||
balrog and beetmover scripts aren't ready for the new scopes yet.
Attachment #8840205 -
Flags: review?(mtabara)
Updated•8 years ago
|
Attachment #8840205 -
Flags: review?(mtabara) → review+
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5031173e44bc1f8ed49d1b748428513c4ef96c3b
bug 1340609 - adjust beetmover+balrog scopes until we're ready. r=mtabara
Assignee | ||
Comment 21•8 years ago
|
||
This patch:
1) updates balrog, beetmover, and signing scriptworkers to scriptworker 2.3.0, which updates the restricted scopes.
2) removes the scriptworker_worker_types from scriptworker.yaml. If we don't specify here, we'll fall back to the constants, which are more up to date: https://github.com/mozilla-releng/scriptworker/blob/master/scriptworker/constants.py#L131
Nothing's broken here; we also check for the provisioner:
https://github.com/mozilla-releng/scriptworker/blob/master/scriptworker/cot/verify.py#L231-L234
This part of the patch is just to clean things up.
Attachment #8840215 -
Flags: review?(mtabara)
Comment 22•8 years ago
|
||
Comment on attachment 8840215 [details] [diff] [review]
scriptworker-2.3.0.diff
lgtm! - we're backward compatible with restricted scopes so it shouldn't affect current beetmoverworkers.
Attachment #8840215 -
Flags: review?(mtabara) → review+
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/build/puppet/rev/f9d2577ebe3cb1877125b3daa0355750d76e55e3
bug 1340609 - scriptworker 2.3.0 for balrog, beetmover, signing scriptworkers. r=mtabara
Reporter | ||
Updated•8 years ago
|
Attachment #8840160 -
Flags: review?(rail) → review+
Comment 24•8 years ago
|
||
bugherder |
Assignee | ||
Comment 25•8 years ago
|
||
https://hg.mozilla.org/build/puppet/rev/f02587a07bf485da1ba15d36ad29b7ff625f11a7
bug 1340609 - enable release signing on signing scriptworkers. r=rail
Assignee | ||
Comment 26•8 years ago
|
||
Uplifted to aurora yesterday. I think we're done here.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 27•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•