cleanup the cleanupdb

- add tests
- factorize common code
- fix rebase

Signed-off-by: Pierre Tardy <tardyp@gmail.com>

master/buildbot/db/logs.py

@@ -83,7 +83,7 @@ def getLogBySlug(self, stepid, slug):
         tbl = self.db.model.logs
         return self._getLog((tbl.c.slug == slug) & (tbl.c.stepid == stepid))
 
-    def getLogs(self, stepid):
+    def getLogs(self, stepid=None):
         def thd(conn):
             tbl = self.db.model.logs
             q = tbl.select()
@@ -241,19 +241,18 @@ def compressLog(self, logid):
         def thd(conn):
             # get the set of chunks
             tbl = self.db.model.logchunks
-            q = sa.select([tbl.c.first_line, tbl.c.last_line, sa.func.length(tbl.c.content), tbl.c.compressed])
+            q = sa.select([tbl.c.first_line, tbl.c.last_line, sa.func.length(tbl.c.content),
+                           tbl.c.compressed])
             q = q.where(tbl.c.logid == logid)
             q = q.order_by(tbl.c.first_line)
             rows = conn.execute(q)
             uncompressed_length = 0
             numchunks = 0
             totlength = 0
-            totlines = 0
             for row in rows:
                 if row.compressed == 0:
                     uncompressed_length += row.length_1
                 totlength += row.length_1
-                totlines = row.last_line - row.first_line
                 numchunks += 1
 
             # do nothing if its not worth.
@@ -265,7 +264,7 @@ def thd(conn):
             rows = conn.execute(q)
             wholelog = ""
             for row in rows:
-                wholelog += self.COMPRESSION_BYID[row.compressed](row.content).decode('utf-8') + "\n"
+                wholelog += self.COMPRESSION_BYID[row.compressed]["read"](row.content).decode('utf-8') + "\n"
 
             d = tbl.delete()
             d = d.where(tbl.c.logid == logid)

master/buildbot/scripts/base.py

@@ -17,11 +17,79 @@
 import copy
 import os
 import stat
+import sys
+import traceback
 
 from twisted.python import runtime
+
+from buildbot import config as config_module
+from contextlib import contextmanager
+from twisted.internet import defer
 from twisted.python import usage
 
 
+@contextmanager
+def captureErrors(errors, msg):
+    try:
+        yield
+    except errors as e:
+        print(msg)
+        print(e)
+        defer.returnValue(1)
+
+
+def checkBasedir(config):
+    if not config['quiet']:
+        print("checking basedir")
+
+    if not isBuildmasterDir(config['basedir']):
+        return False
+
+    if runtime.platformType != 'win32':  # no pids on win32
+        if not config['quiet']:
+            print("checking for running master")
+        pidfile = os.path.join(config['basedir'], 'twistd.pid')
+        if os.path.exists(pidfile):
+            print("'%s' exists - is this master still running?" % (pidfile,))
+            return False
+
+    tac = getConfigFromTac(config['basedir'])
+    if tac:
+        if isinstance(tac.get('rotateLength', 0), str):
+            print("ERROR: rotateLength is a string, it should be a number")
+            print("ERROR: Please, edit your buildbot.tac file and run again")
+            print("ERROR: See http://trac.buildbot.net/ticket/2588 for more details")
+            return False
+        if isinstance(tac.get('maxRotatedFiles', 0), str):
+            print("ERROR: maxRotatedFiles is a string, it should be a number")
+            print("ERROR: Please, edit your buildbot.tac file and run again")
+            print("ERROR: See http://trac.buildbot.net/ticket/2588 for more details")
+            return False
+
+    return True
+
+
+def loadConfig(config, configFileName='master.cfg'):
+    if not config['quiet']:
+        print("checking %s" % configFileName)
+
+    try:
+        master_cfg = config_module.MasterConfig.loadConfig(
+            config['basedir'], configFileName)
+    except config_module.ConfigErrors as e:
+        print("Errors loading configuration:")
+
+        for msg in e.errors:
+            print("  " + msg)
+        return
+    except Exception:
+        print("Errors loading configuration:")
+        traceback.print_exc(file=sys.stdout)
+        return
+
+    return master_cfg
+
+
 def isBuildmasterDir(dir):
     def print_error(error_message):
         print("%s\ninvalid buildmaster directory '%s'" % (error_message, dir))

