x/build: add -longcrypto TryBot

[FiloSottile]

Currently, the -longtest TryBots run on CLs that touch crypto/tls, to run the slow-ish BoGo test suite.

Also, cmd/dist runs the GOFIPS140=$VERSION go test crypto/... tests only in long mode.

This is sub-optimal in two ways: CLs that touch crypto/tls are excruciatingly slow to TryBot, and flake and suffer from broken builders all the time; meanwhile, most crypto CLs are not tested against older versions of the FIPS module.

Can we add a new TryBot which is enabled for all CLs that touch crypto/... and that tests in long mode only crypto/... and GOFIPS140=$VERSION go test crypto/...?

/cc @golang/release @golang/security

[gabyhelp]

Related Issues

• x/build: consider opting-in to longtest builders by default for cmd/go changes, if/when they're under 15 min #42661 (closed)
• x/build/cmd/coordinator: include -longtest and -race builders for trybots on release branches #37827
• x/build: shard and scale the `longtest` SlowBots #37439 (closed)
• x/build/cmd/coordinator: include full set of tests (rather than limited trybot set) when a builder is requested via SlowBots #38279 (closed)
• x/build/cmd/release{,bot}: include long tests in pre-release testing #29252 (closed)
• x/build: add longtest builders for all first class ports #56822 (closed)

Related Code Changes

• [dev.boringcrypto] crypto/tls/fipsonly: new package to force FIPS-allowed TLS settings
• [dev.boringcrypto] misc/boring: skip long tests during build.release

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[FiloSottile]

most crypto CLs are not tested against older versions of the FIPS module

CL 733844 is an example of a potential breakage not caught by the current TryBots.

[Jorropo]

most crypto CLs are not tested against older versions of the FIPS module

CL 733844 is an example of a potential breakage not caught by the current TryBots.

The diff is missleading because it's parent CL were missing a hunk (git add -p mistake).

FWIW in CL 733842 I did removed something I shouldn't have (relied by older FIPS code) as you describe, and CI did fail even without longtest.
Altho the error message was cryptic to say the least:

cannot parse row "?   \tcrypto/internal/boring/bbig\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/boring/sig\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/constanttime\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/cryptotest\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/entropy\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/entropy/v1.0.0\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/fips140/v1.0.0-c2097c7c\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/fips140/v1.0.0-c2097c7c/alias\t[no test files]", invalid character '?' looking for beginning of value
cannot parse row "?   \tcrypto/internal/fips140/v1.0.0-c2097c7c/check\t[no test files]", invalid character '?' looking for beginning of value
[...]

I don't think much needs to be done at least for this very particular CL.

[FiloSottile]

Ah, I was misremembering. TryBots do try to compile with old modules, but they don’t run the tests with them.

[mknyszek]

I'm not super enthusiastic about adding even more build configurations, especially ones needing more custom logic in cmd/dist or in the build script.

Sampling builders a bit, it looks like the longtest builders take about 10 minutes longer than regular builds, excluding pending times for shards, which usually occur when there isn't a lot of activity and new machines need to be spun up (but this is also true for the non-longtest builders, hence why I exclude it).

It sounds like the problems with longtest are that it's too frequently flaky, or that the extra time is too long of a wait. This issue is more about working around those problems than fixing them.

If they're truly insurmountable, then sure, we can go down this path; I don't mind doing the work.

But in an attempt to take a more principled stance, I want to take a moment and try to actually make sure that we're making a decent choice here, and that we can't do something better, even in the short term. Here are some alternatives that target these problems with longtest directly:

1. A fix for longtest being flaky would be to always run it before submit, on every change. One thing we've been meaning to leverage on the LUCI side is support for Commit-Queue+2, which will run trybots for you and submit whole stacks of changes (without having to set Auto-Submit+1 on the entire stack). We already intend to have a different policy for Commit-Queue+2 (rebase changes before testing), so it may also make sense to 'upgrade' the standard builders to longtest before submitting. This would not slow down the main feedback loop, only submission, and would catch more issues before they hit CI.
2. There hasn't been activity on build: all.bash takes too long #26473 in 6 years. I don't think we've done a lot to try to improve test execution times in that time period, which is a very long time.

For (1), I think we should just finally prioritize that. It will get us fully integrated with the LUCI presubmit workflow and finally solve some long-standing build issues. I will look into it this week. This might even be enough. If we start running longtest prior to submission always, then the purpose being to catch issues before submission is moot. The current trybot workflow just ends up being for a quick check and a faster feedback loop, and most of the time they will still (hopefully) be enough.

For (2), in December, I built a tool to construct a pprof profile out of our test results: https://cs.opensource.google/go/x/build/+/master:cmd/resultdbpprof/main.go;drc=46693c1d68709b56071dab871071d38c6a73004c.

I collected profiles from both a regular gotip-linux-amd64 builder and a gotip-linux-amd64-longtest builder:

gotip-linux-amd64.pb.gz
gotip-linux-amd64-longtest.pb.gz

In the gotip-linux-amd64-longtest profile, we spend about 20% of all test execution time in packages containing the string "crypto," so it's already true that a significant chunk of the time in longtest is already spent on the crypto tests. (Note: this is a profile, so it doesn't measure latency within packages at all, just total test execution time.) We also spend about 20% of all test execution time in tests matching "cmd/go."

Can we do anything to make the crypto test suite and/or cmd/go test suite faster? Is there anything that sticks out to you in the profile?

