Control cleanup tasks from DBConnector

This moves the change pruning logic out of db.changes.  Fixes #1906.

master/buildbot/db/changes.py

@@ -19,10 +19,8 @@
 
 from buildbot.util import json
 import sqlalchemy as sa
-from twisted.python import log
 from buildbot.changes.changes import Change
 from buildbot.db import base
-from buildbot import util
 
 class ChangesConnectorComponent(base.DBConnectorComponent):
     """
@@ -139,10 +137,6 @@ def thd(conn):
 
             return change
         d = self.db.pool.do(thd)
-        # prune changes, if necessary
-        d.addCallback(lambda _ : self._prune_changes(change.number))
-        # return the change
-        d.addCallback(lambda _ : change)
         return d
 
     def getChangeInstance(self, changeid):
@@ -232,16 +226,6 @@ def _flush_cache(self):
 
     # utility methods
 
-    _last_prune = 0
-    def _prune_changes(self, last_added_changeid):
-        # this is an expensive operation, so only do it once per minute, in case
-        # addChange is called frequently
-        if not self.changeHorizon or self._last_prune > util.now() - 60:
-            return
-        self._last_prune = util.now()
-        log.msg("pruning changes")
-        self.pruneChanges(self.changeHorizon)
-
     def pruneChanges(self, changeHorizon):
         def thd(conn):
             changes_tbl = self.db.model.changes

master/buildbot/db/connector.py

@@ -15,7 +15,8 @@
 
 import base64
 
-from twisted.python import threadable
+from twisted.python import threadable, log
+from twisted.application import internet, service
 from buildbot.db import enginestrategy
 
 from buildbot import util
@@ -55,7 +56,7 @@ def connect(self):
     def stop(self):
         pass
 
-class DBConnector(object):
+class DBConnector(service.MultiService):
     """
     The connection between Buildbot and its backend database.  This is
     generally accessible as master.db, but is also used during upgrades.
@@ -68,7 +69,12 @@ class DBConnector(object):
     synchronized = ["notify", "_end_operation"] # TODO: remove
     MAX_QUERY_TIMES = 1000
 
+    # Period, in seconds, of the cleanup task.  This master will perform
+    # periodic cleanup actions on this schedule.
+    CLEANUP_PERIOD = 3600
+
     def __init__(self, master, db_url, basedir):
+        service.MultiService.__init__(self)
         self.master = master
         self.basedir = basedir
         "basedir for this master - used for upgrades"
@@ -108,6 +114,10 @@ def __init__(self, master, db_url, basedir):
         self.state = state.StateConnectorComponent(self)
         "L{buildbot.db.state.StateConnectorComponent} instance"
 
+        self.cleanup_timer = internet.TimerService(self.CLEANUP_PERIOD, self.doCleanup)
+        self.cleanup_timer.setServiceParent(self)
+
+        self.changeHorizon = None # default value; set by master
 
     def _getCurrentTime(self): # TODO: remove
         # this is a seam for use in testing
@@ -678,5 +688,14 @@ def _txn_getChangeNumberedNow(self, t, changeid):
         c.number = changeid
         return c
 
+    def doCleanup(self):
+        """
+        Perform any periodic database cleanup tasks.
+
+        @returns: Deferred
+        """
+        d = self.changes.pruneChanges(self.changeHorizon)
+        d.addErrback(log.err, 'while pruning changes')
+        return d
 
 threadable.synchronize(DBConnector)

master/buildbot/master.py

@@ -288,7 +288,9 @@ def do_load(_):
                 raise KeyError("must have a 'slaves' key")
 
             if changeHorizon is not None:
-                self.change_svc.changeHorizon = changeHorizon
+                self.change_svc.changeHorizon = changeHorizon # TODO: remove
+                # TODO: get this from master.config.changeHorizon
+                self.db.changeHorizon = changeHorizon
 
             change_source = config.get('change_source', [])
             if isinstance(change_source, (list, tuple)):
@@ -516,6 +518,7 @@ def loadDatabase(self, db_url, db_poll_interval=None):
             return
 
         self.db = connector.DBConnector(self, db_url, self.basedir)
+        self.db.setServiceParent(self)
         if self.changeCacheSize:
             pass # TODO: set this in self.db.changes, or in self.config?
         self.db.start()

master/buildbot/test/unit/test_db_connector.py

@@ -15,12 +15,13 @@
 
 import os
 import mock
+from twisted.internet import defer, reactor
 from twisted.trial import unittest
 from buildbot.db import connector
 from buildbot.test.util import db
 from buildbot.test.fake import fakedb
 
-class DBConnector_Basic(db.RealDatabaseMixin, unittest.TestCase):
+class DBConnector(db.RealDatabaseMixin, unittest.TestCase):
     """
     Basic tests of the DBConnector class - all start with an empty DB
     """
@@ -95,3 +96,28 @@ def do_test(_):
                       ('bar', 'other prop', 'BS')])
         d.addCallback(do_test)
         return d
+
+    def test_doCleanup(self):
+        # patch out all of the cleanup tasks; note that we can't patch dbc.doCleanup
+        # directly, since it's already been incorporated into the TimerService
+        cleanups = set([])
+        def pruneChanges(*args):
+            cleanups.add('pruneChanges')
+            return defer.succeed(None)
+        self.dbc.changes.pruneChanges = pruneChanges
+
+        self.dbc.startService()
+
+        d = defer.Deferred()
+        def check(_):
+            self.assertEqual(cleanups, set(['pruneChanges']))
+        d.addCallback(check)
+
+        # shut down the service lest we leave an unclean reactor
+        d.addCallback(lambda _ : self.dbc.stopService())
+
+        # take advantage of the fact that TimerService runs immediately; otherwise, we'd need to find
+        # a way to inject task.Clock into it
+        reactor.callLater(0.001, d.callback, None)
+
+        return d