Merge lp:~allenap/maas/rpc-alt-cluster-rpc-fixture into lp:~maas-committers/maas/trunk

[Gavin Panella]

New fixture AlternativeClusterRPCFixture that enables end-to-end testing against a stub cluster RPC implementation.

The existing ClusterRPCFixture wires clients up to the "real" cluster RPC implementation, which was a bit short-sighted of me; it is increasingly dependent on external data, which will entail ever more elaborate test fixtures. It's better to work against an accurately stubbed RPC implementation and lean on CI/QA to exercise end-to-end.

[Blake Rouse (blake-rouse)]

Looks good. Just a couple of comments inline, no real blockers.

review: Approve

[Jeroen T. Vermeulen (jtv)]

From your description, this sounds like a substantial improvement. But can you find the “mot juste” (as the French would call it; English doesn't quite seem to have the right word) for the new fixture's name? “Alternative” tells me nothing more than that I'm probably looking at the second-best choice out of two.

.

Have you checked for lint? The fixture's docstring looks to me as if it's just slightly too wide.

.

Also in the docstring, why is the io.flush() needed? Is that something you'd call with the real thing?

.

Does it make sense to break from PEP8 style for addCluster and makeCluster?

.

In makeCluster, could you document the return value, and also say in a comment why the cluster needs Identify added to it?

.

I think this fixture gets to a point where it would be helpful, if possible, to have a test for the fixture itself.

review: Approve

[Gavin Panella (allenap)]

Thanks for your review Blake. I've replied to your comments.

Next: Jeroen.

[Gavin Panella (allenap)]

> From your description, this sounds like a substantial improvement.  But can
> you find the “mot juste” (as the French would call it; English doesn't quite
> seem to have the right word) for the new fixture's name?  “Alternative” tells
> me nothing more than that I'm probably looking at the second-best choice out
> of two.

I've changed it to RegionToClusterRPCFixture now.

>
> .
>
> Have you checked for lint?  The fixture's docstring looks to me as if it's
> just slightly too wide.

It's clean; the line is right up into the 78th column.

>
> .
>
> Also in the docstring, why is the io.flush() needed?  Is that something you'd
> call with the real thing?

This is needed. The iosim.connect() call wires up the client and server
outside of the reactor's control. If you need data to move between these
you need to pump it "by hand". Once you get the hang of it it's quite
easy, and powerful too; by setting debug=True on the pump or passing it
into the flush() or pump() methods you get a log of the IO passing back
and forth.

>
> .
>
> Does it make sense to break from PEP8 style for addCluster and makeCluster?

It's not strictly breaking from PEP8:

  Function names should be lowercase, with words separated by
  underscores as necessary to improve readability.

  mixedCase is allowed only in contexts where that's already the
  prevailing style (e.g. threading.py), to retain backwards
  compatibility.

In fact, we've broken from PEP8 by using underscores in all of our test
names. The fixtures library itself uses a mix of lower_case and
mixedCase names.

We have a mix of both in MAAS, and I think we have to accept it. Twisted
is all mixedCase. Plus, with tab-completion it's hardly a burden.

>
> .
>
> In makeCluster, could you document the return value, and also say in a comment
> why the cluster needs Identify added to it?

Done.

>
> .
>
> I think this fixture gets to a point where it would be helpful, if possible,
> to have a test for the fixture itself.

Agreed. I have just been yak shaving for too long and can't face any
more right now!

Thanks for the review!

[Julian Edwards (julian-edwards)]

On 09/08/14 02:02, Gavin Panella wrote:
>> From your description, this sounds like a substantial improvement. But can
>> > you find the “mot juste” (as the French would call it; English doesn't quite
>> > seem to have the right word) for the new fixture's name? “Alternative” tells
>> > me nothing more than that I'm probably looking at the second-best choice out
>> > of two.
> I've changed it to RegionToClusterRPCFixture now.
>

I know it's easy to bikeshed over names, but they are important.

We now have

 - ClusterRPCFixture
 - RegionToClusterRPCFixture

with no real indication of the difference from the names. Especially
since they are both Region-To-Cluster!

Can I suggest this new one is MockClusterRPCFixture? I don't think the
"RegionToCluster" bit adds anything.

[Gavin Panella (allenap)]

> I know it's easy to bikeshed over names, but they are important.
>
> We now have
>
> - ClusterRPCFixture
> - RegionToClusterRPCFixture
>
> with no real indication of the difference from the names. Especially
> since they are both Region-To-Cluster!
>
> Can I suggest this new one is MockClusterRPCFixture? I don't think the
> "RegionToCluster" bit adds anything.

The absence of direction in the name has confused a couple of people
that I know of so far (not seriously confused, just a double-take), so
I think the longer name has some value. ClusterRPCFixture is also
deprecated now, and I'll try to remove it when I can. It's used in
only one module so shouldn't be too much of a chore.

[Julian Edwards (julian-edwards)]

On Monday 11 Aug 2014 08:27:22 you wrote:
> > I know it's easy to bikeshed over names, but they are important.
> >
> > We now have
> >
> > - ClusterRPCFixture
> > - RegionToClusterRPCFixture
> >
> > with no real indication of the difference from the names. Especially
> > since they are both Region-To-Cluster!
> >
> > Can I suggest this new one is MockClusterRPCFixture? I don't think the
> > "RegionToCluster" bit adds anything.
>
> The absence of direction in the name has confused a couple of people
> that I know of so far (not seriously confused, just a double-take), so
> I think the longer name has some value. ClusterRPCFixture is also
> deprecated now, and I'll try to remove it when I can. It's used in
> only one module so shouldn't be too much of a chore.

Can we at least get a Mock in there so we know it's been stubbed out?

=== modified file 'src/maasserver/rpc/testing/fixtures.py'
--- src/maasserver/rpc/testing/fixtures.py	2014-07-21 11:21:00 +0000
+++ src/maasserver/rpc/testing/fixtures.py	2014-08-08 16:03:07 +0000
@@ -14,19 +14,31 @@
 __metaclass__ = type
 __all__ = [
     "ClusterRPCFixture",
+    "RegionToClusterRPCFixture",
 ]
 
 from collections import defaultdict
+from warnings import warn
 
 from crochet import run_in_reactor
 import fixtures
 from maasserver import eventloop
 from maasserver.enum import NODEGROUP_STATUS
 from maasserver.models.nodegroup import NodeGroup
-from provisioningserver.rpc import clusterservice
+from maasserver.rpc.regionservice import RegionServer
+from provisioningserver.rpc import (
+    cluster,
+    clusterservice,
+    )
 from provisioningserver.rpc.interfaces import IConnection
-from provisioningserver.rpc.testing import call_responder
+from provisioningserver.rpc.testing import (
+    call_responder,
+    make_amp_protocol_factory,
+    )
 from testtools.monkey import patch
+from twisted.internet import defer
+from twisted.internet.protocol import Factory
+from twisted.test import iosim
 from zope.interface import implementer
 
 
@@ -49,6 +61,24 @@
 
 
 class ClusterRPCFixture(fixtures.Fixture):
+    """Deprecated: use :py:class:`RegionToClusterRPCFixture` instead.
+
+    This creates connections to the "real" cluster RPC implementation,
+    but this relies on real data. This makes tests fragile, and, as time
+    progresses, will result in ever more elaborate test fixtures to get
+    that data into place.
+
+    Instead, use :py:class:`RegionToClusterRPCFixture`, which helps you
+    stub out those RPC calls that you're testing against, and verifies
+    each call's arguments *and* response against the RPC schema.
+    """
+
+    def __init__(self):
+        super(ClusterRPCFixture, self).__init__()
+        warn(
+            ("ClusterRPCFixture is deprecated; use "
+             "RegionToClusterRPCFixture instead."),
+            DeprecationWarning)
 
     def setUp(self):
         super(ClusterRPCFixture, self).setUp()
@@ -70,3 +100,78 @@
                 fake_connections[connection.ident].add(connection)
         # Patch the fake connections into place for this fixture's lifetime.
         self.addCleanup(patch(rpc_service, "connections", fake_connections))
+
+
+class RegionToClusterRPCFixture(fixtures.Fixture):
+    """Patch in a stub cluster RPC implementation to enable end-to-end testing.
+
+    Use this in *region* tests.
+
+    Example usage::
+
+      nodegroup = factory.make_node_group()
+      fixture = self.useFixture(RegionToClusterRPCFixture())
+      protocol, io = fixture.makeCluster(nodegroup, region.Identify)
+      protocol.Identify.return_value = defer.succeed({"ident": "foobar"})
+
+      client = getClientFor(nodegroup.uuid)
+      result = client(region.Identify)
+      io.flush()  # Call this in the reactor thread.
+
+      self.assertThat(result, ...)
+
+    """
+
+    def setUp(self):
+        super(RegionToClusterRPCFixture, self).setUp()
+        # We need the event-loop up and running.
+        if not eventloop.loop.running:
+            raise RuntimeError(
+                "Please start the event-loop before using this fixture.")
+        self.rpc = get_service_in_eventloop("rpc").wait(10)
+        # The RPC service uses a defaultdict(set) to manage connections, but
+        # let's check those assumptions.
+        assert isinstance(self.rpc.connections, defaultdict)
+        assert self.rpc.connections.default_factory is set
+        # Patch a fake connections dict into place for this fixture's lifetime.
+        self.addCleanup(patch(self.rpc, "connections", defaultdict(set)))
+
+    def addCluster(self, protocol):
+        """Add a new stub cluster using the given `protocol`.
+
+        The `protocol` should be an instance of `amp.AMP`.
+
+        :returns: py:class:`twisted.test.iosim.IOPump`
+        """
+        server_factory = Factory.forProtocol(RegionServer)
+        server_factory.service = self.rpc
+        server = server_factory.buildProtocol(addr=None)
+        return iosim.connect(
+            server, iosim.makeFakeServer(server),
+            protocol, iosim.makeFakeClient(protocol),
+            debug=False,  # Debugging is useful, but too noisy by default.
+        )
+
+    def makeCluster(self, nodegroup, *commands):
+        """Make and add a new stub cluster connection with the `commands`.
+
+        See `make_amp_protocol_factory` for details.
+
+        Note that if the ``Identify`` call is not amongst `commands`, it will
+        be added. In addition, its return value is also set to return the UUID
+        of `nodegroup`. There's a good reason: the first thing that
+        `RegionServer` does when a connection is made is call `Identify`. This
+        has to succeed or the connection will never been added to the RPC
+        service's list of connections.
+
+        :returns: A 2-tuple of the protocol instance created and the
+            py:class:`twisted.test.iosim.IOPump` as returned by `addCluster`.
+        """
+        if cluster.Identify not in commands:
+            commands = commands + (cluster.Identify,)
+        protocol_factory = make_amp_protocol_factory(*commands)
+        protocol = protocol_factory()
+        ident_response = {"ident": nodegroup.uuid.decode("ascii")}
+        protocol.Identify.side_effect = (
+            lambda protocol: defer.succeed(ident_response.copy()))
+        return protocol, self.addCluster(protocol)

=== modified file 'src/provisioningserver/rpc/testing/__init__.py'
--- src/provisioningserver/rpc/testing/__init__.py	2014-07-29 12:21:29 +0000
+++ src/provisioningserver/rpc/testing/__init__.py	2014-08-08 16:03:07 +0000
@@ -15,6 +15,7 @@
 __all__ = [
     "are_valid_tls_parameters",
     "call_responder",
+    "ClusterToRegionRPCFixture",
     "make_amp_protocol_factory",
     "TwistedLoggerFixture",
 ]
@@ -106,14 +107,14 @@
 })
 
 
