Merge pull request #1408 from djmitche/bug3073

Refactor housekeeping when a master stops

master/buildbot/data/masters.py

@@ -14,7 +14,9 @@
 # Copyright Buildbot Team Members
 
 from buildbot.data import base
+from buildbot.data import resultspec
 from buildbot.data import types
+from buildbot.status.results import RETRY
 from buildbot.util import epoch2datetime
 from twisted.internet import defer
 from twisted.internet import reactor
@@ -119,15 +121,15 @@ def expireMasters(self, _reactor):
             deactivated = yield self.master.db.masters.setMasterState(
                 masterid=m['id'], active=False, _reactor=_reactor)
             if deactivated:
-                self._masterDeactivated(m['id'], m['name'])
+                yield self._masterDeactivated(m['id'], m['name'])
 
     @base.updateMethod
     @defer.inlineCallbacks
     def masterStopped(self, name, masterid):
         deactivated = yield self.master.db.masters.setMasterState(
             masterid=masterid, active=False)
         if deactivated:
-            self._masterDeactivated(masterid, name)
+            yield self._masterDeactivated(masterid, name)
 
     @defer.inlineCallbacks
     def _masterDeactivated(self, masterid, name):
@@ -139,7 +141,37 @@ def _masterDeactivated(self, masterid, name):
         yield self.master.data.rtypes.changesource._masterDeactivated(
             masterid=masterid)
 
-        yield self.master.doMasterHouseKeeping(masterid=masterid)
+        # for each build running on that instance..
+        # TODO: this query iterates over *all* builds - potentially a huge list!
+        builds = yield self.master.data.get(('builds',),
+                                            filters=[resultspec.Filter('masterid', 'eq', [masterid]),
+                                                     resultspec.Filter('results', 'eq', [None])])
+        for build in builds:
+            # stop any running steps..
+            steps = yield self.master.data.get(
+                ('builds', build['buildid'], 'steps'),
+                filters=[resultspec.Filter('results', 'eq', [None])])
+            for step in steps:
+                # finish remaining logs for those steps..
+                logs = yield self.master.data.get(
+                    ('steps', step['stepid'], 'logs'),
+                    filters=[resultspec.Filter('complete', 'eq',
+                                               [False])])
+                for log in logs:
+                    yield self.master.data.updates.finishLog(
+                        logid=log['logid'])
+                yield self.master.data.updates.finishStep(
+                    stepid=step['stepid'], results=RETRY)
+            # then stop the build itself
+            yield self.master.data.updates.finishBuild(
+                buildid=build['buildid'], results=RETRY)
+
+        # unclaim all of the build requests owned by the deactivated instance
+        buildrequests = yield self.master.db.buildrequests.getBuildRequests(
+            complete=False, claimed=masterid)
+        yield self.master.db.buildrequests.unclaimBuildRequests(
+            brids=[br['buildrequestid'] for br in buildrequests])
+
         self.produceEvent(
             dict(masterid=masterid, name=name, active=False),
             'stopped')

master/buildbot/db/builds.py

@@ -15,7 +15,6 @@
 
 import sqlalchemy as sa
 
-from buildbot.db import NULL
 from buildbot.db import base
 from buildbot.util import epoch2datetime
 from buildbot.util import json
@@ -152,18 +151,6 @@ def thd(conn):
                          results=results)
         return self.db.pool.do(thd)
 
-    def finishBuildsFromMaster(self, masterid, results, _reactor=reactor):
-        def thd(conn):
-            tbl = self.db.model.builds
-            q = tbl.update()
-            q = q.where(tbl.c.masterid == masterid)
-            q = q.where(tbl.c.results == NULL)
-
-            conn.execute(q,
-                         complete_at=_reactor.seconds(),
-                         results=results)
-        return self.db.pool.do(thd)
-
     def getBuildProperties(self, bid):
         def thd(conn):
             bp_tbl = self.db.model.build_properties

master/buildbot/master.py

@@ -49,7 +49,6 @@
 from buildbot.process.users.manager import UserManagerManager
 from buildbot.schedulers.manager import SchedulerManager
 from buildbot.status.master import Status
-from buildbot.status.results import RETRY
 from buildbot.util import ascii2unicode
 from buildbot.util import check_functional_environment
 from buildbot.util import datetime2epoch
