launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/maas-pserv-shorter-timeout into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/maas-pserv-shorter-timeout/+merge/101769
-- 
https://code.launchpad.net/~allenap/maas/maas-pserv-shorter-timeout/+merge/101769
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/maas-pserv-shorter-timeout into lp:maas.

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-04-12 04:16:23 +0000
+++ src/maas/settings.py	2012-04-12 16:23:22 +0000
@@ -257,6 +257,9 @@
 # match the setting in etc/pserv.yaml.
 PSERV_URL = "http://%s:test@localhost:5241/api"; % getuser()
 
+# Time-out for socket operations against the Provisioning API.
+PSERV_TIMEOUT = 7.0  # seconds.
+
 # Use a real provisioning server?  If yes, the URL for the provisioning
 # server's API should be set in PSERV_URL.  If this is set to False, for
 # testing or demo purposes, MAAS will use an internal fake service.

=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py	2012-04-12 14:30:56 +0000
+++ src/maasserver/provisioning.py	2012-04-12 16:23:22 +0000
@@ -233,13 +233,9 @@
         self.method_name = method_name
         self.method = method
 
-    def __call__(self, *args, **kwargs):
+    def __call__(self, *args):
         try:
-            result = self.method(*args, **kwargs)
-            # The call was a success, discard persistent errors for
-            # components referenced by this method.
-            register_working_components(self.method_name)
-            return result
+            result = self.method(*args)
         except xmlrpclib.Fault as e:
             # Register failing component.
             register_failing_component(e)
@@ -249,6 +245,11 @@
                 raise
             else:
                 raise friendly_fault
+        else:
+            # The call was a success, discard persistent errors for
+            # components referenced by this method.
+            register_working_components(self.method_name)
+            return result
 
 
 class ProvisioningProxy:
@@ -262,18 +263,32 @@
     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:
+        if callable(attribute):
+            # This attribute is callable.  Wrap it in a caller.
+            return ProvisioningCaller(attribute_name, attribute)
+        else:
             # This is a regular attribute.  Return it as-is.
             return attribute
-        else:
-            # This attribute is callable.  Wrap it in a caller.
-            return ProvisioningCaller(attribute_name, attribute)
+
+
+class ProvisioningTransport(xmlrpclib.Transport):
+    """An XML-RPC transport that sets a low socket timeout."""
+
+    @property
+    def timeout(self):
+        return settings.PSERV_TIMEOUT
+
+    def make_connection(self, host):
+        """See `xmlrpclib.Transport.make_connection`.
+
+        This also sets the desired socket timeout.
+        """
+        connection = xmlrpclib.Transport.make_connection(self, host)
+        connection.timeout = self.timeout
+        return connection
 
 
 def get_provisioning_api_proxy():
@@ -286,8 +301,9 @@
     if settings.USE_REAL_PSERV:
         # Use a real provisioning server.  This requires PSERV_URL to be
         # set.
+        xmlrpc_transport = ProvisioningTransport(use_datetime=True)
         xmlrpc_proxy = xmlrpclib.ServerProxy(
-            settings.PSERV_URL, allow_none=True, use_datetime=True)
+            settings.PSERV_URL, transport=xmlrpc_transport, allow_none=True)
     else:
         # Create a fake.  The code that provides the testing fake is not
         # available in an installed production system, so import it only

=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py	2012-04-12 14:07:18 +0000
+++ src/maasserver/tests/test_provisioning.py	2012-04-12 16:23:22 +0000
@@ -43,6 +43,7 @@
     name_arch_in_cobbler_style,
     present_detailed_user_friendly_fault,
     present_user_friendly_fault,
+    ProvisioningTransport,
     select_profile_for_node,
     SHORT_PRESENTATIONS,
     )
@@ -322,7 +323,7 @@
         def raise_missing_profile(*args, **kwargs):
             raise Fault(PSERV_FAULT.NO_SUCH_PROFILE, "Unknown profile.")
 
-        self.papi.patch('add_node', raise_missing_profile)
+        self.patch(self.papi.proxy, 'add_node', raise_missing_profile)
         with ExpectedException(MAASAPIException):
             node = factory.make_node(architecture='amd32k')
             provisioning.provision_post_save_Node(
@@ -333,7 +334,7 @@
         def raise_fault(*args, **kwargs):
             raise Fault(PSERV_FAULT.NO_COBBLER, factory.getRandomString())
 
-        self.papi.patch('add_node', raise_fault)
+        self.patch(self.papi.proxy, 'add_node', raise_fault)
         with ExpectedException(MAASAPIException):
             node = factory.make_node(architecture='amd32k')
             provisioning.provision_post_save_Node(
@@ -413,7 +414,7 @@
         def raise_fault(*args, **kwargs):
             raise Fault(8002, factory.getRandomString())
 
-        self.papi.patch('add_node', raise_fault)
+        self.patch(self.papi.proxy, 'add_node', raise_fault)
 
         with ExpectedException(MAASAPIException, ".*provisioning server.*"):
             self.papi.add_node('node', 'profile', 'power', '')
@@ -423,7 +424,7 @@
         def raise_provisioning_error(*args, **kwargs):
             raise Fault(PSERV_FAULT.NO_COBBLER, factory.getRandomString())
 
-        self.papi.patch('add_node', raise_provisioning_error)
+        self.patch(self.papi.proxy, 'add_node', raise_provisioning_error)
 
         with ExpectedException(MAASAPIException, ".*Cobbler.*"):
             self.papi.add_node('node', 'profile', 'power', '')
@@ -434,7 +435,7 @@
         def raise_provisioning_error(*args, **kwargs):
             raise Fault(fault_code, factory.getRandomString())
 
-        self.papi.patch(papi_method, raise_provisioning_error)
+        self.patch(self.papi.proxy, papi_method, raise_provisioning_error)
 
         try:
             method = getattr(self.papi, papi_method)
@@ -559,3 +560,15 @@
         preseed = self.papi.nodes[node.system_id]['ks_meta']['MAAS_PRESEED']
         self.assertEqual(
             compose_cloud_init_preseed(token), b64decode(preseed))
+
+
+class TestProvisioningTransport(TestCase):
+    """Tests for :class:`ProvisioningTransport`."""
+
+    def test_make_connection(self):
+        transport = ProvisioningTransport()
+        connection = transport.make_connection("nowhere.example.com")
+        # The connection has not yet been established.
+        self.assertIsNone(connection.sock)
+        # The desired timeout has been modified.
+        self.assertEqual(transport.timeout, connection.timeout)