Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: -exec prevents test caching #27207

Open
stevenh opened this issue Aug 24, 2018 · 12 comments
Open

cmd/go: -exec prevents test caching #27207

stevenh opened this issue Aug 24, 2018 · 12 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@stevenh
Copy link
Contributor

stevenh commented Aug 24, 2018

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.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 24, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 24, 2018
@gopherbot
Copy link

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

@bcmills
Copy link
Contributor

bcmills commented Sep 11, 2018

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
Copy link
Contributor

bcmills commented Sep 12, 2018

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
Copy link
Contributor Author

stevenh commented Sep 12, 2018

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
Copy link
Contributor

bcmills commented Sep 13, 2018

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
Copy link
Contributor Author

stevenh commented Sep 13, 2018

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
Copy link
Contributor

rsc commented Sep 26, 2018

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

@stevenh
Copy link
Contributor Author

stevenh commented Sep 27, 2018

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
Copy link
Contributor

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

@rsc
Copy link
Contributor

rsc commented Oct 24, 2018

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

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 24, 2018
@stevenh
Copy link
Contributor Author

stevenh commented Oct 25, 2018

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 andybons removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 5, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Dec 5, 2018
@andybons
Copy link
Member

andybons commented Dec 5, 2018

Postponing to 1.13 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants