launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06835
[Merge] lp:~jtv/maas/better-provisioning-errors-2 into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/better-provisioning-errors-2 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/better-provisioning-errors-2/+merge/98775
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-2/+merge/98775
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/better-provisioning-errors-2 into lp:maas.
=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py 2012-03-19 06:04:57 +0000
+++ src/maasserver/exceptions.py 2012-03-22 03:11:17 +0000
@@ -30,10 +30,6 @@
"""User can't be deleted."""
-class MissingProfileException(MAASException):
- """System profile does not exist."""
-
-
class MAASAPIException(Exception):
"""Base class for MAAS' API exceptions.
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-21 14:20:54 +0000
+++ src/maasserver/models.py 2012-03-22 03:11:17 +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.
@@ -293,9 +289,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):
@@ -316,13 +311,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 13:03:25 +0000
+++ src/maasserver/provisioning.py 2012-03-22 03:11:17 +0000
@@ -11,6 +11,7 @@
__metaclass__ = type
__all__ = [
'get_provisioning_api_proxy',
+ 'ProvisioningProxy',
]
from logging import getLogger
@@ -25,7 +26,6 @@
post_save,
)
from django.dispatch import receiver
-from maasserver.exceptions import MissingProfileException
from maasserver.models import (
Config,
MACAddress,
@@ -34,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.
@@ -44,7 +153,7 @@
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
@@ -59,7 +168,9 @@
"USE_REAL_PSERV to False, on an installed MAAS. "
"Don't do either.")
raise
- return get_fake_provisioning_api_proxy()
+ xmlrpc_proxy = get_fake_provisioning_api_proxy()
+
+ return ProvisioningProxy(xmlrpc_proxy)
def get_metadata_server_url():
@@ -123,18 +234,7 @@
profile = select_profile_for_node(instance, papi)
power_type = instance.get_effective_power_type()
metadata = compose_metadata(instance)
- try:
- papi.add_node(instance.system_id, profile, power_type, metadata)
- except xmlrpclib.Fault as e:
- if e.faultCode == PSERV_FAULT.NO_SUCH_PROFILE:
- raise MissingProfileException(dedent("""
- System profile %s does not exist. 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.
- """ % profile).lstrip('\n'))
- else:
- raise
+ papi.add_node(instance.system_id, profile, power_type, metadata)
def set_node_mac_addresses(node):
=== 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-22 03:11:17 +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.
=== 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-22 03:11:17 +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-21 14:45:42 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-03-22 03:11:17 +0000
@@ -15,7 +15,6 @@
from xmlrpclib import Fault
from maasserver import provisioning
-from maasserver.exceptions import MissingProfileException
from maasserver.models import (
ARCHITECTURE,
Config,
@@ -27,6 +26,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,9 +125,9 @@
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.*')
+ Fault, value_re='.*maas-import-isos.*')
with expectation:
node = factory.make_node(architecture='amd32k')
provisioning.provision_post_save_Node(
@@ -139,7 +139,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 +235,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."""
