Add ability for cbuildbot master to synchronize with cbuildbot slaves.

Add ability for cbuildbot master to synchronize with cbuildbot slaves.

TEST=Ran several non-checked in unit tests (will check in later) and tested
with cbuildbot as it is now.
BUG=5989

[sosa, 2010-08-23 05:45:39]

Can you comment on the approach?  It seems I will have to synchronize outside
the cbuildbot scheduler as the structure does not allow for arbitrary junction
points.

Here's the proposed workflow.

PFQ slaves linked together w/ one master slave.

Master slave gives source stamp to other slaves so they all rev.  Slaves /
master build -- and communicate status at completition.  If they fail to
communicate after a specified timeout, they assume the build has failed and they
all fail and the pushes aren't pushed.  Also master fails if one of the slaves
report a fail.  An optimization would be to broadcast the fail to remaining
slaves but I'm not sure if that's desired behavior.

I'll change CL for the scheduler tomorrow.

[sosa, 2010-08-23 05:47:45]

Also sending to Scott

[scottz-goog, 2010-08-23 19:34:59]

For the most part this looks fine, can you give a higher level run down on what
you are solving here? 


On 2010/08/23 05:47:45, sosa wrote:
> Also sending to Scott

[sosa, 2010-08-23 20:41:08]

I'm cleaning up some more and adding comments.  I just re-uploaded removing the
sourcestamp logic because I found I can use Triggers to do the same thing.

The problem with using simply the buildbot to schedule is that it doesn't allow
you to form a barrier.  I want all the incrementals to be lined up (i.e. build
from the same source stamp) and then if all slaves pass - rev the ebuilds.  If
the buildbot had the ability for one slave to check the status of another slave,
I could use that.  Unfortuantely after talking with MA, this is not possible so
I have to handle this myself.

[Mandeep Singh Baines, 2010-08-23 20:52:14]

I feel like the code would be a lot cleaner if all the IP stuff were in one
file. So combine _comm, _master, and _slave.

http://codereview.chromium.org/3165052/diff/1/4
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/3165052/diff/1/4#newcode15
bin/cbuildbot_config.py:15: 'master' : 1,
Use True or False and add this to 'default'.

http://codereview.chromium.org/3165052/diff/5001/6002
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3165052/diff/5001/6002#newcode10
bin/cbuildbot_comm.py:10: BUFFER = 4096
BUFFER_SIZE would be a better name.

http://codereview.chromium.org/3165052/diff/5001/6004
File bin/master.py (right):

http://codereview.chromium.org/3165052/diff/5001/6004#newcode34
bin/master.py:34: sock.connect((slave, _COMM_PORT))
_COMM_PORT is not defined. Shouldn't this be comm.COMM_PORT?

http://codereview.chromium.org/3165052/diff/5001/6004#newcode38
bin/master.py:38: received = sock.recv(_BUFFER)
Shouldn't this be comm.BUFFER.

[sosa, 2010-08-24 01:46:29]

PTAL.

I've addressed all your comments.  I refactored after testing (moving variables
into a common.py) so I missed some constants.  Because of that I've added unit
tests for the two methods to this CL.  Scenarios tested:

1) Slave say ok
2) Master times out from waiting for slave.
3) Slave times out waiting to connect to master.
4) Slave says it failed.

[sosa, 2010-08-24 01:49:18]

I also added a whole bunch of documentation.

[Mandeep Singh Baines, 2010-08-24 16:36:39]

http://codereview.chromium.org/3165052/diff/14001/15002
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3165052/diff/14001/15002#newcode13
bin/cbuildbot_comm.py:13: from cbuildbot import RunCommand
Maybe a separate CL to put RunCommand into a library.

http://codereview.chromium.org/3165052/diff/14001/15002#newcode14
bin/cbuildbot_comm.py:14: from cbuildbot_config import config
This is not used.