master/buildbot/scripts/cleanupdb.py

@@ -13,14 +13,13 @@
 #
 # Copyright Buildbot Team Members
 
+from __future__ import print_function
 from __future__ import with_statement
 
 import os
 import sys
-import time
 
-from .upgrade_master import checkBasedir
-from .upgrade_master import loadConfig
+from buildbot import config as config_module
 from buildbot import monkeypatches
 from buildbot.db import connector
 from buildbot.master import BuildMaster
@@ -32,11 +31,11 @@
 @defer.inlineCallbacks
 def doCleanupDatabase(config, master_cfg):
     if not config['quiet']:
-        print "cleaning database (%s)" % (master_cfg.db['db_url'])
+        print("cleaning database (%s)" % (master_cfg.db['db_url']))
 
     master = BuildMaster(config['basedir'])
     master.config = master_cfg
-    print master.config.logCompressionMethod
+    print(master.config.logCompressionMethod)
     db = connector.DBConnector(master, basedir=config['basedir'])
 
     yield db.setup(check_version=False, verbose=not config['quiet'])
@@ -49,47 +48,56 @@ def doCleanupDatabase(config, master_cfg):
         i += 1
         if not config['quiet'] and percent != i * 100 / len(res):
             percent = i * 100 / len(res)
-            print " {}%  {} saved".format(percent, saved)
+            print(" {0}%  {1} saved".format(percent, saved))
             saved = 0
             sys.stdout.flush()
 
     if master_cfg.db['db_url'].startswith("sqlite"):
         if not config['quiet']:
-            print "executing sqlite vacuum function..."
+            print("executing sqlite vacuum function...")
 
+        # sqlite vacuum function rebuild the whole database to claim
+        # free disk space back
         def thd(engine):
             r = engine.execute("vacuum;")
             r.close()
         yield db.pool.do(thd)
 
 
 @in_reactor
-@defer.inlineCallbacks
-def cleanupDatabase(config, _noMonkey=False):
-    if not _noMonkey:  # pragma: no cover
+def cleanupDatabase(config, _noMonkey=False):  # pragma: no cover
+    # we separate the actual implementation to protect unit tests
+    # from @in_reactor which stops the reactor
+    if not _noMonkey:
         monkeypatches.patch_all()
+    return _cleanupDatabase(config, _noMonkey=False)
+
+
+@defer.inlineCallbacks
+def _cleanupDatabase(config, _noMonkey=False):
 
-    if not checkBasedir(config):
+    if not base.checkBasedir(config):
         defer.returnValue(1)
         return
 
+    config['basedir'] = os.path.abspath(config['basedir'])
     os.chdir(config['basedir'])
 
-    try:
+    with base.captureErrors((SyntaxError, ImportError),
+                            "Unable to load 'buildbot.tac' from '%s':" % (config['basedir'],)):
         configFile = base.getConfigFileFromTac(config['basedir'])
-    except (SyntaxError, ImportError), e:
-        print "Unable to load 'buildbot.tac' from '%s':" % config['basedir']
-        print e
-        defer.returnValue(1)
-        return
-    master_cfg = loadConfig(config, configFile)
+
+    with base.captureErrors(config_module.ConfigErrors,
+                            "Unable to load '%s' from '%s':" % (configFile, config['basedir'])):
+        master_cfg = base.loadConfig(config, configFile)
+
     if not master_cfg:
         defer.returnValue(1)
         return
 
     yield doCleanupDatabase(config, master_cfg)
 
     if not config['quiet']:
-        print "cleanup complete"
+        print("cleanup complete")
 
     defer.returnValue(0)

master/buildbot/scripts/runner.py

@@ -656,6 +656,8 @@ class CleanupDBOptions(base.BasedirMixin, base.SubcommandOptions):
     subcommandFunction = "buildbot.scripts.cleanupdb.cleanupDatabase"
     optFlags = [
         ["quiet", "q", "Do not emit the commands being run"],
+        # when this command has several maintainance jobs, we should make
+        # them optional here. For now there is only one.
     ]
     optParameters = [
     ]