@@ -250,7 +249,9 @@ def sigusr1(*args):
             self.masterid = yield self.db.masters.findMasterId(
                 name=self.name)
 
-            yield self.doMasterHouseKeeping(self.masterid)
+            # mark this master as stopped, in case it crashed before
+            yield self.data.updates.masterStopped(name=self.name,
+                                                  masterid=self.masterid)
 
             for serviceFactory in self.config.services.values():
                 yield serviceFactory.setServiceParent(self)
@@ -285,19 +286,6 @@ def stopService(self):
         log.msg("BuildMaster is stopped")
         self._master_initialized = False
 
-    @defer.inlineCallbacks
-    def doMasterHouseKeeping(self, masterid):
-        # House keeping method, when a master is stopped, disappear or
-        # starts (if it has crashed before)
-        # unclaim the unfinished buildrequest, and finish the unfinished builds
-        buildrequests = yield self.db.buildrequests.getBuildRequests(
-            complete=False, claimed=masterid)
-
-        yield self.db.buildrequests.unclaimBuildRequests(
-            brids=[br['buildrequestid'] for br in buildrequests])
-
-        yield self.db.builds.finishBuildsFromMaster(masterid, RETRY)
-
     def reconfig(self):
         # this method wraps doConfig, ensuring it is only ever called once at
         # a time, and alerting the user if the reconfig takes too long

master/buildbot/test/fake/fakedata.py

@@ -104,13 +104,13 @@ def masterActive(self, name, masterid):
         self.testcase.assertIsInstance(masterid, int)
         if masterid:
             self.testcase.assertEqual(masterid, 1)
-        self.masterActive = True
+        self.thisMasterActive = True
         return defer.succeed(None)
 
     def masterStopped(self, name, masterid):
         self.testcase.assertIsInstance(name, unicode)
         self.testcase.assertEqual(masterid, 1)
-        self.masterActive = False
+        self.thisMasterActive = False
         return defer.succeed(None)
 
     def expireMasters(self):

master/buildbot/test/fake/fakedb.py

@@ -1805,14 +1805,6 @@ def finishBuild(self, buildid, results, _reactor=reactor):
             b['results'] = results
         return defer.succeed(None)
 
-    def finishBuildsFromMaster(self, masterid, results, _reactor=reactor):
-        now = _reactor.seconds()
-        for (id, row) in self.builds.items():
-            if row['masterid'] == masterid and row['results'] == None:
-                row['complete_at'] = now
-                row['results'] = results
-        return defer.succeed(None)
-
     def getBuildProperties(self, bid):
         if bid in self.builds:
             return defer.succeed(self.builds[bid]['properties'])

master/buildbot/test/unit/test_data_masters.py

@@ -16,6 +16,7 @@
 import mock
 
 from buildbot.data import masters
+from buildbot.status.results import RETRY
 from buildbot.test.fake import fakedb
 from buildbot.test.fake import fakemaster
 from buildbot.test.util import endpoint
@@ -152,24 +153,7 @@ class Master(interfaces.InterfaceTests, unittest.TestCase):
     def setUp(self):
         self.master = fakemaster.make_master(wantMq=True, wantDb=True,
                                              wantData=True, testcase=self)
-        # mock out the _masterDeactivated methods this will call
-        self.master.data.rtypes.builder = mock.Mock(
-            spec=self.master.data.rtypes.builder)
-        self.master.data.rtypes.builder._masterDeactivated.side_effect = \
-            lambda masterid: defer.succeed(None)
-
-        self.master.data.rtypes.scheduler = mock.Mock(
-            spec=self.master.data.rtypes.scheduler)
-        self.master.data.rtypes.scheduler._masterDeactivated.side_effect = \
-            lambda masterid: defer.succeed(None)
-
-        self.master.data.rtypes.changesource = mock.Mock(
-            spec=self.master.data.rtypes.changesource)
-        self.master.data.rtypes.changesource._masterDeactivated.side_effect = \
-            lambda masterid: defer.succeed(None)
-
         self.rtype = masters.Master(self.master)
