From 46d89e7cd90ed027a2dccf7383144a6e66f81124 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Jun 28 2019 21:56:37 +0000 Subject: [PATCH 1/2] Add logic to retry when fetching data from dist-git --- diff --git a/greenwave/resources.py b/greenwave/resources.py index 715ca4a..64f0915 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -138,6 +138,7 @@ def retrieve_yaml_remote_rule(rev, pkg_name, pkg_namespace): _retrieve_gating_yaml_error = 'Error occurred looking for gating.yaml file in the dist-git repo.' +@greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError) def _retrieve_yaml_remote_rule_web(rev, pkg_name, pkg_namespace): """ Retrieve the gating.yaml file from the dist-git web UI. """ data = { @@ -170,8 +171,16 @@ def _retrieve_yaml_remote_rule_git_archive(rev, pkg_name, pkg_namespace): dist_git_base_url = current_app.config['DIST_GIT_BASE_URL'].rstrip('/') dist_git_url = f'{dist_git_base_url}/{pkg_namespace}/{pkg_name}' cmd = ['git', 'archive', f'--remote={dist_git_url}', rev, 'gating.yaml'] - git_archive = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - output, error_output = git_archive.communicate() + # Retry thrice if TimeoutExpired exception is raised + MAX_RETRY = 3 + for tries in range(MAX_RETRY): + try: + git_archive = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + output, error_output = git_archive.communicate(timeout=30) + break + except subprocess.TimeoutExpired: + git_archive.kill() + continue if git_archive.returncode != 0: error_output = error_output.decode('utf-8') From 4af92870abe8f4f5e317284084d8e7cd084d22de Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Jul 05 2019 14:48:03 +0000 Subject: [PATCH 2/2] Add retry logic to session instead of retrying the entire method Currently, greenwave uses a retry decorator which reattempts a piece of code in case of a particular exception. Instead, it might be better to just retry the request in-case of a timeout rather than running the entire piece of code again. --- diff --git a/greenwave/config.py b/greenwave/config.py index f427d0f..f657746 100644 --- a/greenwave/config.py +++ b/greenwave/config.py @@ -25,10 +25,6 @@ class Config(object): REQUESTS_TIMEOUT = (6.1, 15) REQUESTS_VERIFY = True - # General options for retrying failed operations (querying external services) - RETRY_TIMEOUT = 6 - RETRY_INTERVAL = 2 - POLICIES_DIR = '/etc/greenwave/policies' MESSAGING = 'fedmsg' diff --git a/greenwave/consumers/waiverdb.py b/greenwave/consumers/waiverdb.py index 8cbeee7..f282fc6 100644 --- a/greenwave/consumers/waiverdb.py +++ b/greenwave/consumers/waiverdb.py @@ -13,7 +13,6 @@ import logging import json import fedmsg.consumers -import requests import greenwave.app_factory from greenwave.api_v1 import subject_type_identifier_to_list @@ -23,6 +22,7 @@ from greenwave.monitor import ( messaging_tx_sent_ok_counter, messaging_tx_failed_counter) from greenwave.policies import applicable_decision_context_product_version_pairs from greenwave.utils import right_before_this_time +from greenwave.request_session import get_requests_session try: import fedora_messaging.api @@ -31,7 +31,7 @@ except ImportError: pass -requests_session = requests.Session() +requests_session = get_requests_session() log = logging.getLogger(__name__) diff --git a/greenwave/request_session.py b/greenwave/request_session.py new file mode 100644 index 0000000..aad59e1 --- /dev/null +++ b/greenwave/request_session.py @@ -0,0 +1,24 @@ +import requests + +from requests.adapters import HTTPAdapter +from requests.packages.urllib3.util.retry import Retry + +from greenwave import __version__ + + +def get_requests_session(): + """ Get http(s) session for request processing. """ + + session = requests.Session() + retry = Retry( + total=3, + read=3, + connect=3, + backoff_factor=1, + status_forcelist=(500, 502, 503, 504), + ) + adapter = HTTPAdapter(max_retries=retry) + session.mount('http://', adapter) + session.mount('https://', adapter) + session.headers["User-Agent"] = f"greenwave {__version__}" + return session diff --git a/greenwave/resources.py b/greenwave/resources.py index 64f0915..bdf23ae 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -9,8 +9,6 @@ waiverdb, etc..). import logging import re import json -import requests -import urllib3.exceptions from io import BytesIO import tarfile import subprocess @@ -20,14 +18,12 @@ import xmlrpc.client from flask import current_app from werkzeug.exceptions import BadGateway -from greenwave import __version__ from greenwave.cache import cached -import greenwave.utils +from greenwave.request_session import get_requests_session log = logging.getLogger(__name__) -requests_session = requests.Session() -requests_session.headers["User-Agent"] = f"greenwave {__version__}" +requests_session = get_requests_session() class ResultsRetriever(object): @@ -85,7 +81,6 @@ class ResultsRetriever(object): @cached -@greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError) def retrieve_scm_from_koji(nvr): """ Retrieve cached rev and namespace from koji using the nvr """ koji_url = current_app.config['KOJI_BASE_URL'] @@ -138,7 +133,6 @@ def retrieve_yaml_remote_rule(rev, pkg_name, pkg_namespace): _retrieve_gating_yaml_error = 'Error occurred looking for gating.yaml file in the dist-git repo.' -@greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError) def _retrieve_yaml_remote_rule_web(rev, pkg_name, pkg_namespace): """ Retrieve the gating.yaml file from the dist-git web UI. """ data = { @@ -199,7 +193,6 @@ def _retrieve_yaml_remote_rule_git_archive(rev, pkg_name, pkg_namespace): # NOTE - not cached, for now. -@greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError) def retrieve_waivers(product_version, subject_type, subject_identifiers, when): if not subject_identifiers: return [] @@ -227,7 +220,6 @@ def retrieve_waivers(product_version, subject_type, subject_identifiers, when): # NOTE - not cached. -@greenwave.utils.retry(timeout=300, interval=30, wait_on=urllib3.exceptions.NewConnectionError) def retrieve_decision(greenwave_url, data): timeout = current_app.config['REQUESTS_TIMEOUT'] verify = current_app.config['REQUESTS_VERIFY'] diff --git a/greenwave/tests/test_utils.py b/greenwave/tests/test_utils.py index 65f3a31..c3abd12 100644 --- a/greenwave/tests/test_utils.py +++ b/greenwave/tests/test_utils.py @@ -9,38 +9,7 @@ from requests import ConnectionError, ConnectTimeout, Timeout from werkzeug.exceptions import InternalServerError import greenwave.app_factory -from greenwave.utils import json_error, retry - - -def test_retry_passthrough(): - """ Ensure that retry doesn't gobble exceptions. """ - expected = "This is the exception." - - @retry(timeout=0.1, interval=0.1, wait_on=Exception) - def f(): - raise Exception(expected) - - with pytest.raises(Exception) as actual: - f() - - assert expected in str(actual) - - -def test_retry_count(): - """ Ensure that retry doesn't gobble exceptions. """ - expected = "This is the exception." - - calls = [] - - @retry(timeout=0.3, interval=0.1, wait_on=Exception) - def f(): - calls.append(1) - raise Exception(expected) - - with pytest.raises(Exception): - f() - - assert sum(calls) == 3 +from greenwave.utils import json_error @pytest.mark.parametrize(('error, expected_status_code,' diff --git a/greenwave/utils.py b/greenwave/utils.py index e218dfe..1166144 100644 --- a/greenwave/utils.py +++ b/greenwave/utils.py @@ -3,7 +3,6 @@ import functools import logging import os -import time import hashlib import datetime @@ -114,33 +113,6 @@ def insert_headers(response): return response -def retry(timeout=None, interval=None, wait_on=Exception): - """ A decorator that allows to retry a section of code... - ...until success or timeout. - - If omitted, the values for `timeout` and `interval` are - taken from the global configuration. - """ - def wrapper(function): - @functools.wraps(function) - def inner(*args, **kwargs): - _timeout = timeout or current_app.config['RETRY_TIMEOUT'] - _interval = interval or current_app.config['RETRY_INTERVAL'] - # These can be configured per-function, or globally if omitted. - start = time.time() - while True: - try: - return function(*args, **kwargs) - except wait_on as e: # pylint: disable=broad-except - log.warning("Exception %r raised from %r. Retry in %rs", - e, function, _interval) - time.sleep(_interval) - if (time.time() - start) >= _timeout: - raise # This re-raises the last exception. - return inner - return wrapper - - def sha1_mangle_key(key): """ Like dogpile.cache.util.sha1_mangle_key, but works correctly on