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

x/tools: tests consistently failing on linux-amd64-longtest builder #32831

Closed
bcmills opened this issue Jun 28, 2019 · 9 comments
Closed

x/tools: tests consistently failing on linux-amd64-longtest builder #32831

bcmills opened this issue Jun 28, 2019 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.13

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 28, 2019

There are two x/tools tests consistently failing in the longtest builder (example).

golang.org/x/tools/go/internal/gcimporter:

haserrors/haserrors.go:2:22: cannot convert "" (untyped string constant) to untyped int
haserrors/haserrors.go:3:18: undeclared name: undefined
cgo failed: [go tool cgo -objdir /workdir/tmp/os_signal_internal_pty_C916411543 -- -I /workdir/tmp/os_signal_internal_pty_C916411543 pty.go]: signal: killed
cgo failed: [go tool cgo -objdir /workdir/tmp/net_C637398552 -- -I /workdir/tmp/net_C637398552 cgo_linux.go cgo_resnew.go cgo_socknew.go cgo_unix.go]: signal: killed
Assembler messages:
Fatal error: can't create /workdir/tmp/os_signal_internal_pty_C916411543/_cgo_.o: No such file or directory
Assembler messages:
Fatal error: can't create /workdir/tmp/net_C637398552/_cgo_.o: No such file or directory
FAIL	golang.org/x/tools/go/internal/gcimporter	27.795s

golang.org/x/tools/go/ssa:

signal: killed
FAIL	golang.org/x/tools/go/ssa	46.257s

Consistently-failing tests can mask real regressions, and the longtest builder is especially important because it is often the only builder running those tests.

CC @ianthehat
@gopherbot gopherbot added this to the Unreleased milestone Jun 28, 2019
@bcmills bcmills added Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 28, 2019
@bcmills bcmills mentioned this issue Jun 28, 2019
Open
@bcmills
Copy link
Contributor Author

bcmills commented Aug 29, 2019 •
edited

The go/ssa failure seems to be fixed, but go/internal/gcimporter is still failing consistently (https://build.golang.org/log/7aad7eab99b2982e013ba0dbf6bb1ff11fe02be2).

@ianthehat, this is one of the few remaining builder failures on release-branch.go1.13 ahead of the 1.13 release (#11811). Could you get someone to look into it?

@bcmills bcmills modified the milestones: Unreleased, Go1.13 Aug 29, 2019
@heschi
Copy link
Contributor

heschi commented Aug 30, 2019

@dmitshur

The gcimporter tests use ~2G and ~2.5G of RAM respectively. I strongly suspect they're getting OOM killed but I don't know how to prove it.

@gopherbot
Copy link

Change https://golang.org/cl/192677 mentions this issue: go/internal/gcimporter: disable gcimporter test on linux-amd64-longtest

@bcmills
Copy link
Contributor Author

bcmills commented Aug 30, 2019

Is it feasible to reduce the footprint of the test? 2–2.5GiB seems awfully high...

@heschi
Copy link
Contributor

heschi commented Aug 30, 2019

That would seem like a good idea, but not as a release blocker.

Looking at a gomote, it has 8 cores and 8G RAM. The go command runs one command per core, and I have no trouble believing it eats up all the memory with 8 concurrent fairly heavy tests. Unless there's some way to reduce the parallelism, skipping them on the builders seems appropriate to me.

@bcmills
Copy link
Contributor Author

bcmills commented Aug 30, 2019

The go command has a -p flag, but I'm not sure how to plumb it down to the builders.

(@dmitshur, @toothrot, @bradfitz?)

@dmitshur
Copy link
Contributor

dmitshur commented Aug 30, 2019 •
edited

It looks like the longtest builder unintentionally has 7.2 GB RAM instead of 14.4. That is half the amount compared to normal linux/amd64 builders that have 15 GB. I'm sending a CL to fix that soon.

@gopherbot
Copy link

Change https://golang.org/cl/192679 mentions this issue: dashboard: change linux-amd64-longtest machine to n1-highcpu-16

gopherbot pushed a commit to golang/build that referenced this issue Aug 30, 2019
@dmitshur
dashboard: change linux-amd64-longtest machine to n1-highcpu-16
3235619 
CL 167638 made a change to add more CPU resources to the longtest
builder, with the intention of going from 4 vCPUs, 15 GB RAM to
16 vCPUs, 14.4 GB RAM.

It used n1-highcpu-8 GCE machine type, which actually has 8 vCPUs
and 7.2 GB RAM.¹ Having less RAM than before wasn't the intention.

Fix that by changing n1-highcpu-8 to n1-highcpu-16, which matches
the comment.

¹ https://cloud.google.com/compute/docs/machine-types

Updates golang/go#32831
Updates golang/go#33986
Updates golang/go#25886

Change-Id: I8426867fe33b3bf86576cb13d0d6113cd87f30c1
Reviewed-on: https://go-review.googlesource.com/c/build/+/192679
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@andybons andybons mentioned this issue Sep 2, 2019
Closed
@bcmills
Copy link
Contributor Author

bcmills commented Sep 3, 2019

Looks like Dmitri's change worked and there were no other longtest failures being masked — the builder seems to be happy now.

@bcmills bcmills closed this as completed Sep 3, 2019
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@dmitshur @codebien
dashboard: change linux-amd64-longtest machine to n1-highcpu-16
2234ae0 
CL 167638 made a change to add more CPU resources to the longtest
builder, with the intention of going from 4 vCPUs, 15 GB RAM to
16 vCPUs, 14.4 GB RAM.

It used n1-highcpu-8 GCE machine type, which actually has 8 vCPUs
and 7.2 GB RAM.¹ Having less RAM than before wasn't the intention.

Fix that by changing n1-highcpu-8 to n1-highcpu-16, which matches
the comment.

¹ https://cloud.google.com/compute/docs/machine-types

Updates golang/go#32831
Updates golang/go#33986
Updates golang/go#25886

Change-Id: I8426867fe33b3bf86576cb13d0d6113cd87f30c1
Reviewed-on: https://go-review.googlesource.com/c/build/+/192679
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Sep 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@dmitshur @bcmills @gopherbot @heschi