launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07079
[Merge] lp:~rvb/maas/use-long-lasting-warnings into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/use-long-lasting-warnings into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/use-long-lasting-warnings/+merge/101553
This branch uses (and cleans up) the long lasting utilities functions in src/maasserver/components.py to actually publish/remove long-lasting warning messages when exceptions are thrown/not thrown by the methods in src/maasserver/provisioning.py.
Since there is already a central point when all the papi method are called (ProvisioningCaller), I thought it would be much simpler to hook up the long lasting error mechanism in there rather than to decorate all the papi methods. This is why the decorator 'persistent_errors' is not longer needed.
Also, I changed 'register_persistent_error' so that it now takes the error_message itself rather than the error. This way this module (components.py) does not need to know how to covert an error into a friendly error message, that's done by the called and it's much cleaner.
= Pre-imp notes =
I've done a pre-imp chat with jtv about this.
--
https://code.launchpad.net/~rvb/maas/use-long-lasting-warnings/+merge/101553
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/use-long-lasting-warnings into lp:maas.
=== modified file 'src/maasserver/components.py'
--- src/maasserver/components.py 2012-04-10 15:38:04 +0000
+++ src/maasserver/components.py 2012-04-11 13:39:20 +0000
@@ -12,15 +12,11 @@
__all__ = [
"discard_persistent_error",
"get_persistent_errors",
- "persistent_errors",
"register_persistent_error",
]
-from collections import Sequence
import threading
-from maasserver.provisioning import present_user_friendly_fault
-
class COMPONENT:
COBBLER = 'cobbler server'
@@ -36,14 +32,9 @@
_PERSISTENT_ERRORS_LOCK = threading.Lock()
-def _display_fault(error):
- return present_user_friendly_fault(error)
-
-
-def register_persistent_error(component, error):
+def register_persistent_error(component, error_message):
with _PERSISTENT_ERRORS_LOCK:
global _PERSISTENT_ERRORS
- error_message = _display_fault(error)
_PERSISTENT_ERRORS[component] = error_message
@@ -55,26 +46,3 @@
def get_persistent_errors():
return _PERSISTENT_ERRORS.values()
-
-
-def persistent_errors(exceptions, component):
- """A method decorator used to report if the decorated method ran
- successfully or raised an exception. If one of the provided exception
- is raised by the decorated method, the component is marked as failing.
- If the method runs successfully, the component is maked as working (
- if any error has been previously reported for this component).
- """
- if not isinstance(exceptions, Sequence):
- exceptions = (exceptions, )
-
- def wrapper(func):
- def _wrapper(*args, **kwargs):
- try:
- res = func(*args, **kwargs)
- discard_persistent_error(component)
- return res
- except exceptions, e:
- register_persistent_error(component, e)
- raise
- return _wrapper
- return wrapper
=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py 2012-04-11 08:23:00 +0000
+++ src/maasserver/provisioning.py 2012-04-11 13:39:20 +0000
@@ -28,6 +28,11 @@
post_save,
)
from django.dispatch import receiver
+from maasserver.components import (
+ COMPONENT,
+ discard_persistent_error,
+ register_persistent_error,
+ )
from maasserver.exceptions import MAASAPIException
from maasserver.models import (
Config,
@@ -157,12 +162,36 @@
:rtype: :class:`MAASAPIException`, or None.
"""
+# A mapping method_name -> list of components.
+# For each method name, indicate the list of components that the method
+# uses. This way, when calling the method is a success, if means that
+# the related components are working properly.
+METHOD_COMPONENTS = {
+ 'add_node': [COMPONENT.PSERV, COMPONENT.COBBLER, COMPONENT.IMPORT_ISOS],
+ 'modify_nodes': [COMPONENT.PSERV, COMPONENT.COBBLER],
+ 'delete_nodes_by_name': [COMPONENT.PSERV, COMPONENT.COBBLER],
+}
+
+# A mapping exception -> component.
+# For each exception in this dict, the related component is there to
+# tell us which component will be marked as 'failing' when this
+# exception is raised.
+EXCEPTIONS_COMPONENTS = {
+ PSERV_FAULT.NO_COBBLER: COMPONENT.COBBLER,
+ PSERV_FAULT.COBBLER_AUTH_FAILED: COMPONENT.COBBLER,
+ PSERV_FAULT.COBBLER_AUTH_ERROR: COMPONENT.COBBLER,
+ PSERV_FAULT.NO_SUCH_PROFILE: COMPONENT.IMPORT_ISOS,
+ PSERV_FAULT.GENERIC_COBBLER_ERROR: COMPONENT.COBBLER,
+ 8002: COMPONENT.PSERV,
+}
+
class ProvisioningCaller:
"""Wrapper for an XMLRPC call.
- Runs xmlrpc exceptions through `present_user_friendly_fault` for better
+ - Runs xmlrpc exceptions through `present_user_friendly_fault` for better
presentation to the user.
+ - Registers failing/working components.
"""
def __init__(self, method):
@@ -170,8 +199,21 @@
def __call__(self, *args, **kwargs):
try:
- return self.method(*args, **kwargs)
+ result = self.method(*args, **kwargs)
+ # The call was a success, discard persistent errors for
+ # components referenced by this method.
+ components = METHOD_COMPONENTS.get(self.method.__name__, [])
+ for component in components:
+ discard_persistent_error(component)
+ return result
except xmlrpclib.Fault as e:
+ # Register failing component.
+ component = EXCEPTIONS_COMPONENTS.get(e.faultCode, None)
+ if component is not None:
+ detailed_friendly_fault = str(
+ present_detailed_user_friendly_fault(e))
+ register_persistent_error(component, detailed_friendly_fault)
+ # Raise a more user-friendly error.
friendly_fault = present_user_friendly_fault(e)
if friendly_fault is None:
raise
=== modified file 'src/maasserver/tests/test_components.py'
--- src/maasserver/tests/test_components.py 2012-04-10 15:47:37 +0000
+++ src/maasserver/tests/test_components.py 2012-04-11 13:39:20 +0000
@@ -19,7 +19,6 @@
COMPONENT,
discard_persistent_error,
get_persistent_errors,
- persistent_errors,
register_persistent_error,
)
from maasserver.testing.enum import map_enum
@@ -41,35 +40,34 @@
super(PersistentErrorsUtilitiesTest, self).setUp()
self._PERSISTENT_ERRORS = {}
self.patch(components, '_PERSISTENT_ERRORS', self._PERSISTENT_ERRORS)
- self.patch(components, '_display_fault', simple_error_display)
def test_register_persistent_error_registers_error(self):
- error = Exception(factory.getRandomString())
+ error_message = factory.getRandomString()
component = get_random_component()
- register_persistent_error(component, error)
+ register_persistent_error(component, error_message)
self.assertItemsEqual(
- {component: simple_error_display(error)}, self._PERSISTENT_ERRORS)
+ {component: error_message}, self._PERSISTENT_ERRORS)
def test_register_persistent_error_stores_last_error(self):
- error = Exception(factory.getRandomString())
- error2 = Exception(factory.getRandomString())
+ error_message = factory.getRandomString()
+ error_message2 = factory.getRandomString()
component = get_random_component()
- register_persistent_error(component, error)
- register_persistent_error(component, error2)
+ register_persistent_error(component, error_message)
+ register_persistent_error(component, error_message2)
self.assertItemsEqual(
- {component: simple_error_display(error2)}, self._PERSISTENT_ERRORS)
+ {component: error_message2}, self._PERSISTENT_ERRORS)
def test_discard_persistent_error_discards_error(self):
- error = Exception(factory.getRandomString())
+ error_message = factory.getRandomString()
component = get_random_component()
- register_persistent_error(component, error)
+ register_persistent_error(component, error_message)
discard_persistent_error(component)
self.assertItemsEqual({}, self._PERSISTENT_ERRORS)
def test_discard_persistent_error_can_be_called_many_times(self):
- error = Exception(factory.getRandomString())
+ error_message = factory.getRandomString()
component = get_random_component()
- register_persistent_error(component, error)
+ register_persistent_error(component, error_message)
discard_persistent_error(component)
discard_persistent_error(component)
self.assertItemsEqual({}, self._PERSISTENT_ERRORS)
@@ -77,63 +75,9 @@
def get_persistent_errors_returns_text_for_error_codes(self):
errors, components = [], []
for i in range(3):
- error = Exception(factory.getRandomString())
+ error_message = factory.getRandomString()
component = get_random_component()
- register_persistent_error(component, error)
- errors.append(error)
+ register_persistent_error(component, error_message)
+ errors.append(error_message)
components.append(component)
self.assertItemsEqual(errors, get_persistent_errors())
-
- def test_error_sensor_registers_error_if_exception_raised(self):
- error = NotImplementedError(factory.getRandomString())
- component = get_random_component()
-
- @persistent_errors(NotImplementedError, component)
- def test_method():
- raise error
-
- self.assertRaises(NotImplementedError, test_method)
- self.assertItemsEqual(
- [simple_error_display(error)], get_persistent_errors())
-
- def test_error_sensor_registers_does_not_register_unknown_error(self):
- component = get_random_component()
-
- @persistent_errors(NotImplementedError, component)
- def test_method():
- raise ValueError()
-
- self.assertRaises(ValueError, test_method)
- self.assertItemsEqual([], get_persistent_errors())
-
- def test_error_sensor_discards_error_if_method_runs_successfully(self):
- error = Exception(factory.getRandomString())
- component = get_random_component()
- register_persistent_error(component, error)
-
- @persistent_errors(NotImplementedError, component)
- def test_method():
- pass
-
- self.assertItemsEqual(
- [simple_error_display(error)], get_persistent_errors())
- test_method()
- self.assertItemsEqual([], get_persistent_errors())
-
- def test_error_sensor_does_not_discard_error_if_unknown_exception(self):
- error = Exception(factory.getRandomString())
- component = get_random_component()
- register_persistent_error(component, error)
-
- @persistent_errors(NotImplementedError, component)
- def test_method():
- raise ValueError()
-
- self.assertItemsEqual(
- [simple_error_display(error)], get_persistent_errors())
- try:
- test_method()
- except ValueError:
- pass
- self.assertItemsEqual(
- [simple_error_display(error)], get_persistent_errors())
=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py 2012-04-11 08:23:00 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-04-11 13:39:20 +0000
@@ -16,7 +16,15 @@
from xmlrpclib import Fault
from django.conf import settings
-from maasserver import provisioning
+from maasserver import (
+ components,
+ provisioning,
+ )
+from maasserver.components import (
+ COMPONENT,
+ get_persistent_errors,
+ register_persistent_error,
+ )
from maasserver.exceptions import MAASAPIException
from maasserver.models import (
ARCHITECTURE,
@@ -420,6 +428,104 @@
with ExpectedException(MAASAPIException, ".*Cobbler.*"):
self.papi.add_node('node', 'profile', 'power', '')
+ def patch_and_call_papi_method(self, fault, papi_method='add_node'):
+ def raise_provisioning_error(*args, **kwargs):
+ raise Fault(fault, factory.getRandomString())
+
+ self.papi.patch(papi_method, raise_provisioning_error)
+
+ try:
+ method = getattr(self.papi, papi_method)
+ method()
+ except MAASAPIException:
+ pass
+
+ def test_error_regitered_when_NO_COBBLER_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(PSERV_FAULT.NO_COBBLER)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "The provisioning server was unable to reach the Cobbler",
+ errors[0])
+
+ def test_error_regitered_when_COBBLER_AUTH_FAILED_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(PSERV_FAULT.COBBLER_AUTH_FAILED)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "The provisioning server failed to authenticate", errors[0])
+
+ def test_error_regitered_when_COBBLER_AUTH_ERROR_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(PSERV_FAULT.COBBLER_AUTH_ERROR)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "The Cobbler server no longer accepts the provisioning",
+ errors[0])
+
+ def test_error_regitered_when_GENERIC_COBBLER_ERROR_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(PSERV_FAULT.GENERIC_COBBLER_ERROR)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "The provisioning service encountered a problem with",
+ errors[0])
+
+ def test_error_regitered_when_NO_SUCH_PROFILE_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(PSERV_FAULT.NO_SUCH_PROFILE)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "System profile does not exist", errors[0])
+
+ def test_error_regitered_when_8002_raised(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ self.patch_and_call_papi_method(8002)
+ errors = get_persistent_errors()
+ self.assertEqual(1, len(errors))
+ self.assertIn(
+ "Unable to reach provisioning server", errors[0])
+
+ def test_failing_components_cleared_if_add_node_works(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ register_persistent_error(COMPONENT.PSERV, factory.getRandomString())
+ register_persistent_error(COMPONENT.COBBLER, factory.getRandomString())
+ register_persistent_error(
+ COMPONENT.IMPORT_ISOS, factory.getRandomString())
+ self.papi.add_node('node', 'hostname', 'profile', 'power', '')
+ self.assertEqual([], get_persistent_errors())
+
+ def test_only_failing_components_are_cleared_if_modify_nodes_works(self):
+ # Only the components listed in METHOD_COMPONENTS[method_name]
+ # are cleared with the run of method_name is successfull.
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ other_error = factory.getRandomString()
+ other_component = factory.getRandomString()
+ register_persistent_error(other_component, other_error)
+ self.papi.modify_nodes({})
+ self.assertEqual([other_error], get_persistent_errors())
+
+ def test_failing_components_cleared_if_modify_nodes_works(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ register_persistent_error(COMPONENT.PSERV, factory.getRandomString())
+ register_persistent_error(COMPONENT.COBBLER, factory.getRandomString())
+ self.papi.modify_nodes({})
+ self.assertEqual([], get_persistent_errors())
+
+ def test_failing_components_cleared_if_delete_nodes_by_name_works(self):
+ self.patch(components, '_PERSISTENT_ERRORS', {})
+ register_persistent_error(COMPONENT.PSERV, factory.getRandomString())
+ register_persistent_error(COMPONENT.COBBLER, factory.getRandomString())
+ other_error = factory.getRandomString()
+ register_persistent_error(factory.getRandomString(), other_error)
+ self.papi.modify_nodes({})
+ self.assertEqual([other_error], get_persistent_errors())
+
class TestProvisioningWithFake(ProvisioningTests, ProvisioningFakeFactory,
TestCase):
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-11 07:30:47 +0000
+++ src/maasserver/tests/test_views.py 2012-04-11 13:39:20 +0000
@@ -39,7 +39,6 @@
SSHKey,
UserProfile,
)
-from maasserver.provisioning import present_user_friendly_fault
from maasserver.testing import (
get_data,
reload_object,
@@ -1028,9 +1027,10 @@
# Create component with getRandomString to be sure
# to display all the errors.
component = factory.getRandomString()
- error = Fault(fault, factory.getRandomString())
+ error_message = factory.getRandomString()
+ error = Fault(fault, error_message)
errors.append(error)
- register_persistent_error(component, error)
+ register_persistent_error(component, error_message)
links = [
reverse('index'),
reverse('node-list'),
@@ -1041,5 +1041,6 @@
self.assertThat(
response.content,
MatchesAll(
- *[Contains(escape(str(present_user_friendly_fault(error))))
+ *[Contains(
+ escape(error.faultString))
for error in errors]))
