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

x/review/git-codereview: failures with "invalid pending output" due to missing Gerrit metadata #50576

Open
bcmills opened this issue Jan 12, 2022 · 7 comments
Labels
NeedsInvestigation release-blocker
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 12, 2022

The x/review/git-codereview tests for the pending command occasionally fail due to missing Gerrit metadata for some of the CLs, particularly on slower builders. Looks like at least a missed error check, since the error that causes the truncated metadata is not present in the logs.

greplogs --dashboard -md -l -e 'invalid pending output' --since=2020-01-01

2022-01-12T15:53:47-b04b288-6e8b7e4/netbsd-386-9_0-n2

    pending_test.go:396: invalid pending output:
        work REVHASH..REVHASH (current branch)
        + REVHASH v14
        + REVHASH v13
        + REVHASH v12
        + REVHASH v11
        + REVHASH v10
        + REVHASH v9
        + REVHASH v8
        + REVHASH v7
        + REVHASH v6
        + REVHASH v5
        + REVHASH v4
        + REVHASH v3
        + REVHASH v2
        + REVHASH v1
        + REVHASH msg
        
        
        want:
        work REVHASH..REVHASH (current branch, all mailed)
        + REVHASH v14 (CL 1234 -2 +1, mailed)
        + REVHASH v13 (CL 1234 -2 +1, mailed)
        + REVHASH v12 (CL 1234 -2 +1, mailed)
        + REVHASH v11 (CL 1234 -2 +1, mailed)
        + REVHASH v10 (CL 1234 -2 +1, mailed)
        + REVHASH v9 (CL 1234 -2 +1, mailed)
        + REVHASH v8 (CL 1234 -2 +1, mailed)
        + REVHASH v7 (CL 1234 -2 +1, mailed)
        + REVHASH v6 (CL 1234 -2 +1, mailed)
        + REVHASH v5 (CL 1234 -2 +1, mailed)
        + REVHASH v4 (CL 1234 -2 +1, mailed)
        + REVHASH v3 (CL 1234 -2 +1, mailed)
        + REVHASH v2 (CL 1234 -2 +1, mailed)
        + REVHASH v1 (CL 1234 -2 +1, mailed)
        + REVHASH msg (CL 1234 -2 +1, mailed, submitted)

2021-12-07T20:14:56-b8ead20-b37a539/solaris-amd64-oraclerel

    pending_test.go:396: invalid pending output:
        work REVHASH..REVHASH (current branch)
        + REVHASH v14 (CL 1234 -2 +1, mailed)
        + REVHASH v13 (CL 1234 -2 +1, mailed)
        + REVHASH v12 (CL 1234 -2 +1, mailed)
        + REVHASH v11 (CL 1234 -2 +1, mailed)
        + REVHASH v10 (CL 1234 -2 +1, mailed)
        + REVHASH v9 (CL 1234 -2 +1, mailed)
        + REVHASH v8 (CL 1234 -2 +1, mailed)
        + REVHASH v7 (CL 1234 -2 +1, mailed)
        + REVHASH v6 (CL 1234 -2 +1, mailed)
        + REVHASH v5 (CL 1234 -2 +1, mailed)
        + REVHASH v4
        + REVHASH v3
        + REVHASH v2
        + REVHASH v1
        + REVHASH msg (CL 1234 -2 +1, submitted)
        
        
        want:
        work REVHASH..REVHASH (current branch, all mailed)
        + REVHASH v14 (CL 1234 -2 +1, mailed)
        + REVHASH v13 (CL 1234 -2 +1, mailed)
        + REVHASH v12 (CL 1234 -2 +1, mailed)
        + REVHASH v11 (CL 1234 -2 +1, mailed)
        + REVHASH v10 (CL 1234 -2 +1, mailed)
        + REVHASH v9 (CL 1234 -2 +1, mailed)
        + REVHASH v8 (CL 1234 -2 +1, mailed)
        + REVHASH v7 (CL 1234 -2 +1, mailed)
        + REVHASH v6 (CL 1234 -2 +1, mailed)
        + REVHASH v5 (CL 1234 -2 +1, mailed)
        + REVHASH v4 (CL 1234 -2 +1, mailed)
        + REVHASH v3 (CL 1234 -2 +1, mailed)
        + REVHASH v2 (CL 1234 -2 +1, mailed)
        + REVHASH v1 (CL 1234 -2 +1, mailed)
        + REVHASH msg (CL 1234 -2 +1, mailed, submitted)

