Navigation Menu

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

Jump to bottom

cmd/vendor/github.com/google/pprof/internal/driver: TestHttpsInsecure flaky under load #24611

Closed
josharian opened this issue Mar 30, 2018 · 8 comments
Closed

cmd/vendor/github.com/google/pprof/internal/driver: TestHttpsInsecure flaky under load #24611

josharian opened this issue Mar 30, 2018 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.11

Comments

@josharian
Copy link
Contributor

This was also reported as #22594, where it was assumed to be specific to some builders. But the issue is that the test is flaky when the underlying machine is heavily loaded.

I can reproduce it on my speedy darwin/amd64 laptop with:

$ cd cmd/vendor/github.com/google/pprof/internal/driver
$ go test -c
$ stress -p 64 ./driver.test

Result: "849 runs so far, 24 failures", which is > 2% failure rate.

I also saw this happen during a regular local all.bash, and also earlier on a trybot run--which confused me into wasting time, assuming I had introduced a bug with a compiler change.

Also, the test takes 10-15s, 5s of which is spent burning CPU. :(

We should figure out how to make this non-flaky, and ideally also less resource-intensive. Or at the very least we should skip it in testing.Short mode, and leave it to be run once #12508 is fixed.

cc @hyangah (?)

Marking as Go 1.11 because I am frustrated with the number of flaky tests we have.
@josharian josharian added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels Mar 30, 2018
@josharian josharian added this to the Go1.11 milestone Mar 30, 2018
@mvdan
Copy link
Member

mvdan commented Mar 31, 2018

@josharian what's the exact failure message you get? I wonder if this is google/pprof#328.

Also see google/pprof#343 - the pprof people are aware of the number of flakes they tend to introduce given our vast variety of builders.

@mvdan mvdan mentioned this issue Mar 31, 2018
Closed
@aalexand
Copy link
Contributor

@josharian Curious - what's the stress tool you used? https://people.seas.harvard.edu/~apw/stress doesn't seem to have the -p flag.

@mvdan
Copy link
Member

mvdan commented Mar 31, 2018

If it's a tool people in the Go team use, and it's not part of the Go tree, it's very usually in the x repos: https://godoc.org/golang.org/x/tools/cmd/stress

aalexand added a commit to aalexand/pprof that referenced this issue Mar 31, 2018
@aalexand
Switch to use goroutine profile in TestHttpsInsecure.
75eb1f8 
Trying to test against /debug/pprof/profile has been giving some flakes
and trouble so far (google#146, google#253, google#328, golang/go#24611, golang/go#22594). While
some of the discussions the failures triggered appear to be useful (such
as whether CPU profiles are expected to be working on Windows XP or
not), that kind of testing is not really in the scope for this
particular test. This change switches the test to use goroutine profile
instead which is hopefully much less dependent on the environment. The
change also makes the test much faster to run.
@aalexand aalexand mentioned this issue Mar 31, 2018
Merged
nolanmar511 pushed a commit to google/pprof that referenced this issue Apr 2, 2018
@aalexand @nolanmar511
Switch to use goroutine profile in TestHttpsInsecure. (#350)
36d5638 
Trying to test against /debug/pprof/profile has been giving some flakes
and trouble so far (#146, #253, #328, golang/go#24611, golang/go#22594). While
some of the discussions the failures triggered appear to be useful (such
as whether CPU profiles are expected to be working on Windows XP or
not), that kind of testing is not really in the scope for this
particular test. This change switches the test to use goroutine profile
instead which is hopefully much less dependent on the environment. The
change also makes the test much faster to run.
@josharian
Copy link
Contributor Author

What is the procedure for re-vendoring? I'm excited to get the upsteam fix pulled in.

@mvdan
Copy link
Member

mvdan commented Apr 3, 2018

@josharian last time I just did a copy manually, careful to not introduce unwanted parts of the repository. I wondered if a tool was being used to keep track of vendored packages, but apparently it's never been the case.

@mvdan
Copy link
Member

mvdan commented Apr 3, 2018

I'll submit a CL now, since I was also wanting to re-vendor to pull in flakiness fixes: #24405

@mvdan
Copy link
Member

mvdan commented Apr 3, 2018

Actually, we are still blocked on an issue - #24508. I'll ping the pprof people in case they are going to have a fix soon, but otherwise I'll sync the vendored copy with that test disabled.

@mvdan mvdan self-assigned this Apr 4, 2018
@gopherbot
Copy link

Change https://golang.org/cl/105275 mentions this issue: cmd/vendor/.../pprof: update to 0f7d9ba1

@golang golang locked and limited conversation to collaborators Apr 6, 2019
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@josharian @aalexand @mvdan @gopherbot