From 114eef2bb0a62e1ef5ab2966c6d44a134e2c6a94 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Nov 19 2020 10:48:49 +0000 Subject: Cache Koji client and requests in resultsdb-consumer --- diff --git a/functional-tests/consumers/test_resultsdb.py b/functional-tests/consumers/test_resultsdb.py index 3ad2c32..edab36d 100644 --- a/functional-tests/consumers/test_resultsdb.py +++ b/functional-tests/consumers/test_resultsdb.py @@ -554,7 +554,7 @@ def test_consume_new_result_container_image( } } handler = create_resultdb_handler(greenwave_server) - handler.koji_proxy = None + handler.koji_base_url = None handler.consume(message) # get old decision diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py index 0931e8d..d307586 100644 --- a/greenwave/consumers/resultsdb.py +++ b/greenwave/consumers/resultsdb.py @@ -17,7 +17,6 @@ from greenwave.subjects.factory import ( create_subject_from_data, UnknownSubjectDataError, ) -from greenwave.xmlrpc_server_proxy import get_server_proxy log = logging.getLogger(__name__) @@ -59,12 +58,7 @@ class ResultsDBHandler(Consumer): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - - koji_base_url = self.flask_app.config['KOJI_BASE_URL'] - if koji_base_url: - self.koji_proxy = get_server_proxy(koji_base_url) - else: - self.koji_proxy = None + self.koji_base_url = self.flask_app.config['KOJI_BASE_URL'] @staticmethod def announcement_subject(message): @@ -133,7 +127,7 @@ class ResultsDBHandler(Consumer): product_version = subject_product_version( subject, - self.koji_proxy, + self.koji_base_url, brew_task_id, ) diff --git a/greenwave/product_versions.py b/greenwave/product_versions.py index b15fbdf..0a961a3 100644 --- a/greenwave/product_versions.py +++ b/greenwave/product_versions.py @@ -8,6 +8,8 @@ import re import socket import xmlrpc.client +from greenwave.resources import retrieve_koji_build, retrieve_koji_task_request + log = logging.getLogger(__name__) @@ -41,17 +43,16 @@ def _guess_product_version(toparse, koji_build=False): def _guess_koji_build_product_version( - subject_identifier, koji_proxy, koji_task_id=None): + subject_identifier, koji_base_url, koji_task_id=None): try: if not koji_task_id: log.debug('Getting Koji task ID for build %r', subject_identifier) - build = koji_proxy.getBuild(subject_identifier) or {} + build = retrieve_koji_build(subject_identifier, koji_base_url) or {} koji_task_id = build.get('task_id') if not koji_task_id: return None - log.debug('Getting Koji task request ID %r', koji_task_id) - target = koji_proxy.getTaskRequest(koji_task_id)[1] + target = retrieve_koji_task_request(koji_task_id, koji_base_url)[1] return _guess_product_version(target, koji_build=True) except (xmlrpc.client.ProtocolError, socket.error) as err: raise ConnectionError('Could not reach Koji: {}'.format(err)) @@ -61,7 +62,7 @@ def _guess_koji_build_product_version( def subject_product_version( subject, - koji_proxy=None, + koji_base_url=None, koji_task_id=None): if subject.product_version: return subject.product_version @@ -72,6 +73,6 @@ def subject_product_version( if product_version: return product_version - if koji_proxy and subject.is_koji_build: + if koji_base_url and subject.is_koji_build: return _guess_koji_build_product_version( - subject.identifier, koji_proxy, koji_task_id) + subject.identifier, koji_base_url, koji_task_id) diff --git a/greenwave/resources.py b/greenwave/resources.py index 1790c3a..fe1ee7b 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -24,6 +24,13 @@ log = logging.getLogger(__name__) requests_session = get_requests_session() +def _requests_timeout(): + timeout = current_app.config['REQUESTS_TIMEOUT'] + if isinstance(timeout, tuple): + return timeout[1] + return timeout + + class BaseRetriever: def __init__(self, ignore_ids, when, url): self.ignore_ids = ignore_ids @@ -137,9 +144,16 @@ class NoSourceException(RuntimeError): @cached +def retrieve_koji_task_request(nvr, koji_url): + log.debug('Getting Koji task request ID %r', nvr) + proxy = get_server_proxy(koji_url, _requests_timeout()) + return proxy.getTaskRequest(nvr) + + +@cached def retrieve_koji_build(nvr, koji_url): log.debug('Getting Koji build %r', nvr) - proxy = get_server_proxy(koji_url) + proxy = get_server_proxy(koji_url, _requests_timeout()) return proxy.getBuild(nvr) diff --git a/greenwave/tests/conftest.py b/greenwave/tests/conftest.py index 4c61cca..696b0f5 100644 --- a/greenwave/tests/conftest.py +++ b/greenwave/tests/conftest.py @@ -1,3 +1,4 @@ +import mock import pytest from greenwave.app_factory import create_app @@ -18,3 +19,10 @@ def app(): @pytest.fixture def client(app): yield app.test_client() + + +@pytest.fixture +def koji_proxy(): + mock_proxy = mock.Mock() + with mock.patch('greenwave.resources.get_server_proxy', return_value=mock_proxy): + yield mock_proxy diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index f7d2e34..4f0427c 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -1293,7 +1293,7 @@ def test_remote_rule_policy_on_demand_policy(namespace): @pytest.mark.parametrize('two_rules', (True, False)) -def test_on_demand_policy_match(two_rules): +def test_on_demand_policy_match(two_rules, koji_proxy): """ Proceed other rules when there's no source URL in Koji build """ nvr = 'httpd-2.4.el9000' @@ -1317,24 +1317,21 @@ def test_on_demand_policy_match(two_rules): "test_case_name": "fake.testcase.tier0.validation" }) - with mock.patch('greenwave.resources.get_server_proxy') as koji_server: - koji_server_instance = mock.MagicMock() - koji_server_instance.getBuild.return_value = {'extra': {'source': None}} - koji_server.return_value = koji_server_instance - policy = OnDemandPolicy.create_from_json(serverside_json) + koji_proxy.getBuild.return_value = {'extra': {'source': None}} + policy = OnDemandPolicy.create_from_json(serverside_json) - policy_matches = policy.matches(subject=subject) + policy_matches = policy.matches(subject=subject) - koji_server_instance.getBuild.assert_called_once() - assert policy_matches + koji_proxy.getBuild.assert_called_once() + assert policy_matches - results = DummyResultsRetriever( - subject, 'fake.testcase.tier0.validation', 'PASSED' - ) - decision = policy.check('fedora-30', subject, results) - if two_rules: - assert len(decision) == 1 - assert isinstance(decision[0], RuleSatisfied) + results = DummyResultsRetriever( + subject, 'fake.testcase.tier0.validation', 'PASSED' + ) + decision = policy.check('fedora-30', subject, results) + if two_rules: + assert len(decision) == 1 + assert isinstance(decision[0], RuleSatisfied) def test_remote_rule_policy_on_demand_policy_required(): diff --git a/greenwave/tests/test_product_versions.py b/greenwave/tests/test_product_versions.py index 2127185..f626c24 100644 --- a/greenwave/tests/test_product_versions.py +++ b/greenwave/tests/test_product_versions.py @@ -2,20 +2,19 @@ import socket -import mock import pytest from greenwave import product_versions @pytest.mark.parametrize('task_id', (None, 3)) -def test_guess_koji_build_product_version_socket_error(task_id): +def test_guess_koji_build_product_version_socket_error(task_id, koji_proxy, app): subject_identifier = 'release-e2e-test-1.0.1685-1.el5' - mock_proxy = mock.Mock() - mock_proxy.getBuild.side_effect = mock_proxy.getTaskRequest.side_effect = ( + koji_proxy.getBuild.side_effect = koji_proxy.getTaskRequest.side_effect = ( socket.timeout('timed out') ) expected = 'Could not reach Koji: timed out' with pytest.raises(ConnectionError, match=expected): # pylint: disable=protected-access - product_versions._guess_koji_build_product_version(subject_identifier, mock_proxy, task_id) + product_versions._guess_koji_build_product_version( + subject_identifier, 'http://localhost:5006/kojihub', task_id) diff --git a/greenwave/tests/test_resultsdb_consumer.py b/greenwave/tests/test_resultsdb_consumer.py index 339b749..c625719 100644 --- a/greenwave/tests/test_resultsdb_consumer.py +++ b/greenwave/tests/test_resultsdb_consumer.py @@ -339,28 +339,23 @@ def test_guess_product_version(): assert product_version == 'rhel-8' -def test_guess_product_version_with_koji(): - koji_proxy = mock.MagicMock() +def test_guess_product_version_with_koji(koji_proxy, app): koji_proxy.getBuild.return_value = {'task_id': 666} koji_proxy.getTaskRequest.return_value = ['git://example.com/project', 'rawhide', {}] - app = create_app() - with app.app_context(): - subject = create_subject('container-build', 'fake_koji_build') - product_version = subject_product_version(subject, koji_proxy) + subject = create_subject('container-build', 'fake_koji_build') + product_version = subject_product_version(subject, 'http://localhost:5006/kojihub') + koji_proxy.getBuild.assert_called_once_with('fake_koji_build') koji_proxy.getTaskRequest.assert_called_once_with(666) assert product_version == 'fedora-rawhide' -def test_guess_product_version_with_koji_without_task_id(): - koji_proxy = mock.MagicMock() +def test_guess_product_version_with_koji_without_task_id(koji_proxy, app): koji_proxy.getBuild.return_value = {'task_id': None} - app = create_app() - with app.app_context(): - subject = create_subject('container-build', 'fake_koji_build') - product_version = subject_product_version(subject, koji_proxy) + subject = create_subject('container-build', 'fake_koji_build') + product_version = subject_product_version(subject, 'http://localhost:5006/kojihub') koji_proxy.getBuild.assert_called_once_with('fake_koji_build') koji_proxy.getTaskRequest.assert_not_called() @@ -573,7 +568,7 @@ def test_real_fedora_messaging_msg(mock_retrieve_results): } handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub) - handler.koji_proxy = None + handler.koji_base_url = None handler.flask_app.config['policies'] = Policy.safe_load_all(policies) with handler.flask_app.app_context(): handler.consume(message) @@ -596,7 +591,7 @@ def test_real_fedora_messaging_msg(mock_retrieve_results): } -def test_container_brew_build(mock_retrieve_results): +def test_container_brew_build(mock_retrieve_results, koji_proxy): message = { 'msg': { 'submit_time': '2019-08-27T13:57:53.490376', @@ -642,11 +637,9 @@ def test_container_brew_build(mock_retrieve_results): } handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub) - koji_proxy = mock.MagicMock() koji_proxy.getBuild.return_value = None koji_proxy.getTaskRequest.return_value = [ 'git://example.com/project', 'example_product_version', {}] - handler.koji_proxy = koji_proxy handler.flask_app.config['policies'] = Policy.safe_load_all(policies) with handler.flask_app.app_context(): diff --git a/greenwave/tests/test_retrieve_gating_yaml.py b/greenwave/tests/test_retrieve_gating_yaml.py index 9d2141e..dc9f49e 100644 --- a/greenwave/tests/test_retrieve_gating_yaml.py +++ b/greenwave/tests/test_retrieve_gating_yaml.py @@ -97,19 +97,16 @@ def test_retrieve_scm_from_build_without_namespace(): assert pkg_name == 'foo' -def test_retrieve_scm_from_koji_build_not_found(): +def test_retrieve_scm_from_koji_build_not_found(koji_proxy): nvr = 'foo-1.2.3-1.fc29' app = create_app('greenwave.config.TestingConfig') with app.app_context(): expected_error = '404 Not Found: Failed to find Koji build for "{}" at "{}"'.format( nvr, app.config['KOJI_BASE_URL'] ) - with mock.patch('greenwave.resources.get_server_proxy') as koji_server: - proxy = mock.MagicMock() - proxy.getBuild.return_value = {} - koji_server.return_value = proxy - with pytest.raises(NotFound, match=expected_error): - retrieve_scm_from_koji(nvr) + koji_proxy.getBuild.return_value = {} + with pytest.raises(NotFound, match=expected_error): + retrieve_scm_from_koji(nvr) def test_retrieve_scm_from_build_with_missing_rev(): @@ -170,10 +167,8 @@ def test_retrieve_yaml_remote_rule_connection_error(): ) -@mock.patch('greenwave.resources.get_server_proxy') -def test_retrieve_scm_from_koji_build_socket_error(mock_xmlrpc_client): - mock_auth_server = mock_xmlrpc_client.return_value - mock_auth_server.getBuild.side_effect = socket.error('Socket is closed') +def test_retrieve_scm_from_koji_build_socket_error(koji_proxy): + koji_proxy.getBuild.side_effect = socket.error('Socket is closed') app = greenwave.app_factory.create_app() nvr = 'nethack-3.6.1-3.fc29' expected_error = 'Could not reach Koji: Socket is closed' diff --git a/greenwave/tests/test_xmlrpc_server_proxy.py b/greenwave/tests/test_xmlrpc_server_proxy.py index bf59cb6..f0b4804 100644 --- a/greenwave/tests/test_xmlrpc_server_proxy.py +++ b/greenwave/tests/test_xmlrpc_server_proxy.py @@ -16,18 +16,15 @@ from greenwave import xmlrpc_server_proxy ) @mock.patch('greenwave.xmlrpc_server_proxy.Transport') @mock.patch('greenwave.xmlrpc_server_proxy.SafeTransport') -def test_get_server_proxy_app_context( +def test_get_server_proxy( mock_safe_transport, mock_transport, url, expected_transport, timeout, expected_timeout, - app, ): - with app.app_context(): - app.config['REQUESTS_TIMEOUT'] = timeout - xmlrpc_server_proxy.get_server_proxy(url) + xmlrpc_server_proxy.get_server_proxy(url, timeout) if expected_transport == xmlrpc_server_proxy.Transport: mock_transport.__init__.assert_called_once_with(url, expected_timeout) @@ -35,3 +32,16 @@ def test_get_server_proxy_app_context( elif expected_transport == xmlrpc_server_proxy.SafeTransport: mock_safe_transport.__init__.assert_called_once_with(url, expected_timeout) mock_transport.__init__.assert_not_called() + + +def test_get_server_proxy_cached(): + """Server proxy objects are cached""" + s1 = xmlrpc_server_proxy.get_server_proxy('https://localhost:5000/api', timeout=None) + s2 = xmlrpc_server_proxy.get_server_proxy('https://localhost:5000/api', timeout=None) + assert s1 is s2 + + s3 = xmlrpc_server_proxy.get_server_proxy('https://localhost:5000/api', timeout=10) + assert s1 is not s3 + + s4 = xmlrpc_server_proxy.get_server_proxy('https://localhost:5001/api', timeout=None) + assert s1 is not s4 diff --git a/greenwave/xmlrpc_server_proxy.py b/greenwave/xmlrpc_server_proxy.py index 158ac6b..8b166fc 100644 --- a/greenwave/xmlrpc_server_proxy.py +++ b/greenwave/xmlrpc_server_proxy.py @@ -5,11 +5,11 @@ Provides an "xmlrpc.client.ServerProxy" object with a timeout on the socket. """ import urllib.parse import xmlrpc.client +from functools import lru_cache -from flask import current_app, has_app_context - -def get_server_proxy(uri, timeout=None): +@lru_cache(maxsize=None) +def get_server_proxy(uri, timeout): """ Create an :py:class:`xmlrpc.client.ServerProxy` instance with a socket timeout. @@ -17,19 +17,12 @@ def get_server_proxy(uri, timeout=None): Args: uri (str): The connection point on the server in the format of scheme://host/target. - timeout (int): The timeout to set on the transport socket. This defaults to the Flask - configuration `REQUESTS_TIMEOUT` if there is an application context. + timeout (int): The timeout to set on the transport socket. Returns: xmlrpc.client.ServerProxy: An instance of :py:class:`xmlrpc.client.ServerProxy` with a socket timeout set. """ - if timeout is None and has_app_context(): - if isinstance(current_app.config['REQUESTS_TIMEOUT'], tuple): - timeout = current_app.config['REQUESTS_TIMEOUT'][1] - else: - timeout = current_app.config['REQUESTS_TIMEOUT'] - parsed_uri = urllib.parse.urlparse(uri) if parsed_uri.scheme == 'https': transport = SafeTransport(timeout=timeout)