launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06381
[Merge] lp:~allenap/maas/pserv-config-file into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/pserv-config-file into lp:maas with lp:~allenap/maas/disable-rabbit-in-pserv as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/pserv-config-file/+merge/93256
This cleans up some stuff - like purging setproctitle from the project - and offloads configuration validation to formencode.
--
https://code.launchpad.net/~allenap/maas/pserv-config-file/+merge/93256
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pserv-config-file into lp:maas.
=== modified file 'Makefile'
--- Makefile 2012-02-14 14:35:07 +0000
+++ Makefile 2012-02-15 17:33:31 +0000
@@ -77,7 +77,7 @@
$(RM) -r docs/_build/
pserv.pid: bin/twistd.pserv
- bin/twistd.pserv --pidfile=$@ maas-pserv --port=8001
+ bin/twistd.pserv --pidfile=$@ maas-pserv --config-file=etc/pserv.yaml
pserv-start: pserv.pid
=== modified file 'buildout.cfg'
--- buildout.cfg 2012-02-15 10:58:09 +0000
+++ buildout.cfg 2012-02-15 17:33:31 +0000
@@ -68,9 +68,10 @@
[pserv]
recipe = zc.recipe.egg
eggs =
+ formencode
oops-datedir-repo
oops-twisted
- setproctitle
+ pyyaml
twisted
txamqp
entry-points =
=== added directory 'etc'
=== added file 'etc/pserv.yaml'
--- etc/pserv.yaml 1970-01-01 00:00:00 +0000
+++ etc/pserv.yaml 2012-02-15 17:33:31 +0000
@@ -0,0 +1,20 @@
+# Provisioning Server (pserv) configuration.
+
+# The port on which the Provisioning API will be made available.
+#port: 8001
+# Where to log. This log can be rotated by sending SIGUSR1 to the
+# running server.
+logfile: "pserv.log"
+
+# OOPS configuration (optional).
+# oops:
+# directory: "/tmp/killed/by"
+# reporter: "death"
+
+# Message broker configuration (optional, not currently used).
+# broker:
+# host: "localhost"
+# port: 1543
+# username: "lemmy"
+# password: "overkill"
+# vhost: "/hammersmith"
=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py 2012-02-15 17:33:31 +0000
+++ src/provisioningserver/plugin.py 2012-02-15 17:33:31 +0000
@@ -12,6 +12,12 @@
__all__ = []
from amqpclient import AMQFactory
+from formencode import Schema
+from formencode.validators import (
+ Int,
+ RequireIfPresent,
+ String,
+ )
from provisioningserver.cobblerclient import CobblerSession
from provisioningserver.remote import ProvisioningAPI_XMLRPC
from provisioningserver.services import (
@@ -22,7 +28,6 @@
FakeCobbler,
FakeTwistedProxy,
)
-import setproctitle
from twisted.application.internet import (
TCPClient,
TCPServer,
@@ -38,39 +43,63 @@
)
from twisted.web.resource import Resource
from twisted.web.server import Site
+import yaml
from zope.interface import implements
+class ConfigOops(Schema):
+ """Configuration validator for OOPS options."""
+
+ if_key_missing = None
+
+ directory = String(if_missing=b"")
+ reporter = String(if_missing=b"")
+
+ chained_validators = (
+ RequireIfPresent("reporter", present="directory"),
+ )
+
+
+class ConfigBroker(Schema):
+ """Configuration validator for message broker options."""
+
+ if_key_missing = None
+
+ host = String(if_missing=b"localhost")
+ port = Int(min=1, max=65535, if_missing=5673)
+ username = String(if_missing=None)
+ password = String(if_missing=None)
+ vhost = String(if_missing="/")
+
+
+class Config(Schema):
+ """Configuration validator."""
+
+ if_key_missing = None
+
+ port = Int(min=1, max=65535, if_missing=8001)
+ logfile = String(not_empty=True)
+ oops = ConfigOops
+ broker = ConfigBroker
+
+ @classmethod
+ def parse(cls, stream):
+ """Load a YAML configuration from `stream` and validate."""
+ return cls().to_python(yaml.load(stream))
+
+ @classmethod
+ def load(cls, filename):
+ """Load a YAML configuration from `filename` and validate."""
+ with open(filename, "rb") as stream:
+ return cls.parse(stream)
+
+
class Options(usage.Options):
"""Command line options for the provisioning server."""
optParameters = [
- ["port", None, 8001, "Port to serve on."],
- ["logfile", "l", "pserv.log", "Logfile name."],
- ["oops-dir", "r", None, "Where to write OOPS reports"],
- ["oops-reporter", "o", "MAAS-PS", "String identifying this service."],
- ]
-
- # Move these back into optParameters when RabbitMQ is a required component
- # of a running MaaS installation.
- optParameters_FOR_RABBIT = [
- ["brokerport", "p", 5672, "Broker port"],
- ["brokerhost", "h", '127.0.0.1', "Broker host"],
- ["brokeruser", "u", None, "Broker user"],
- ["brokerpassword", "a", None, "Broker password"],
- ["brokervhost", "v", '/', "Broker vhost"],
- ]
-
- def postOptions(self):
- for int_arg in ('port',):
- try:
- self[int_arg] = int(self[int_arg])
- except (TypeError, ValueError):
- raise usage.UsageError("--%s must be an integer." % int_arg)
- if not self["oops-reporter"] and self["oops-dir"]:
- raise usage.UsageError(
- "A reporter must be supplied to identify reports "
- "from this service from other OOPS reports.")
+ ["config-file", "c", "pserv.yaml", "Configuration file to load."],
+ ]
class ProvisioningServiceMaker(object):
@@ -84,44 +113,41 @@
self.tapname = name
self.description = description
- def makeService(self, options, _set_proc_title=True):
- """Construct a service.
-
- :param _set_proc_title: For testing; if `False` this will stop the
- obfuscation of command-line parameters in the process title.
- """
- # Required to hide the command line options that include a password.
- # There is a small window where it can be seen though, between
- # invocation and when this code runs. TODO: Make this optional (so
- # that we don't override process title in tests).
- if _set_proc_title:
- setproctitle.setproctitle("maas provisioning service")
-
+ def makeService(self, options):
+ """Construct a service."""
services = MultiService()
- log_service = LogService(options["logfile"])
+ config_file = options["config-file"]
+ if config_file is None:
+ config = Config.parse(b"")
+ else:
+ config = Config.load(config_file)
+
+ log_service = LogService(config["logfile"])
log_service.setServiceParent(services)
- oops_dir = options["oops-dir"]
- oops_reporter = options["oops-reporter"]
+ oops_config = config["oops"]
+ oops_dir = oops_config["directory"]
+ oops_reporter = oops_config["reporter"]
oops_service = OOPSService(log_service, oops_dir, oops_reporter)
oops_service.setServiceParent(services)
- broker_port = options.get("brokerport")
- broker_host = options.get("brokerhost")
- broker_user = options.get("brokeruser")
- broker_password = options.get("brokerpassword")
- broker_vhost = options.get("brokervhost")
+ broker_config = config["broker"]
+ broker_port = broker_config["port"]
+ broker_host = broker_config["host"]
+ broker_username = broker_config["username"]
+ broker_password = broker_config["password"]
+ broker_vhost = broker_config["vhost"]
# Connecting to RabbitMQ is optional; it is not yet a required
# component of a running MaaS installation.
- if broker_user is not None and broker_password is not None:
+ if broker_username is not None and broker_password is not None:
cb_connected = lambda ignored: None # TODO
cb_disconnected = lambda ignored: None # TODO
cb_failed = lambda (connector, reason): (
log.err(reason, "Connection failed"))
client_factory = AMQFactory(
- broker_user, broker_password, broker_vhost,
+ broker_username, broker_password, broker_vhost,
cb_connected, cb_disconnected, cb_failed)
client_service = TCPClient(
broker_host, broker_port, client_factory)
@@ -140,7 +166,7 @@
site_root = Resource()
site_root.putChild("api", ProvisioningAPI_XMLRPC(session))
site = Site(site_root)
- site_port = options["port"]
+ site_port = config["port"]
site_service = TCPServer(site_port, site)
site_service.setName("site")
site_service.setServiceParent(services)
=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py 2012-02-15 17:33:31 +0000
+++ src/provisioningserver/tests/test_plugin.py 2012-02-15 17:33:31 +0000
@@ -13,10 +13,11 @@
from functools import partial
import os
-from unittest import skip
from fixtures import TempDir
+import formencode
from provisioningserver.plugin import (
+ Config,
Options,
ProvisioningServiceMaker,
)
@@ -27,39 +28,76 @@
)
from twisted.application.service import MultiService
from twisted.python.usage import UsageError
+import yaml
+
+
+class TestConfig(TestCase):
+ """Tests for `provisioningserver.plugin.Config`."""
+
+ def test_defaults(self):
+ expected = {
+ 'broker': {
+ 'host': 'localhost',
+ 'password': None,
+ 'port': 5673,
+ 'username': None,
+ 'vhost': u'/',
+ },
+ 'logfile': '/some/where.log',
+ 'oops': {
+ 'directory': '',
+ 'reporter': '',
+ },
+ 'port': 8001,
+ }
+ mandatory_arguments = {
+ "logfile": "/some/where.log",
+ }
+ observed = Config.to_python(mandatory_arguments)
+ self.assertEqual(expected, observed)
+
+ def test_parse(self):
+ # Configuration can be parsed from a snipped of YAML.
+ observed = Config.parse(
+ b'logfile: "/some/where.log"')
+ self.assertEqual("/some/where.log", observed["logfile"])
+
+ def test_load(self):
+ # Configuration can be loaded and parsed from a file.
+ filename = os.path.join(
+ self.useFixture(TempDir()).path, "config.yaml")
+ with open(filename, "wb") as stream:
+ stream.write(b'logfile: "/some/where.log"')
+ observed = Config.load(filename)
+ self.assertEqual("/some/where.log", observed["logfile"])
+
+ def test_load_example(self):
+ # The example configuration can be loaded and validated.
+ filename = os.path.join(
+ os.path.dirname(__file__), os.pardir,
+ os.pardir, os.pardir, "etc", "pserv.yaml")
+ Config.load(filename)
+
+ def test_oops_directory_without_reporter(self):
+ # It is an error to omit the OOPS reporter if directory is specified.
+ config = (
+ 'logfile: "/some/where.log"\n'
+ 'oops:\n'
+ ' directory: /tmp/oops\n'
+ )
+ expected = MatchesException(
+ formencode.Invalid, "oops: You must give a value for reporter")
+ self.assertThat(
+ partial(Config.parse, config),
+ Raises(expected))
class TestOptions(TestCase):
"""Tests for `provisioningserver.plugin.Options`."""
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation. Re-enable "
- "when RabbitMQ is once again needed; remove "
- "the other similarly named test.")
- def test_defaults_SKIPPED(self):
- options = Options()
- expected = {
- "brokerhost": "127.0.0.1",
- "brokerpassword": None,
- "brokerport": 5672,
- "brokeruser": None,
- "brokervhost": "/",
- "logfile": "pserv.log",
- "oops-dir": None,
- "oops-reporter": "MAAS-PS",
- "port": 8001,
- }
- self.assertEqual(expected, options.defaults)
-
def test_defaults(self):
options = Options()
- expected = {
- "logfile": "pserv.log",
- "oops-dir": None,
- "oops-reporter": "MAAS-PS",
- "port": 8001,
- }
+ expected = {"config-file": "pserv.yaml"}
self.assertEqual(expected, options.defaults)
def check_exception(self, options, message, *arguments):
@@ -68,123 +106,28 @@
partial(options.parseOptions, arguments),
Raises(MatchesException(UsageError, message)))
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation.")
- def test_option_brokeruser_required(self):
- options = Options()
- self.check_exception(
- options,
- "--brokeruser must be specified")
-
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation.")
- def test_option_brokerpassword_required(self):
- options = Options()
- self.check_exception(
- options,
- "--brokerpassword must be specified",
- "--brokeruser", "Bob")
-
def test_parse_minimal_options(self):
options = Options()
# The minimal set of options that must be provided.
arguments = []
options.parseOptions(arguments) # No error.
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation. Re-enable "
- "when RabbitMQ is once again needed; remove "
- "the other similarly named test.")
- def test_parse_int_options_SKIPPED(self):
- # Some options are converted to ints.
- options = Options()
- arguments = [
- "--brokerpassword", "Hoskins",
- "--brokerport", "4321",
- "--brokeruser", "Bob",
- "--port", "3456",
- ]
- options.parseOptions(arguments)
- self.assertEqual(4321, options["brokerport"])
- self.assertEqual(3456, options["port"])
-
- def test_parse_int_options(self):
- # Some options are converted to ints.
- options = Options()
- arguments = [
- "--port", "3456",
- ]
- options.parseOptions(arguments)
- self.assertEqual(3456, options["port"])
-
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation. Re-enable "
- "when RabbitMQ is once again needed; remove "
- "the other similarly named test.")
- def test_parse_broken_int_options_SKIPPED(self):
- # An error is raised if the integer options do not contain integers.
- options = Options()
- arguments = [
- "--brokerpassword", "Hoskins",
- "--brokerport", "Jr.",
- "--brokeruser", "Bob",
- ]
- self.assertRaises(
- UsageError, options.parseOptions, arguments)
-
- def test_parse_broken_int_options(self):
- # An error is raised if the integer options do not contain integers.
- options = Options()
- arguments = [
- "--port", "Metallica",
- ]
- self.assertRaises(
- UsageError, options.parseOptions, arguments)
-
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation. Re-enable "
- "when RabbitMQ is once again needed; remove "
- "the other similarly named test.")
- def test_oops_dir_without_reporter_SKIPPED(self):
- # It is an error to omit the OOPS reporter if directory is specified.
- options = Options()
- arguments = [
- "--brokerpassword", "Hoskins",
- "--brokeruser", "Bob",
- "--oops-dir", "/some/where",
- "--oops-reporter", "",
- ]
- expected = MatchesException(
- UsageError, "A reporter must be supplied")
- self.assertThat(
- partial(options.parseOptions, arguments),
- Raises(expected))
-
- def test_oops_dir_without_reporter(self):
- # It is an error to omit the OOPS reporter if directory is specified.
- options = Options()
- arguments = [
- "--oops-dir", "/some/where",
- "--oops-reporter", "",
- ]
- expected = MatchesException(
- UsageError, "A reporter must be supplied")
- self.assertThat(
- partial(options.parseOptions, arguments),
- Raises(expected))
-
class TestProvisioningServiceMaker(TestCase):
"""Tests for `provisioningserver.plugin.ProvisioningServiceMaker`."""
- def get_log_file(self):
- return os.path.join(
- self.useFixture(TempDir()).path, "pserv.log")
+ def setUp(self):
+ super(TestProvisioningServiceMaker, self).setUp()
+ self.tempdir = self.useFixture(TempDir()).path
+
+ def write_config(self, config):
+ if "logfile" not in config:
+ logfile = os.path.join(self.tempdir, "pserv.log")
+ config = dict(config, logfile=logfile)
+ config_filename = os.path.join(self.tempdir, "config.yaml")
+ with open(config_filename, "wb") as stream:
+ yaml.dump(config, stream)
+ return config_filename
def test_init(self):
service_maker = ProvisioningServiceMaker("Harry", "Hill")
@@ -196,9 +139,9 @@
Only the site service is created when no options are given.
"""
options = Options()
- options["logfile"] = self.get_log_file()
+ options["config-file"] = self.write_config({})
service_maker = ProvisioningServiceMaker("Harry", "Hill")
- service = service_maker.makeService(options, _set_proc_title=False)
+ service = service_maker.makeService(options)
self.assertIsInstance(service, MultiService)
self.assertSequenceEqual(
["log", "oops", "site"],
@@ -206,23 +149,17 @@
self.assertEqual(
len(service.namedServices), len(service.services),
"Not all services are named.")
- site_service = service.getServiceNamed("site")
- self.assertEqual(options["port"], site_service.args[0])
- @skip(
- "RabbitMQ is not yet a required component "
- "of a running MaaS installation.")
def test_makeService_with_broker(self):
"""
The log, oops, site, and amqp services are created when the broker
user and password options are given.
"""
options = Options()
- options["brokerpassword"] = "Hoskins"
- options["brokeruser"] = "Bob"
- options["logfile"] = self.get_log_file()
+ options["config-file"] = self.write_config(
+ {"broker": {"username": "Bob", "password": "Hoskins"}})
service_maker = ProvisioningServiceMaker("Harry", "Hill")
- service = service_maker.makeService(options, _set_proc_title=False)
+ service = service_maker.makeService(options)
self.assertIsInstance(service, MultiService)
self.assertSequenceEqual(
["amqp", "log", "oops", "site"],
@@ -230,8 +167,3 @@
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])
- site_service = service.getServiceNamed("site")
- self.assertEqual(options["port"], site_service.args[0])
=== modified file 'versions.cfg'
--- versions.cfg 2012-02-15 10:37:18 +0000
+++ versions.cfg 2012-02-15 17:33:31 +0000
@@ -7,6 +7,7 @@
django-piston =
docutils =
fixtures =
+formencode = 1.2.4
ipython =
nose =
nose-subunit =
@@ -19,8 +20,8 @@
# https://code.djangoproject.com/ticket/16250
psycopg2 = 2.4.1
python-subunit =
+pyyaml = 3.10
rabbitfixture = 0.3.2
-setproctitle =
South =
testresources >= 0.2.4-r58
testtools =
