Merge lp:~allenap/maas/spurious-regionservice-tests into lp:~maas-committers/maas/trunk

[Gavin Panella]

Fix a couple of leaking RegionService tests, and tidy up exception handling a bit.

This was a leftover from a brief attempt to fix bug 1555236.

[Lee Trager (ltrager)]

LGTM!

review: Approve

=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2016-10-28 15:58:32 +0000
+++ src/maasserver/rpc/regionservice.py	2016-11-14 07:34:58 +0000
@@ -642,19 +642,17 @@
             # and into the database.
             self.ident = rack_controller.system_id
             self.factory.service._addConnectionFor(self.ident, self)
+
+            # A local rack is treated differently to one that's remote.
             self.host = self.transport.getHost()
             self.hostIsRemote = isinstance(
                 self.host, (IPv4Address, IPv6Address))
 
-            # Get the region ID if we're dealing with a non-local rack; we
-            # won't need to bother for local racks.
+            # Only register the connection into the database when it's a valid
+            # IPv4 or IPv6. Only time it is not an IPv4 or IPv6 address is
+            # when mocking a connection.
             if self.hostIsRemote:
                 region_id = yield getRegionID()
-
-            # Only register the connection into the database when its a valid
-            # IPv4 and IPv6. Only time it is not an IPv4 or IPv6 address is
-            # when mocking a connection.
-            if self.hostIsRemote:
                 yield deferToDatabase(
                     registerConnection, region_id, rack_controller, self.host)
 
@@ -663,22 +661,21 @@
                 "Process [%s] - registered rack controller '%s'." % (
                     os.getpid(), self.ident))
 
-            # Done registering the rack controller and connection.
-            return {'system_id': self.ident}
-        except Exception as exc:
-            # Some error occurred here. Remove the connection in the database,
-            # log the error and alert the connected rack controller. The
-            # connected rack controller will drop the connection.
-            if (self.ident is not None and self.hostIsRemote):
-                yield deferToDatabase(
-                    unregisterConnection, region_id, self.ident, self.host)
+        except:
+            # Ensure we're not hanging onto this connection.
+            self.factory.service._removeConnectionFor(self.ident, self)
+            # Tell the logs about it.
             msg = (
                 "Failed to register rack controller '%s' into the "
-                "database. Connection has been dropped." % (
-                    self.ident))
-            log.err(exc, msg)
+                "database. Connection will be dropped." % self.ident)
+            log.err(None, msg)
+            # Finally, tell the callers.
             raise exceptions.CannotRegisterRackController(msg)
 
+        else:
+            # Done registering the rack controller and connection.
+            return {'system_id': self.ident}
+
     @inlineCallbacks
     def performHandshake(self):
         authenticated = yield self.authenticateCluster()

=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2016-10-28 15:58:32 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2016-11-14 07:34:58 +0000
@@ -41,6 +41,7 @@
 from maasserver.rpc import regionservice
 from maasserver.rpc.regionservice import (
     getRegionID,
+    ignoreCancellation,
     Region,
     RegionAdvertising,
     RegionAdvertisingService,
@@ -569,7 +570,7 @@
             CannotRegisterRackController)
         self.assertEquals((
             "Failed to register rack controller 'None' into the database. "
-            "Connection has been dropped.",), error.args)
+            "Connection will be dropped.",), error.args)
 
 
 class TestRegionService(MAASTestCase):
@@ -777,6 +778,7 @@
     def test_stopping_closes_connections_cleanly(self):
         service = RegionService()
         service.starting = Deferred()
+        service.starting.addErrback(ignoreCancellation)
         service.factory.protocol = HandshakingRegionServer
         connections = {
             service.factory.buildProtocol(None),
@@ -801,6 +803,7 @@
     def test_stopping_logs_errors_when_closing_connections(self):
         service = RegionService()
         service.starting = Deferred()
+        service.starting.addErrback(ignoreCancellation)
         service.factory.protocol = HandshakingRegionServer
         connections = {
             service.factory.buildProtocol(None),