diff --git a/master/buildbot/config.py b/master/buildbot/config.py index 45f17cfed83..1791def67db 100644 --- a/master/buildbot/config.py +++ b/master/buildbot/config.py @@ -84,7 +84,7 @@ def __init__(self): self.logMaxSize = None self.logMaxTailSize = None self.properties = properties.Properties() - self.mergeRequests = None + self.collapseRequests = None self.codebaseGenerator = None self.prioritizeBuilders = None self.multiMaster = False @@ -129,7 +129,7 @@ def __init__(self): 'db', "db_poll_interval", "db_url", "eventHorizon", "logCompressionLimit", "logCompressionMethod", "logEncoding", "logHorizon", "logMaxSize", "logMaxTailSize", "manhole", - "mergeRequests", "metrics", "mq", "multiMaster", "prioritizeBuilders", + "collapseRequests", "metrics", "mq", "multiMaster", "prioritizeBuilders", "projectName", "projectURL", "properties", "protocols", "revlink", "schedulers", "slavePortnum", "slaves", "status", "title", "titleURL", "user_managers", "validation", 'www' @@ -292,12 +292,12 @@ def copy_str_param(name, alt_key=None): else: self.properties.update(properties, filename) - mergeRequests = config_dict.get('mergeRequests') - if (mergeRequests not in (None, True, False) - and not callable(mergeRequests)): - error("mergeRequests must be a callable, True, or False") + collapseRequests = config_dict.get('collapseRequests') + if (collapseRequests not in (None, True, False) + and not callable(collapseRequests)): + error("collapseRequests must be a callable, True, or False") else: - self.mergeRequests = mergeRequests + self.collapseRequests = collapseRequests codebaseGenerator = config_dict.get('codebaseGenerator') if (codebaseGenerator is not None and @@ -696,7 +696,7 @@ def __init__(self, name=None, slavename=None, slavenames=None, builddir=None, slavebuilddir=None, factory=None, tags=None, category=None, nextSlave=None, nextBuild=None, locks=None, env=None, - properties=None, mergeRequests=None, description=None, + properties=None, collapseRequests=None, description=None, canStartBuild=None): # name is required, and can't start with '_' @@ -783,7 +783,7 @@ def __init__(self, name=None, slavename=None, slavenames=None, if not isinstance(self.env, dict): error("builder's env must be a dictionary") self.properties = properties or {} - self.mergeRequests = mergeRequests + self.collapseRequests = collapseRequests self.description = description @@ -809,8 +809,8 @@ def getConfigDict(self): rv['env'] = self.env if self.properties: rv['properties'] = self.properties - if self.mergeRequests is not None: - rv['mergeRequests'] = self.mergeRequests + if self.collapseRequests is not None: + rv['collapseRequests'] = self.collapseRequests if self.description: rv['description'] = self.description return rv diff --git a/master/buildbot/data/buildsets.py b/master/buildbot/data/buildsets.py index 6fa5f302830..91f09848250 100644 --- a/master/buildbot/data/buildsets.py +++ b/master/buildbot/data/buildsets.py @@ -18,7 +18,9 @@ from buildbot.data import base from buildbot.data import sourcestamps as sourcestampsapi from buildbot.data import types +from buildbot.process.buildrequest import BuildRequestCollapser from buildbot.status.results import FAILURE +from buildbot.status.results import SKIPPED from buildbot.status.results import SUCCESS from buildbot.status.results import WARNINGS from buildbot.util import datetime2epoch @@ -140,6 +142,8 @@ def addBuildset(self, waited_for, scheduler=None, sourcestamps=[], reason=u'', submitted_at=epoch2datetime(submitted_at), parent_buildid=parent_buildid, parent_relationship=parent_relationship) + yield BuildRequestCollapser(self.master, brids.values()).collapse() + # get each of the sourcestamps for this buildset (sequentially) bsdict = yield self.master.db.buildsets.getBuildset(bsid) sourcestamps = [ @@ -185,11 +189,14 @@ def maybeBuildsetComplete(self, bsid, _reactor=reactor): brdicts = yield self.master.db.buildrequests.getBuildRequests(bsid=bsid) - # figure out the overall results of the buildset: SUCCESS unless - # at least one build was not SUCCESS or WARNINGS. + # figure out the overall results of the buildset: + # - SUCCESS unless at least one build was not SUCCESS or WARNINGS. + # - Or SKIPPED cumulative_results = SUCCESS for brdict in brdicts: - if brdict['results'] not in (SUCCESS, WARNINGS): + if brdict['results'] == SKIPPED: + cumulative_results = SKIPPED + elif brdict['results'] not in (SUCCESS, WARNINGS): cumulative_results = FAILURE # get a copy of the buildset diff --git a/master/buildbot/process/botmaster.py b/master/buildbot/process/botmaster.py index c197e9405d0..1936f02f126 100644 --- a/master/buildbot/process/botmaster.py +++ b/master/buildbot/process/botmaster.py @@ -57,10 +57,6 @@ def __init__(self, master): # self.locks holds the real Lock instances self.locks = {} - # self.mergeRequests is the callable override for merging build - # requests - self.mergeRequests = None - self.shuttingDown = False self.lastSlavePortnum = None diff --git a/master/buildbot/process/builder.py b/master/buildbot/process/builder.py index 012eb1c5a6c..3e04238d642 100644 --- a/master/buildbot/process/builder.py +++ b/master/buildbot/process/builder.py @@ -540,26 +540,26 @@ def maybeStartBuild(self, slavebuilder, breqs, _reactor=reactor): # a few utility functions to make the maybeStartBuild a bit shorter and # easier to read - def getMergeRequestsFn(self): - """Helper function to determine which mergeRequests function to use - from L{_mergeRequests}, or None for no merging""" + def getCollapseRequestsFn(self): + """Helper function to determine which collapseRequests function to use + from L{_collapseRequests}, or None for no merging""" # first, seek through builder, global, and the default - mergeRequests_fn = self.config.mergeRequests - if mergeRequests_fn is None: - mergeRequests_fn = self.master.config.mergeRequests - if mergeRequests_fn is None: - mergeRequests_fn = True + collapseRequests_fn = self.config.collapseRequests + if collapseRequests_fn is None: + collapseRequests_fn = self.master.config.collapseRequests + if collapseRequests_fn is None: + collapseRequests_fn = True # then translate False and True properly - if mergeRequests_fn is False: - mergeRequests_fn = None - elif mergeRequests_fn is True: - mergeRequests_fn = Builder._defaultMergeRequestFn + if collapseRequests_fn is False: + collapseRequests_fn = None + elif collapseRequests_fn is True: + collapseRequests_fn = Builder._defaultCollapseRequestFn - return mergeRequests_fn + return collapseRequests_fn - def _defaultMergeRequestFn(self, req1, req2): - return req1.canBeMergedWith(req2) + def _defaultCollapseRequestFn(self, brdict1, brdict2): + return buildrequest.BuildRequest.canBeCollapsed(self.master, brdict1, brdict2) class BuilderControl: diff --git a/master/buildbot/process/buildrequest.py b/master/buildbot/process/buildrequest.py index 0c48211459c..23d696bef63 100644 --- a/master/buildbot/process/buildrequest.py +++ b/master/buildbot/process/buildrequest.py @@ -16,14 +16,103 @@ import calendar from buildbot import interfaces +from buildbot.data import resultspec from buildbot.db import buildrequests from buildbot.process import properties from buildbot.status.results import FAILURE +from buildbot.status.results import SKIPPED from twisted.internet import defer from twisted.python import log from zope.interface import implements +class BuildRequestCollapser(object): + # brids is a list of the new added buildrequests id + # This class is called before generated the 'new' event for the buildrequest + + # Before adding new buildset/buildrequests, we must examine each unclaimed + # buildrequest. + # EG: + # 1. get the list of all unclaimed buildrequests: + # - We must exclude all buildsets which have at least 1 claimed buildrequest + # 2. For each unclaimed buildrequests, if compatible with the new request + # (sourcestamps match, except for revision) Then: + # 2.1. claim it + # 2.2. complete it with result SKIPPED + + def __init__(self, master, brids): + self.master = master + self.brids = brids + + @defer.inlineCallbacks + def _getUnclaimedBRDicts(self, builderid): + # As soon as a buildset contains at least one claimed buildRequest, then it's too late + # to collapse other buildRequests of this buildset. + claim_brdicts = yield self.master.data.get(('builders', + builderid, + 'buildrequests'), + [resultspec.Filter('claimed', + 'eq', + [True])]) + ign_buildSetIDs = [brdict['buildsetid'] for brdict in claim_brdicts] + + # Retrieve the list of Brdicts for all unclaimed builds which "can" be collapsed + unclaim_brdicts = yield self.master.data.get(('builders', + builderid, + 'buildrequests'), + [resultspec.Filter('claimed', + 'eq', + [False]), + resultspec.Filter('buildsetid', + 'ne', + ign_buildSetIDs)]) + # sort by submitted_at, so the first is the oldest + unclaim_brdicts.sort(key=lambda brd: brd['submitted_at']) + defer.returnValue(unclaim_brdicts) + + @defer.inlineCallbacks + def collapse(self): + collapseBRs = [] + + for brid in self.brids: + # Get the BuildRequest object + brdict = yield self.master.data.get(('buildrequests', brid)) + # Retreive the buildername + builderid = brdict['builderid'] + bldrdict = yield self.master.data.get(('builders', builderid)) + # Get the builder object + bldr = self.master.botmaster.builders.get(bldrdict['name']) + # Get the Collapse BuildRequest function (from the configuration) + collapseRequestsFn = bldr.getCollapseRequestsFn() if bldr else None + unclaim_brDicts = yield self._getUnclaimedBRDicts(builderid) + + # short circuit if there is no merging to do + if not collapseRequestsFn or not unclaim_brDicts: + continue + + for unclaim_brDict in unclaim_brDicts: + if unclaim_brDict['buildrequestid'] == brdict['buildrequestid']: + continue + + canCollapse = yield collapseRequestsFn(bldr, brdict, unclaim_brDict) + + if canCollapse is True: + collapseBRs.append(unclaim_brDict) + + brids = [brDict['buildrequestid'] for brDict in collapseBRs] + if collapseBRs: + bsids = list(set([brDict['buildsetid'] for brDict in collapseBRs])) + # Claim the buildrequests + yield self.master.data.updates.claimBuildRequests(brids) + # complete the buildrequest with result SKIPPED. + yield self.master.data.updates.completeBuildRequests(brids, + SKIPPED) + for bsid in bsids: + # Buildset will be complete with result=SKIPPED + yield self.master.data.updates.maybeBuildsetComplete(bsid) + defer.returnValue(brids) + + class TempSourceStamp(object): # temporary fake sourcestamp; attributes are added below @@ -52,7 +141,7 @@ class BuildRequest(object): A rolled-up encapsulation of all of the data relevant to a build request. - This class is used by the C{nextBuild} and C{mergeRequests} configuration + This class is used by the C{nextBuild} and C{collapseRequests} configuration parameters, as well as in starting a build. Construction of a BuildRequest object is a heavyweight process involving a lot of database queries, so it should be avoided where possible. See bug #1894. @@ -150,24 +239,24 @@ def _make_br(cls, brid, brdict, master): defer.returnValue(buildrequest) + @staticmethod @defer.inlineCallbacks - def canBeMergedWith(self, other): + def canBeCollapsed(master, brdict1, brdict2): """ - Returns true if both requests can be merged, via Deferred. + Returns true if both buildrequest can be merged, via Deferred. - This implements Buildbot's default merging strategy. + This implements Buildbot's default collapse strategy. """ - - # short-circuit: if these are for the same buildset, merge away - if self.bsid == other.bsid: + # short-circuit: if these are for the same buildset, collapse away + if brdict1['buildsetid'] == brdict2['buildsetid']: defer.returnValue(True) return # get the buidlsets for each buildrequest - selfBuildsets = yield self.master.data.get( - ('buildsets', str(self.bsid))) - otherBuildsets = yield self.master.data.get( - ('buildsets', str(other.bsid))) + selfBuildsets = yield master.data.get( + ('buildsets', str(brdict1['buildsetid']))) + otherBuildsets = yield master.data.get( + ('buildsets', str(brdict2['buildsetid']))) # extract sourcestamps, as dictionaries by codebase selfSources = dict((ss['codebase'], ss) @@ -175,7 +264,7 @@ def canBeMergedWith(self, other): otherSources = dict((ss['codebase'], ss) for ss in otherBuildsets['sourcestamps']) - # if the sets of codebases do not match, we can't merge + # if the sets of codebases do not match, we can't collapse if set(selfSources) != set(otherSources): defer.returnValue(False) return @@ -194,7 +283,7 @@ def canBeMergedWith(self, other): if selfSS['project'] != otherSS['project']: defer.returnValue(False) return - # anything with a patch won't be merged + # anything with a patch won't be collapsed if selfSS['patch'] or otherSS['patch']: defer.returnValue(False) return diff --git a/master/buildbot/process/buildrequestdistributor.py b/master/buildbot/process/buildrequestdistributor.py index 3c66c46f444..45db04bb9da 100644 --- a/master/buildbot/process/buildrequestdistributor.py +++ b/master/buildbot/process/buildrequestdistributor.py @@ -44,8 +44,6 @@ class BuildChooserBase(object): # The default implementation of this class implements a default # chooseNextBuild() that delegates out to two other functions: # * bc.popNextBuild() - get the next (slave, breq) pair - # * bc.mergeRequests(breq) - perform a merge for this breq and return - # the list of breqs consumed by the merge (including breq itself) def __init__(self, bldr, master): self.bldr = bldr @@ -62,11 +60,7 @@ def chooseNextBuild(self): defer.returnValue((None, None)) return - breqs = yield self.mergeRequests(breq) - for b in breqs: - self._removeBuildRequest(b) - - defer.returnValue((slave, breqs)) + defer.returnValue((slave, [breq])) # Must be implemented by subclass def popNextBuild(self): @@ -74,13 +68,6 @@ def popNextBuild(self): # it's just one breq raise NotImplementedError("Subclasses must implement this!") - # Must be implemented by subclass - def mergeRequests(self, breq): - # Merge the chosen breq with any other breqs that are compatible - # Returns a list of the breqs chosen (and should include the - # original breq as well!) - raise NotImplementedError("Subclasses must implement this!") - # - Helper functions that are generally useful to all subclasses - @defer.inlineCallbacks def _fetchUnclaimedBrdicts(self): @@ -189,8 +176,6 @@ def __init__(self, bldr, master): self.nextBuild = self.bldr.config.nextBuild - self.mergeRequestsFn = self.bldr.getMergeRequestsFn() - @defer.inlineCallbacks def popNextBuild(self): nextBuild = (None, None) @@ -231,10 +216,6 @@ def popNextBuild(self): defer.returnValue(nextBuild) - def mergeRequests(self, breq): - # TODO: merging is not supported in nine for the moment - return defer.succeed([breq]) - @defer.inlineCallbacks def _getNextUnclaimedBuildRequest(self): # ensure the cache is there diff --git a/master/buildbot/test/unit/test_config.py b/master/buildbot/test/unit/test_config.py index b70ca403070..2f38492cf2f 100644 --- a/master/buildbot/test/unit/test_config.py +++ b/master/buildbot/test/unit/test_config.py @@ -53,7 +53,7 @@ logMaxTailSize=None, logMaxSize=None, properties=properties.Properties(), - mergeRequests=None, + collapseRequests=None, prioritizeBuilders=None, protocols={}, multiMaster=False, @@ -471,18 +471,18 @@ def test_load_global_properties_invalid(self): dict(properties='yes')) self.assertConfigError(self.errors, "must be a dictionary") - def test_load_global_mergeRequests_bool(self): - self.do_test_load_global(dict(mergeRequests=False), - mergeRequests=False) + def test_load_global_collapseRequests_bool(self): + self.do_test_load_global(dict(collapseRequests=False), + collapseRequests=False) - def test_load_global_mergeRequests_callable(self): + def test_load_global_collapseRequests_callable(self): callable = lambda: None - self.do_test_load_global(dict(mergeRequests=callable), - mergeRequests=callable) + self.do_test_load_global(dict(collapseRequests=callable), + collapseRequests=callable) - def test_load_global_mergeRequests_invalid(self): + def test_load_global_collapseRequests_invalid(self): self.cfg.load_global(self.filename, - dict(mergeRequests='yes')) + dict(collapseRequests='yes')) self.assertConfigError(self.errors, "must be a callable, True, or False") @@ -1187,7 +1187,7 @@ def test_defaults(self): locks=[], env={}, properties={}, - mergeRequests=None, + collapseRequests=None, description=None) def test_unicode_name(self): @@ -1202,7 +1202,7 @@ def test_args(self): name='b', slavename='s1', slavenames='s2', builddir='bd', slavebuilddir='sbd', factory=self.factory, category='c', nextSlave=lambda: 'ns', nextBuild=lambda: 'nb', locks=['l'], - env=dict(x=10), properties=dict(y=20), mergeRequests='mr', + env=dict(x=10), properties=dict(y=20), collapseRequests='cr', description='buzz') self.assertIdentical(cfg.factory, self.factory) self.assertAttributes(cfg, @@ -1214,7 +1214,7 @@ def test_args(self): locks=['l'], env={'x': 10}, properties={'y': 20}, - mergeRequests='mr', + collapseRequests='cr', description='buzz') def test_getConfigDict(self): @@ -1224,7 +1224,7 @@ def test_getConfigDict(self): name='b', slavename='s1', slavenames='s2', builddir='bd', slavebuilddir='sbd', factory=self.factory, tags=['c'], nextSlave=ns, nextBuild=nb, locks=['l'], - env=dict(x=10), properties=dict(y=20), mergeRequests='mr', + env=dict(x=10), properties=dict(y=20), collapseRequests='cr', description='buzz') self.assertEqual(cfg.getConfigDict(), {'builddir': 'bd', 'tags': ['c'], @@ -1232,7 +1232,7 @@ def test_getConfigDict(self): 'env': {'x': 10}, 'factory': self.factory, 'locks': ['l'], - 'mergeRequests': 'mr', + 'collapseRequests': 'cr', 'name': 'b', 'nextBuild': nb, 'nextSlave': ns, @@ -1241,12 +1241,12 @@ def test_getConfigDict(self): 'slavenames': ['s2', 's1'], }) - def test_getConfigDict_mergeRequests(self): - for mr in (False, lambda a, b, c: False): - cfg = config.BuilderConfig(name='b', mergeRequests=mr, + def test_getConfigDict_collapseRequests(self): + for cr in (False, lambda a, b, c: False): + cfg = config.BuilderConfig(name='b', collapseRequests=cr, factory=self.factory, slavename='s1') self.assertEqual(cfg.getConfigDict(), {'builddir': 'b', - 'mergeRequests': mr, + 'collapseRequests': cr, 'name': 'b', 'slavebuilddir': 'b', 'factory': self.factory, diff --git a/master/buildbot/test/unit/test_data_buildsets.py b/master/buildbot/test/unit/test_data_buildsets.py index 758748bf99a..8beba370925 100644 --- a/master/buildbot/test/unit/test_data_buildsets.py +++ b/master/buildbot/test/unit/test_data_buildsets.py @@ -155,7 +155,7 @@ def setUp(self): project='pr', repository='rep', revision='rev', created_at=89834834), fakedb.Builder(id=42, name='bldr1'), - fakedb.Builder(id=43, name='bldr1'), + fakedb.Builder(id=43, name='bldr2'), ]) SS234_DATA = {'branch': u'br', 'codebase': u'cb', 'patch': None, diff --git a/master/buildbot/test/unit/test_process_builder.py b/master/buildbot/test/unit/test_process_builder.py index 50c5e0459fe..2e52e39f91f 100644 --- a/master/buildbot/test/unit/test_process_builder.py +++ b/master/buildbot/test/unit/test_process_builder.py @@ -158,8 +158,8 @@ def test_maybeStartBuild_failsToStart(self): self.assertBuildsStarted([('slave', [10])]) @defer.inlineCallbacks - def do_test_getMergeRequestsFn(self, builder_param=None, - global_param=None, expected=0): + def do_test_getCollapseRequestsFn(self, builder_param=None, + global_param=None, expected=0): cble = lambda: None builder_param = builder_param == 'callable' and cble or builder_param global_param = global_param == 'callable' and cble or global_param @@ -168,38 +168,38 @@ def do_test_getMergeRequestsFn(self, builder_param=None, if builder_param is None: yield self.makeBuilder() else: - yield self.makeBuilder(mergeRequests=builder_param) + yield self.makeBuilder(collapseRequests=builder_param) - self.master.config.mergeRequests = global_param + self.master.config.collapseRequests = global_param - fn = self.bldr.getMergeRequestsFn() + fn = self.bldr.getCollapseRequestsFn() - if fn == builder.Builder._defaultMergeRequestFn: + if fn == builder.Builder._defaultCollapseRequestFn: fn = "default" elif fn is cble: fn = 'callable' self.assertEqual(fn, expected) - def test_getMergeRequestsFn_defaults(self): - self.do_test_getMergeRequestsFn(None, None, "default") + def test_getCollapseRequestsFn_defaults(self): + self.do_test_getCollapseRequestsFn(None, None, "default") - def test_getMergeRequestsFn_global_True(self): - self.do_test_getMergeRequestsFn(None, True, "default") + def test_getCollapseRequestsFn_global_True(self): + self.do_test_getCollapseRequestsFn(None, True, "default") - def test_getMergeRequestsFn_global_False(self): - self.do_test_getMergeRequestsFn(None, False, None) + def test_getCollapseRequestsFn_global_False(self): + self.do_test_getCollapseRequestsFn(None, False, None) - def test_getMergeRequestsFn_global_function(self): - self.do_test_getMergeRequestsFn(None, 'callable', 'callable') + def test_getCollapseRequestsFn_global_function(self): + self.do_test_getCollapseRequestsFn(None, 'callable', 'callable') - def test_getMergeRequestsFn_builder_True(self): - self.do_test_getMergeRequestsFn(True, False, "default") + def test_getCollapseRequestsFn_builder_True(self): + self.do_test_getCollapseRequestsFn(True, False, "default") - def test_getMergeRequestsFn_builder_False(self): - self.do_test_getMergeRequestsFn(False, True, None) + def test_getCollapseRequestsFn_builder_False(self): + self.do_test_getCollapseRequestsFn(False, True, None) - def test_getMergeRequestsFn_builder_function(self): - self.do_test_getMergeRequestsFn('callable', None, 'callable') + def test_getCollapseRequestsFn_builder_function(self): + self.do_test_getCollapseRequestsFn('callable', None, 'callable') # other methods diff --git a/master/buildbot/test/unit/test_process_buildrequest.py b/master/buildbot/test/unit/test_process_buildrequest.py index 3338163cf54..125905dd754 100644 --- a/master/buildbot/test/unit/test_process_buildrequest.py +++ b/master/buildbot/test/unit/test_process_buildrequest.py @@ -13,12 +13,172 @@ # # Copyright Buildbot Team Members +import mock + from buildbot.process import buildrequest from buildbot.test.fake import fakedb from buildbot.test.fake import fakemaster +from twisted.internet import defer from twisted.trial import unittest +class TestBuildRequestCollapser(unittest.TestCase): + + @defer.inlineCallbacks + def setUp(self): + self.master = fakemaster.make_master(testcase=self, + wantData=True, + wantDb=True) + self.master.botmaster = mock.Mock(name='botmaster') + self.master.botmaster.builders = {} + self.builders = {} + self.bldr = yield self.createBuilder('A', builderid=77) + + @defer.inlineCallbacks + def createBuilder(self, name, builderid=None): + if builderid is None: + b = fakedb.Builder(name=name) + yield self.master.db.insertTestData([b]) + builderid = b.id + + bldr = mock.Mock(name=name) + bldr.name = name + bldr.master = self.master + self.master.botmaster.builders[name] = bldr + self.builders[name] = bldr + bldr.getCollapseRequestsFn = lambda: False + + defer.returnValue(bldr) + + def tearDown(self): + pass + + @defer.inlineCallbacks + def do_request_collapse(self, rows, brids, exp): + yield self.master.db.insertTestData(rows) + brCollapser = buildrequest.BuildRequestCollapser(self.master, brids) + self.assertEqual(exp, (yield brCollapser.collapse())) + + def test_collapseRequests_no_other_request(self): + + def collapseRequests_fn(builder, brdict1, brdict2): + # Allow all requests + self.fail("Should never be called") + return True + + self.bldr.getCollapseRequestsFn = lambda: collapseRequests_fn + rows = [ + fakedb.Builder(id=77, name='A'), + fakedb.SourceStamp(id=234, codebase='A'), + fakedb.Change(changeid=14, codebase='A', sourcestampid=234), + fakedb.Buildset(id=30, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), + fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + ] + self.do_request_collapse(rows, [19], []) + + def test_collapseRequests_no_collapse(self): + + def collapseRequests_fn(builder, brdict1, brdict2): + # Fail all collapse attempts + return False + + rows = [ + fakedb.Builder(id=77, name='A'), + fakedb.SourceStamp(id=234, codebase='C'), + fakedb.Buildset(id=30, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), + fakedb.SourceStamp(id=235, codebase='C'), + fakedb.Buildset(id=31, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=235, buildsetid=31), + fakedb.SourceStamp(id=236, codebase='C'), + fakedb.Buildset(id=32, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=236, buildsetid=32), + fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=20, buildsetid=31, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=21, buildsetid=32, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + ] + + self.bldr.getCollapseRequestsFn = lambda: collapseRequests_fn + self.do_request_collapse(rows, [21], []) + + def test_collapseRequests_collapse_all(self): + + def collapseRequests_fn(builder, brdict1, brdict2): + # collapse all attempts + return True + + rows = [ + fakedb.Builder(id=77, name='A'), + fakedb.SourceStamp(id=234, codebase='C'), + fakedb.Buildset(id=30, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), + fakedb.SourceStamp(id=235, codebase='C'), + fakedb.Buildset(id=31, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=235, buildsetid=31), + fakedb.SourceStamp(id=236, codebase='C'), + fakedb.Buildset(id=32, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=236, buildsetid=32), + fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=20, buildsetid=31, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=21, buildsetid=32, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + ] + + self.bldr.getCollapseRequestsFn = lambda: collapseRequests_fn + self.do_request_collapse(rows, [21], [19, 20]) + + def test_collapseRequests_collapse_default(self): + + def collapseRequests_fn(builder, brdict1, brdict2): + return buildrequest.BuildRequest.canBeCollapsed(builder.master, brdict1, brdict2) + + rows = [ + fakedb.Builder(id=77, name='A'), + fakedb.SourceStamp(id=234, codebase='C'), + fakedb.Buildset(id=30, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), + fakedb.SourceStamp(id=235, codebase='C'), + fakedb.Buildset(id=31, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=235, buildsetid=31), + fakedb.SourceStamp(id=236, codebase='C'), + fakedb.SourceStamp(id=237, codebase='A'), + fakedb.Buildset(id=32, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=236, buildsetid=32), + fakedb.Buildset(id=33, reason='foo', + submitted_at=1300305712, results=-1), + fakedb.BuildsetSourceStamp(sourcestampid=237, buildsetid=33), + fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=20, buildsetid=31, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=21, buildsetid=32, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + fakedb.BuildRequest(id=22, buildsetid=33, builderid=77, + priority=13, submitted_at=1300305712, results=-1), + ] + + self.bldr.getCollapseRequestsFn = lambda: collapseRequests_fn + self.do_request_collapse(rows, [22], []) + self.do_request_collapse(rows, [21], [19, 20]) + + class TestBuildRequest(unittest.TestCase): def test_fromBrdict(self): @@ -239,7 +399,7 @@ def check(_): d.addCallback(check) return d - def test_canBeMergedWith_different_codebases_raises_error(self): + def test_canBeCollapsed_different_codebases_raises_error(self): """ This testcase has two buildrequests Request Change Codebase Revision Comment ---------------------------------------------------------------------- @@ -249,7 +409,7 @@ def test_canBeMergedWith_different_codebases_raises_error(self): Merge cannot be performd and raises error: Merging requests requires both requests to have the same codebases """ - brs = [] # list of buildrequests + brDicts = [] # list of buildrequests dictionnary master = fakemaster.make_master(testcase=self, wantData=True, wantDb=True) master.db.insertTestData([ @@ -279,18 +439,14 @@ def test_canBeMergedWith_different_codebases_raises_error(self): # use getBuildRequest to minimize the risk from changes to the format # of the brdict d = master.db.buildrequests.getBuildRequest(288) - d.addCallback(lambda brdict: - buildrequest.BuildRequest.fromBrdict(master, brdict)) - d.addCallback(lambda br: brs.append(br)) + d.addCallback(lambda brdict: brDicts.append(brdict)) d.addCallback(lambda _: master.db.buildrequests.getBuildRequest(289)) - d.addCallback(lambda brdict: - buildrequest.BuildRequest.fromBrdict(master, brdict)) - d.addCallback(lambda br: brs.append(br)) - d.addCallback(lambda _: brs[0].canBeMergedWith(brs[1])) + d.addCallback(lambda brdict: brDicts.append(brdict)) + d.addCallback(lambda _: buildrequest.BuildRequest.canBeCollapsed(master, brDicts[0], brDicts[1])) - def check(canbeMergedWith): - self.assertEqual(canbeMergedWith, False) + def check(canBeCollapsed): + self.assertEqual(canBeCollapsed, False) d.addCallback(check) return d diff --git a/master/buildbot/test/unit/test_process_buildrequestdistributor.py b/master/buildbot/test/unit/test_process_buildrequestdistributor.py index 52d4ff9544e..3dd38948c3e 100644 --- a/master/buildbot/test/unit/test_process_buildrequestdistributor.py +++ b/master/buildbot/test/unit/test_process_buildrequestdistributor.py @@ -126,7 +126,7 @@ def maybeStartBuild(slave, builds): return d bldr.maybeStartBuild = maybeStartBuild bldr.canStartWithSlavebuilder = lambda _: True - bldr.getMergeRequestsFn = lambda: False + bldr.getCollapseRequestsFn = lambda: False bldr.slaves = [] bldr.getAvailableSlaves = lambda: [s for s in bldr.slaves if s.isAvailable()] @@ -429,7 +429,6 @@ def test_no_slavebuilders(self): @defer.inlineCallbacks def test_limited_by_slaves(self): - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=11, buildsetid=11, builderid=77, @@ -442,8 +441,6 @@ def test_limited_by_slaves(self): @defer.inlineCallbacks def test_sorted_by_submit_time(self): - self.master.config.mergeRequests = False - # same as "limited_by_slaves" but with rows swapped self.addSlaves({'test-slave1': 1}) rows = self.base_rows + [ @@ -457,7 +454,6 @@ def test_sorted_by_submit_time(self): @defer.inlineCallbacks def test_limited_by_available_slaves(self): - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 0, 'test-slave2': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=10, buildsetid=11, builderid=77, @@ -471,8 +467,6 @@ def test_limited_by_available_slaves(self): @defer.inlineCallbacks def test_slow_db(self): # test what happens if the "getBuildRequests" fetch takes a "long time" - - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1}) # wrap to simulate a "long" db access @@ -501,7 +495,6 @@ def test_limited_by_canStartBuild(self): that limits the possible options.""" self.bldr.config.nextSlave = nth_slave(-1) - self.master.config.mergeRequests = False slaves_attempted = [] @@ -554,7 +547,6 @@ def test_limited_by_canStartBuild_deferreds(self): * patch using SkipSlavesThatCantGetLock to disable the 'rejectedSlaves' feature""" self.bldr.config.nextSlave = nth_slave(-1) - self.master.config.mergeRequests = False slaves_attempted = [] @@ -601,7 +593,6 @@ def _canStartBuild(slave, breq): @defer.inlineCallbacks def test_limited_by_canStartWithSlavebuilder(self): self.bldr.config.nextSlave = nth_slave(-1) - self.master.config.mergeRequests = False slaves_attempted = [] @@ -624,7 +615,6 @@ def _canStartWithSlavebuilder(slavebuilder): @defer.inlineCallbacks def test_unlimited(self): self.bldr.config.nextSlave = nth_slave(-1) - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=10, buildsetid=11, builderid=77, @@ -647,7 +637,6 @@ def maybeStartBuild(slave, builds): return defer.succeed(False) self.bldr.maybeStartBuild = maybeStartBuild - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=10, buildsetid=11, builderid=77, @@ -672,7 +661,6 @@ def maybeStartBuild(slave, builds, _fail=[False]): return defer.succeed(ret) self.bldr.maybeStartBuild = maybeStartBuild - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=10, buildsetid=11, builderid=77, @@ -696,7 +684,6 @@ def maybeStartBuild(slave, builds, _fail=[False]): @defer.inlineCallbacks def test_limited_by_requests(self): self.bldr.config.nextSlave = nth_slave(1) - self.master.config.mergeRequests = False self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) rows = self.base_rows + [ fakedb.BuildRequest(id=11, buildsetid=11, builderid=77), @@ -852,7 +839,6 @@ def nextSlave(bldr, lst): def do_test_nextBuild(self, nextBuild, exp_choice=None): self.bldr.config.nextSlave = nth_slave(-1) self.bldr.config.nextBuild = nextBuild - self.master.config.mergeRequests = False rows = self.make_slaves(4) @@ -899,90 +885,3 @@ def nextBuild(bldr, lst): result = self.do_test_nextBuild(nextBuild) self.assertEqual(1, len(self.flushLoggedErrors(RuntimeError))) return result - - # merge tests - @defer.inlineCallbacks - def test_mergeRequest_no_other_request(self): - """ Test if builder test for codebases in requests """ - self.bldr.config.nextSlave = nth_slave(0) - # set up all of the data required for a BuildRequest object - rows = [ - fakedb.Builder(id=77, name='A'), - fakedb.SourceStamp(id=234, codebase='A'), - fakedb.Change(changeid=14, codebase='A', sourcestampid=234), - fakedb.Buildset(id=30, reason='foo', - submitted_at=1300305712, results=-1), - fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), - fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, - priority=13, submitted_at=1300305712, results=-1), - ] - - self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) - - def mergeRequests_fn(builder, breq, other): - # Allow all requests - self.fail("Should never be called") - return True - self.bldr.getMergeRequestsFn = lambda: mergeRequests_fn - - # check if the request remains the same - yield self.do_test_maybeStartBuildsOnBuilder(rows=rows, - exp_claims=[19], - exp_builds=[ - ('test-slave1', [19]), - ]) - - @defer.inlineCallbacks - def test_mergeRequests_no_merging(self): - """ Test if builder test for codebases in requests """ - self.bldr.config.nextSlave = nth_slave(0) - # set up all of the data required for a BuildRequest object - rows = [ - fakedb.Builder(id=77, name='A'), - fakedb.SourceStamp(id=234, codebase='C'), - fakedb.Buildset(id=30, reason='foo', - submitted_at=1300305712, results=-1), - fakedb.BuildsetSourceStamp(sourcestampid=234, buildsetid=30), - fakedb.SourceStamp(id=235, codebase='C'), - fakedb.Buildset(id=31, reason='foo', - submitted_at=1300305712, results=-1), - fakedb.BuildsetSourceStamp(sourcestampid=235, buildsetid=31), - fakedb.SourceStamp(id=236, codebase='C'), - fakedb.Buildset(id=32, reason='foo', - submitted_at=1300305712, results=-1), - fakedb.BuildsetSourceStamp(sourcestampid=236, buildsetid=32), - fakedb.BuildRequest(id=19, buildsetid=30, builderid=77, - priority=13, submitted_at=1300305712, results=-1), - fakedb.BuildRequest(id=20, buildsetid=31, builderid=77, - priority=13, submitted_at=1300305712, results=-1), - fakedb.BuildRequest(id=21, buildsetid=32, builderid=77, - priority=13, submitted_at=1300305712, results=-1), - ] - - self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) - - def mergeRequests_fn(builder, breq, other): - # Fail all merge attempts - return False - self.bldr.getMergeRequestsFn = lambda: mergeRequests_fn - - # check if all are merged - yield self.do_test_maybeStartBuildsOnBuilder(rows=rows, - exp_claims=[19, 20], - exp_builds=[ - ('test-slave1', [19]), - ('test-slave2', [20]), - ]) - - @defer.inlineCallbacks - def test_mergeRequests_fails(self): - def mergeRequests_fn(*args): - raise RuntimeError("xx") - self.bldr.getMergeRequestsFn = lambda: mergeRequests_fn - - self.addSlaves({'test-slave1': 1, 'test-slave2': 1}) - rows = self.base_rows + [ - fakedb.BuildRequest(id=11, buildsetid=11, buildername="bldr"), - ] - yield self.do_test_maybeStartBuildsOnBuilder(rows=rows, - exp_claims=[], exp_builds=[]) diff --git a/master/contrib/SimpleConfig.py b/master/contrib/SimpleConfig.py index eeaa5545bcb..5ca56f401b6 100644 --- a/master/contrib/SimpleConfig.py +++ b/master/contrib/SimpleConfig.py @@ -191,7 +191,7 @@ def __init__(self, # Later we can make this smarter and disable merging just changes # which are at tags, or enable merging just on builders that are # way too slow and don't mind missing a tag - self['mergeRequests'] = False + self['collapseRequests'] = False # PORT NUMBERS # It's hard to keep port numbers straight for multiple projects, diff --git a/master/docs/developer/config.rst b/master/docs/developer/config.rst index a4c2b17aede..1470aecdf0f 100644 --- a/master/docs/developer/config.rst +++ b/master/docs/developer/config.rst @@ -88,10 +88,10 @@ described in :ref:`developer-Reconfiguration`. A :py:class:`~buildbot.process.properties.Properties` instance containing global properties, from :bb:cfg:`properties`. - .. py:attribute:: mergeRequests + .. py:attribute:: collapseRequests - A callable, or True or False, describing how to merge requests; from - :bb:cfg:`mergeRequests`. + A callable, or True or False, describing how to collapse requests; from + :bb:cfg:`collapseRequests`. .. py:attribute:: prioritizeBuilders @@ -257,9 +257,9 @@ Builder Configuration The builder's properties, as a dictionary. - .. py:attribute:: mergeRequests + .. py:attribute:: collapseRequests - The builder's mergeRequests callable. + The builder's collapseRequests callable. .. py:attribute:: description diff --git a/master/docs/manual/cfg-builders.rst b/master/docs/manual/cfg-builders.rst index c1609ba63dd..39e3083a908 100644 --- a/master/docs/manual/cfg-builders.rst +++ b/master/docs/manual/cfg-builders.rst @@ -107,9 +107,9 @@ Other optional keys may be set on each ``BuilderConfig``: .. index:: Builds; merging -``mergeRequests`` - Specifies how build requests for this builder should be merged. - See :ref:`Merging-Build-Requests`, below. +``collapseRequests`` + Specifies how build requests for this builder should be collapsed. + See :ref:`Collapsing-Build-Requests`, below. .. index:: Properties; builder @@ -123,18 +123,18 @@ Other optional keys may be set on each ``BuilderConfig``: .. index:: Builds; merging -.. _Merging-Build-Requests: +.. _Collapsing-Build-Requests: -Merging Build Requests -~~~~~~~~~~~~~~~~~~~~~~ +Collapsing Build Requests +~~~~~~~~~~~~~~~~~~~~~~~~~ -When more than one build request is available for a builder, Buildbot can "merge" the requests into a single build. +When more than one build request is available for a builder, Buildbot can "collapse" the requests into a single build. This is desirable when build requests arrive more quickly than the available slaves can satisfy them, but has the drawback that separate results for each build are not available. Requests are only candidated for a merge if both requests have exactly the same :ref:`codebases`. -This behavior can be controlled globally, using the :bb:cfg:`mergeRequests` parameter, and on a per-:class:`Builder` basis, using the ``mergeRequests`` argument to the :class:`Builder` configuration. -If ``mergeRequests`` is given, it completely overrides the global configuration. +This behavior can be controlled globally, using the :bb:cfg:`collapseRequests` parameter, and on a per-:class:`Builder` basis, using the ``collapseRequests`` argument to the :class:`Builder` configuration. +If ``collapseRequests`` is given, it completely overrides the global configuration. For either configuration parameter, a value of ``True`` (the default) causes buildbot to merge BuildRequests that have "compatible" source stamps. Source stamps are compatible if: @@ -146,7 +146,7 @@ Source stamps are compatible if: A configuration value of ``False`` indicates that requests should never be merged. The configuration value can also be a callable, specifying a custom merging function. -See :ref:`Merge-Request-Functions` for details. +See :ref:`Collapse-Request-Functions` for details. .. index:: Builds; priority diff --git a/master/docs/manual/cfg-global.rst b/master/docs/manual/cfg-global.rst index d105c4710f9..4411d47341d 100644 --- a/master/docs/manual/cfg-global.rst +++ b/master/docs/manual/cfg-global.rst @@ -336,7 +336,7 @@ The available caches are: c['buildCacheSize'] = 15 -.. bb:cfg:: mergeRequests +.. bb:cfg:: collapseRequests .. index:: Builds; merging @@ -345,12 +345,12 @@ Merging Build Requests .. code-block:: python - c['mergeRequests'] = True + c['collapseRequests'] = True -This is a global default value for builders' :bb:cfg:`mergeRequests` parameter, and controls the merging of build requests. +This is a global default value for builders' :bb:cfg:`collapseRequests` parameter, and controls the merging of build requests. This parameter can be overridden on a per-builder basis. -See :ref:`Merging-Build-Requests` for the allowed values for this parameter. +See :ref:`Collapsing-Build-Requests` for the allowed values for this parameter. .. note:: diff --git a/master/docs/manual/customization.rst b/master/docs/manual/customization.rst index b914d2f7e03..957c56afcf0 100644 --- a/master/docs/manual/customization.rst +++ b/master/docs/manual/customization.rst @@ -26,27 +26,27 @@ For example, the following will generate a builder for each of a range of suppor factory=f, slavenames=pytest_slaves)) -.. _Merge-Request-Functions: +.. _Collapse-Request-Functions: -Merge Request Functions ------------------------ +Collapse Request Functions +-------------------------- -.. index:: Builds; merging +.. index:: Builds; collapsing .. warning: This section is no longer accurate in Buildbot 0.9.x -The logic Buildbot uses to decide which build request can be merged can be customized by providing a Python function (a callable) instead of ``True`` or ``False`` described in :ref:`Merging-Build-Requests`. +The logic Buildbot uses to decide which build request can be merged can be customized by providing a Python function (a callable) instead of ``True`` or ``False`` described in :ref:`Collapsing-Build-Requests`. The callable will be invoked with three positional arguments: a :class:`Builder` object and two :class:`BuildRequest` objects. It should return true if the requests can be merged, and False otherwise. For example:: - def mergeRequests(builder, req1, req2): + def collapseRequests(builder, req1, req2): "any requests with the same branch can be merged" return req1.source.branch == req2.source.branch - c['mergeRequests'] = mergeRequests + c['collapseRequests'] = collapseRequests In many cases, the details of the :class:`SourceStamp`\s and :class:`BuildRequest`\s are important. In this example, only :class:`BuildRequest`\s with the same "reason" are merged; thus developers forcing builds for different reasons will see distinct builds. @@ -56,18 +56,18 @@ Note, in particular, that this function returns a Deferred as of Buildbot-0.9.0. :: @defer.inlineCallbacks - def mergeRequests(builder, req1, req2): + def collapseRequests(builder, req1, req2): if (yield req1.source.canBeMergedWith(req2.source)) and req1.reason == req2.reason: defer.returnValue(True) else: defer.returnValue(False) - c['mergeRequests'] = mergeRequests + c['collapseRequests'] = collapseRequests -If it's necessary to perform some extended operation to determine whether two requests can be merged, then the ``mergeRequests`` callable may return its result via Deferred. +If it's necessary to perform some extended operation to determine whether two requests can be merged, then the ``collapseRequests`` callable may return its result via Deferred. Note, however, that the number of invocations of the callable is proportional to the square of the request queue length, so a long-running callable may cause undesirable delays when the queue length grows. For example:: - def mergeRequests(builder, req1, req2): + def collapseRequests(builder, req1, req2): d = defer.gatherResults([ getMergeInfo(req1.source.revision), getMergeInfo(req2.source.revision), @@ -76,7 +76,7 @@ For example:: return info1 == info2 d.addCallback(process) return d - c['mergeRequests'] = mergeRequests + c['collapseRequests'] = collapseRequests .. _Builder-Priority-Functions: diff --git a/master/docs/manual/introduction.rst b/master/docs/manual/introduction.rst index b1b031cab5c..5bf6ca34802 100644 --- a/master/docs/manual/introduction.rst +++ b/master/docs/manual/introduction.rst @@ -137,7 +137,7 @@ Once a :class:`SlaveBuilder` is available, the :class:`Builder` pulls one or mor These requests are merged into a single :class:`Build` instance, which includes the :class:`SourceStamp` that describes what exact version of the source code should be used for the build. The :class:`Build` is then randomly assigned to a free :class:`SlaveBuilder` and the build begins. -The behaviour when :class:`BuildRequest`\s are merged can be customized, :ref:`Merging-Build-Requests`. +The behaviour when :class:`BuildRequest`\s are merged can be customized, :ref:`Collapsing-Build-Requests`. .. _Status-Delivery-Architecture: