Merge pull request #1544 from tardyp/cleanupdb

Enhancement for master housekeeping

.gitignore

@@ -12,7 +12,7 @@
 MANIFEST
 _trial_temp*
 dist
-/build
+build/
 .build
 *.egg-info
 master/docs/buildbot.html

master/buildbot/buildslave/manager.py

@@ -100,6 +100,9 @@ def reconfigServiceSlaves(self, new_config):
         timer = metrics.Timer("BuildSlaveManager.reconfigServiceSlaves")
         timer.start()
 
+        # first we deconfigure everything to let the slaves register again
+        yield self.master.data.updates.deconfigureAllBuidslavesForMaster(self.master.masterid)
+
         # arrange slaves by name
         old_by_name = dict([(s.slavename, s)
                             for s in list(self)

master/buildbot/data/buildslaves.py

@@ -130,3 +130,12 @@ def buildslaveDisconnected(self, buildslaveid, masterid):
             masterid=masterid)
         bs = yield self.master.data.get(('buildslaves', buildslaveid))
         self.produceEvent(bs, 'disconnected')
+
+    @base.updateMethod
+    def deconfigureAllBuidslavesForMaster(self, masterid):
+        # unconfigure all slaves for this master
+        return self.master.db.buildslaves.deconfigureAllBuidslavesForMaster(
+            masterid=masterid)
+
+    def _masterDeactivated(self, masterid):
+        return self.deconfigureAllBuidslavesForMaster(masterid)

master/buildbot/data/masters.py

@@ -20,6 +20,8 @@
 from buildbot.util import epoch2datetime
 from twisted.internet import defer
 from twisted.internet import reactor
+from twisted.python import log
+
 
 # time, in minutes, after which a master that hasn't checked in will be
 # marked as inactive
@@ -110,7 +112,7 @@ def masterActive(self, name, masterid, _reactor=reactor):
 
     @base.updateMethod
     @defer.inlineCallbacks
-    def expireMasters(self, _reactor=reactor):
+    def expireMasters(self, forceHouseKeeping=False, _reactor=reactor):
         too_old = epoch2datetime(_reactor.seconds() - 60 * EXPIRE_MINUTES)
         masters = yield self.master.db.masters.getMasters()
         for m in masters:
@@ -122,6 +124,8 @@ def expireMasters(self, _reactor=reactor):
                 masterid=m['id'], active=False, _reactor=_reactor)
             if deactivated:
                 yield self._masterDeactivated(m['id'], m['name'])
+            elif forceHouseKeeping:
+                yield self._masterDeactivatedHousekeeping(m['id'], m['name'])
 
     @base.updateMethod
     @defer.inlineCallbacks
@@ -132,8 +136,12 @@ def masterStopped(self, name, masterid):
             yield self._masterDeactivated(masterid, name)
 
     @defer.inlineCallbacks
-    def _masterDeactivated(self, masterid, name):
+    def _masterDeactivatedHousekeeping(self, masterid, name):
+        log.msg("doing housekeeping for master %d %s" % (masterid, name))
+
         # common code for deactivating a master
+        yield self.master.data.rtypes.buildslave._masterDeactivated(
+            masterid=masterid)
         yield self.master.data.rtypes.builder._masterDeactivated(
             masterid=masterid)
         yield self.master.data.rtypes.scheduler._masterDeactivated(
@@ -156,9 +164,9 @@ def _masterDeactivated(self, masterid, name):
                     ('steps', step['stepid'], 'logs'),
                     filters=[resultspec.Filter('complete', 'eq',
                                                [False])])
-                for log in logs:
+                for _log in logs:
                     yield self.master.data.updates.finishLog(
-                        logid=log['logid'])
+                        logid=_log['logid'])
                 yield self.master.data.updates.finishStep(
                     stepid=step['stepid'], results=RETRY, hidden=False)
             # then stop the build itself
@@ -171,6 +179,10 @@ def _masterDeactivated(self, masterid, name):
         yield self.master.db.buildrequests.unclaimBuildRequests(
             brids=[br['buildrequestid'] for br in buildrequests])
 
+    @defer.inlineCallbacks
+    def _masterDeactivated(self, masterid, name):
+        yield self._masterDeactivatedHousekeeping(masterid, name)
+
         self.produceEvent(
             dict(masterid=masterid, name=name, active=False),
             'stopped')

master/buildbot/db/buildslaves.py

@@ -35,8 +35,7 @@ def findBuildslaveId(self, name):
                 info={},
             ))
 
