458939 - buildbotcustom.steps.test.GraphServerPost needs to use Twisted's http library instead of urlopen()

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[[e-mail] (:bhearsum)]

The moz2 Buildbot hung twice today because it couldn't submit results to the graph server. This caused the tree to burn, forced us to restarted release builds, and required lots of manually intervention on windows slaves because of hung processes.

Like I mentioned in the original bug, we really need to be using Twisted's http library here.

https://bugzilla.mozilla.org/show_bug.cgi?id=433710#c25

https://bugzilla.mozilla.org/show_bug.cgi?id=458937

Comment 2

[[e-mail] (:bhearsum)]

This bug needs to get resolved soon. Again we had something up with graphs.m.o hang the master.

Comment 3

[alice nodelman [:alice] [:anode]]

A quick fix here would be to add an appropriate socket timeout value and then catch the exception that occurs on timeout.  This is how talos deal with graph server issues - it catches the timeout and then reports an appropriate error.

In the case of build machines it could report the error and then continue with normal operation.

Comment 4

[[e-mail] (:bhearsum)]

Unfortunately, urlopen doesn't have a 'timeout' option, and I don't think we want to use socket.setdefaulttimeout() - that would effect the master as a whole (and probably cause an implosion).

If no one else takes this by the new year, I guess I will.

Comment 5

[[e-mail] (:bhearsum)]

I'm testing a fix for this today.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: GraphServerPost, nonblocking

Okay, here's a non-blocking version of this BuildStep. Each getPage Deferred has a callback which does the TinderboxPrint, and an errback to detect failure. The DeferredList fires once all of the getPaged Deferreds have finished, and does the final log printing and determines overall success or failure.

I had to add flunkOnFailure=True to make sure this step burns the overall build.

I couldn't figure out a way to cause the HTTP posts to hang, so I added '127.0.0.1 graphs-stage.mozilla.org' to test the errbacks and failure code. Unfortunately, this means that I couldn't 100% confirm that the hang is gone, but this code is non-blocking, so I'm pretty confident about it.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 358220 [details] [diff] [review]
GraphServerPost, nonblocking

r=me. Two nits, I'd use 
if "RETURN:" in line:
instead of
if line.find("RETURN:") > -1:

The latter is too perlish ;-)

A bit more substance has:

I'd move from self.addCompleteLog() in postFinished() to self.addStdout in doTinderboxPrint(). That gives you incremental logs as the posts succeed. And you don't have to carry around .summary.

Nice to see us moving over to POST on the way, did you make sure that the graphserver takes those allright? At least it looks like that used to be GET requests.

Comment 8

[alice nodelman [:alice] [:anode]]

Comment on attachment 358220 [details] [diff] [review]
GraphServerPost, nonblocking

This looks fine to me as well, has it been running on stage at all?

Comment 9

[[e-mail] (:bhearsum)]

Thanks for the nits Axel, I'll address them and re-post.

Alice, I've run this on staging-master in both the leak test and codesighs scenario without issue.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: GraphServerPost, nonblocking, nits addressed

Okay, I've addressed your nits here Axel. I thought about trying to get rid of postFinished, but I don't think I can without bailing after the first error, which doesn't seem ideal. I think it's cleaner to set overall step status here, anyways.

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 358391 [details] [diff] [review]
GraphServerPost, nonblocking, nits addressed

and yet another nit, in postFailed, I'd self.stdio.addStderr instead of addStdout. Didn't occur to me that we can change that one on the first round of review.

r=me.

Comment 12

[[e-mail] (:bhearsum)]

Attachment: use addStderr in postFailed

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 358408 [details] [diff] [review]
use addStderr in postFailed

changeset:   180:e43363144d85

Comment 14

[[e-mail] (:bhearsum)]

This changed landed without issue.