http://codereview.chromium.org/3165052/diff/14001/15002#newcode140
bin/cbuildbot_comm.py:140: def CheckSlavesStatus(configuration):
Returns a boolean but the name doesn't imply that. Maybe give a more booleany
name like IsBlahBlah or BlahBlahDone.

http://codereview.chromium.org/3165052/diff/14001/15004
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/3165052/diff/14001/15004#newcode22
bin/cbuildbot_config.py:22: 'master' : 1,
Use a boolean instead of 1 where appropriate. Also, can u add master to the
default config and set to False.

[sosa, 2010-08-24 17:49:45]

PTAL.  Fixed also some style issues with function docstrings.

http://codereview.chromium.org/3165052/diff/14001/15002
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3165052/diff/14001/15002#newcode13
bin/cbuildbot_comm.py:13: from cbuildbot import RunCommand
I'll have another CL after this one to pull RunCommand from cros_mark_as_stable
and this one into one.

On 2010/08/24 16:36:39, Mandeep Singh Baines wrote:
> Maybe a separate CL to put RunCommand into a library.

http://codereview.chromium.org/3165052/diff/14001/15002#newcode14
bin/cbuildbot_comm.py:14: from cbuildbot_config import config
On 2010/08/24 16:36:39, Mandeep Singh Baines wrote:
> This is not used.

Done.

http://codereview.chromium.org/3165052/diff/14001/15002#newcode140
bin/cbuildbot_comm.py:140: def CheckSlavesStatus(configuration):
On 2010/08/24 16:36:39, Mandeep Singh Baines wrote:
> Returns a boolean but the name doesn't imply that. Maybe give a more booleany
> name like IsBlahBlah or BlahBlahDone.

Done.

http://codereview.chromium.org/3165052/diff/14001/15004
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/3165052/diff/14001/15004#newcode22
bin/cbuildbot_config.py:22: 'master' : 1,
On 2010/08/24 16:36:39, Mandeep Singh Baines wrote:
> Use a boolean instead of 1 where appropriate. Also, can u add master to the
> default config and set to False.

Done.

[Mandeep Singh Baines, 2010-08-24 18:44:44]

LGTM

[scottz-goog, 2010-08-24 20:00:39]

Just a few tweaks and questions but general this clean up is awesome!
On 2010/08/24 18:44:44, Mandeep Singh Baines wrote:
> LGTM

[scottz-goog, 2010-08-24 21:57:11]

pushing drafts

http://codereview.chromium.org/3165052/diff/1/2
File bin/cbuildbot.py (right):

http://codereview.chromium.org/3165052/diff/1/2#newcode16
bin/cbuildbot.py:16: import cbuildbot_comm as comm
Why are we renaming this? Generally we don't want to rename any modules as it
makes it harder to identify where a code path came from without searching.

http://codereview.chromium.org/3165052/diff/1/2#newcode196
bin/cbuildbot.py:196: # Am I a master or slave bot.
Does this particular if define whether or not you are a master or slave? It
seems like this comment is misplaced.

http://codereview.chromium.org/3165052/diff/1/5
File bin/master.py (right):

http://codereview.chromium.org/3165052/diff/1/5#newcode1
bin/master.py:1: #!/usr/bin/python
no #! please.

http://codereview.chromium.org/3165052/diff/1/5#newcode93
bin/master.py:93: 
no empty lines.

http://codereview.chromium.org/3165052/diff/1/6
File bin/slave.py (right):

http://codereview.chromium.org/3165052/diff/1/6#newcode7
bin/slave.py:7: import cbuildbot_comm as comm
is there a reason we are renaming this?

http://codereview.chromium.org/3165052/diff/1/6#newcode90
bin/slave.py:90: #if __name__ == "__main__":
Since people have to directly call it to run all this is there any reason to
comment it all out? I would just add a print that says something like "Running
in test mode if you didn't mean to do this ctrl-c" or something like that.

http://codereview.chromium.org/3165052/diff/1/6#newcode104
bin/slave.py:104: 
empty lines.

