cmd/go: -exec prevents test caching

[stevenh]

What version of Go are you using (go version)?

go version go1.10.3 freebsd/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

freebsd/amd64

What did you do?

go test -exec wrap1.sh ./...
go test -exec wrap1.sh ./..

What did you expect to see?

tests should be cached on the second attempt

What did you see instead?

full tests ran

The reason for this is the following line: https://github.com/golang/go/blob/master/src/cmd/go/internal/test/test.go#L1106

        execCmd := work.FindExecCmd()
	if !c.disableCache && len(execCmd) == 0 {
		testlogArg = []string{"-test.testlogfile=" + a.Objdir + "testlog.txt"}
	}

While the documented set of 'cacheable' test flags don't include -exec it seems like this should be supported.

While looking into this issue I also noticed this specific criteria is not mentioned when GODEBUG includes gocachetest=1 so at the least is should be output e.g.

        execCmd := work.FindExecCmd()
        if len(execCmd) != 0 {
                fmt.Fprintf(os.Stderr, "testcache: caching disabled for cmd: %q\n", execCmd)
        } else if !c.disableCache {
                testlogArg = []string{"-test.testlogfile=" + a.Objdir + "testlog.txt"}
        }

With more investigation I also identified that tryCacheWithID actually includes work.ExecCmd in the calculation of the cache.ActionID.

I tried removing the execCmd check in builderRunTest and everything worked as expected, cached results are cached returned if the -exec parameter matches.

[gopherbot]

Change https://golang.org/cl/131415 mentions this issue: cmd/go: Add -exec to 'cacheable' test flags

[bcmills]

Somewhat related to #26790, in that a -exec argument that is modified should ideally invalidate the test results.

(We could, in theory, hash all of the things that the -exec tool touches by running it under strace, at least on Linux, but that seems like a lot of extra complexity for a minor increase in cache fidelity.)

[bcmills]

From the review thread:

If what [you're] asking is does the hash of script itself not just the script name get included then I believe the answer is no

That's exactly my concern. But it's deeper than just hashing the script: if it's, say, a script that sources other scripts, or a tool configured by one of the user's dotfiles, then if we cache it we'll miss changes any changes to the configuration or dependencies.

Moreover, if the wrapper is an interactive tool — such as a debugger — then we really don't want to cache it: the user may well be running it for the interactive effect, not the test result. In my experience, wrappers are generally used for debugging, profiling, and benchmarking, and in all of those cases subsequent runs are useful for collecting more (or different) data.

[stevenh]

That’s true and in those examples but not in all, our use case of using -exec is to ensure the binary has the required permissions to run blocking this from caching the results essentially makes caching unusable.

I don’t think we can make any assumptions about everyone’s use cases so how about we give the user control over caching of results when -exec is used via an environment variable?

[bcmills]

I don’t think we can make any assumptions about everyone’s use cases so how about we give the user control over caching of results when -exec is used via an environment variable?

In general, we would rather have fewer configuration points than more, and we would rather have orthogonal tools rather than integrated ones.

For the specific use-case of checking permissions, it seems that the usual solution is to run the entire build in a container or sandbox (such as gVisor). Is there a reason you need to apply it at the level of individual test runs?

[stevenh]

I don't believe that would help in this case as it needs cap_net_admin so we need run:

setcap cap_net_admin+ep <test_binary>

[rsc]

I think given that we don't know for sure, exec should continue to prevent test caching.

[stevenh]

What do you think about adding an override as this is causing significant pain with our CID/CD pipeline so would be nice to be able to instruct go to allow caching when the user knows its not an issue?

[ianlancetaylor]

@stevenh Sounds like you want the reverse of #23799; perhaps you should add your use case there.

[rsc]

For the setcap problem, can you run the whole go test process tree with that extra capability?

[stevenh]

That wouldn't be easy, I believe you would need to set the capabilities on go binary it self to be able to run it as non-root or call go via a wrapper process which set the capabilities when run as root with the test run as root; both aren't great prospects.

I think allowing the user control over the caching of tests run through a script, would be much more flexible and easier to use. Sure it has provides the user with the ability to shoot them selves in the foot if not done with care but that's to be expected.

Following other options something like providing a env option e.g. GOCACHING=test_exec this could also be extended to allow control over 'cacheable' test flags too if needed; thoughts?

[andybons]

Postponing to 1.13 for now.