@@ -665,7 +667,12 @@ def getSynopsis(self):
 
     longdesc = """
     This command takes an existing buildmaster working directory and
-    do some optimization on the database
+    do some optimization on the database.
+
+    This command is frontend for various database maintainance jobs:
+
+    - optimiselogs: This optimization groups logs into bigger chunks
+      to apply higher level of compression.
 
     This command uses the database specified in
     the master configuration file.  If you wish to use a database other than

master/buildbot/scripts/upgrade_master.py

@@ -15,72 +15,16 @@
 from __future__ import print_function
 
 import os
-import sys
-import traceback
 
-from buildbot import config as config_module
 from buildbot import monkeypatches
 from buildbot.db import connector
 from buildbot.master import BuildMaster
 from buildbot.scripts import base
 from buildbot.util import in_reactor
 from twisted.internet import defer
-from twisted.python import runtime
 from twisted.python import util
 
 
-def checkBasedir(config):
-    if not config['quiet']:
-        print("checking basedir")
-
-    if not base.isBuildmasterDir(config['basedir']):
-        return False
-
-    if runtime.platformType != 'win32':  # no pids on win32
-        if not config['quiet']:
-            print("checking for running master")
-        pidfile = os.path.join(config['basedir'], 'twistd.pid')
-        if os.path.exists(pidfile):
-            print("'%s' exists - is this master still running?" % (pidfile,))
-            return False
-
-    tac = base.getConfigFromTac(config['basedir'])
-    if tac:
-        if isinstance(tac.get('rotateLength', 0), str):
-            print("ERROR: rotateLength is a string, it should be a number")
-            print("ERROR: Please, edit your buildbot.tac file and run again")
-            print("ERROR: See http://trac.buildbot.net/ticket/2588 for more details")
-            return False
-        if isinstance(tac.get('maxRotatedFiles', 0), str):
-            print("ERROR: maxRotatedFiles is a string, it should be a number")
-            print("ERROR: Please, edit your buildbot.tac file and run again")
-            print("ERROR: See http://trac.buildbot.net/ticket/2588 for more details")
-            return False
-
-    return True
-
-
-def loadConfig(config, configFileName='master.cfg'):
-    if not config['quiet']:
-        print("checking %s" % configFileName)
-
-    try:
-        master_cfg = config_module.MasterConfig.loadConfig(
-            config['basedir'], configFileName)
-    except config_module.ConfigErrors as e:
-        print("Errors loading configuration:")
-
-        for msg in e.errors:
-            print("  " + msg)
-        return
-    except Exception:
-        print("Errors loading configuration:")
-        traceback.print_exc(file=sys.stdout)
-        return
-
-    return master_cfg
-
-
 def installFile(config, target, source, overwrite=False):
     with open(source, "rt") as f:
         new_contents = f.read()
@@ -139,7 +83,7 @@ def upgradeFiles(config):
             try:
                 print("Notice: Moving %s to %s." % (index_html, root_html))
                 print("        You can (and probably want to) remove it if "
-                    "you haven't modified this file.")
+                      "you haven't modified this file.")
                 os.renames(index_html, root_html)
             except Exception as e:
                 print("Error moving %s to %s: %s" % (index_html, root_html,
@@ -166,7 +110,7 @@ def upgradeMaster(config, _noMonkey=False):
     if not _noMonkey:  # pragma: no cover
         monkeypatches.patch_all()
 
-    if not checkBasedir(config):
+    if not base.checkBasedir(config):
         defer.returnValue(1)
         return
 
@@ -180,7 +124,7 @@ def upgradeMaster(config, _noMonkey=False):
 
         defer.returnValue(1)
         return
-    master_cfg = loadConfig(config, configFile)
+    master_cfg = base.loadConfig(config, configFile)
     if not master_cfg:
         defer.returnValue(1)
         return

master/buildbot/test/fake/fakedb.py

@@ -2042,7 +2042,7 @@ def getLogBySlug(self, stepid, slug):
             return defer.succeed(None)
         return defer.succeed(self._row2dict(row))
 
-    def getLogs(self, stepid):
+    def getLogs(self, stepid=None):
         return defer.succeed([
             self._row2dict(row)
             for row in self.logs.itervalues()

master/buildbot/test/unit/test_db_logs.py

@@ -103,7 +103,7 @@ def getLogBySlug(self, stepid, slug):
 
     def test_signature_getLogs(self):
         @self.assertArgSpecMatches(self.db.logs.getLogs)
-        def getLogs(self, stepid):
+        def getLogs(self, stepid=None):
             pass
 
     def test_signature_getLogLines(self):