launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/log-rotation into lp:launchpad-buildd.

Commit message:
Apply more reasonable log handling, as well as logrotate.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/log-rotation/+merge/258780

Apply more reasonable log handling, as well as logrotate.  This replaces Twisted's horrible default of rotating the file once per million bytes.

I borrowed the RotatableFileLogObserver class from Launchpad, and amended it to use SIGHUP as its reopening signal.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/log-rotation into lp:launchpad-buildd.

=== modified file 'buildd-slave.tac'
--- buildd-slave.tac	2014-08-06 07:10:48 +0000
+++ buildd-slave.tac	2015-05-11 14:38:58 +0000
@@ -5,19 +5,30 @@
 # XXX: dsilvers: 2005/01/21: Currently everything logged in the slave gets
 # passed through to the twistd log too. this could get dangerous/big
 
-from twisted.application import service, strports
-from lpbuildd.slave import XMLRPCBuildDSlave
+from ConfigParser import SafeConfigParser
+import os
+
+from twisted.application import (
+    service,
+    strports,
+    )
+from twisted.scripts.twistd import ServerOptions
+from twisted.web import (
+    resource,
+    server,
+    static,
+    )
+
 from lpbuildd.binarypackage import BinaryPackageBuildManager
 from lpbuildd.livefs import LiveFilesystemBuildManager
-from lpbuildd.sourcepackagerecipe import (
-    SourcePackageRecipeBuildManager)
-from lpbuildd.translationtemplates import (
-    TranslationTemplatesBuildManager)
-
-from twisted.web import server, resource, static
-from ConfigParser import SafeConfigParser
-
-import os
+from lpbuildd.log import RotatableFileLogObserver
+from lpbuildd.slave import XMLRPCBuildDSlave
+from lpbuildd.sourcepackagerecipe import SourcePackageRecipeBuildManager
+from lpbuildd.translationtemplates import TranslationTemplatesBuildManager
+
+
+options = ServerOptions()
+options.parseOptions()
 
 conffile = os.environ.get('BUILDD_SLAVE_CONFIG', 'buildd-slave-example.conf')
 
@@ -32,6 +43,8 @@
 slave.registerBuilder(LiveFilesystemBuildManager, "livefs")
 
 application = service.Application('BuildDSlave')
+application.addComponent(
+    RotatableFileLogObserver(options.get('logfile')), ignoreClass=1)
 builddslaveService = service.IServiceCollection(application)
 
 root = resource.Resource()
@@ -39,7 +52,7 @@
 root.putChild('filecache', static.File(conf.get('slave', 'filecache')))
 slavesite = server.Site(root)
 
-strports.service(slave.slave._config.get("slave","bindport"),
+strports.service(slave.slave._config.get("slave", "bindport"),
                  slavesite).setServiceParent(builddslaveService)
 
 # You can interact with a running slave like this:

=== modified file 'debian/changelog'
--- debian/changelog	2015-04-17 20:49:23 +0000
+++ debian/changelog	2015-05-11 14:38:58 +0000
@@ -1,7 +1,13 @@
 launchpad-buildd (127) UNRELEASED; urgency=low
 
+  [ William Grant ]
   * Refactor lpbuildd.binarypackage tests to be readable.
 
+  [ Colin Watson ]
+  * Apply more reasonable log handling, as well as logrotate.
+    RotatableFileLogObserver class borrowed from Launchpad, amended to use
+    SIGHUP as its reopening signal.
+
  -- William Grant <wgrant@xxxxxxxxxx>  Sat, 18 Apr 2015 06:41:52 +1000
 
 launchpad-buildd (126) trusty; urgency=medium

=== modified file 'debian/control'
--- debian/control	2014-06-25 01:50:28 +0000
+++ debian/control	2015-05-11 14:38:58 +0000
@@ -22,7 +22,7 @@
 Package: python-lpbuildd
 Section: python
 Architecture: all
-Depends: python, python-twisted-core, python-twisted-web, python-apt, ${misc:Depends}
+Depends: python, python-twisted-core, python-twisted-web, python-zope.interface, python-apt, ${misc:Depends}
 Breaks: launchpad-buildd (<< 88)
 Replaces: launchpad-buildd (<< 88)
 Description: Python libraries for a Launchpad buildd slave

=== modified file 'debian/launchpad-buildd.init'
--- debian/launchpad-buildd.init	2011-11-11 13:43:50 +0000
+++ debian/launchpad-buildd.init	2015-05-11 14:38:58 +0000
@@ -92,6 +92,15 @@
     test -r $PIDFILE && kill -TERM $(cat $PIDFILE) || true
 }
 
