1179660 - xpcshell test certificates are always rebuilt and take about half the time of a no-op build

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Mike Hommey [:glandium]]

STR:
- mach build
- mach build

On my machine the second "mach build", which is (mostly) a no-op takes 45s, 23s of which are spent rebuilding all the xpcshell test certificates.

I have a simple patch that removes the buildid from the script dependencies, making the build skipped, but we have to keep in mind bug 1170431. 
That patch however hardcodes the date, which will likely fail after a while.

That being said, maybe we should step back a little and think about it from a different angle: if those certificates have emission dates and expiry dates set in stone in the -test.zip file, downloading that file in a few months to re-run the xpcshell tests without rebuilding from the given changeset will likely have an expiry problem. Which makes me wonder if those certificates shouldn't, in fact, be created during the test run, instead of during the build.

With that being said, I can understand that this would be much more work than one may want to put (at least speaking for myself), and at the moment I care more about making no-op builds not waste their time with this.

A kind of reasonable option, I guess, would be to move the creation of those certificates to a separate "tier" that only gets built when we build the test archive or before running xpcshell tests.

Alternatively, we could have a buildid-like file that the build system only changes, say, once a week (duration depending on what the expiry for those is, and what the expectations for those certificates are)

Thoughts?

Comment 1

[Mark Goodwin [:mgoodwin]]

(In reply to Mike Hommey [:glandium] from comment #0)
> Which makes me wonder if those
> certificates shouldn't, in fact, be created during the test run, instead of
> during the build.
<snip/>
> A kind of reasonable option, I guess, would be to move the creation of those
> certificates to a separate "tier" that only gets built when we build the
> test archive or before running xpcshell tests.

Something like this sounds like the right answer to me.

I have pretty much zero knowledge of how the build system works; are you able to explain at a high level what we'd need to do to achieve this?
 
> Alternatively, we could have a buildid-like file that the build system only
> changes, say, once a week (duration depending on what the expiry for those
> is, and what the expectations for those certificates are)

This could also work.

Comment 2

[(not currently active) Ted Mielczarek]

For the certs we use for Mochitest, we check them into the tree and give them a 10 year expiry. This sucks because it's a potential future failure, but it's better than regenerating them all the time.

Could we do something hacky like set the mtime of the generated certs to their expiry date, and then make them have a dependency on a file that we `touch` so they'd only get regenerated if they were expired? (Maybe we'd want to make it expiry - some time so that they would get regenerated before you wound up using them.)

Comment 3

[Mark Goodwin [:mgoodwin]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> For the certs we use for Mochitest, we check them into the tree and give
> them a 10 year expiry. This sucks because it's a potential future failure,
> but it's better than regenerating them all the time.

As Bug 1174288 indicates, there's an active effort to move toward more frequent certificate generation for xpcshell tests. I'm guessing that the main motivation here is ensuring out tests are less fragile (since we avoid temptation to hard-code certificate info in tests, etc); of course, Keeler will know for sure.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

There are a few motivations behind generating these certificates programmatically rather than checking them into the tree. See bug 1166976 comment 0.

Generating them when the tests run rather than at compile time might be a reasonable solution, but I'm concerned about tests timing out on the B2G emulators. Having another tier-like thing also seems like a good approach if it's not a lot of work.

We could also do something like take the major version of gecko, guess when it'll be released based on the 6-week cycle, and have the certificates be valid for a year or so before and after that. That way, they wouldn't depend on the buildid but would still have a reasonable validity period. I'm worried that will be a bit fragile, though, because the schedule isn't always 6 weeks.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: MozReview Request: bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

This is to avoid a dependency on the buildid so we don't have to
regenerate all of the test certificate with every ./mach build.
This can cause problems very near midnight on New Year's Eve.
If this happens, kick off a new build and consider reexamining
your current work-life balance.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> Created attachment 8634462 [details]
> MozReview Request: bug 1179660 - define 'now' as the first second of the
> current year for pycert r?Cykesiopka

This isn't a perfect solution, but it'll do for now. We can do something more comprehensive later, but for now we need to fix the immediate issue of doing a lot of work every single build for no good reason.

Comment 7

[Mike Hommey [:glandium]]

I'm working on moving those tests out of the normal build, fwiw.

Comment 8

[:Cykesiopka]

Comment on attachment 8634462 [details]
MozReview Request: bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

https://reviewboard.mozilla.org/r/13401/#review12047

LGTM.
Adding a time bomb is never fun, but this does workaround the issue, and this will probably get defused soon anyways.

::: security/manager/ssl/tests/unit/test_cert_eku/moz.build:569
(Diff revision 1)
> -    props.inputs = [input_file, '!/config/buildid']
> +    props.inputs = [input_file]

test_cert_eku/generate.py needs to be updated for this as well.

Comment 9

[(Away)]

Is this patch ready to land?

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to David Major [:dmajor] from comment #9)
> Is this patch ready to land?

Depends on how close :glandium is to the real solution rather than this temporary fix.

Comment 11

[(Away)]

I find it frustrating when improvements get put off in anticipation of other changes that may or may not ever materialize. In the meantime lots of developers are still losing lots of time on this.

Comment 12

[Mike Hommey [:glandium]]

Please land.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a6a5576d93d

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0a49e8322a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb0a49e8322a

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

It seems to me this bug still exists.

Comment 18

[Mike Hommey [:glandium]]

bug 1188468 is opened for this and has some debugging information.