1374589 - Port windows tests which require signed builds to in-tree tasks

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Johan Lorenzo [:jlorenzo]]

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 1

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

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

Comment 3

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

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

Comment 4

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

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

Comment 5

[Justin Wood (:Callek)]

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])

Comment 6

[Johan Lorenzo [:jlorenzo]]

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 7

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

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

Comment 8

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

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

Comment 9

[Johan Lorenzo [:jlorenzo]]

It turns out chain of trust is blocking further testing on try[1]. We need [2] to continue testing

[1] https://tools.taskcluster.net/groups/BpcJH73-Sh-ZCnWNprUv1Q/tasks/ZRSHBQ56RYufcnFOJcu3fQ/runs/0/logs/public%2Flogs%2Fchain_of_trust.log
[2] https://github.com/mozilla-releng/scriptworker/issues/121

Comment 10

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153510/diff/5-6/

Comment 11

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153510/diff/6-7/

Comment 12

[Johan Lorenzo [:jlorenzo]]

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].

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7bc534c0604
Port windows tests which require signed builds to in-tree tasks r=Callek

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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] ]

Comment 15

[Johan Lorenzo [:jlorenzo]]

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.

Comment 16

[Aki Sasaki (not active)]

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

[Mozilla Autoland]

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 18

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8882399 [details]
Bug 1374589 - Port windows tests which require signed builds to in-tree tasks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153510/diff/7-8/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b7ea1de6ba0
Port windows tests which require signed builds to in-tree tasks r=Callek

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b7ea1de6ba0

Comment 21

[Johan Lorenzo [:jlorenzo]]

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.

Comment 23

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1374589 - Activate XPCShell on autoland and mozilla-inbound r=Callek

Nit: We sign builds on every branch but we don't run XPCShell on integration branches.

Comment 24

[Justin Wood (:Callek)]

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...

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de026113ad0
Activate XPCShell on autoland and mozilla-inbound r=Callek

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4de026113ad0

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.