+#
+#       Function that reloads a buildd slave
+#
+d_reload() {
+    CONF=$1
+    PIDFILE="$PIDROOT"/"$CONF".pid
+    test -r $PIDFILE && kill -HUP $(cat $PIDFILE) || true
+}
+
 CONFS=$(cd $CONFROOT; ls|grep -v "^-"|grep -v "~$")
 
 case "$1" in
@@ -128,8 +137,13 @@
 	sleep 1
 	$0 start
         ;;
+  reload)
+        for conf in $CONFS; do
+            d_reload $conf
+        done
+        ;;
   *)
-        echo "Usage: $SCRIPTNAME {start|stop|restart|force-reload}" >&2
+        echo "Usage: $SCRIPTNAME {start|stop|restart|force-reload|reload}" >&2
         exit 1
         ;;
 esac

=== added file 'debian/launchpad-buildd.logrotate'
--- debian/launchpad-buildd.logrotate	1970-01-01 00:00:00 +0000
+++ debian/launchpad-buildd.logrotate	2015-05-11 14:38:58 +0000
@@ -0,0 +1,13 @@
+/var/log/launchpad-buildd/*.log {
+    rotate 14
+    daily
+    dateext
+    delaycompress
+    compress
+    notifempty
+    missingok
+    create 0644 buildd buildd
+    postrotate
+        service launchpad-buildd reload
+    endscript
+}

=== modified file 'debian/rules'
--- debian/rules	2014-08-04 23:40:28 +0000
+++ debian/rules	2015-05-11 14:38:58 +0000
@@ -45,6 +45,7 @@
 	dh_installchangelogs
 	dh_installinit
 	dh_installcron
+	dh_installlogrotate
 	dh_strip
 	dh_compress
 	dh_fixperms

=== added file 'lpbuildd/log.py'
--- lpbuildd/log.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/log.py	2015-05-11 14:38:58 +0000
@@ -0,0 +1,48 @@
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+import signal
+import sys
+
+from twisted.internet import reactor
+from twisted.python import (
+    log,
+    logfile,
+    )
+from zope.interface import implements
+
+
+class RotatableFileLogObserver:
+    """A log observer that uses a log file and reopens it on SIGHUP."""
+
+    implements(log.ILogObserver)
+
+    def __init__(self, logfilepath):
+        """Set up the logfile and possible signal handler.
+
+        Installs the signal handler for SIGHUP to make the process re-open
+        the log file.
+
+        :param logfilepath: The path to the logfile. If None, stdout is used
+            for logging and no signal handler will be installed.
+        """
+        if logfilepath is None:
+            logFile = sys.stdout
+        else:
+            logFile = logfile.LogFile.fromFullPath(
+                logfilepath, rotateLength=None)
+            # Override if signal is set to None or SIG_DFL (0)
+            if not signal.getsignal(signal.SIGHUP):
+                def signalHandler(signal, frame):
+                    reactor.callFromThread(logFile.reopen)
+                signal.signal(signal.SIGHUP, signalHandler)
+        self.observer = log.FileLogObserver(logFile)
+
+    def __call__(self, eventDict):
+        self.observer.emit(eventDict)