-class RegionRPCFixture(fixtures.Fixture):
+class ClusterToRegionRPCFixture(fixtures.Fixture):
     """Patch in a stub region RPC implementation to enable end-to-end testing.
 
     Use this in *cluster* tests.
 
     Example usage::
 
-      fixture = self.useFixture(RegionRPCFixture())
+      fixture = self.useFixture(ClusterToRegionRPCFixture())
       protocol, io = fixture.makeEventLoop(region.Identify)
       protocol.Identify.return_value = defer.succeed({"ident": "foobar"})
 
@@ -126,7 +127,7 @@
     """
 
     def setUp(self):
-        super(RegionRPCFixture, self).setUp()
+        super(ClusterToRegionRPCFixture, self).setUp()
         # If services are running, what do we do with any existing RPC
         # service? Do we shut it down and patch in? Do we just patch in and
         # move the running service aside? If it's not running, do we patch

=== modified file 'src/provisioningserver/rpc/tests/test_power.py'
--- src/provisioningserver/rpc/tests/test_power.py	2014-08-05 15:48:54 +0000
+++ src/provisioningserver/rpc/tests/test_power.py	2014-08-08 16:03:07 +0000
@@ -36,7 +36,7 @@
     power,
     region,
     )
-from provisioningserver.rpc.testing import RegionRPCFixture
+from provisioningserver.rpc.testing import ClusterToRegionRPCFixture
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
@@ -50,7 +50,7 @@
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
 
     def patch_rpc_methods(self):
