launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06229
[Merge] lp:~allenap/maas/log-and-oops-services into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/log-and-oops-services into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/log-and-oops-services/+merge/91291
The getRotatableLogFileObserver() and setUpOOPSHandler() functions
could only be run once in a process, which makes them a little hard to
rest. So, I have changed them into Service()s, so they can be started
and stopped by Twisted, and by us in tests.
--
https://code.launchpad.net/~allenap/maas/log-and-oops-services/+merge/91291
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/log-and-oops-services into lp:maas.
=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py 2012-02-01 15:14:09 +0000
+++ src/provisioningserver/plugin.py 2012-02-02 15:19:18 +0000
@@ -8,75 +8,32 @@
unicode_literals,
)
-import signal
-import sys
+__metaclass__ = type
+__all__ = []
from amqpclient import AMQFactory
-import oops
-from oops_datedir_repo import DateDirRepo
-from oops_twisted import (
- Config as oops_config,
- defer_publisher,
- OOPSObserver,
+from provisioningserver.remote import Provisioning
+from provisioningserver.services import (
+ LogService,
+ OOPSService,
)
import setproctitle
-from twisted.application.internet import TCPClient
+from twisted.application.internet import (
+ TCPClient,
+ TCPServer,
+ )
from twisted.application.service import (
IServiceMaker,
MultiService,
)
-from twisted.internet import reactor
from twisted.plugin import IPlugin
from twisted.python import (
log,
usage,
)
-from twisted.python.log import (
- addObserver,
- FileLogObserver,
- )
-from twisted.python.logfile import LogFile
from zope.interface import implements
-__metaclass__ = type
-__all__ = []
-
-
-def getRotatableLogFileObserver(filename):
- """Setup a L{LogFile} for the given application."""
- if filename != '-':
- logfile = LogFile.fromFullPath(
- filename, rotateLength=None, defaultMode=0644)
-
- def signal_handler(sig, frame):
- reactor.callFromThread(logfile.reopen)
- signal.signal(signal.SIGUSR1, signal_handler)
- else:
- logfile = sys.stdout
- return FileLogObserver(logfile)
-
-
-def setUpOOPSHandler(options, logfile):
- """Add OOPS handling based on the passed command line options."""
- config = oops_config()
-
- # Add the oops publisher that writes files in the configured place
- # if the command line option was set.
-
- if options["oops-dir"]:
- repo = DateDirRepo(options["oops-dir"])
- config.publishers.append(
- defer_publisher(oops.publish_new_only(repo.publish)))
-
- if options["oops-reporter"]:
- config.template['reporter'] = options["oops-reporter"]
-
- observer = OOPSObserver(config, logfile.emit)
- addObserver(observer.emit)
- return observer
-
-
class Options(usage.Options):
"""Command line options for the provisioning server."""
@@ -127,11 +84,15 @@
if _set_proc_title:
setproctitle.setproctitle("maas provisioning service")
- logfile = getRotatableLogFileObserver(options["logfile"])
- setUpOOPSHandler(options, logfile)
-
services = MultiService()
+ logging_service = LogService(options["logfile"])
+ logging_service.setServiceParent(services)
+
+ oops_service = OOPSService(
+ logging_service, options["oops-dir"], options["oops-reporter"])
+ oops_service.setServiceParent(services)
+
broker_port = options["brokerport"]
broker_host = options["brokerhost"]
broker_user = options["brokeruser"]
@@ -150,6 +111,7 @@
cb_connected, cb_disconnected, cb_failed)
client_service = TCPClient(
broker_host, broker_port, client_factory)
- services.addService(client_service)
+ client_service.setName("amqp")
+ client_service.setServiceParent(services)
return services
=== added file 'src/provisioningserver/services.py'
--- src/provisioningserver/services.py 1970-01-01 00:00:00 +0000
+++ src/provisioningserver/services.py 2012-02-02 15:19:18 +0000
@@ -0,0 +1,104 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""..."""
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ "LogService",
+ "OOPSService",
+ ]
+
+import signal
+import sys
+
+import oops
+from oops_datedir_repo import DateDirRepo
+from oops_twisted import (
+ Config as oops_config,
+ defer_publisher,
+ OOPSObserver,
+ )
+from twisted.application.service import Service
+from twisted.internet import reactor
+from twisted.python.log import (
+ addObserver,
+ FileLogObserver,
+ removeObserver,
+ )
+from twisted.python.logfile import LogFile
+
+
+class LogService(Service):
+
+ name = "log"
+
+ def __init__(self, filename):
+ self.filename = filename
+ self.logfile = None
+ self.observer = None
+
+ def _signal_handler(self, sig, frame):
+ reactor.callFromThread(self.logfile.reopen)
+
+ def startService(self):
+ Service.startService(self)
+ if self.filename != '-':
+ self.logfile = LogFile.fromFullPath(
+ self.filename, rotateLength=None, defaultMode=0644)
+ assert signal.getsignal(signal.SIGUSR1) is signal.SIG_DFL, (
+ "A signal handler is already installed for SIGUSR1.")
+ signal.signal(signal.SIGUSR1, self._signal_handler)
+ else:
+ self.logfile = sys.stdout
+ self.observer = FileLogObserver(self.logfile)
+ self.observer.start()
+
+ def stopService(self):
+ Service.stopService(self)
+ # Must use == here; the handler returned from getsignal() is not the
+ # same object as self._signal_handler, even though im_class, im_func,
+ # and im_self *are* all identical. Don't know why this should be.
+ if signal.getsignal(signal.SIGUSR1) == self._signal_handler:
+ signal.signal(signal.SIGUSR1, signal.SIG_DFL)
+ self.observer.stop()
+ self.observer = None
+ self.logfile.close()
+ self.logfile = None
+
+
+class OOPSService(Service):
+
+ name = "oops"
+
+ def __init__(self, log_service, oops_dir, oops_reporter):
+ self.config = None
+ self.log_service = log_service
+ self.oops_dir = oops_dir
+ self.oops_reporter = oops_reporter
+
+ def startService(self):
+ Service.startService(self)
+ self.config = oops_config()
+ # Add the oops publisher that writes files in the configured place if
+ # the command line option was set.
+ if self.oops_dir:
+ repo = DateDirRepo(self.oops_dir)
+ self.config.publishers.append(
+ defer_publisher(oops.publish_new_only(repo.publish)))
+ if self.oops_reporter:
+ self.config.template['reporter'] = self.oops_reporter
+ self.observer = OOPSObserver(
+ self.config, self.log_service.observer.emit)
+ addObserver(self.observer.emit)
+
+ def stopService(self):
+ Service.stopService(self)
+ removeObserver(self.observer.emit)
+ self.observer = None
+ self.config = None
=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py 2012-02-01 15:24:22 +0000
+++ src/provisioningserver/tests/test_plugin.py 2012-02-02 15:19:18 +0000
@@ -11,32 +11,21 @@
__metaclass__ = type
__all__ = []
-from cStringIO import StringIO
from functools import partial
import os
from unittest import skip
from fixtures import TempDir
-from oops_twisted import OOPSObserver
from provisioningserver.plugin import (
Options,
ProvisioningServiceMaker,
- setUpOOPSHandler,
)
from testtools import TestCase
-from testtools.content import (
- Content,
- UTF8_TEXT,
- )
from testtools.matchers import (
MatchesException,
Raises,
)
from twisted.application.service import MultiService
-from twisted.python.log import (
- FileLogObserver,
- theLogPublisher,
- )
from twisted.python.usage import UsageError
@@ -85,10 +74,7 @@
def test_parse_minimal_options(self):
options = Options()
# The minimal set of options that must be provided.
- arguments = [
- "--brokerpassword", "Hoskins",
- "--brokeruser", "Bob",
- ]
+ arguments = []
options.parseOptions(arguments) # No error.
def test_parse_int_options(self):
@@ -129,72 +115,53 @@
Raises(expected))
-class TestSetUpOOPSHandler(TestCase):
- """Tests for `provisioningserver.plugin.setUpOOPSHandler`."""
-
- def setUp(self):
- super(TestSetUpOOPSHandler, self).setUp()
- self.observers = theLogPublisher.observers[:]
- self.logfile = StringIO()
- self.addDetail("log", Content(UTF8_TEXT, self.logfile.getvalue))
- self.log = FileLogObserver(self.logfile)
-
- def tearDown(self):
- super(TestSetUpOOPSHandler, self).tearDown()
- theLogPublisher.observers[:] = self.observers
-
- def makeObserver(self, settings):
- options = Options()
- options["brokerpassword"] = "Hoskins"
- options["brokeruser"] = "Bob"
- options.update(settings)
- observer = setUpOOPSHandler(options, self.log)
- return options, observer
-
- def test_minimal(self):
- options, observer = self.makeObserver({})
- self.assertIsInstance(observer, OOPSObserver)
- self.assertEqual([], observer.config.publishers)
- self.assertEqual(
- {"reporter": options.defaults["oops-reporter"]},
- observer.config.template)
-
- def test_with_all_params(self):
- settings = {
- "oops-reporter": "Sidebottom",
- "oops-dir": self.useFixture(TempDir()).path,
- }
- options, observer = self.makeObserver(settings)
- self.assertIsInstance(observer, OOPSObserver)
- self.assertEqual(1, len(observer.config.publishers))
- self.assertEqual(
- {"reporter": "Sidebottom"},
- observer.config.template)
-
-
class TestProvisioningServiceMaker(TestCase):
"""Tests for `provisioningserver.plugin.ProvisioningServiceMaker`."""
+ def get_log_file(self):
+ return os.path.join(
+ self.useFixture(TempDir()).path,
+ "provisioningserver.log")
+
def test_init(self):
service_maker = ProvisioningServiceMaker("Harry", "Hill")
self.assertEqual("Harry", service_maker.tapname)
self.assertEqual("Hill", service_maker.description)
- def makeOptions(self, settings):
- options = Options()
- options["brokerpassword"] = "Hoskins"
- options["brokeruser"] = "Bob"
- options.update(settings)
- return options
-
def test_makeService(self):
- logfile = os.path.join(
- self.useFixture(TempDir()).path, "provisioningserver.log")
- options = self.makeOptions({"logfile": logfile})
- service_maker = ProvisioningServiceMaker("Harry", "Hill")
- service = service_maker.makeService(options, _set_proc_title=False)
- self.assertIsInstance(service, MultiService)
- self.assertEqual(1, len(service.services))
- [client_service] = service.services
- self.assertEqual(options["brokerhost"], client_service.args[0])
- self.assertEqual(options["brokerport"], client_service.args[1])
+ """
+ Only the log and oops services are created when no options are given.
+ """
+ options = Options()
+ options["logfile"] = self.get_log_file()
+ service_maker = ProvisioningServiceMaker("Harry", "Hill")
+ service = service_maker.makeService(options, _set_proc_title=False)
+ self.assertIsInstance(service, MultiService)
+ self.assertEqual(
+ ["log", "oops"],
+ sorted(service.namedServices))
+ self.assertEqual(
+ len(service.namedServices), len(service.services),
+ "Not all services are named.")
+
+ def test_makeService_with_broker(self):
+ """
+ The log, oops and amqp services are created when no the broker user
+ and password options are given.
+ """
+ options = Options()
+ options["brokerpassword"] = "Hoskins"
+ options["brokeruser"] = "Bob"
+ options["logfile"] = self.get_log_file()
+ service_maker = ProvisioningServiceMaker("Harry", "Hill")
+ service = service_maker.makeService(options, _set_proc_title=False)
+ self.assertIsInstance(service, MultiService)
+ self.assertEqual(
+ ["amqp", "log", "oops"],
+ sorted(service.namedServices))
+ self.assertEqual(
+ len(service.namedServices), len(service.services),
+ "Not all services are named.")
+ amqp_client_service = service.getServiceNamed("amqp")
+ self.assertEqual(options["brokerhost"], amqp_client_service.args[0])
+ self.assertEqual(options["brokerport"], amqp_client_service.args[1])
=== added file 'src/provisioningserver/tests/test_services.py'
--- src/provisioningserver/tests/test_services.py 1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_services.py 2012-02-02 15:19:18 +0000
@@ -0,0 +1,120 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.services`."""
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+import os
+import signal
+import sys
+
+from fixtures import TempDir
+from oops_twisted import OOPSObserver
+from provisioningserver.services import (
+ LogService,
+ OOPSService,
+ )
+from testtools import TestCase
+from testtools.content import content_from_file
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.application.service import MultiService
+from twisted.python.log import (
+ FileLogObserver,
+ theLogPublisher,
+ )
+from twisted.python.logfile import LogFile
+
+
+class TestServicesBase:
+
+ run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
+ def setUp(self):
+ super(TestServicesBase, self).setUp()
+ self.observers = theLogPublisher.observers[:]
+ self.services = MultiService()
+ self.services.privilegedStartService()
+ self.services.startService()
+
+ def tearDown(self):
+ super(TestServicesBase, self).tearDown()
+ d = self.services.stopService()
+ # The log file must be read in right after services have stopped,
+ # before the temporary directory where the log lives is removed.
+ d.addBoth(lambda ignore: self.addDetailFromLog())
+ d.addBoth(lambda ignore: self.assertNoObserversLeftBehind())
+ return d
+
+ def addDetailFromLog(self):
+ content = content_from_file(self.log_filename, buffer_now=True)
+ self.addDetail("log", content)
+
+ def assertNoObserversLeftBehind(self):
+ self.assertEqual(self.observers, theLogPublisher.observers)
+
+
+class TestLogService(TestServicesBase, TestCase):
+ """Tests for `provisioningserver.services.LogService`."""
+
+ def test_log_to_stdout(self):
+ log_service = LogService("-")
+ log_service.setServiceParent(self.services)
+ self.assertIsInstance(log_service.observer, FileLogObserver)
+ self.assertEqual("-", log_service.filename)
+ self.assertEqual(sys.stdout, log_service.logfile)
+ # The SIGUSR1 signal handler is untouched.
+ self.assertEqual(
+ signal.getsignal(signal.SIGUSR1),
+ signal.SIG_DFL)
+
+ def test_log_to_file(self):
+ tempdir = self.useFixture(TempDir()).path
+ log_filename = os.path.join(tempdir, "test.log")
+ log_service = LogService(log_filename)
+ log_service.setServiceParent(self.services)
+ self.assertIsInstance(log_service.observer, FileLogObserver)
+ self.assertEqual(log_filename, log_service.filename)
+ self.assertIsInstance(log_service.logfile, LogFile)
+ self.assertEqual(log_filename, log_service.logfile.path)
+ # The SIGUSR1 signal handler is set.
+ self.assertEqual(
+ signal.getsignal(signal.SIGUSR1),
+ log_service._signal_handler)
+
+
+class TestOOPSService(TestServicesBase, TestCase):
+ """Tests for `provisioningserver.services.OOPSService`."""
+
+ def setUp(self):
+ super(TestOOPSService, self).setUp()
+ # OOPSService relies upon LogService.
+ self.tempdir = self.useFixture(TempDir()).path
+ self.log_filename = os.path.join(self.tempdir, "test.log")
+ self.log_service = LogService(self.log_filename)
+ self.log_service.setServiceParent(self.services)
+
+ def test_minimal(self):
+ oops_service = OOPSService(self.log_service, None, None)
+ oops_service.setServiceParent(self.services)
+ observer = oops_service.observer
+ self.assertIsInstance(observer, OOPSObserver)
+ self.assertEqual([], observer.config.publishers)
+ self.assertEqual({}, observer.config.template)
+
+ def test_with_all_params(self):
+ oops_dir = os.path.join(self.tempdir, "oops")
+ oops_service = OOPSService(self.log_service, oops_dir, "Sidebottom")
+ oops_service.setServiceParent(self.services)
+ observer = oops_service.observer
+ self.assertIsInstance(observer, OOPSObserver)
+ self.assertEqual(1, len(observer.config.publishers))
+ self.assertEqual(
+ {"reporter": "Sidebottom"},
+ observer.config.template)
