launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/fake_provisioning into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/fake_provisioning/+merge/98579

This shaves a yak that just held me up for about a day.  The cause was non-obvious code in how we obtain a proxy for talking to the provisioning server.

In pseudo-code, here's how maasserver currently does that:

def pseudo_get_provisioning_api_proxy():
    use_fake = (settings.PSERV_URL is None)
    if use_fake:
        return make_a_fake_provisioning_api_proxy()
    else:
        return make_a_real_provisioning_api_proxy(settings.PSERV_URL)

In more detail, make_a_fake_provisioning_api_proxy() does:

    logger.warn("PSERV_URL is not set; using a fake provisioning server.")
    try:
        from maasserver.testing import get_fake_provisioning_api_proxy
    except ImportError:
        # We must be here because maasserver.testing wasn't available,
        # which probably means that we're on a production system,
        # which means that PSERV_URL should have been set.
        raise RuntimeError("PSERV_URL should be set.")

The meaning of the actual code took some figuring out because PSERV_URL combines two meanings (“here's pserv's URL” and “use a real/fake pserv”), leaving the code to figure out the configuration's implicit intent.  It needs to be intelligent about that, and I hate & distrust intelligent code.  That's why we've got a log message and a tenuous error to deal with mismatches between the inferred intent and the environment.  All this needs to be more direct and less ambiguous.

But to get back to the story:

In the other branch I was working on, I needed to wrap the pserv proxy but hit very strange problems.  Figuring this out took most of the lost time.  As it turns out, one of our many TestCase classes does this:

    def setUp(self):
        super(me, self).setUp()
        self.papi = get_fake_provisioning_api()
        self.patch(maasserver.provisioning, 'get_provisioning_api_proxy', lambda: self.papi)

(The real code is a bit more complicated; it uses MonkeyPatch and useFixture instead of self.patch, but I don't know why.)

And thus, _in some of the tests_, get_provisioning_api_proxy wasn't being called at all!  The purpose of this monkey-patch is to make both the test code and the code that it tests use the same fake pserv, so as to simulate a coherent session.  The tested code may call get_provisioning_api_proxy in any number of places, and the tests will want to access the provisioning API proxy, and within a given test it should always be the same proxy.

But don't you _always_ want a coherent session even when you use a fake pserv?  In production the proxy is stateless but threaded, so you need to produce a fresh proxy on every call, but in testing you want a singleton.  In this branch, I changed things as follows:

 * Move the singleton nature of the fake pserv proxy into get_fake_provisioning_api_proxy.

 * Separate the “use a real/fake pserv” setting from the “here's pserv's URL” setting.
  - PSERV_URL can always be pre-set to the default, in any of our configs.
  - If you say you want a real pserv, not setting PSERV_URL is an error plain and simple.
  - If you say you want a fake pserv, you don't need a logged warning about it.
  - Using a fake pserv in production breaks.  Tough cookies.
  - Using a real pserv when you don't actually have one breaks.  More tough cookies.

 * Add a “reset” switch to the global fake pserv proxy, and push it between tests.

Yes, a global singleton is a bit nasty.  I'd be happy to hear about better ideas.  But bear in mind that any test that needs the reset, is also written in such a way that it won't work against a real provisioning server that may carry previous state.  So that's probably best avoided anyway.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/fake_provisioning/+merge/98579
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/fake_provisioning into lp:maas.

=== modified file 'src/maas/demo.py'
--- src/maas/demo.py	2012-03-19 12:23:46 +0000
+++ src/maas/demo.py	2012-03-21 06:16:39 +0000
@@ -39,9 +39,6 @@
 # enable it.
 LONGPOLL_PATH = None
 
-# This should match the setting in /etc/pserv.yaml.
-PSERV_URL = "http://localhost:5241/api";
-
 MAAS_CLI = os.path.join(os.getcwd(), 'bin', 'maas')
 
 LOGGING = {

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2012-03-19 21:40:38 +0000
+++ src/maas/development.py	2012-03-21 06:16:39 +0000
@@ -34,6 +34,9 @@
 # cluster is running in the branch.
 TEST_RUNNER = 'maastesting.runner.TestRunner'
 
+# Use a fake provisioning server for test/demo purposes.
+USE_REAL_PSERV = False
+
 # Invalid strings should be visible.
 TEMPLATE_STRING_IF_INVALID = '#### INVALID STRING ####'
 

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-03-19 21:40:38 +0000
+++ src/maas/settings.py	2012-03-21 06:16:39 +0000
@@ -252,9 +252,14 @@
     'version': 1,
 }
 
-# The location of the Provisioning API XML-RPC endpoint. If PSERV_URL is None,
-# use the fake Provisioning API.
-PSERV_URL = None
+# The location of the Provisioning API XML-RPC endpoint.  This should
+# match the setting in /etc/pserv.yaml.
+PSERV_URL = "http://localhost:5241/api";
+
+# 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.
+USE_REAL_PSERV = True
 
 # Allow the user to override settings in maas_local_settings.
 import_local_settings()

=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py	2012-03-19 06:04:57 +0000
+++ src/maasserver/provisioning.py	2012-03-21 06:16:39 +0000
@@ -16,7 +16,6 @@
 from textwrap import dedent
 from urllib import urlencode
 from urlparse import urljoin
-import warnings
 import xmlrpclib
 
 from django.conf import settings
@@ -41,21 +40,17 @@
     implementation. This will not be available in a packaged version of MAAS,
     in which case an error is raised.
     """
-    url = settings.PSERV_URL
-    if url is None:
-        try:
-            from maasserver import testing
-        except ImportError:
-            # This is probably in a package.
-            raise RuntimeError("PSERV_URL must be defined.")
-        else:
-            warnings.warn(
-                "PSERV_URL is None; using the fake Provisioning API.",
-                RuntimeWarning)
-            return testing.get_fake_provisioning_api_proxy()
+    if settings.USE_REAL_PSERV:
+        # Use a real provisioning server.  This requires PSERV_URL to be
+        # set.
+        return xmlrpclib.ServerProxy(
+            settings.PSERV_URL, allow_none=True, use_datetime=True)
     else:
-        return xmlrpclib.ServerProxy(
-            url, allow_none=True, use_datetime=True)
+        # 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()
 
 
 def get_metadata_server_url():

=== modified file 'src/maasserver/testing/__init__.py'
--- src/maasserver/testing/__init__.py	2012-03-13 05:34:38 +0000
+++ src/maasserver/testing/__init__.py	2012-03-21 06:16:39 +0000
@@ -19,16 +19,38 @@
 
 from provisioningserver.testing import fakeapi
 
+# Current (singleton) fake provisioning API server.
+fake_provisioning_proxy = None
+
 
 def get_fake_provisioning_api_proxy():
-    papi_fake = fakeapi.FakeSynchronousProvisioningAPI()
-    distro = papi_fake.add_distro(
-        "distro-%s" % uuid1().get_hex(),
-        "initrd", "kernel")
-    papi_fake.add_profile(
-        "profile-%s" % uuid1().get_hex(),
-        distro)
-    return papi_fake
+    """Produce a fake provisioning API proxy.
+
+    The fake server is a singleton, so as to provide a realistically coherent
+    fake session.  If you want a clean slate, call
+    `reset_fake_provisioning_api_proxy` and the next call here will create a
+    fresh instance.
+    """
+    global fake_provisioning_proxy
+    if fake_provisioning_proxy is None:
+        fake_provisioning_proxy = fakeapi.FakeSynchronousProvisioningAPI()
+        distro = fake_provisioning_proxy.add_distro(
+            "distro-%s" % uuid1().get_hex(),
+            "initrd", "kernel")
+        fake_provisioning_proxy.add_profile(
+            "profile-%s" % uuid1().get_hex(),
+            distro)
+    return fake_provisioning_proxy
+
+
+def reset_fake_provisioning_api_proxy():
+    """Reset the fake provisioning API server.
+
+    The next call to `get_fake_provisioning_api_proxy` will create a fresh
+    instance.
+    """
+    global fake_provisioning_proxy
+    fake_provisioning_proxy = None
 
 
 def reload_object(model_object):

=== modified file 'src/maasserver/testing/testcase.py'
--- src/maasserver/testing/testcase.py	2012-03-13 05:34:38 +0000
+++ src/maasserver/testing/testcase.py	2012-03-21 06:16:39 +0000
@@ -15,8 +15,7 @@
     'TestModelTestCase',
     ]
 
-from fixtures import MonkeyPatch
-from maasserver.testing import get_fake_provisioning_api_proxy
+from maasserver.testing import reset_fake_provisioning_api_proxy
 from maasserver.testing.factory import factory
 import maastesting.testcase
 
@@ -25,11 +24,7 @@
 
     def setUp(self):
         super(TestCase, self).setUp()
-        papi_fake = get_fake_provisioning_api_proxy()
-        papi_fake_fixture = MonkeyPatch(
-            "maasserver.provisioning.get_provisioning_api_proxy",
-            lambda: papi_fake)
-        self.useFixture(papi_fake_fixture)
+        self.addCleanup(reset_fake_provisioning_api_proxy)
 
 
 class TestModelTestCase(TestCase, maastesting.testcase.TestModelTestCase):