-    def buildslaveConfigured(self, buildslaveid, masterid, builderids):
-
+    def deconfigureAllBuidslavesForMaster(self, masterid):
         def thd(conn):
             # first remove the old configured buildermasterids for this master and slave
             # as sqlalchemy does not support delete with join, we need to do that in 2 queries
@@ -46,25 +45,47 @@ def thd(conn):
             j = j.outerjoin(bm_tbl)
             q = sa.select([cfg_tbl.c.buildermasterid], from_obj=[j])
             q = q.where(bm_tbl.c.masterid == masterid)
-            q = q.where(cfg_tbl.c.buildslaveid == buildslaveid)
             buildermasterids = [row['buildermasterid'] for row in conn.execute(q)]
-            q = cfg_tbl.delete()
-            q = q.where(cfg_tbl.c.buildslaveid == buildslaveid)
-            q = q.where(cfg_tbl.c.buildermasterid.in_(buildermasterids))
-            conn.execute(q)
-            # finally, get the buildermasterids that are configured
+            if buildermasterids:
+                q = cfg_tbl.delete()
+                q = q.where(cfg_tbl.c.buildermasterid.in_(buildermasterids))
+                conn.execute(q)
+
+        return self.db.pool.do(thd)
+
+    def buildslaveConfigured(self, buildslaveid, masterid, builderids):
+        # nothing to add
+        if not builderids:
+            return defer.succeed(None)
+
+        def thd(conn):
+
+            # get the buildermasterids that are configured
+            cfg_tbl = self.db.model.configured_buildslaves
             bm_tbl = self.db.model.builder_masters
             q = sa.select([bm_tbl.c.id], from_obj=[bm_tbl])
             q = q.where(bm_tbl.c.masterid == masterid)
             q = q.where(bm_tbl.c.builderid.in_(builderids))
             buildermasterids = [row['id'] for row in conn.execute(q)]
+
+            # and insert them
+            q = cfg_tbl.insert()
+            try:
+                conn.execute(q,
+                             [{'buildslaveid': buildslaveid, 'buildermasterid': buildermasterid}
+                              for buildermasterid in buildermasterids])
+            except (sa.exc.IntegrityError, sa.exc.ProgrammingError):
+                # if some rows are already present, insert one by one.
+                pass
+            else:
+                return
+
             for buildermasterid in buildermasterids:
+                # insert them one by one
                 q = cfg_tbl.insert()
                 try:
-                    conn.execute(q,
-                                 {'buildslaveid': buildslaveid, 'buildermasterid': buildermasterid})
+                    conn.execute(q, {'buildslaveid': buildslaveid, 'buildermasterid': buildermasterid})
                 except (sa.exc.IntegrityError, sa.exc.ProgrammingError):
-                    # TODO
                     # if the row is already present, silently fail..
                     pass
 

master/buildbot/master.py

@@ -171,12 +171,16 @@ def create_child_services(self):
         self.status = Status(self)
         self.status.setServiceParent(self)
 
+        self.masterHouskeepingTimer = 0
+
         @defer.inlineCallbacks
         def heartbeat():
             if self.masterid is not None:
                 yield self.data.updates.masterActive(name=self.name,
                                                      masterid=self.masterid)
-            yield self.data.updates.expireMasters()
+            # force housekeeping once a day
+            yield self.data.updates.expireMasters((self.masterHouskeepingTimer % (24 * 60)) == 0)
+            self.masterHouskeepingTimer += 1
         self.masterHeartbeatService = internet.TimerService(60, heartbeat)
         self.masterHeartbeatService.setServiceParent(self)
 

master/buildbot/test/fake/fakedata.py

@@ -129,7 +129,7 @@ def masterStopped(self, name, masterid):
         self.thisMasterActive = False
         return defer.succeed(None)
 
-    def expireMasters(self):
+    def expireMasters(self, forceHouseKeeping=False):
         return defer.succeed(None)
 
     @defer.inlineCallbacks
@@ -407,6 +407,10 @@ def buildslaveDisconnected(self, buildslaveid, masterid):
             buildslaveid=buildslaveid,
             masterid=masterid)
 
+    def deconfigureAllBuidslavesForMaster(self, masterid):
+        return self.master.db.buildslaves.deconfigureAllBuidslavesForMaster(
+            masterid=masterid)
+
 
 class FakeDataConnector(object):
     # FakeDataConnector delegates to the real DataConnector so it can get all

master/buildbot/test/fake/fakedb.py