-        fixture = self.useFixture(RegionRPCFixture())
+        fixture = self.useFixture(ClusterToRegionRPCFixture())
         protocol, io = fixture.makeEventLoop(
             region.MarkNodeBroken, region.UpdateNodePowerState,
             region.SendEvent)
@@ -205,7 +205,7 @@
         return power_action, power_action_obj_execute
 
     def patch_rpc_methods(self, return_value={}, side_effect=None):
-        fixture = self.useFixture(RegionRPCFixture())
+        fixture = self.useFixture(ClusterToRegionRPCFixture())
         protocol, io = fixture.makeEventLoop(
             region.MarkNodeBroken, region.UpdateNodePowerState,
             region.SendEvent)
@@ -434,7 +434,7 @@
         return power_action, power_action_obj_execute
 
     def patch_rpc_methods(self, return_value={}, side_effect=None):
-        fixture = self.useFixture(RegionRPCFixture())
+        fixture = self.useFixture(ClusterToRegionRPCFixture())
         protocol, io = fixture.makeEventLoop(
             region.MarkNodeBroken, region.SendEvent)
         protocol.MarkNodeBroken.return_value = return_value

=== modified file 'src/provisioningserver/tests/test_events.py'
--- src/provisioningserver/tests/test_events.py	2014-07-30 14:57:14 +0000
+++ src/provisioningserver/tests/test_events.py	2014-08-08 16:03:07 +0000
@@ -31,7 +31,7 @@
     )
 from provisioningserver.rpc import region
 from provisioningserver.rpc.exceptions import NoSuchEventType
-from provisioningserver.rpc.testing import RegionRPCFixture
+from provisioningserver.rpc.testing import ClusterToRegionRPCFixture
 from provisioningserver.utils import map_enum
 from testtools.deferredruntest import AsynchronousDeferredRunTest
 from testtools.matchers import (
@@ -54,7 +54,7 @@
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
 
     def patch_rpc_methods(self, side_effect=None):
-        fixture = self.useFixture(RegionRPCFixture())
+        fixture = self.useFixture(ClusterToRegionRPCFixture())
         protocol, io = fixture.makeEventLoop(
             region.SendEvent, region.RegisterEventType)
         protocol.SendEvent.side_effect = side_effect