http://codereview.chromium.org/3165052/diff/23001/10003
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3165052/diff/23001/10003#newcode59
bin/cbuildbot_comm.py:59: print >> sys.stderr, \
please use parens

http://codereview.chromium.org/3165052/diff/23001/10003#newcode81
bin/cbuildbot_comm.py:81: if not slave_config[1]['master'] and \
please use parens

http://codereview.chromium.org/3165052/diff/23001/10003#newcode169
bin/cbuildbot_comm.py:169: except Queue.Empty, qe:
If you aren't going to use qe we can leave it out of here.

http://codereview.chromium.org/3165052/diff/23001/10003#newcode183
bin/cbuildbot_comm.py:183: except Queue.Empty, qe:
again with qe

http://codereview.chromium.org/3165052/diff/23001/10004
File bin/cbuildbot_comm_unittest.py (right):

http://codereview.chromium.org/3165052/diff/23001/10004#newcode66
bin/cbuildbot_comm_unittest.py:66: # Master should check statuses in another
thread.
indent?

http://codereview.chromium.org/3165052/diff/23001/10004#newcode72
bin/cbuildbot_comm_unittest.py:72: if True != return_value:
if not return_value?

http://codereview.chromium.org/3165052/diff/23001/10004#newcode78
bin/cbuildbot_comm_unittest.py:78: 
space

http://codereview.chromium.org/3165052/diff/23001/10005
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/3165052/diff/23001/10005#newcode8
bin/cbuildbot_config.py:8: #               to determine whether to uprev. 
Should not be set if master set.
Can you elaborate on what important is, how is this different than a closer?
Nothing below uses this is that correct for this CL?

[sosa, 2010-08-24 22:41:07]

PTAL. I realized I wasn't testing one code path so I added another unit test.

http://codereview.chromium.org/3165052/diff/23001/10003
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3165052/diff/23001/10003#newcode59
bin/cbuildbot_comm.py:59: print >> sys.stderr, \
On 2010/08/24 21:57:11, scottz wrote:
> please use parens

Done.

http://codereview.chromium.org/3165052/diff/23001/10003#newcode81
bin/cbuildbot_comm.py:81: if not slave_config[1]['master'] and \
On 2010/08/24 21:57:11, scottz wrote:
> please use parens

Done.

http://codereview.chromium.org/3165052/diff/23001/10003#newcode169
bin/cbuildbot_comm.py:169: except Queue.Empty, qe:
On 2010/08/24 21:57:11, scottz wrote:
> If you aren't going to use qe we can leave it out of here.

Done.

http://codereview.chromium.org/3165052/diff/23001/10003#newcode183
bin/cbuildbot_comm.py:183: except Queue.Empty, qe:
On 2010/08/24 21:57:11, scottz wrote:
> again with qe

Done.

http://codereview.chromium.org/3165052/diff/23001/10004
File bin/cbuildbot_comm_unittest.py (right):

http://codereview.chromium.org/3165052/diff/23001/10004#newcode66
bin/cbuildbot_comm_unittest.py:66: # Master should check statuses in another
thread.
On 2010/08/24 21:57:11, scottz wrote:
> indent?

Done.

http://codereview.chromium.org/3165052/diff/23001/10004#newcode72
bin/cbuildbot_comm_unittest.py:72: if True != return_value:
On 2010/08/24 21:57:11, scottz wrote:
> if not return_value? 

Done.

http://codereview.chromium.org/3165052/diff/23001/10004#newcode78
bin/cbuildbot_comm_unittest.py:78: 
On 2010/08/24 21:57:11, scottz wrote:
> space

Done.

http://codereview.chromium.org/3165052/diff/23001/10005
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/3165052/diff/23001/10005#newcode8
bin/cbuildbot_config.py:8: #               to determine whether to uprev. 
Should not be set if master set.
On 2010/08/24 21:57:11, scottz wrote:
> Can you elaborate on what important is, how is this different than a closer?
> Nothing below uses this is that correct for this CL?

Done.

[scottz-goog, 2010-08-27 16:27:49]

LGTM