@@ -1346,6 +1346,7 @@ def insertTestData(self, rows):
                     name=row.name,
                     info=row.info)
             elif isinstance(row, ConfiguredBuildslave):
+                row.id = row.buildermasterid * 10000 + row.buildslaveid
                 self.configured[row.id] = dict(
                     buildermasterid=row.buildermasterid,
                     buildslaveid=row.buildslaveid)
@@ -1430,13 +1431,15 @@ def buildslaveConnected(self, buildslaveid, masterid, slaveinfo):
             self.connected[conn_id] = new_conn
         return defer.succeed(None)
 
-    def buildslaveConfigured(self, buildslaveid, masterid, builderids):
+    def deconfigureAllBuidslavesForMaster(self, masterid):
         buildermasterids = [_id for _id, (builderid, mid) in self.db.builders.builder_masters.items()
                             if mid == masterid]
         for k, v in self.configured.items():
-            if v['buildslaveid'] == buildslaveid and v['buildermasterid'] in buildermasterids:
+            if v['buildermasterid'] in buildermasterids:
                 del self.configured[k]
 
+    def buildslaveConfigured(self, buildslaveid, masterid, builderids):
+
         buildermasterids = [_id for _id, (builderid, mid) in self.db.builders.builder_masters.items()
                             if mid == masterid and builderid in builderids]
         if len(buildermasterids) != len(builderids):

master/buildbot/test/unit/test_data_masters.py

@@ -242,7 +242,7 @@ def test_signature_expireMasters(self):
         @self.assertArgSpecMatches(
             self.master.data.updates.expireMasters,  # fake
             self.rtype.expireMasters)  # real
-        def expireMasters(self):
+        def expireMasters(self, forceHouseKeeping=False):
             pass
 
     @defer.inlineCallbacks

master/buildbot/test/unit/test_db_buildslaves.py

@@ -104,6 +104,11 @@ def test_signature_buildslaveConfigured(self):
         def buildslaveConfigured(self, buildslaveid, masterid, builderids):
             pass
 
+    def test_signature_deconfigureAllBuidslavesForMaster(self):
+        @self.assertArgSpecMatches(self.db.buildslaves.deconfigureAllBuidslavesForMaster)
+        def deconfigureAllBuidslavesForMaster(self, masterid):
+            pass
+
     @defer.inlineCallbacks
     def test_findBuildslaveId_insert(self):
         id = yield self.db.buildslaves.findBuildslaveId(name=u"xyz")
@@ -172,7 +177,7 @@ def test_getBuildslave_multiple_connections(self):
                               connected_to=[10, 11], configured_on=[
                                   {'builderid': 20, 'masterid': 10},
                                   {'builderid': 20, 'masterid': 11},
-                              ]))
+                                  ]))
 
     @defer.inlineCallbacks
     def test_getBuildslave_by_name_not_configured(self):
@@ -248,7 +253,7 @@ def test_getBuildslave_with_multiple_masters_masterid(self):
                          dict(id=30, name='zero', slaveinfo={'a': 'b'},
                               configured_on=[
                                   {'masterid': 11, 'builderid': 20},
-                              ], connected_to=[]))
+                                  ], connected_to=[]))
 
     @defer.inlineCallbacks
     def test_getBuildslave_with_multiple_masters_builderid_masterid(self):
@@ -260,7 +265,7 @@ def test_getBuildslave_with_multiple_masters_builderid_masterid(self):
                          dict(id=30, name='zero', slaveinfo={'a': 'b'},
                               configured_on=[
                                   {'masterid': 11, 'builderid': 20},
-                              ], connected_to=[]))
+                                  ], connected_to=[]))
 
     @defer.inlineCallbacks
     def test_getBuildslave_by_name_with_multiple_masters_builderid_masterid(self):
@@ -272,7 +277,7 @@ def test_getBuildslave_by_name_with_multiple_masters_builderid_masterid(self):
                          dict(id=30, name='zero', slaveinfo={'a': 'b'},
                               configured_on=[
                                   {'masterid': 11, 'builderid': 20},
-                              ], connected_to=[]))
+                                  ], connected_to=[]))
 
     @defer.inlineCallbacks
     def test_getBuildslaves_no_config(self):
@@ -440,7 +445,10 @@ def test_buildslaveDisconnected_already_disconnected(self):
     @defer.inlineCallbacks
     def test_buildslaveConfigured(self):
         yield self.insertTestData(self.baseRows + self.multipleMasters)
