From 9f55ce724d120055bf571500e975bfdc3b5276ca Mon Sep 17 00:00:00 2001 From: mprahl Date: Tue, 2 Jul 2019 11:50:58 -0400 Subject: [PATCH] Add a global requests session with retry logic configured This also replaces the usage of other request sessions. --- module_build_service/resolver/MBSResolver.py | 13 +- module_build_service/utils/request_utils.py | 49 ++++++++ tests/test_resolver/test_mbs.py | 126 +++++++++---------- 3 files changed, 116 insertions(+), 72 deletions(-) create mode 100644 module_build_service/utils/request_utils.py diff --git a/module_build_service/resolver/MBSResolver.py b/module_build_service/resolver/MBSResolver.py index 7258482c..3d8bda1d 100644 --- a/module_build_service/resolver/MBSResolver.py +++ b/module_build_service/resolver/MBSResolver.py @@ -27,13 +27,13 @@ import logging import kobo.rpmlib -import requests from module_build_service import db, conf from module_build_service import models from module_build_service.errors import UnprocessableEntity from module_build_service.resolver.base import GenericResolver from module_build_service.utils.general import import_mmd, load_mmd +from module_build_service.utils.request_utils import requests_session log = logging.getLogger() @@ -44,9 +44,6 @@ class MBSResolver(GenericResolver): def __init__(self, config): self.mbs_prod_url = config.mbs_url - self.session = requests.Session() - adapter = requests.adapters.HTTPAdapter(max_retries=3) - self.session.mount(self.mbs_prod_url, adapter) self._generic_error = "Failed to query MBS with query %r returned HTTP status %s" def _query_from_nsvc(self, name, stream, version=None, context=None, state="ready"): @@ -95,7 +92,7 @@ class MBSResolver(GenericResolver): modules = [] while True: - res = self.session.get(self.mbs_prod_url, params=query) + res = requests_session.get(self.mbs_prod_url, params=query) if not res.ok: raise RuntimeError(self._generic_error % (query, res.status_code)) @@ -135,7 +132,7 @@ class MBSResolver(GenericResolver): """ query = {"page": 1, "per_page": 1, "short": True} query.update(kwargs) - res = self.session.get(self.mbs_prod_url, params=query) + res = requests_session.get(self.mbs_prod_url, params=query) if not res.ok: raise RuntimeError(self._generic_error % (query, res.status_code)) @@ -159,7 +156,7 @@ class MBSResolver(GenericResolver): "verbose": True, "virtual_stream": virtual_stream, } - res = self.session.get(self.mbs_prod_url, params=query) + res = requests_session.get(self.mbs_prod_url, params=query) if not res.ok: raise RuntimeError(self._generic_error % (query, res.status_code)) @@ -466,7 +463,7 @@ class MBSResolver(GenericResolver): return new_requires def get_modulemd_by_koji_tag(self, tag): - resp = self.session.get(self.mbs_prod_url, params={"koji_tag": tag, "verbose": True}) + resp = requests_session.get(self.mbs_prod_url, params={"koji_tag": tag, "verbose": True}) data = resp.json() if data["items"]: modulemd = data["items"][0]["modulemd"] diff --git a/module_build_service/utils/request_utils.py b/module_build_service/utils/request_utils.py new file mode 100644 index 00000000..c077b18e --- /dev/null +++ b/module_build_service/utils/request_utils.py @@ -0,0 +1,49 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2019 Red Hat, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +import requests +from requests.packages.urllib3.util.retry import Retry + + +def get_requests_session(auth=False): + """ + Create a requests session with retries configured. + + :return: the configured requests session + :rtype: requests.Session + """ + session = requests.Session() + retry = Retry( + total=3, + read=3, + connect=3, + backoff_factor=0.5, + status_forcelist=(500, 502, 503, 504), + ) + retry.BACKOFF_MAX = 2 + adapter = requests.adapters.HTTPAdapter(max_retries=retry) + session.mount("http://", adapter) + session.mount("https://", adapter) + return session + + +# TODO: Use this instead of requests.get directly throughout the codebase +requests_session = get_requests_session() diff --git a/tests/test_resolver/test_mbs.py b/tests/test_resolver/test_mbs.py index 271feab4..24048a04 100644 --- a/tests/test_resolver/test_mbs.py +++ b/tests/test_resolver/test_mbs.py @@ -34,7 +34,7 @@ base_dir = os.path.join(os.path.dirname(__file__), "..") class TestMBSModule: - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_module_modulemds_nsvc(self, mock_session, testmodule_mmd_9c690d0e): """ Tests for querying a module from mbs """ mock_res = Mock() @@ -52,7 +52,7 @@ class TestMBSModule: "meta": {"next": None}, } - mock_session().get.return_value = mock_res + mock_session.get.return_value = mock_res resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") module_mmds = resolver.get_module_modulemds( @@ -76,10 +76,10 @@ class TestMBSModule: "state": "ready", "virtual_stream": ["f28"], } - mock_session().get.assert_called_once_with(mbs_url, params=expected_query) + mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert nsvcs == expected - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_module_modulemds_partial( self, mock_session, testmodule_mmd_9c690d0e, testmodule_mmd_c2c572ed ): @@ -109,7 +109,7 @@ class TestMBSModule: "meta": {"next": None}, } - mock_session().get.return_value = mock_res + mock_session.get.return_value = mock_res resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") ret = resolver.get_module_modulemds("testmodule", "master", version) nsvcs = set( @@ -131,10 +131,10 @@ class TestMBSModule: "per_page": 10, "state": "ready", } - mock_session().get.assert_called_once_with(mbs_url, params=expected_query) + mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert nsvcs == expected - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_module_build_dependencies( self, mock_session, platform_mmd, testmodule_mmd_9c690d0e ): @@ -171,7 +171,7 @@ class TestMBSModule: }, ] - mock_session().get.return_value = mock_res + mock_session.get.return_value = mock_res expected = set(["module-f28-build"]) resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") result = resolver.get_module_build_dependencies( @@ -207,11 +207,11 @@ class TestMBSModule: call(mbs_url, params=expected_queries[0]), call(mbs_url, params=expected_queries[1]), ] - mock_session().get.mock_calls = expected_calls - assert mock_session().get.call_count == 2 + mock_session.get.mock_calls = expected_calls + assert mock_session.get.call_count == 2 assert set(result) == expected - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_module_build_dependencies_empty_buildrequires( self, mock_session, testmodule_mmd_9c690d0e ): @@ -242,7 +242,7 @@ class TestMBSModule: } ] - mock_session().get.return_value = mock_res + mock_session.get.return_value = mock_res expected = set() @@ -262,10 +262,10 @@ class TestMBSModule: "per_page": 10, "state": "ready", } - mock_session().get.assert_called_once_with(mbs_url, params=expected_query) + mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert set(result) == expected - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_resolve_profiles(self, mock_session, formatted_testmodule_mmd, platform_mmd): mock_res = Mock() @@ -283,7 +283,7 @@ class TestMBSModule: "meta": {"next": None}, } - mock_session().get.return_value = mock_res + mock_session.get.return_value = mock_res resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") result = resolver.resolve_profiles( formatted_testmodule_mmd, ("buildroot", "srpm-buildroot") @@ -339,7 +339,7 @@ class TestMBSModule: "state": "ready", } - mock_session().get.assert_called_once_with(mbs_url, params=expected_query) + mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert result == expected @patch( @@ -363,56 +363,54 @@ class TestMBSModule: expected = {"buildroot": set(["foo"]), "srpm-buildroot": set(["bar"])} assert result == expected - def test_get_empty_buildrequired_modulemds(self): + @patch("module_build_service.resolver.MBSResolver.requests_session") + def test_get_empty_buildrequired_modulemds(self, mock_session): resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") + mock_session.get.return_value = Mock(ok=True) + mock_session.get.return_value.json.return_value = {"items": [], "meta": {"next": None}} - with patch.object(resolver, "session") as session: - session.get.return_value = Mock(ok=True) - session.get.return_value.json.return_value = {"items": [], "meta": {"next": None}} + result = resolver.get_buildrequired_modulemds("nodejs", "10", "platform:el8:1:00000000") + assert [] == result - result = resolver.get_buildrequired_modulemds("nodejs", "10", "platform:el8:1:00000000") - assert [] == result - - def test_get_buildrequired_modulemds(self): + @patch("module_build_service.resolver.MBSResolver.requests_session") + def test_get_buildrequired_modulemds(self, mock_session): resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") + mock_session.get.return_value = Mock(ok=True) + with models.make_session(conf) as db_session: + mock_session.get.return_value.json.return_value = { + "items": [ + { + "name": "nodejs", + "stream": "10", + "version": 1, + "context": "c1", + "modulemd": mmd_to_str( + tests.make_module(db_session, "nodejs:10:1:c1", store_to_db=False), + ), + }, + { + "name": "nodejs", + "stream": "10", + "version": 2, + "context": "c1", + "modulemd": mmd_to_str( + tests.make_module(db_session, "nodejs:10:2:c1", store_to_db=False), + ), + }, + ], + "meta": {"next": None}, + } - with patch.object(resolver, "session") as session: - session.get.return_value = Mock(ok=True) - with models.make_session(conf) as db_session: - session.get.return_value.json.return_value = { - "items": [ - { - "name": "nodejs", - "stream": "10", - "version": 1, - "context": "c1", - "modulemd": mmd_to_str( - tests.make_module(db_session, "nodejs:10:1:c1", store_to_db=False), - ), - }, - { - "name": "nodejs", - "stream": "10", - "version": 2, - "context": "c1", - "modulemd": mmd_to_str( - tests.make_module(db_session, "nodejs:10:2:c1", store_to_db=False), - ), - }, - ], - "meta": {"next": None}, - } + result = resolver.get_buildrequired_modulemds("nodejs", "10", "platform:el8:1:00000000") - result = resolver.get_buildrequired_modulemds("nodejs", "10", "platform:el8:1:00000000") + assert 1 == len(result) + mmd = result[0] + assert "nodejs" == mmd.get_module_name() + assert "10" == mmd.get_stream_name() + assert 1 == mmd.get_version() + assert "c1" == mmd.get_context() - assert 1 == len(result) - mmd = result[0] - assert "nodejs" == mmd.get_module_name() - assert "10" == mmd.get_stream_name() - assert 1 == mmd.get_version() - assert "c1" == mmd.get_context() - - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_module_count(self, mock_session): mock_res = Mock() mock_res.ok.return_value = True @@ -420,18 +418,18 @@ class TestMBSModule: "items": [{"name": "platform", "stream": "f28", "version": "3", "context": "00000000"}], "meta": {"total": 5}, } - mock_session.return_value.get.return_value = mock_res + mock_session.get.return_value = mock_res resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") count = resolver.get_module_count(name="platform", stream="f28") assert count == 5 - mock_session.return_value.get.assert_called_once_with( + mock_session.get.assert_called_once_with( "https://mbs.fedoraproject.org/module-build-service/1/module-builds/", params={"name": "platform", "page": 1, "per_page": 1, "short": True, "stream": "f28"}, ) - @patch("requests.Session") + @patch("module_build_service.resolver.MBSResolver.requests_session") def test_get_latest_with_virtual_stream(self, mock_session, platform_mmd): mock_res = Mock() mock_res.ok.return_value = True @@ -447,13 +445,13 @@ class TestMBSModule: ], "meta": {"total": 5}, } - mock_session.return_value.get.return_value = mock_res + mock_session.get.return_value = mock_res resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="mbs") mmd = resolver.get_latest_with_virtual_stream("platform", "virtualf28") assert mmd.get_module_name() == "platform" - mock_session.return_value.get.assert_called_once_with( + mock_session.get.assert_called_once_with( "https://mbs.fedoraproject.org/module-build-service/1/module-builds/", params={ "name": "platform",