launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06818
[Merge] lp:~jtv/maas/better-provisioning-errors into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/better-provisioning-errors into lp:maas with lp:~jtv/maas/fake_provisioning as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/better-provisioning-errors/+merge/98623
This catches errors that come into maasserver from the xmlrpc proxy that talks to the provisioning server.
We don't have any class of our own that all the pserv calls are channeled through, where I could catch errors and re-represent them into something more user-friendly. (Compare to CobblerClient, for instance, where all calls go through the same “call” method: a single “try/catch” would have been enough there).
Instead, I wrapped the xmlrpc proxy (or the fake pserv API object, in testing) in something that's very similar to xmlrpclib.ServerProxy itself: whenever you try to obtain a method from it, you get a callable object that catches xmlrpclib.Fault and tries to provide it with a more user-friendly error string.
Some similar but more ad-hoc logic in the add_node signal handler now becomes obsolete. One consideration of that code remains valid though: because these are user-facing diagnostics of integration issues, it's probably a good thing to have their descriptions in maasserver. Taking a very “local” view of faults can be good for debugging precision, but it's likely to be unhelpful to users.
We'll probably want to improve on the error messages later. Unless we see problems that are absolutely fatal, I believe polishing up the messages is work for later. The main thing right now is to put is in a situation where we have that option.
--
https://code.launchpad.net/~jtv/maas/better-provisioning-errors/+merge/98623
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/better-provisioning-errors into lp:maas.
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-21 07:30:59 +0000
+++ src/maasserver/models.py 2012-03-21 12:39:44 +0000
@@ -175,20 +175,16 @@
)
+def get_papi():
+ """Return a provisioning server API proxy."""
+ # Avoid circular imports.
+ from maasserver.provisioning import get_provisioning_api_proxy
+ return get_provisioning_api_proxy()
+
+
class NodeManager(models.Manager):
"""A utility to manage the collection of Nodes."""
- # Twisted XMLRPC proxy for talking to the provisioning API. Created
- # on demand.
- provisioning_proxy = None
-
- def _set_provisioning_proxy(self):
- """Set up the provisioning-API proxy if needed."""
- # Avoid circular imports.
- from maasserver.provisioning import get_provisioning_api_proxy
- if self.provisioning_proxy is None:
- self.provisioning_proxy = get_provisioning_api_proxy()
-
def filter_by_ids(self, query, ids=None):
"""Filter `query` result set by system_id values.
@@ -289,9 +285,8 @@
:return: Those Nodes for which shutdown was actually requested.
:rtype: list
"""
- self._set_provisioning_proxy()
nodes = self.get_editable_nodes(by_user, ids=ids)
- self.provisioning_proxy.stop_nodes([node.system_id for node in nodes])
+ get_papi().stop_nodes([node.system_id for node in nodes])
return nodes
def start_nodes(self, ids, by_user, user_data=None):
@@ -312,13 +307,11 @@
:rtype: list
"""
from metadataserver.models import NodeUserData
- self._set_provisioning_proxy()
nodes = self.get_editable_nodes(by_user, ids=ids)
if user_data is not None:
for node in nodes:
NodeUserData.objects.set_user_data(node, user_data)
- self.provisioning_proxy.start_nodes(
- [node.system_id for node in nodes])
+ get_papi().start_nodes([node.system_id for node in nodes])
return nodes
=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py 2012-03-21 12:39:44 +0000
+++ src/maasserver/provisioning.py 2012-03-21 12:39:44 +0000
@@ -11,6 +11,7 @@
__metaclass__ = type
__all__ = [
'get_provisioning_api_proxy',
+ 'ProvisioningProxy',
]
from textwrap import dedent
@@ -33,6 +34,115 @@
from provisioningserver.enum import PSERV_FAULT
+def present_user_friendly_fault(fault):
+ """Return a more user-friendly exception to represent `fault`.
+
+ :param fault: An exception raised by, or received across, xmlrpc.
+ :type fault: :class:`xmlrpclib.Fault`
+ :return: A more user-friendly exception, if one can be produced.
+ Otherwise, this returns None and the original exception should be
+ re-raised. (This is left to the caller in order to minimize
+ erosion of the backtrace).
+ :rtype: :class:`Exception`, or None.
+ """
+ params = {
+ 'fault_code': fault.faultCode,
+ 'fault_string': fault.faultString,
+ }
+ presentations = {
+ PSERV_FAULT.NO_COBBLER: """
+ The provisioning server was unable to reach the Cobbler service:
+ %(fault_string)s
+
+ Check pserv.log, and restart MaaS if needed.
+ """,
+ PSERV_FAULT.COBBLER_AUTH_FAILED: """
+ The provisioning server failed to authenticate with the Cobbler
+ service: %(fault_string)s.
+
+ This may mean that Cobbler's authentication configuration has
+ changed. Check /var/log/cobbler/ and pserv.log.
+ """,
+ PSERV_FAULT.COBBLER_AUTH_ERROR: """
+ The Cobbler server no longer accepts the provisioning server's
+ authentication token. This should not happen; it may indicate
+ that the server is under unsustainable load.
+ """,
+ PSERV_FAULT.NO_SUCH_PROFILE: """
+ System profile does not exist: %(fault_string)s.
+
+ Has the maas-import-isos script been run? This will run
+ automatically from time to time, but if it is failing, an
+ administrator may need to run it manually.
+ """,
+ PSERV_FAULT.GENERIC_COBBLER_ERROR: """
+ The provisioning service encountered a problem with the Cobbler
+ server, fault code %(fault_code)s: %(fault_string)s
+
+ If the error message is not clear, you may need to check the
+ Cobbler logs in /var/log/cobbler/ or pserv.log.
+ """,
+ 8002: """
+ Unable to reach provisioning server (%(fault_string)s).
+
+ Check pserv.log and your PSERV_URL setting, and restart MaaS if
+ needed.
+ """,
+ }
+ user_friendly_text = presentations.get(fault.faultCode)
+ if user_friendly_text is None:
+ return None
+ else:
+ user_friendly_text = dedent(user_friendly_text.lstrip('\n') % params)
+ return xmlrpclib.Fault(fault.faultCode, user_friendly_text)
+
+
+class ProvisioningCaller:
+ """Wrapper for an XMLRPC call.
+
+ Runs xmlrpc exceptions through `present_user_friendly_fault` for better
+ presentation to the user.
+ """
+
+ def __init__(self, method):
+ self.method = method
+
+ def __call__(self, *args, **kwargs):
+ try:
+ return self.method(*args, **kwargs)
+ except xmlrpclib.Fault as e:
+ friendly_fault = present_user_friendly_fault(e)
+ if friendly_fault is None:
+ raise
+ else:
+ raise friendly_fault
+
+
+class ProvisioningProxy:
+ """Proxy for calling the provisionig service.
+
+ This wraps an XMLRPC :class:`ServerProxy`, but translates exceptions
+ coming in from, or across, the xmlrpc mechanism into more helpful ones
+ for the user.
+ """
+
+ def __init__(self, xmlrpc_proxy):
+ self.proxy = xmlrpc_proxy
+
+ def patch(self, method, replacement):
+ setattr(self.proxy, method, replacement)
+
+ def __getattr__(self, attribute_name):
+ """Return a wrapped version of the requested method."""
+ attribute = getattr(self.proxy, attribute_name)
+ if getattr(attribute, '__call__', None) is None:
+ # This is a regular attribute. Return it as-is.
+ return attribute
+ else:
+ # This attribute is callable. Wrap it in a caller.
+ return ProvisioningCaller(attribute)
+
+
def get_provisioning_api_proxy():
"""Return a proxy to the Provisioning API.
@@ -43,14 +153,16 @@
if settings.USE_REAL_PSERV:
# Use a real provisioning server. This requires PSERV_URL to be
# set.
- return xmlrpclib.ServerProxy(
+ xmlrpc_proxy = xmlrpclib.ServerProxy(
settings.PSERV_URL, allow_none=True, use_datetime=True)
else:
# Create a fake. The code that provides the testing fake is not
# available in an installed production system, so import it only
# when a fake is requested.
from maasserver.testing import get_fake_provisioning_api_proxy
- return get_fake_provisioning_api_proxy()
+ xmlrpc_proxy = get_fake_provisioning_api_proxy()
+
+ return ProvisioningProxy(xmlrpc_proxy)
def get_metadata_server_url():
=== modified file 'src/maasserver/testing/tests/test_module.py'
--- src/maasserver/testing/tests/test_module.py 2012-03-13 05:34:38 +0000
+++ src/maasserver/testing/tests/test_module.py 2012-03-21 12:39:44 +0000
@@ -40,8 +40,9 @@
# TestCase.setUp() patches in a fake provisioning API so that we can
# observe what the signal handlers are doing.
papi_fake = provisioning.get_provisioning_api_proxy()
+ self.assertIsInstance(papi_fake, provisioning.ProvisioningProxy)
self.assertIsInstance(
- papi_fake, fakeapi.FakeSynchronousProvisioningAPI)
+ papi_fake.proxy, fakeapi.FakeSynchronousProvisioningAPI)
# The fake has some limited, automatically generated, sample
# data. This is required for many tests to run. First there is a
# sample distro.
=== removed file 'src/maasserver/tests/test_maasavahi.py'
--- src/maasserver/tests/test_maasavahi.py 2012-03-19 20:52:17 +0000
+++ src/maasserver/tests/test_maasavahi.py 1970-01-01 00:00:00 +0000
@@ -1,120 +0,0 @@
-# Copyright 2012 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for the Avahi export of MAAS."""
-
-from __future__ import (
- print_function,
- unicode_literals,
- )
-
-__metaclass__ = type
-__all__ = []
-
-from collections import defaultdict
-
-from maastesting.testcase import TestCase
-
-import maasserver.maasavahi
-from maasserver.maasavahi import (
- MAASAvahiService,
- setup_maas_avahi_service,
- )
-from maasserver.models import (
- Config,
- config_manager,
- )
-
-
-class MockZeroconfServiceFactory:
- """Factory used to track usage of the zeroconfservice module.
-
- An instance is meant to be patched as
- maasserver.maasavahi.ZeroconfService. It will register instances
- created, as well as the parameters and methods called on each instance.
- """
-
- def __init__(self):
- self.instances = []
-
- def __call__(self, *args, **kwargs):
- mock = MockZeroconfService(*args, **kwargs)
- self.instances.append(mock)
- return mock
-
-
-class MockZeroconfService:
-
- def __init__(self, name, port, stype):
- self.name = name
- self.port = port
- self.stype = stype
- self.calls = []
-
- def publish(self):
- self.calls.append('publish')
-
- def unpublish(self):
- self.calls.append('unpublish')
-
-
-class TestMAASAvahiService(TestCase):
-
- def setup_mock_avahi(self):
- # Unregister other signals from Config, otherwise
- # the one registered in urls.py, will interfere with these tests
- self.patch(
- config_manager, '_config_changed_connections', defaultdict(set))
-
- mock_avahi = MockZeroconfServiceFactory()
- self.patch(
- maasserver.maasavahi, 'ZeroconfService', mock_avahi)
- return mock_avahi
-
- def test_publish_exports_name_over_avahi(self):
- mock_avahi = self.setup_mock_avahi()
- service = MAASAvahiService()
- Config.objects.set_config('maas_name', 'My Test')
- service.publish()
- # One ZeroconfService should have been created
- self.assertEquals(1, len(mock_avahi.instances))
- zeroconf = mock_avahi.instances[0]
- self.assertEquals('My Test MAAS Server', zeroconf.name)
- self.assertEquals(80, zeroconf.port)
- self.assertEquals('_maas._tcp', zeroconf.stype)
-
- # And published.
- self.assertEquals(['publish'], zeroconf.calls)
-
- def test_publish_twice_unpublishes_first(self):
- mock_avahi = self.setup_mock_avahi()
- service = MAASAvahiService()
- Config.objects.set_config('maas_name', 'My Test')
- service.publish()
- service.publish()
-
- # Two ZeroconfService should have been created. The first
- # should have been published, and unpublished,
- # while the second one should have one publish call.
- self.assertEquals(2, len(mock_avahi.instances))
- self.assertEquals(
- ['publish', 'unpublish'], mock_avahi.instances[0].calls)
- self.assertEquals(
- ['publish'], mock_avahi.instances[1].calls)
-
- def test_setup_maas_avahi_service(self):
- mock_avahi = self.setup_mock_avahi()
- Config.objects.set_config('maas_name', 'First Name')
- setup_maas_avahi_service()
-
- # Name should have been published.
- self.assertEquals(1, len(mock_avahi.instances))
- self.assertEquals(
- 'First Name MAAS Server', mock_avahi.instances[0].name)
-
- Config.objects.set_config('maas_name', 'Second Name')
-
- # A new publication should have occured.
- self.assertEquals(2, len(mock_avahi.instances))
- self.assertEquals(
- 'Second Name MAAS Server', mock_avahi.instances[1].name)
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-03-19 15:41:55 +0000
+++ src/maasserver/tests/test_models.py 2012-03-21 12:39:44 +0000
@@ -40,6 +40,7 @@
SYSTEM_USERS,
UserProfile,
)
+from maasserver.provisioning import get_provisioning_api_proxy
from maasserver.testing.enum import map_enum
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
@@ -276,9 +277,8 @@
output = Node.objects.stop_nodes([node.system_id], user)
self.assertItemsEqual([node], output)
- self.assertEqual(
- 'stop',
- Node.objects.provisioning_proxy.power_status[node.system_id])
+ power_status = get_provisioning_api_proxy().power_status
+ self.assertEqual('stop', power_status[node.system_id])
def test_stop_nodes_ignores_uneditable_nodes(self):
nodes = [self.make_node(factory.make_user()) for counter in range(3)]
@@ -294,9 +294,8 @@
output = Node.objects.start_nodes([node.system_id], user)
self.assertItemsEqual([node], output)
- self.assertEqual(
- 'start',
- Node.objects.provisioning_proxy.power_status[node.system_id])
+ power_status = get_provisioning_api_proxy().power_status
+ self.assertEqual('start', power_status[node.system_id])
def test_start_nodes_ignores_uneditable_nodes(self):
nodes = [self.make_node(factory.make_user()) for counter in range(3)]
=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py 2012-03-20 03:25:29 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-03-21 12:39:44 +0000
@@ -27,6 +27,7 @@
compose_metadata,
get_metadata_server_url,
name_arch_in_cobbler_style,
+ present_user_friendly_fault,
select_profile_for_node,
)
from maasserver.testing.enum import map_enum
@@ -125,7 +126,7 @@
def raise_missing_profile(*args, **kwargs):
raise Fault(PSERV_FAULT.NO_SUCH_PROFILE, "Unknown profile.")
- self.papi.add_node = raise_missing_profile
+ self.papi.patch('add_node', raise_missing_profile)
expectation = ExpectedException(
MissingProfileException, value_re='.*maas-import-isos.*')
with expectation:
@@ -139,7 +140,7 @@
def raise_fault(*args, **kwargs):
raise Fault(PSERV_FAULT.NO_COBBLER, error_text)
- self.papi.add_node = raise_fault
+ self.papi.patch('add_node', raise_fault)
with ExpectedException(Fault, ".*%s.*" % error_text):
node = factory.make_node(architecture='amd32k')
provisioning.provision_post_save_Node(
@@ -235,6 +236,68 @@
},
parse_qs(metadata['maas-metadata-credentials']))
+ def test_papi_xmlrpc_faults_are_reported_helpfully(self):
+ error = factory.getRandomString()
+
+ def raise_fault(*args, **kwargs):
+ raise Fault(8002, error)
+
+ self.papi.patch('add_node', raise_fault)
+
+ with ExpectedException(Fault, ".*provisioning server.*"):
+ self.papi.add_node('node', 'profile', 'power', {})
+
+ def test_provisioning_errors_are_reported_helpfully(self):
+ error = factory.getRandomString()
+
+ def raise_provisioning_error(*args, **kwargs):
+ raise Fault(PSERV_FAULT.NO_COBBLER, error)
+
+ self.papi.patch('add_node', raise_provisioning_error)
+
+ with ExpectedException(Fault, ".*Cobbler.*"):
+ self.papi.add_node('node', 'profile', 'power', {})
+
+ def test_present_user_friendly_fault_describes_pserv_fault(self):
+ self.assertIn(
+ "provisioning server",
+ present_user_friendly_fault(Fault(8002, 'error')).faultString)
+
+ def test_present_user_friendly_fault_describes_cobbler_fault(self):
+ fault = Fault(PSERV_FAULT.NO_COBBLER, factory.getRandomString())
+ self.assertIn(
+ "Cobbler", present_user_friendly_fault(fault).faultString)
+
+ def test_present_user_friendly_fault_describes_cobbler_auth_fail(self):
+ random_text = factory.getRandomString()
+ fault = Fault(PSERV_FAULT.COBBLER_AUTH_FAILED, random_text)
+ self.assertIn(
+ "Cobbler", present_user_friendly_fault(fault).faultString)
+
+ def test_present_user_friendly_fault_describes_cobbler_auth_error(self):
+ random_text = factory.getRandomString()
+ fault = Fault(PSERV_FAULT.COBBLER_AUTH_ERROR, random_text)
+ self.assertIn(
+ "Cobbler", present_user_friendly_fault(fault).faultString)
+
+ def test_present_user_friendly_fault_describes_missing_profile(self):
+ random_text = factory.getRandomString()
+ fault = Fault(PSERV_FAULT.NO_SUCH_PROFILE, random_text)
+ self.assertIn(
+ "maas-import-isos",
+ present_user_friendly_fault(fault).faultString)
+
+ def test_present_user_friendly_fault_describes_generic_cobbler_fail(self):
+ random_text = factory.getRandomString()
+ fault = Fault(PSERV_FAULT.GENERIC_COBBLER_ERROR, random_text)
+ friendly_fault = present_user_friendly_fault(fault)
+ self.assertIn("Cobbler", friendly_fault.faultString)
+ self.assertIn(random_text, friendly_fault.faultString)
+
+ def test_present_user_friendly_fault_returns_None_for_other_fault(self):
+ self.assertIsNone(
+ present_user_friendly_fault(Fault(9999, "Kaboomski.")))
+
class TestProvisioningWithFake(ProvisioningTests, TestCase):
"""Tests for the Provisioning API using a fake."""
=== renamed file 'src/maasserver/tests/test_zeroconfservice.py' => 'src/maasserver/tests/test_zeroconfservice.py.THIS'
=== removed file 'src/maasserver/zeroconfservice.py'
--- src/maasserver/zeroconfservice.py 2012-03-20 21:16:06 +0000
+++ src/maasserver/zeroconfservice.py 1970-01-01 00:00:00 +0000
@@ -1,62 +0,0 @@
-# Copyright 2008 (c) Pierre Duquesne <stackp@xxxxxxxxx>
-# Copyright 2012 Canonical Ltd.
-# This software is licensed under the GNU Affero General Public
-# License version 3 (see the file LICENSE).
-# Example code taken from http://stackp.online.fr/?p=35
-
-import avahi
-import dbus
-
-__all__ = [
- "ZeroconfService",
- ]
-
-
-class ZeroconfService:
- """A simple class to publish a network service with zeroconf using avahi.
- """
-
- def __init__(self, name, port, stype="_http._tcp",
- domain="", host="", text=""):
- """Create an object that can publish a service over Avahi.
-
- :param name: The name of the service to be published.
- :param port: The port number where it's published.
- :param stype: The service type string.
- """
- self.name = name
- self.stype = stype
- self.domain = domain
- self.host = host
- self.port = port
- self.text = text
-
- def publish(self):
- """Publish the service through Avahi."""
- bus = dbus.SystemBus()
- server = dbus.Interface(
- bus.get_object(avahi.DBUS_NAME, avahi.DBUS_PATH_SERVER),
- avahi.DBUS_INTERFACE_SERVER)
-
- group = dbus.Interface(
- bus.get_object(avahi.DBUS_NAME, server.EntryGroupNew()),
- avahi.DBUS_INTERFACE_ENTRY_GROUP)
-
- group.AddService(
- avahi.IF_UNSPEC, avahi.PROTO_UNSPEC, dbus.UInt32(0),
- self.name, self.stype, self.domain, self.host,
- dbus.UInt16(self.port), self.text)
-
- group.Commit()
- self.group = group
-
- def unpublish(self):
- """Unpublish the service through Avahi."""
- self.group.Reset()
-
-
-if __name__ == "__main__":
- service = ZeroconfService(name="TestService", port=3000)
- service.publish()
- raw_input("Press any key to unpublish the service ")
- service.unpublish()
Follow ups
