From 114eef2bb0a62e1ef5ab2966c6d44a134e2c6a94 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
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)