-        self.master.doMasterHouseKeeping = mock.Mock()
 
     def test_signature_masterActive(self):
         @self.assertArgSpecMatches(
@@ -235,13 +219,10 @@ def test_masterStopped(self):
                           last_active=clock.seconds()),
         ])
 
+        self.rtype._masterDeactivated = mock.Mock()
         yield self.rtype.masterStopped(name=u'aname', masterid=13)
-        self.assertEqual(self.master.mq.productions, [
-            (('masters', '13', 'stopped'),
-             dict(masterid=13, name='aname', active=False)),
-        ])
-        self.master.doMasterHouseKeeping. \
-            assert_called_with(masterid=13)
+        self.rtype._masterDeactivated. \
+            assert_called_with(13, 'aname')
 
     @defer.inlineCallbacks
     def test_masterStopped_already(self):
@@ -253,10 +234,9 @@ def test_masterStopped_already(self):
                           last_active=0),
         ])
 
+        self.rtype._masterDeactivated = mock.Mock()
         yield self.rtype.masterStopped(name=u'aname', masterid=13)
-        self.assertEqual(self.master.mq.productions, [])
-        self.master.doMasterHouseKeeping. \
-            assert_not_called()
+        self.rtype._masterDeactivated.assert_not_called()
 
     def test_signature_expireMasters(self):
         @self.assertArgSpecMatches(
@@ -277,6 +257,8 @@ def test_expireMasters(self):
                           last_active=0),
         ])
 
+        self.rtype._masterDeactivated = mock.Mock()
+
         # check after 10 minutes, and see #14 deactivated; #15 gets deactivated
         # by another master, so it's not included here
         clock.advance(600)
@@ -285,15 +267,58 @@ def test_expireMasters(self):
         master = yield self.master.db.masters.getMaster(14)
         self.assertEqual(master, dict(id=14, name='other',
                                       active=False, last_active=None))
-        self.assertEqual(self.master.mq.productions, [
-            (('masters', '14', 'stopped'),
-             dict(masterid=14, name='other', active=False)),
+        self.rtype._masterDeactivated. \
+            assert_called_with(14, 'other')
+
+    @defer.inlineCallbacks
+    def test_masterDeactivated(self):
+        self.master.db.insertTestData([
+            fakedb.Master(id=14, name='other', active=0,
+                          last_active=0),
+
+            # set up a running build with some steps
+            fakedb.Builder(id=77, name='b1'),
+            fakedb.Buildslave(id=13, name='sl'),
+            fakedb.Buildset(id=8822),
+            fakedb.BuildRequest(id=82, builderid=77, buildsetid=8822),
+            fakedb.BuildRequestClaim(brid=82, masterid=14,
+                                     claimed_at=SOMETIME),
+            fakedb.Build(id=13, builderid=77, masterid=14, buildslaveid=13,
+                         buildrequestid=82, number=3, results=None),
+            fakedb.Step(id=200, buildid=13),
+            fakedb.Log(id=2000, stepid=200, num_lines=2),
+            fakedb.LogChunk(logid=2000, first_line=1, last_line=2,
+                            content=u'ab\ncd')
         ])
+
+        # mock out the _masterDeactivated methods this will call
+        for rtype in 'builder', 'scheduler', 'changesource':
+            m = mock.Mock(name='%s._masterDeactivated' % rtype)
+            m.side_effect = lambda masterid: defer.succeed(None)
+            getattr(self.master.data.rtypes, rtype)._masterDeactivated = m
+
+        # and the update methods..
+        for meth in 'finishBuild', 'finishStep', 'finishLog':
+            m = mock.Mock(name='updates.' + meth)
+            m.side_effect = lambda *args, **kwargs: defer.succeed(None)
+            setattr(self.master.data.updates, meth, m)
+
+        yield self.rtype._masterDeactivated(14, 'other')
+
         self.master.data.rtypes.builder._masterDeactivated. \
             assert_called_with(masterid=14)
         self.master.data.rtypes.scheduler._masterDeactivated. \
             assert_called_with(masterid=14)
         self.master.data.rtypes.changesource._masterDeactivated. \
             assert_called_with(masterid=14)
-        self.master.doMasterHouseKeeping. \
-            assert_called_with(masterid=14)
+
+        # see that we finished off that build and its steps and logs
+        updates = self.master.data.updates
+        updates.finishLog.assert_called_with(logid=2000)
+        updates.finishStep.assert_called_with(stepid=200, results=RETRY)
+        updates.finishBuild.assert_called_with(buildid=13, results=RETRY)
+
+        self.assertEqual(self.master.mq.productions, [
+            (('masters', '14', 'stopped'),
+             dict(masterid=14, name='other', active=False)),
+        ])

master/buildbot/test/unit/test_db_builds.py

@@ -109,11 +109,6 @@ def test_signature_finishBuild(self):
         def finishBuild(self, buildid, results):
             pass
 
-    def test_signature_finishBuildsFromMaster(self):
-        @self.assertArgSpecMatches(self.db.builds.finishBuildsFromMaster)
-        def finishBuildsFromMaster(self, masterid, results):
-            pass
-
     def test_signature_getBuildProperties(self):
         @self.assertArgSpecMatches(self.db.builds.getBuildProperties)
         def getBuildProperties(self, bid):
@@ -239,30 +234,6 @@ def test_finishBuild(self):
                                      state_string=u'test',
                                      results=7))
 
