Add a retry mechanism on all API calls

fixes smarkets#314

marge/gitlab.py

@@ -1,25 +1,124 @@
+from collections import namedtuple
 import json
 import logging as log
-from collections import namedtuple
-
 import requests
+from retry import retry
+
+
+class ApiError(Exception):
+    @property
+    def error_message(self):
+        args = self.args
+        if len(args) != 2:
+            return None
+
+        arg = args[1]
+        if isinstance(arg, dict):
+            return arg.get('message')
+        return arg
+
+
+class BadRequest(ApiError):
+    pass
+
+
+class Unauthorized(ApiError):
+    pass
+
+
+class Forbidden(ApiError):
+    pass
+
+
+class NotFound(ApiError):
+    pass
+
+
+class MethodNotAllowed(ApiError):
+    pass
+
+
+class NotAcceptable(ApiError):
+    pass
+
+
+class Conflict(ApiError):
+    pass
+
+
+class Unprocessable(ApiError):
+    pass
+
+
+class InternalServerError(ApiError):
+    pass
+
+
+class TooManyRequests(ApiError):
+    pass
+
+
+class BadGateway(ApiError):
+    pass
+
+
+class ServiceUnavailable(ApiError):
+    pass
+
+
+class GatewayTimeout(ApiError):
+    pass
+
+
+class UnexpectedError(ApiError):
+    pass
+
+
+HTTP_ERRORS = {
+    400: BadRequest,
+    401: Unauthorized,
+    403: Forbidden,
+    404: NotFound,
+    405: MethodNotAllowed,
+    406: NotAcceptable,
+    409: Conflict,
+    422: Unprocessable,
+    429: TooManyRequests,
+    500: InternalServerError,
+    502: BadGateway,
+    503: ServiceUnavailable,
+    504: GatewayTimeout,
+}
 
 
 class Api:
-    def __init__(self, gitlab_url, auth_token):
+    def __init__(self, gitlab_url, auth_token, append_api_version=True):
         self._auth_token = auth_token
-        self._api_base_url = gitlab_url.rstrip('/') + '/api/v4'
-
+        self._api_base_url = gitlab_url.rstrip('/')
+
+        # The `append_api_version` flag facilitates testing.
+        if append_api_version:
+            self._api_base_url += '/api/v4'
+
+    @retry(
+        (requests.exceptions.Timeout,
+         Conflict,
+         BadGateway,
+         ServiceUnavailable,
+         InternalServerError,
+         TooManyRequests,),
+        tries=4,
+        delay=20,
+        backoff=2,
+        jitter=(3, 10,)
+    )
     def call(self, command, sudo=None):
         method = command.method
         url = self._api_base_url + command.endpoint
         headers = {'PRIVATE-TOKEN': self._auth_token}
         if sudo:
             headers['SUDO'] = f'{sudo}'
         log.debug('REQUEST: %s %s %r %r', method.__name__.upper(), url, headers, command.call_args)
-        # Timeout to prevent indefinitely hanging requests. 60s is very conservative,
-        # but should be short enough to not cause any practical annoyances. We just
-        # crash rather than retry since marge-bot should be run in a restart loop anyway.
         try:
             response = method(url, headers=headers, timeout=60, **command.call_args)
         except requests.exceptions.Timeout as err:
@@ -40,26 +139,15 @@ def call(self, command, sudo=None):
         if response.status_code == 304:
             return False  # Not Modified
 
-        errors = {
-            400: BadRequest,
-            401: Unauthorized,
-            403: Forbidden,
-            404: NotFound,
-            405: MethodNotAllowed,
-            406: NotAcceptable,
-            409: Conflict,
-            422: Unprocessable,
-            500: InternalServerError,
-        }
-
         def other_error(code, msg):
-            exception = InternalServerError if 500 < code < 600 else UnexpectedError
+            exception = InternalServerError if 500 <= code < 600 else UnexpectedError
             return exception(code, msg)
 
-        error = errors.get(response.status_code, other_error)
+        error = HTTP_ERRORS.get(response.status_code, other_error)
         try:
             err_message = response.json()
         except json.JSONDecodeError:
+            log.error('failed to parse error as json from response: %s', response.text)
             err_message = response.reason
 
         raise error(response.status_code, err_message)
@@ -145,59 +233,6 @@ def process(val):
     return {key: process(val) for key, val in params.items()}
 
 
-class ApiError(Exception):
-    @property
-    def error_message(self):
-        args = self.args
-        if len(args) != 2:
-            return None
-
-        arg = args[1]
-        if isinstance(arg, dict):
-            return arg.get('message')
-        return arg
-
-
-class BadRequest(ApiError):
-    pass
-
-
-class Unauthorized(ApiError):
-    pass
-
-
-class Forbidden(ApiError):
-    pass
-
-
-class NotFound(ApiError):
-    pass
-
-
-class MethodNotAllowed(ApiError):
-    pass
-
-
-class NotAcceptable(ApiError):
-    pass
-
-
-class Conflict(ApiError):
-    pass
-
-
-class Unprocessable(ApiError):
-    pass
-
-
-class InternalServerError(ApiError):
-    pass
-
-
-class UnexpectedError(ApiError):
-    pass
-
-
 class Resource:
     def __init__(self, api, info):
         self._info = info

pyproject.toml

@@ -14,6 +14,7 @@ ConfigArgParse = "^1.3"
 maya = "^0.6.1"
 PyYAML = "^5.4.1"
 requests = "^2.25.1"
+retry2 = "^0.9.2"
 tzdata = "^2022.7"
 
 [tool.poetry.dev-dependencies]

tests/test_gitlab.py

@@ -1,4 +1,12 @@
-from marge import gitlab
+import unittest
+import os
+
+import marge.gitlab as gitlab
+from marge.gitlab import GET
+
+HTTPBIN = (
+    os.environ["HTTPBIN_URL"] if "HTTPBIN_URL" in os.environ else "https://httpbin.org"
+)
 
 
 class TestVersion:
@@ -11,3 +19,23 @@ def test_parse_no_edition(self):
     def test_is_ee(self):
         assert gitlab.Version.parse('9.4.0-ee').is_ee
         assert not gitlab.Version.parse('9.4.0').is_ee
+
+
+class TestApiCalls(unittest.TestCase):
+    def test_success_immediately_no_response(self):
+        api = gitlab.Api(HTTPBIN, "", append_api_version=False)
+        self.assertTrue(api.call(GET("/status/202")))
+        self.assertTrue(api.call(GET("/status/204")))
+        self.assertFalse(api.call(GET("/status/304")))
+
+    def test_failure_after_all_retries(self):
+        api = gitlab.Api(HTTPBIN, "", append_api_version=False)
+
+        with self.assertRaises(gitlab.Conflict):
+            api.call(GET("/status/409"))
+
+        with self.assertRaises(gitlab.TooManyRequests):
+            api.call(GET("/status/429"))
+
+        with self.assertRaises(gitlab.GatewayTimeout):
+            api.call(GET("/status/504"))