+
         # should remove builder 21, and add 22
+        yield self.db.buildslaves.deconfigureAllBuidslavesForMaster(masterid=10)
+
         yield self.db.buildslaves.buildslaveConfigured(
             buildslaveid=30, masterid=10, builderids=[20, 22])
 
@@ -450,6 +458,54 @@ def test_buildslaveConfigured(self):
             {'builderid': 20, 'masterid': 10},
             {'builderid': 22, 'masterid': 10}]))
 
+    @defer.inlineCallbacks
+    def test_buildslaveConfiguredTwice(self):
+        yield self.insertTestData(self.baseRows + self.multipleMasters)
+
+        # should remove builder 21, and add 22
+        yield self.db.buildslaves.deconfigureAllBuidslavesForMaster(masterid=10)
+
+        yield self.db.buildslaves.buildslaveConfigured(
+            buildslaveid=30, masterid=10, builderids=[20, 22])
+
+        # configure again (should eat the duplicate insertion errors)
+        yield self.db.buildslaves.buildslaveConfigured(
+            buildslaveid=30, masterid=10, builderids=[20, 21, 22])
+
+        bs = yield self.db.buildslaves.getBuildslave(30)
+        self.assertEqual(sorted(bs['configured_on']), sorted([
+            {'builderid': 20, 'masterid': 11},
+            {'builderid': 20, 'masterid': 10},
+            {'builderid': 21, 'masterid': 10},
+            {'builderid': 22, 'masterid': 10}]))
+
+    @defer.inlineCallbacks
+    def test_nothingConfigured(self):
+        yield self.insertTestData(self.baseRows + self.multipleMasters)
+
+        # should remove builder 21, and add 22
+        yield self.db.buildslaves.deconfigureAllBuidslavesForMaster(masterid=10)
+        yield self.db.buildslaves.buildslaveConfigured(
+            buildslaveid=30, masterid=10, builderids=[])
+
+        # should only keep builder for master 11
+        bs = yield self.db.buildslaves.getBuildslave(30)
+        self.assertEqual(sorted(bs['configured_on']), sorted([
+            {'builderid': 20, 'masterid': 11}]))
+
+    @defer.inlineCallbacks
+    def test_deconfiguredAllSlaves(self):
+        yield self.insertTestData(self.baseRows + self.multipleMasters)
+
+        res = yield self.db.buildslaves.getBuildslaves(masterid=11)
+        self.assertEqual(len(res), 2)
+
+        # should remove all slave configured for masterid 11
+        yield self.db.buildslaves.deconfigureAllBuidslavesForMaster(masterid=11)
+
+        res = yield self.db.buildslaves.getBuildslaves(masterid=11)
+        self.assertEqual(len(res), 0)
+
 
 class RealTests(Tests):
 

master/docs/developer/database.rst

@@ -746,6 +746,15 @@ buildslaves
 
         Record the given buildslave as being configured on the given master and for given builders.
 
+
+    .. py:method:: deconfigureAllBuidslavesForMaster(masterid)
+
+        :param integer masterid: the ID of the master to which it configured
+        :returns: Deferred
+
+        Unregister all the slaves configured to a master for given builders.
+        This shall happen when master disabled or before reconfiguration
+
 changes
 ~~~~~~~
 

master/docs/developer/rtype-buildslave.rst

@@ -149,3 +149,20 @@ All update methods are available as attributes of ``master.data.updates``.
         Record the given buildslave as no longer attached to the given master.
         This method also sends a message indicating the disconnection.
 
+    .. py:method:: buildslaveConfigured(buildslaveid, masterid, builderids)
+
+        :param integer buildslaveid: the ID of the buildslave or None
+        :param integer masterid: the ID of the master to which it configured
+        :param list of integer builderids: the ID of the builders to which it is configured
+        :returns: Deferred
+
+        Record the given buildslave as being configured on the given master and for given builders.
+
+
+    .. py:method:: deconfigureAllBuidslavesForMaster(masterid)
+
+        :param integer masterid: the ID of the master to which it configured
+        :returns: Deferred
+
+        Unregister all the slaves configured to a master for given builders.
+        This shall happen when master disabled or before reconfiguration

www/base/src/app/builders/builders.controller.coffee

@@ -16,7 +16,6 @@ class Builders extends Controller
                     active = true
             return active
         $scope.settings = bbSettingsService.getSettingsGroup("Builders")
-        console.log $scope.settings
         $scope.$watch('settings', ->
             bbSettingsService.save()
         , true)