sourceppa: Refactor exception handling

Use a for loop for retrying instead of recursion, so we don't get nested
exceptions being thrown. Add a test for this flakiness.

britney2/policies/sourceppa.py

@@ -40,7 +40,7 @@ class SourcePPAPolicy(BasePolicy):
         # self.cache contains self.source_ppas_by_pkg from previous run
         self.cache = {}
 
-    def query_lp_rest_api(self, obj, query, retries=5):
+    def query_lp_rest_api(self, obj, query):
         """Do a Launchpad REST request
 
         Request <LAUNCHPAD_URL><obj>?<query>.
@@ -49,34 +49,28 @@ class SourcePPAPolicy(BasePolicy):
         Raises HTTPError, ValueError, or ConnectionError based on different
         transient failures connecting to launchpad.
         """
-        assert retries > 0
 
-        url = '%s%s?%s' % (LAUNCHPAD_URL, obj, urllib.parse.urlencode(query))
+        for retry in range(5):
-        try:
+            url = '%s%s?%s' % (LAUNCHPAD_URL, obj, urllib.parse.urlencode(query))
-            with urllib.request.urlopen(url, timeout=30) as req:
+            try:
-                code = req.getcode()
+                with urllib.request.urlopen(url, timeout=30) as req:
-                if 200 <= code < 300:
+                    code = req.getcode()
-                    return json.loads(req.read().decode('UTF-8'))
+                    if 200 <= code < 300:
-                raise ConnectionError('Failed to reach launchpad, HTTP %s'
+                        return json.loads(req.read().decode('UTF-8'))
-                                      % code)
+                    raise ConnectionError('Failed to reach launchpad, HTTP %s'
-        except socket.timeout:
+                                          % code)
-            if retries > 1:
+            except socket.timeout as e:
                 self.log("Timeout downloading '%s', will retry %d more times."
-                         % (url, retries))
+                         % (url, 5 - retry - 1))
-                return self.query_lp_rest_api(obj, query, retries - 1)
+                exc = e
-            else:
+            except HTTPError as e:
-                raise
+                if e.code != 503:
-        except HTTPError as e:
+                    raise
-            if e.code != 503:
-                raise
-
-            # 503s are transient
-            if retries > 1:
                 self.log("Caught error 503 downloading '%s', will retry %d more times."
-                         % (url, retries))
+                         % (url, 5 - retry - 1))
-                return self.query_lp_rest_api(obj, query, retries - 1)
+                exc = e
-            else:
+        else:
-                raise
+            raise exc
 
     def lp_get_source_ppa(self, pkg, version):
         """Ask LP what source PPA pkg was copied from"""

tests/test_sourceppa.py

@@ -9,7 +9,7 @@
 import os
 import sys
 import unittest
-from unittest.mock import patch
+from unittest.mock import DEFAULT, patch
 
 PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, PROJECT_DIR)
@@ -124,6 +124,30 @@ class T(unittest.TestCase):
             pol.lp_get_source_ppa('hello', '1.0')
         self.assertEqual(urlopen.call_count, 5)
 
+    @patch('britney2.policies.sourceppa.urllib.request.urlopen')
+    def test_lp_rest_api_flaky(self, urlopen):
+        """If we get a 503, then a 200, we get the right result"""
+        from urllib.error import HTTPError
+
+        def l():
+            for i in range(3):
+                yield HTTPError(None,
+                                503,
+                                'Service Temporarily Unavailable',
+                                None,
+                                None)
+            while True:
+                yield DEFAULT
+
+        context = urlopen.return_value.__enter__.return_value
+        context.getcode.return_value = 200
+        context.read.return_value = b'{"entries": [{"copy_source_archive_link": "https://api.launchpad.net/1.0/team/ubuntu/ppa", "other_stuff": "ignored"}]}'
+        urlopen.side_effect = l()
+        pol = SourcePPAPolicy(FakeOptions, {})
+        pol.lp_get_source_ppa('hello', '1.0')
+        self.assertEqual(urlopen.call_count, 4)
+        self.assertEqual(pol.lp_get_source_ppa('hello', '1.0'), 'https://api.launchpad.net/1.0/team/ubuntu/ppa')
+
     def test_approve_ppa(self):
         """Approve packages by their PPA."""
         shortppa = 'team/ubuntu/ppa'