Closed
Bug 1374589
Opened 7 years ago
Closed 7 years ago
Port windows tests which require signed builds to in-tree tasks
Categories
(Firefox Build System :: Task Configuration, task)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jlorenzo, Assigned: jlorenzo)
References
Details
Attachments
(2 files)
In bug 1362534, we're introducing target.zip which include signed binaries. Once that is done, we can move tests that need these signatures, like xpcshell.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks
https://reviewboard.mozilla.org/r/153510/#review159564
::: taskcluster/ci/test/tests.yml
(Diff revision 3)
> docker-image: {"in-tree": "desktop1604-test"}
> chunks:
> by-test-platform:
> linux64/debug: 10
> android-4.2-x86/opt: 6
> - # windows.*: 1
Hrm, looks like on central this is only 1 chunk right now -- maybe that's related to the failure we see?
(I've seen test failures due to changes in chunking before)
::: taskcluster/taskgraph/loader/build_signing.py:17
(Diff revision 3)
> +LABELS_WHICH_SHOULD_SIGN_CI_BUILDS = (
> + 'build-win32/debug', 'build-win32/opt', 'build-win32-pgo/opt',
> + 'build-win64/debug', 'build-win64/opt', 'build-win64-pgo/opt',
> + # TODO: Activate ASAN when Bug 1377517 is fixed
> + # 'build-win64-asan/opt',
> +)
I'm not a fan of adding hardcodes of explicitly labeled 'what jobs we should run' in the transforms or loaders, I feel that belongs in the kind.yml.
That said, it would take more thought to move it to kind.yml and we can do that in a followup (there is a lot of stuff similar to this in balrog/beetmover)
::: taskcluster/taskgraph/transforms/beetmover.py:273
(Diff revision 3)
> "Can't beetmove a signing task with multiple dependencies")
> signing_dependencies = dep_job.dependencies
> dependencies.update(signing_dependencies)
>
> - attributes = {
> - 'nightly': dep_job.attributes.get('nightly', False),
> + attributes = copy_attributes_from_dependent_job(dep_job)
> + attributes['signed'] = dep_job.attributes.get('signed', False)
NIT: signed is set in copy_attributes_...
::: taskcluster/taskgraph/transforms/beetmover_checksums.py:68
(Diff revision 3)
> for k, v in dep_job.dependencies.items():
> if k.startswith('beetmover'):
> dependencies[k] = v
>
> - attributes = {
> - 'nightly': dep_job.attributes.get('nightly', False),
> + attributes = copy_attributes_from_dependent_job(dep_job)
> + attributes['signed'] = dep_job.attributes.get('signed', False)
Nit: signed is set...
::: taskcluster/taskgraph/transforms/build_signing.py:83
(Diff revision 3)
> - {
> - 'artifacts': ['public/build/target.tar.bz2'],
> + 'artifacts': ['public/build/target.tar.bz2'],
> - 'format': 'gpg',
> + 'format': 'gpg',
> - }, {
> + }]
> +
> + if is_nightly and any(desktop in build_platform for desktop in DESKTOP_BUILD_PLATFORM):
nit: let's not sign target.complete.mar here for anything other than linux, it's invalid on mac and windows (no signed internal bits).
Save ourselves the footgun
::: taskcluster/taskgraph/transforms/signing.py:104
(Diff revision 3)
> - treeherder.setdefault('tier', 1)
> + build_platform = dep_job.attributes.get('build_platform')
> + treeherder.setdefault('platform', _generate_treeherder_platform(
> + dep_th_platform, build_platform, build_type
> + ))
> +
> + # TODO: Make non-nightly (i.e. windows CI builds) Tier 1 once mature enough
What is "mature enough" and whom/where do we determine this?
If unknown I suggest we mark as tier 1 out of the box.
::: taskcluster/taskgraph/transforms/signing.py:154
(Diff revision 3)
> yield task
> +
> +
> +def _generate_treeherder_platform(dep_th_platform, build_platform, build_type):
> + actual_build_type = 'pgo' if '-pgo' in build_platform else build_type
> + return '{}/{}'.format(dep_th_platform, actual_build_type)
This logic is surely fragile (but no less fragile than we've done elsewhere with regard to build_type) so sure.
::: taskcluster/taskgraph/transforms/signing.py:160
(Diff revision 3)
> +
> +def _generate_treeherder_symbol(is_nightly, label):
> + if is_nightly:
> + return 'tc(Ns)'
> + elif '-asan' in label:
> + return 'tc(Bos)'
If we're not signing asan right now, maybe remove this piece?
::: taskcluster/taskgraph/transforms/tests.py:254
(Diff revision 3)
> # chunking-args = test-suite-suffix; "<CHUNK>" in this string will
> # be replaced with the chunk number.
> Optional('chunk-suffix'): basestring,
> +
> + Required('requires-signed-builds', default=False): optionally_keyed_by(
> + 'test-platform', bool),
Nit: Can you format this like 'chunked' above?
::: taskcluster/taskgraph/transforms/tests.py:292
(Diff revision 3)
> # conditional files to determine when these tests should be run
> Optional('when'): Any({
> Optional('files-changed'): [basestring],
> }),
>
> + Optional('build-signing-label'): basestring,
Nit: Can you put this right under 'build-label' and add a docstring to it?
::: taskcluster/taskgraph/util/attributes.py:94
(Diff revision 3)
> + attributes = {
> + 'build_platform': dep_job.attributes.get('build_platform'),
> + 'build_type': dep_job.attributes.get('build_type'),
> + }
> +
> + for attribute_name in _OPTIONAL_ATTRIBUTES:
I'd have done (but not sure if harder to read, or if faster):
`attributes.update({attr: dep_job.attributes[attr] for attr in _OPTIONAL_ATTRIBUTES if attr in dep_job.attributes}`
::: taskcluster/taskgraph/util/scriptworker.py:68
(Diff revision 3)
> +]]
> +
> +CI_SIGNING_CERT_SCOPES = {
> + # No branch-specific configuration required at the moment
> + 'central': 'project:releng:signing:cert:dep-signing',
> + 'default': 'project:releng:signing:cert:dep-signing',
When would branch-specific configuration be required? -- I'm presuming we wouldn't use 'ci signing' even on beta atm (since we use 'nightly' there).
I'm suspecting we can get rid of this and just use dep-signing everywhere.
(Ignore if you disagree or if doing this adds more than an hour of human work [including testing])
Attachment #8882399 -
Flags: review?(bugspam.Callek) → review+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks
https://reviewboard.mozilla.org/r/153510/#review159564
> Hrm, looks like on central this is only 1 chunk right now -- maybe that's related to the failure we see?
>
> (I've seen test failures due to changes in chunking before)
It might be the case. I put back 1 chunk to see how it goes.
> I'm not a fan of adding hardcodes of explicitly labeled 'what jobs we should run' in the transforms or loaders, I feel that belongs in the kind.yml.
>
> That said, it would take more thought to move it to kind.yml and we can do that in a followup (there is a lot of stuff similar to this in balrog/beetmover)
I agree. I added a comment to explicitly tell "this is not the best way to do"
> NIT: signed is set in copy_attributes_...
Good call. I thought I cleared them all. I wonder if that got in after a merge.
> nit: let's not sign target.complete.mar here for anything other than linux, it's invalid on mac and windows (no signed internal bits).
>
> Save ourselves the footgun
Good call. I removed that logic and I added an inline comment to explain the context.
> What is "mature enough" and whom/where do we determine this?
>
> If unknown I suggest we mark as tier 1 out of the box.
Like discussed on ITC, let's land this first as tier 3 and make it tier 1 when signing green on all branches. I updated the comment to be less confusing.
> When would branch-specific configuration be required? -- I'm presuming we wouldn't use 'ci signing' even on beta atm (since we use 'nightly' there).
>
> I'm suspecting we can get rid of this and just use dep-signing everywhere.
>
> (Ignore if you disagree or if doing this adds more than an hour of human work [including testing])
The logic is not needed anymore. It used to be when we tested against nighly scopes. I took that logic out
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Comment 9 is wrong. It turns out we just needed to enable CoT on the win build jobs. Try is now passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a66d52d0d8be81ee2ff1bec993e8a4777466ad51&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=112617167
Starting to land attachment 8882399 [details].
Keywords: leave-open
Comment 13•7 years ago
|
||
Pushed by jlorenzo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7bc534c0604
Port windows tests which require signed builds to in-tree tasks r=Callek
Comment 14•7 years ago
|
||
Backed out for breaking gecko decision task:
https://hg.mozilla.org/integration/autoland/rev/4e4edaddcae3f7566c3ad0e7f95d7139417df138
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a7bc534c0604d7f0134db1ffab6e5a5972fad87d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112649272&repo=autoland
[task 2017-07-07T18:55:38.616740Z] "PUT /queue/v1/task/Vb6Xxr7nQViahDZHsOw8yQ HTTP/1.1" 403 18145
[task 2017-07-07T18:55:38.618360Z] You do not have sufficient scopes. This request requires you
[task 2017-07-07T18:55:38.618482Z] to have one of the following sets of scopes:
[task 2017-07-07T18:55:38.618517Z] [
[task 2017-07-07T18:55:38.618593Z] [
[task 2017-07-07T18:55:38.618785Z] "queue:create-task:highest:scriptworker-prov-v1/depsigning",
[task 2017-07-07T18:55:38.618868Z] "queue:scheduler-id:gecko-level-3"
[task 2017-07-07T18:55:38.618901Z] ],
[task 2017-07-07T18:55:38.618924Z] [
[task 2017-07-07T18:55:38.618972Z] "queue:create-task:very-high:scriptworker-prov-v1/depsigning",
[task 2017-07-07T18:55:38.619185Z] "queue:scheduler-id:gecko-level-3"
[task 2017-07-07T18:55:38.619218Z] ],
[task 2017-07-07T18:55:38.619244Z] [
[task 2017-07-07T18:55:38.619372Z] "queue:create-task:high:scriptworker-prov-v1/depsigning",
[task 2017-07-07T18:55:38.619803Z] "queue:scheduler-id:gecko-level-3"
[task 2017-07-07T18:55:38.619874Z] ],
[task 2017-07-07T18:55:38.619904Z] [
[task 2017-07-07T18:55:38.619951Z] "queue:create-task:medium:scriptworker-prov-v1/depsigning",
[task 2017-07-07T18:55:38.619990Z] "queue:scheduler-id:gecko-level-3"
[task 2017-07-07T18:55:38.620223Z] ],
[task 2017-07-07T18:55:38.620328Z] [
[task 2017-07-07T18:55:38.620595Z] "queue:create-task:low:scriptworker-prov-v1/depsigning",
[task 2017-07-07T18:55:38.620643Z] "queue:scheduler-id:gecko-level-3"
[task 2017-07-07T18:55:38.620673Z] ]
[task 2017-07-07T18:55:38.620698Z] ]
Flags: needinfo?(jlorenzo)
Assignee | ||
Comment 15•7 years ago
|
||
Comment 14 happened because we're missing a scope on autoland (bug 1379594).
In theory, we shouldn't have the signing task running on anything else than mozilla-central and try. However, I looked deeper into the task-selection logic and it's compliant with build-signing/kind.yml + loader/build_signing.py. What happens is: the build-signing task is created when the build is a nightly one or when the platform is Windows, no matter what branch we're on.
Changing that piece of logic doesn't feel right to me. Mainly because that's what we eventually want. At the moment, we target mozilla-central and try, just to workaround bug 1374787. More over, I might break nightly signing if I restrict the projects to try, central, beta and release.
As a consequence, I'd suggest to deal with bug 1374787, first.
Depends on: 1374787
Flags: needinfo?(jlorenzo)
Comment 16•7 years ago
|
||
The depsigning scriptworkers were created for all branches. We should add that scope to level 1 hrough 3, and use them. I don't think depsigning servers block here.
Comment 17•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s a537a28f7571 -d d034fc43e7f6: rebasing 406347:a537a28f7571 "Bug 1374589 - Port windows tests which require signed builds to in-tree tasks r=Callek" (tip)
merging taskcluster/ci/build-signing/kind.yml
merging taskcluster/ci/test/kind.yml
merging taskcluster/ci/test/test-sets.yml
merging taskcluster/ci/test/tests.yml
merging taskcluster/docs/kinds.rst
merging taskcluster/taskgraph/loader/push_apk.py
merging taskcluster/taskgraph/loader/test.py
merging taskcluster/taskgraph/transforms/balrog.py
merging taskcluster/taskgraph/transforms/beetmover.py
merging taskcluster/taskgraph/transforms/beetmover_checksums.py
merging taskcluster/taskgraph/transforms/beetmover_repackage.py
merging taskcluster/taskgraph/transforms/build.py
merging taskcluster/taskgraph/transforms/build_signing.py
merging taskcluster/taskgraph/transforms/checksums_signing.py
merging taskcluster/taskgraph/transforms/job/mozharness.py
merging taskcluster/taskgraph/transforms/job/mozharness_test.py
merging taskcluster/taskgraph/transforms/repackage.py
merging taskcluster/taskgraph/transforms/repackage_signing.py
merging taskcluster/taskgraph/transforms/signing.py
merging taskcluster/taskgraph/transforms/tests.py
merging taskcluster/taskgraph/util/attributes.py
warning: conflicts while merging taskcluster/taskgraph/transforms/repackage.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging taskcluster/taskgraph/transforms/repackage_signing.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
Pushed by jlorenzo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b7ea1de6ba0
Port windows tests which require signed builds to in-tree tasks r=Callek
Comment 20•7 years ago
|
||
bugherder |
Assignee | ||
Comment 21•7 years ago
|
||
XPCShell tests have been running on central for 2 days. The only failure still there is bug 1380627. It requires more investigation and doesn't prevent the tests to run as tier-3. I'm going to close this bug and use 1380627 to promote XPCShell to tier1.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•7 years ago
|
||
Nit: We sign builds on every branch but we don't run XPCShell on integration branches.
Attachment #8888338 -
Flags: review?(bugspam.Callek)
Comment 24•7 years ago
|
||
Comment on attachment 8888338 [details] [diff] [review]
Bug 1374589 - Activate XPCShell on autoland and mozilla-inbound r=Callek
Review of attachment 8888338 [details] [diff] [review]:
-----------------------------------------------------------------
I think I have this somewhere in my patch set on https://reviewboard.mozilla.org/r/157936/ but it should be easy to rebase over...
Attachment #8888338 -
Flags: review?(bugspam.Callek) → review+
Comment 25•7 years ago
|
||
Pushed by jlorenzo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de026113ad0
Activate XPCShell on autoland and mozilla-inbound r=Callek
Comment 26•7 years ago
|
||
bugherder |
Comment 27•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•7 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•