From b3bfb0eef2f2157f4ca4572d2176530f7ab254bb Mon Sep 17 00:00:00 2001 From: z8v <86471569+z8v@users.noreply.github.com> Date: Thu, 22 Sep 2022 21:24:38 +0300 Subject: [PATCH] Add a retry mechanism on all API calls fixes smarkets/marge-bot#314 --- marge/gitlab.py | 185 +++++++++++++++++++++++++------------------ poetry.lock | 28 ++++++- pyproject.toml | 1 + tests/test_gitlab.py | 30 ++++++- 4 files changed, 167 insertions(+), 77 deletions(-) diff --git a/marge/gitlab.py b/marge/gitlab.py index 12b88746..b5e83742 100644 --- a/marge/gitlab.py +++ b/marge/gitlab.py @@ -1,15 +1,117 @@ +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 @@ -17,9 +119,6 @@ def call(self, command, sudo=None): 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 diff --git a/poetry.lock b/poetry.lock index 347cab17..38af8bd2 100644 --- a/poetry.lock +++ b/poetry.lock @@ -263,6 +263,18 @@ calendars = ["convertdate", "hijri-converter"] fasttext = ["fasttext"] langdetect = ["langdetect"] +[[package]] +name = "decorator" +version = "5.1.1" +description = "Decorators for Humans" +category = "main" +optional = false +python-versions = ">=3.5" +files = [ + {file = "decorator-5.1.1-py3-none-any.whl", hash = "sha256:b8c3f85900b9dc423225913c5aace94729fe1fa9763b38939a95226f02d37186"}, + {file = "decorator-5.1.1.tar.gz", hash = "sha256:637996211036b6385ef91435e4fae22989472f9d571faba8927ba8253acbc330"}, +] + [[package]] name = "dill" version = "0.3.6" @@ -893,6 +905,20 @@ urllib3 = ">=1.21.1,<1.27" socks = ["PySocks (>=1.5.6,!=1.5.7)"] use-chardet-on-py3 = ["chardet (>=3.0.2,<6)"] +[[package]] +name = "retry2" +version = "0.9.5" +description = "Easy to use retry decorator." +category = "main" +optional = false +python-versions = ">=2.6" +files = [ + {file = "retry2-0.9.5-py2.py3-none-any.whl", hash = "sha256:f7fee13b1e15d0611c462910a6aa72a8919823988dd0412152bc3719c89a4e55"}, +] + +[package.dependencies] +decorator = ">=3.4.2" + [[package]] name = "setuptools" version = "67.6.1" @@ -1160,4 +1186,4 @@ testing = ["big-O", "flake8 (<5)", "jaraco.functools", "jaraco.itertools", "more [metadata] lock-version = "2.0" python-versions = "^3.7" -content-hash = "d6bd0c6e71a5dc215d52bd0f54c90100d123c0d7cf305e724bfc81938e29b8b8" +content-hash = "fd421c6e0cbd57aee9fb645696d6e544d5f7cae1962de30e50fa126e7daf0a02" diff --git a/pyproject.toml b/pyproject.toml index 9474e0f7..a53a96f3 100644 --- a/pyproject.toml +++ b/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] diff --git a/tests/test_gitlab.py b/tests/test_gitlab.py index a0df5870..1cc95482 100644 --- a/tests/test_gitlab.py +++ b/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"))