-    @defer.inlineCallbacks
-    def test_finishBuildsFromMaster(self):
-        clock = task.Clock()
-        clock.advance(TIME4)
-        yield self.insertTestData(self.backgroundData + self.threeBuilds + [
-            fakedb.Build(
-                id=54, buildrequestid=40, number=50, masterid=89,
-                builderid=77, buildslaveid=13, state_string="test",
-                started_at=TIME1)
-        ])
-        yield self.db.builds.finishBuildsFromMaster(masterid=88, results=7, _reactor=clock)
-        bdict = yield self.db.builds.getBuild(50)
-        validation.verifyDbDict(self, 'builddict', bdict)
-        self.assertEqual(bdict, dict(id=50, number=5, buildrequestid=42,
-                                     masterid=88, builderid=77, buildslaveid=13,
-                                     started_at=epoch2datetime(TIME1),
-                                     complete_at=epoch2datetime(TIME4),
-                                     state_string=u'test',
-                                     results=7))
-        for _id, results in [(50, 7), (51, 7), (52, 5), (54, None)]:
-            bdict = yield self.db.builds.getBuild(_id)
-            validation.verifyDbDict(self, 'builddict', bdict)
-            self.assertEqual(bdict['results'], results)
-
     @defer.inlineCallbacks
     def testgetBuildPropertiesEmpty(self):
         yield self.insertTestData(self.backgroundData + self.threeBuilds)

master/buildbot/test/unit/test_master.py

@@ -209,7 +209,7 @@ def test_startup_ok(self):
 
         @d.addCallback
         def check_started(_):
-            self.assertTrue(self.master.data.updates.masterActive)
+            self.assertTrue(self.master.data.updates.thisMasterActive)
         d.addCallback(lambda _: self.master.stopService())
 
         @d.addCallback
@@ -218,7 +218,7 @@ def check(_):
             self.assertLogged("BuildMaster is running")
 
             # check started/stopped messages
-            self.assertFalse(self.master.data.updates.masterActive)
+            self.assertFalse(self.master.data.updates.thisMasterActive)
         return d
 
     def test_reconfig(self):

master/docs/developer/database.rst

@@ -317,15 +317,6 @@ builds
 
             This update is done unconditionally, even if the build is already finished.
 
-    .. py:method:: finishBuildsFromMaster(masterid, results)
-
-        :param integer masterid: master id
-        :param integer results: build result
-        :returns: Deferred
-
-        Mark the unfinished builds from a given master as finished, with ``complete_at`` set to the current time.
-        This is part of the housekeeping done when a master is lost.
-
     .. py:method:: getBuildProperties(buildid)
 
         :param buildid: build ID

master/docs/developer/rtype-master.rst

@@ -76,6 +76,5 @@ All update methods are available as attributes of ``master.data.updates``.
         :returns: Deferred
 
         Mark this master as inactive.
-        Masters should call this method before completing an expected shutdown.
-        This method will take care of deactivating or removing configuration resources like builders and schedulers as well.
-
+        Masters should call this method before completing an expected shutdown, and on startup.
+        This method will take care of deactivating or removing configuration resources like builders and schedulers as well as marking lost builds and build requests for retry.