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

RESOLVED FIXED

Status

task
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jlorenzo, Assigned: jlorenzo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Duplicate of this bug: 1362534
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+
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
Blocks: 1293783
No longer blocks: 1267427
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
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)
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)
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.
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)
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
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: 2 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1362497
Nit: We sign builds on every branch but we don't run XPCShell on integration branches.
Attachment #8888338 - Flags: review?(bugspam.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...
Attachment #8888338 - Flags: review?(bugspam.Callek) → review+
Pushed by jlorenzo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de026113ad0
Activate XPCShell on autoland and mozilla-inbound r=Callek
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.