Merge lp:~allenap/maas/rpc-rapid-connections into lp:~maas-committers/maas/trunk

[Gavin Panella]

Shorten the time taken for a cluster to initially connect to the region via RPC to around 2 seconds.

Previously it could take between 30 and 90 seconds.

Changes the RPC client service on the cluster to more rapidly connect to the region.

[Raphaël Badin (rvb)]

Looks good. Nice job documenting all the different code paths.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/rpc-rapid-connections into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [58.5 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:3 http://nova.clouds.archive.ubuntu.com trusty Release.gpg [933 B]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Get:5 http://nova.clouds.archive.ubuntu.com trusty Release [58.5 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Get:7 http://security.ubuntu.com trusty-security/main Sources [12.0 kB]
Get:8 http://security.ubuntu.com trusty-security/universe Sources [2,873 B]
Get:9 http://nova.clouds.archive.ubuntu.com trusty/main Sources [1,064 kB]
Get:10 http://security.ubuntu.com trusty-security/main amd64 Packages [37.9 kB]
Get:11 http://security.ubuntu.com trusty-security/universe amd64 Packages [13.6 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Get:12 http://security.ubuntu.com trusty-security/universe Translation-en [6,799 B]
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:13 http://nova.clouds.archive.ubuntu.com trusty/universe Sources [6,399 kB]
Get:14 http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages [1,350 kB]
Get:15 http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages [5,859 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:16 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [26.5 kB]
Get:17 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [16.2 kB]
Get:18 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [66.1 kB]
Get:19 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [39.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 15.1 MB in 7s (2,046 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-sout...

[Julian Edwards (julian-edwards)]

On 10/05/14 04:54, <email address hidden> wrote:
> The proposal to merge lp:~allenap/maas/rpc-rapid-connections into lp:maas has been updated.
>
> Status: Approved => Merged
>
> For more details, see:
> https://code.launchpad.net/~allenap/maas/rpc-rapid-connections/+merge/219030
>

I think this is worthy of a backport to the 1.5 branch.

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2014-03-28 15:17:34 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2014-05-09 17:01:43 +0000
@@ -221,9 +221,13 @@
         instances connected to it.
     """
 
+    INTERVAL_LOW = 2  # seconds.
+    INTERVAL_MID = 10  # seconds.
+    INTERVAL_HIGH = 30  # seconds.
+
     def __init__(self, reactor):
         super(ClusterClientService, self).__init__(
-            self._get_random_interval(), self.update)
+            self._calculate_interval(None, None), self.update)
         self.connections = {}
         self.clock = reactor
 
@@ -248,9 +252,6 @@
         This obtains a list of endpoints from the region then connects
         to new ones and drops connections to those no longer used.
         """
-        # 0. Update interval.
-        self._update_interval()
-        # 1. Obtain RPC endpoints.
         try:
             info_url = self._get_rpc_info_url()
             info_page = yield getPage(info_url)
@@ -258,9 +259,13 @@
             eventloops = info["eventloops"]
             yield self._update_connections(eventloops)
         except ConnectError as error:
+            self._update_interval(None, len(self.connections))
             log.msg("Region not available: %s" % (error,))
         except:
+            self._update_interval(None, len(self.connections))
             log.err()
+        else:
+            self._update_interval(len(eventloops), len(self.connections))
 
     @staticmethod
     def _get_rpc_info_url():
@@ -270,14 +275,39 @@
         url = url.geturl()
         return ascii_url(url)
 
-    @staticmethod
-    def _get_random_interval():
-        """Return a random interval between 30 and 90 seconds."""
-        return random.randint(30, 90)
-
-    def _update_interval(self):
-        """Change the interval randomly to avoid stampedes of clusters."""
-        self._loop.interval = self.step = self._get_random_interval()
+    def _calculate_interval(self, num_eventloops, num_connections):
+        """Calculate the update interval.
+
+        The interval is `INTERVAL_LOW` seconds when there are no
+        connections, so that this can quickly obtain its first
+        connection.
+
+        The interval changes to `INTERVAL_MID` seconds when there are
+        some connections, but fewer than there are event-loops.
+
+        After that it drops back to `INTERVAL_HIGH` seconds.
+        """
+        if num_eventloops is None:
+            # The region is not available; keep trying regularly.
+            return self.INTERVAL_LOW
+        elif num_eventloops == 0:
+            # The region is coming up; keep trying regularly.
+            return self.INTERVAL_LOW
+        elif num_connections == 0:
+            # No connections to the region; keep trying regularly.
+            return self.INTERVAL_LOW
+        elif num_connections < num_eventloops:
+            # Some connections to the region, but not to all event
+            # loops; keep updating reasonably frequently.
+            return self.INTERVAL_MID
+        else:
+            # Fully connected to the region; update every so often.
+            return self.INTERVAL_HIGH
+
+    def _update_interval(self, num_eventloops, num_connections):
+        """Change the update interval."""
+        self._loop.interval = self.step = self._calculate_interval(
+            num_eventloops, num_connections)
 
     @inlineCallbacks
     def _update_connections(self, eventloops):
 
=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2014-03-28 04:31:32 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2014-05-09 17:01:43 +0000
@@ -254,40 +254,6 @@
         observed_rpc_info_url = ClusterClientService._get_rpc_info_url()
         self.assertThat(observed_rpc_info_url, Equals(expected_rpc_info_url))
 
-    def test__get_random_interval(self):
-        # _get_random_interval() returns a random number between 30 and
-        # 90 inclusive.
-        is_between_30_and_90_inclusive = MatchesAll(
-            MatchesAny(GreaterThan(30), Equals(30)),
-            MatchesAny(LessThan(90), Equals(90)))
-        for _ in range(100):
-            self.assertThat(
-                ClusterClientService._get_random_interval(),
-                is_between_30_and_90_inclusive)
-
-    def test__get_random_interval_calls_into_standard_library(self):
-        # _get_random_interval() depends entirely on the standard library.
-        random = self.patch(clusterservice, "random")
-        random.randint.return_value = sentinel.randint
-        self.assertIs(
-            sentinel.randint,
-            ClusterClientService._get_random_interval())
-        self.assertThat(random.randint, MockCalledOnceWith(30, 90))
-
-    def test__update_interval(self):
-        service = ClusterClientService(Clock())
-        # ClusterClientService's superclass, TimerService, creates a
-        # LoopingCall with now=True. We neuter it here because we only
-        # want to observe the behaviour of _update_interval().
-        service.call = (lambda: None, (), {})
-        service.startService()
-        self.assertThat(service.step, MatchesAll(
-            Equals(service._loop.interval), IsInstance(int)))
-        service.step = service._loop.interval = sentinel.undefined
-        service._update_interval()
-        self.assertThat(service.step, MatchesAll(
-            Equals(service._loop.interval), IsInstance(int)))
-
     def test_update_connect_error_is_logged_tersely(self):
         getPage = self.patch(clusterservice, "getPage")
         getPage.side_effect = error.ConnectionRefusedError()
@@ -512,6 +478,53 @@
             service.getClient)
 
 
+class TestClusterClientServiceIntervals(MAASTestCase):
+
+    scenarios = (
+        ("initial", {
+            "num_eventloops": None,
+            "num_connections": None,
+            "expected": ClusterClientService.INTERVAL_LOW,
+        }),
+        ("no-event-loops", {
+            "num_eventloops": 0,
+            "num_connections": sentinel.undefined,
+            "expected": ClusterClientService.INTERVAL_LOW,
+        }),
+        ("no-connections", {
+            "num_eventloops": 1,  # anything > 1.
+            "num_connections": 0,
+            "expected": ClusterClientService.INTERVAL_LOW,
+        }),
+        ("fewer-connections-than-event-loops", {
+            "num_eventloops": 2,  # anything > num_connections.
+            "num_connections": 1,  # anything > 0.
+            "expected": ClusterClientService.INTERVAL_MID,
+        }),
+        ("default", {
+            "num_eventloops": 3,  # same as num_connections.
+            "num_connections": 3,  # same as num_eventloops.
+            "expected": ClusterClientService.INTERVAL_HIGH,
+        }),
+    )
+
+    def make_inert_client_service(self):
+        service = ClusterClientService(Clock())
+        # ClusterClientService's superclass, TimerService, creates a
+        # LoopingCall with now=True. We neuter it here to allow
+        # observation of the behaviour of _update_interval() for
+        # example.
+        service.call = (lambda: None, (), {})
+        return service
+
+    def test__calculate_interval(self):
+        service = self.make_inert_client_service()
+        service.startService()
+        self.assertEqual(
+            self.expected, service._calculate_interval(
+                self.num_eventloops, self.num_connections))
+
+
 class TestClusterClient(MAASTestCase):
 
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)