I look at the profile and genuinely don't know if there's waste or not. Perhaps everything in there needs to happen in exactly the way that it does, but again, it seems to me like we haven't taken a close look at test execution time in 6 years. I admit I might be completely wrong on that from the crypto side (I know that @matloob has done some work on the cmd/go side, and hit a bit of wall). However, improving test execution time has follow-on benefits. Personally, I would like to take some time this week to improve the test execution time of areas I'm familiar with, like the runtime tests, which is about 10% of the longtest execution time. I see some obvious waste there. We run a lot more under gccheckmark than we need to, for example. I also know that we build too many binaries in the trace tests, though that only accounts for about 1% of the time.

In terms of test sharding across machines, we're doing an OK job these days (it was much worse earlier last year). Of the 4 shards we spin up for longtest, 3 of the builders take 12 minutes to run tests, and one takes 8.4 minutes. That's not a perfect balance, but it's not bad. Distributing 3 minutes would make the tests take 11 minutes instead of 12 minutes, which would be about 10% better. Good, but not life-changing.

As another alternative, more shards might help. Doubling the number of shards would allow for a best-case improvement of 5.5 minutes, which would make longtest closer to race in execution time (only about 2 minutes off, excluding pending times), and we run race everywhere. We probably wouldn't get all the way there, but perhaps close enough for it to be tolerable for presubmit (literally just before submission).

[FiloSottile]

I like this. If we do (1) I think I'd be ok with dropping the crypto/tls longtest TryBot, and I wouldn't mind the FIPS 140-3 module tests running only on longtest, so that would solve both the issues I opened this for.

In fact, once we get that assurance that longtest runs on every CL, I might go around sprinkling -short skips to make regular TryBots faster. IMHO, that's more important than longtest being fast, because TryBots sit in the human feedback loop.

I can look at trimming some crypto longtests, but I am not sure that's the right direction. crypto/... does a lot of slow, systematic tests because crypto primitives lend themselves well to that kind of testing. That work is certainly wasted on changes that don't touch the package. The more principled solution there would be to only run those tests when the package (or its usually very few dependencies) are touched, not to reduce testing. But then again, after (1), does longtest speed matter?

One thing I am very worried about, though, is flakiness. If (1) is just turned on today with no automatic retries, it will make the experience of landing changes even more painful, and I am not convinced that pain will translate to less flakiness, because in my experience the landing experience has been getting worse over the years. (For a non-Googler it looks like: mail, restart TryBots sometimes three or four times, get a Code-Review+2, ensure Gerrit is showing it in go.dev/s/needs-review, wait 1-2 business days which can be 3-4 calendar days for two Googler Code-Review+1, hope it did not develop a merge conflict in the meantime. Adding a cycle of manual retries at the end of that would be awful.)

[mknyszek]

One thing I am very worried about, though, is flakiness. If (1) is just turned on today with no automatic retries, it will make the experience of landing changes even more painful, and I am not convinced that pain will translate to less flakiness, because in my experience the landing experience has been getting worse over the years.

That's a good point about not making the overall experience worse.

FWIW, I think if there's flakiness at the end of the Gerrit workflow that's specifically not associated with the CL to be merged, then that should be no worse than today. Commit-Queue+2 just moves where the retries happen, since you won't lose the Googler +1s when re-running Commit-Queue+2.

However, if the flakiness does involve the CL in question, yeah, that forces a full round-trip which can be painful. We may want to consider running longtest early anyway for some parts of the codebase that are far reaching, or just have a lot of the test suite in longtest, to avoid a full round trip through the Googler +1 queue, in which case we're back to longtest builder speed being important (or we add the longcrypto variant).

I can look into automatic retries, but I worry they might just be a patch on the problem. If too many flakes overlap, the problems just manifest later, once N retries fail. I think what needs to change is the baseline flakiness of longtest. LUCI has a much more sophisticated feature called test exoneration which will mark failing tests as "known failing" allowing the merge to proceed. This doesn't have the same issue of the problem manifesting later, but is much more complex to set up, and is primarily designed for Chromium's scale (their commit rate is way higher, and their builders take much longer to run). I'm reluctant to set that up for our scale, IMO it should be feasible to try and make our test suite less flaky just via culture.

Specifically, my hope is that after everyone's full on the new Commit-Queue+2 flow, that longtest will get less flaky simply because it will be in everyone's face. My hypothesis is that many longtest flakes manifest from commits that don't run longtest at all. (Not all certainly, but many.) I think (?) we do a pretty decent job of keeping non-longtest presubmit less flaky, because the chance that a flake is caught during development is much higher, and this probability will increase for longtest flakes with Commit-Queue+2. I can test this hypothesis, I think, by just looking at all longtest test failures and classifying them as either crypto-related, cmd/go-related, or something else.

The test-with-CL-rebased with Commit-Queue+2 will also improve baseline reliability of all builders a little bit, since there's a smaller chance of a bad merge of a CL based on an old commit.

[mknyszek]

I looked at at the last 450 failing builds in CI of gotip-linux-amd64-longtest and found that my hypothesis is mostly true?

$ bb ls -n 450 -status failure -json -fields=infra golang/ci/gotip-linux-amd64-longtest | jq -r '.infra.resultdb.invocation' | sed 's/invocations\///' | rdb query -u -json | jq -r '[.invocationId, .testResult.testId] | @csv' | sed 's/\.Test[^"]*//' | sort | uniq | sed 's/"[^,]*,//' | sed 's/.*crypto.*/crypto/' | sed 's/.*cmd\/go.*/cmd\/go/' | sed 's/.*runtime.*/runtime/' | sed 's/".*"/other/'| sort | uniq -c

     92 cmd/go
    203 crypto
    201 other
    179 runtime

So >50% of flakes failures on longtest have nothing to do with the two main areas (crypto, cmd/go) running longtest in presubmit. That being said, cmd/go and crypto packages are both disproportionately represented, but so is runtime, where do NOT run longtest.