Merge lp:~julian-edwards/maas/download-service-exceptions into lp:~maas-committers/maas/trunk

[Julian Edwards]

Ensure that exceptions are caught inside the PeriodicImageDownloadService, which would otherwise make it stop running.

.

[Jeroen T. Vermeulen (jtv)]

Nicely done. Just one note: shouldn't the existing tests then call try_download instead of maybe_start_download, so that the success case is covered?

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 27 August 2014 07:15:51 you wrote:
> Review: Approve
>
> Nicely done. Just one note: shouldn't the existing tests then call
> try_download instead of maybe_start_download, so that the success case is
> covered?

It's already doing that; it relies on the service itself calling the functions
in the service object.

Thanks for the review!

=== modified file 'src/provisioningserver/pserv_services/image_download_service.py'
--- src/provisioningserver/pserv_services/image_download_service.py	2014-08-27 05:59:05 +0000
+++ src/provisioningserver/pserv_services/image_download_service.py	2014-08-27 07:10:34 +0000
@@ -27,13 +27,17 @@
     GetBootSources,
     GetBootSourcesV2,
     )
-from provisioningserver.utils.twisted import pause
+from provisioningserver.utils.twisted import (
+    pause,
+    retries,
+    )
 from twisted.application.internet import TimerService
 from twisted.internet.defer import (
     DeferredLock,
     inlineCallbacks,
     returnValue,
     )
+from twisted.python import log
 from twisted.spread.pb import NoSuchMethod
 
 
@@ -54,11 +58,24 @@
     def __init__(self, client_service, reactor, cluster_uuid):
         # Call self.check() every self.check_interval.
         super(PeriodicImageDownloadService, self).__init__(
-            self.check_interval, self.maybe_start_download)
+            self.check_interval, self.try_download)
         self.clock = reactor
         self.client_service = client_service
         self.uuid = cluster_uuid
 
+    def try_download(self):
+        """Wrap download attempts in something that catches Failures.
+
+        Log the full error to the Twisted log, and a concise error to
+        the maas log.
+        """
+        def download_failure(failure):
+            log.err(failure)
+            maaslog.error(
+                "Failed to download images: %s", failure.getErrorMessage())
+
+        return self.maybe_start_download().addErrback(download_failure)
+
     @inlineCallbacks
     def _get_boot_sources(self, client):
         """Gets the boot sources from the region."""
@@ -80,13 +97,13 @@
         client = None
         # Retry a few times, since this service usually comes up before
         # the RPC service.
-        for _ in range(3):
+        for elapsed, remaining, wait in retries(15, 5, self.clock):
             try:
                 client = self.client_service.getClient()
                 break
             except NoConnectionsAvailable:
-                yield pause(5)
-        if client is None:
+                yield pause(wait, self.clock)
+        else:
             maaslog.error(
                 "Can't initiate image download, no RPC connection to region.")
             return

=== modified file 'src/provisioningserver/pserv_services/tests/test_image_download_service.py'
--- src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-08-27 03:38:28 +0000
+++ src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-08-27 07:10:34 +0000
@@ -17,6 +17,7 @@
 
 from datetime import timedelta
 
+from fixtures import FakeLogger
 from maastesting.factory import factory
 from maastesting.matchers import (
     get_mock_calls,
@@ -41,9 +42,13 @@
     GetBootSources,
     GetBootSourcesV2,
     )
+from provisioningserver.rpc.testing import TwistedLoggerFixture
 from provisioningserver.testing.testcase import PservTestCase
 from provisioningserver.utils.twisted import pause
-from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.deferredruntest import (
+    AsynchronousDeferredRunTest,
+    extract_result,
+    )
 from twisted.application.internet import TimerService
 from twisted.internet import defer
 from twisted.internet.task import Clock
@@ -205,6 +210,28 @@
         clock.advance(1)
         self.assertFalse(service_lock.locked)
 
+    def test_logs_other_errors(self):
+        service = PeriodicImageDownloadService(
+            sentinel.rpc, Clock(), sentinel.uuid)
+
+        maybe_start_download = self.patch(service, "maybe_start_download")
+        maybe_start_download.return_value = defer.fail(
+            ZeroDivisionError("Such a shame I can't divide by zero"))
+
+        with FakeLogger("maas") as maaslog, TwistedLoggerFixture():
+            d = service.try_download()
+
+        self.assertEqual(None, extract_result(d))
+        self.assertDocTestMatches(
+            "Failed to download images: "
+            "Such a shame I can't divide by zero",
+            maaslog.output)
+
+
+class TestGetBootSources(PservTestCase):
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
     @defer.inlineCallbacks
     def test__get_boot_sources_calls_get_boot_sources_v2_before_v1(self):
         clock = Clock()