2021-11-05T21:18:28-39ade5b-4c7cafd/dragonfly-amd64

    pending_test.go:396: invalid pending output:
        work REVHASH..REVHASH (current branch)
        + REVHASH v14
        + REVHASH v13
        + REVHASH v12
        + REVHASH v11
        + REVHASH v10
        + REVHASH v9
        + REVHASH v8
        + REVHASH v7
        + REVHASH v6
        + REVHASH v5
        + REVHASH v4
        + REVHASH v3
        + REVHASH v2
        + REVHASH v1
        + REVHASH msg (CL 1234 -2 +1, mailed, submitted)
        
        
        want:
        work REVHASH..REVHASH (current branch, all mailed)
        + REVHASH v14 (CL 1234 -2 +1, mailed)
        + REVHASH v13 (CL 1234 -2 +1, mailed)
        + REVHASH v12 (CL 1234 -2 +1, mailed)
        + REVHASH v11 (CL 1234 -2 +1, mailed)
        + REVHASH v10 (CL 1234 -2 +1, mailed)
        + REVHASH v9 (CL 1234 -2 +1, mailed)
        + REVHASH v8 (CL 1234 -2 +1, mailed)
        + REVHASH v7 (CL 1234 -2 +1, mailed)
        + REVHASH v6 (CL 1234 -2 +1, mailed)
        + REVHASH v5 (CL 1234 -2 +1, mailed)
        + REVHASH v4 (CL 1234 -2 +1, mailed)
        + REVHASH v3 (CL 1234 -2 +1, mailed)
        + REVHASH v2 (CL 1234 -2 +1, mailed)
        + REVHASH v1 (CL 1234 -2 +1, mailed)
        + REVHASH msg (CL 1234 -2 +1, mailed, submitted)

2021-10-06T23:01:01-39ade5b-4ffa2f1/dragonfly-amd64

    pending_test.go:260: invalid pending output:
        work REVHASH..REVHASH (current branch, 1 behind)
        + REVHASH
        	msg
        
        	Change-Id: I123456789
        
        	Files in this change:
        		file
        
        
        want:
        work REVHASH..REVHASH (current branch, all mailed, all submitted, 1 behind)
        + REVHASH http://127.0.0.1:PORT/1234 (mailed, submitted)
        	msg
        
        	Change-Id: I123456789
        
        	Code-Review:
        		+1 Grace Emlin
        		-2 George Opher
        	Other-Label:
        		+2 The Owner
        	Files in this change:
        		file
@gopherbot gopherbot added this to the Unreleased milestone Jan 12, 2022
@bcmills bcmills added the NeedsInvestigation label Jan 12, 2022
@bcmills bcmills removed this from the Unreleased milestone Jan 12, 2022
@bcmills bcmills added this to the Backlog milestone Jan 12, 2022
@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 7, 2022

greplogs --dashboard -md -l -e 'invalid pending output' --since=2022-01-13

2022-01-21T00:39:55-b04b288-32636cd/netbsd-386-9_0

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 7, 2022

Marking as release-blocker via #11811.

Although this hasn't failed on any first-class ports yet, the diversity of ports on which it has occurred (NetBSD, Dragonfly, and Solaris so far) suggests that it is not a platform-specific failure mode.

@bcmills bcmills removed this from the Backlog milestone Apr 7, 2022
@bcmills bcmills added this to the Go1.19 milestone Apr 7, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Apr 11, 2022

That test is using a fake Gerrit server (type gerritServer).
The map it serves from is protected by a sync.Mutex.

The fake server does run on a real localhost network port.
I wonder if we are seeing the effect of flaky network dialing.
I can try dialing a second time if the first one fails, but is that a good fix?

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2022

Change https://go.dev/cl/399626 mentions this issue: git-codereview: try Gerrit HTTP requests three times

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2022

Change https://go.dev/cl/399116 mentions this issue: git-codereview: print about all Gerrit API errors

gopherbot pushed a commit to golang/review that referenced this issue Apr 14, 2022
Trying to chase down a problem with slow builders.
If this gets to be too chatty, we can limit it to builders.

For golang/go#50576.

Change-Id: I16c7818153d2444c897b4946ce7baebbd646ab23
Reviewed-on: https://go-review.googlesource.com/c/review/+/399116
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 22, 2022

#49899 may be related; one of those failures also triggered the new logging. (See #49899 (comment).)

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented May 18, 2022

@rsc What are the next steps here? This is still a release blocker because it's causing builder failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants