From 033295bc5e401701d80acf87fbeb5dd817ee7567 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <lholecek@redhat.com>
Date: Aug 14 2018 14:47:29 +0000
Subject: [PATCH 1/5] Fetch the required results only when when needed


Fetches result for specific subject, test case and optionally scenario
only when needed by a rule in policy.

Only single result (`limit=1` instead of `limit=1000` in GET requests)
is fetched at a time since it's very probable that it'll be the one
needed. Next result needs to be fetched only if the result ID matches
one in `ignore_result`.

Since cache keys are now more specific (additionally test case name and
scenario are used), fewer cache entries are invalidated when new result
is added to ResultsDB.

---

diff --git a/functional-tests/consumers/test_resultsdb.py b/functional-tests/consumers/test_resultsdb.py
index 8c7dcd6..167c254 100644
--- a/functional-tests/consumers/test_resultsdb.py
+++ b/functional-tests/consumers/test_resultsdb.py
@@ -320,8 +320,8 @@ def test_invalidate_new_result_with_mocked_cache(
         #'topic_prefix.environment.waiver.new',
     ]
     handler.consume(message)
-    expected = 'greenwave.resources:retrieve_results|koji_build %s' % nvr
-    handler.cache.delete.assert_called_once_with(expected)
+    cache_key = 'greenwave.resources:CachedResults|koji_build {} dist.rpmdeplint'.format(nvr)
+    handler.cache.delete.assert_called_once_with(cache_key)
 
 
 @mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')
@@ -369,7 +369,7 @@ def test_invalidate_new_result_with_real_cache(
                 'id': 'whatever',
                 'outcome': 'doesn\'t matter',
                 'testcase': {
-                    'name': 'dist.rpmdeplint'
+                    'name': 'dist.abicheck'
                 },
                 'data': {
                     'item': [nvr],
diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py
index 9481185..6682397 100644
--- a/greenwave/api_v1.py
+++ b/greenwave/api_v1.py
@@ -4,7 +4,7 @@ from flask import Blueprint, request, current_app, jsonify, url_for, redirect
 from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType, InternalServerError
 from greenwave import __version__
 from greenwave.policies import summarize_answers, RemotePolicy, RemoteRule
-from greenwave.resources import retrieve_results, retrieve_waivers, retrieve_builds_in_update
+from greenwave.resources import ResultsRetriever, retrieve_waivers, retrieve_builds_in_update
 from greenwave.safe_yaml import SafeYAMLError
 from greenwave.utils import insert_headers, jsonp
 
@@ -327,13 +327,12 @@ def make_decision():
                 subject_type, decision_context, product_version))
 
     answers = []
-    results = retrieve_results(subject_type, subject_identifier)
-    results = [r for r in results if r['id'] not in ignore_results]
+    results_retriever = ResultsRetriever(current_app.cache, ignore_results)
     waivers = retrieve_waivers(product_version, subject_type, [subject_identifier])
     waivers = [w for w in waivers if w['id'] not in ignore_waivers]
 
     for policy in subject_policies:
-        answers.extend(policy.check(subject_identifier, results, waivers))
+        answers.extend(policy.check(subject_identifier, results_retriever, waivers))
 
     if build_policies:
         build_nvrs = retrieve_builds_in_update(subject_identifier)
@@ -343,17 +342,13 @@ def make_decision():
         waivers.extend(nvrs_waivers)
 
         for nvr in build_nvrs:
-            nvr_results = retrieve_results('koji_build', nvr)
-            nvr_results = [r for r in nvr_results if r['id'] not in ignore_results]
-            results.extend(nvr_results)
-
             nvr_waivers = [
                 item for item in nvrs_waivers
                 if nvr == item.get('subject_identifier')
             ]
 
             for policy in build_policies:
-                answers.extend(policy.check(nvr, nvr_results, nvr_waivers))
+                answers.extend(policy.check(nvr, results_retriever, nvr_waivers))
 
     res = {
         'policies_satisfied': all(answer.is_satisfied for answer in answers),
@@ -364,7 +359,7 @@ def make_decision():
     }
     if verbose:
         res.update({
-            'results': results,
+            'results': results_retriever.all_retrieved_results(),
             'waivers': waivers,
             'satisfied_requirements':
                 [answer.to_json() for answer in answers if answer.is_satisfied],
diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py
index 999a973..5cf4d5d 100644
--- a/greenwave/consumers/resultsdb.py
+++ b/greenwave/consumers/resultsdb.py
@@ -17,7 +17,6 @@ import fedmsg.consumers
 import requests
 
 import greenwave.app_factory
-import greenwave.cache
 import greenwave.resources
 from greenwave.api_v1 import subject_type_identifier_to_list
 
@@ -25,6 +24,20 @@ from greenwave.api_v1 import subject_type_identifier_to_list
 log = logging.getLogger(__name__)
 
 
+def _invalidate_results_cache(
+        cache, subject_type, subject_identifier, testcase):
+    """
+    Removes results for given parameters from cache.
+    """
+    key = greenwave.resources.results_cache_key(
+        subject_type, subject_identifier, testcase)
+    if not cache.get(key):
+        log.debug("No cache value found for %r", key)
+    else:
+        log.debug("Invalidating cache for %r", key)
+        cache.delete(key)
+
+
 class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):
     """
     Handle a new result.
@@ -123,7 +136,8 @@ class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):
         with self.flask_app.app_context():
             for subject_type, subject_identifier in self.announcement_subjects(message):
                 log.debug('Considering subject %s: %r', subject_type, subject_identifier)
-                self._invalidate_cache(subject_type, subject_identifier)
+                _invalidate_results_cache(
+                    self.cache, subject_type, subject_identifier, testcase)
                 self._publish_decision_changes(subject_type, subject_identifier,
                                                result_id, testcase)
 
@@ -206,20 +220,3 @@ class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):
                     log.debug('Emitted a fedmsg, %r, on the "%s" topic', decision,
                               'greenwave.decision.update')
                     fedmsg.publish(topic='decision.update', msg=decision)
-
-    def _invalidate_cache(self, subject_type, subject_identifier):
-        """
-        Process the given subject and delete cache keys as necessary.
-
-        Args:
-            subject_type (str): A subject type, used to query greenwave.
-            subject_identifier (str): A subject identifier, used to query greenwave.
-        """
-        namespace = None
-        fn = greenwave.resources.retrieve_results
-        key = greenwave.cache.key_generator(namespace, fn)(subject_type, subject_identifier)
-        if not self.cache.get(key):
-            log.debug("No cache value found for %r", key)
-        else:
-            log.debug("Invalidating cache for %r", key)
-            self.cache.delete(key)
diff --git a/greenwave/policies.py b/greenwave/policies.py
index ea0fea8..a1dd46b 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+
 
 from fnmatch import fnmatch
+import itertools
 import logging
 import re
 import greenwave.resources
@@ -262,7 +263,7 @@ class Rule(SafeYAMLObject):
 
     This base class is not used directly.
     """
-    def check(self, subject_type, subject_identifier, results, waivers):
+    def check(self, subject_type, subject_identifier, results_retriever, waivers):
         """
         Evaluate this policy rule for the given item.
 
@@ -271,7 +272,8 @@ class Rule(SafeYAMLObject):
                 (for example, 'koji_build', 'bodhi_update', ...)
             subject_identifier (str): Item we are making a decision about (for
                 example, Koji build NVR, Bodhi update id, ...)
-            results (list): List of result objects looked up in ResultsDB for this item.
+            results_retriever (ResultsRetriever): Object for retrieving data
+                from ResultsDB.
             waivers (list): List of waiver objects looked up in WaiverDB for the results.
 
         Returns:
@@ -290,7 +292,7 @@ class RemoteRule(Rule):
     yaml_tag = '!RemoteRule'
     safe_yaml_attributes = {}
 
-    def check(self, subject_type, subject_identifier, results, waivers):
+    def check(self, subject_type, subject_identifier, results_retriever, waivers):
         if subject_type != 'koji_build':
             return []
 
@@ -320,7 +322,7 @@ class RemoteRule(Rule):
 
         answers = []
         for policy in policies:
-            response = policy.check(subject_identifier, results, waivers)
+            response = policy.check(subject_identifier, results_retriever, waivers)
             if isinstance(response, list):
                 answers.extend(response)
             else:
@@ -345,19 +347,20 @@ class PassingTestCaseRule(Rule):
         'scenario': SafeYAMLString(optional=True),
     }
 
-    def check(self, subject_type, subject_identifier, results, waivers):
-        matching_results = [
-            r for r in results if r['testcase']['name'] == self.test_case_name]
+    def check(self, subject_type, subject_identifier, results_retriever, waivers):
+        matching_results = results_retriever.retrieve(
+            subject_type, subject_identifier, self.test_case_name)
         matching_waivers = [
             w for w in waivers if (w['testcase'] == self.test_case_name and w['waived'] is True)]
 
-        # Rules may optionally specify a scenario to limit applicability.
-        if self.scenario:
-            matching_results = [r for r in matching_results if self.scenario in
-                                r['data'].get('scenario', [])]
+        if self.scenario is not None:
+            matching_results = (
+                result for result in matching_results
+                if self.scenario in result['data']['scenario'])
 
-        # Investigate the absence of results first.
-        if not matching_results:
+        # Investigate the absence of result first.
+        latest_result = next(matching_results, None)
+        if not latest_result:
             if not matching_waivers:
                 return TestResultMissing(subject_type, subject_identifier, self.test_case_name,
                                          self.scenario)
@@ -366,6 +369,7 @@ class PassingTestCaseRule(Rule):
 
         # For compose make decisions based on all architectures and variants.
         if subject_type == 'compose':
+            matching_results = itertools.chain([latest_result], matching_results)
             visited_arch_variants = set()
             answers = []
             for result in matching_results:
@@ -388,8 +392,8 @@ class PassingTestCaseRule(Rule):
         # If we find multiple matching results, we always use the first one which
         # will be the latest chronologically, because ResultsDB always returns
         # results ordered by `submit_time` descending.
-        matching_result = matching_results[0]
-        return self._answer_for_result(matching_result, waivers, subject_type, subject_identifier)
+        return self._answer_for_result(
+            latest_result, waivers, subject_type, subject_identifier)
 
     def to_json(self):
         return {
@@ -399,7 +403,7 @@ class PassingTestCaseRule(Rule):
         }
 
     def _answer_for_result(self, result, waivers, subject_type, subject_identifier):
-        if result['outcome'] in ['PASSED', 'INFO']:
+        if result['outcome'] in ('PASSED', 'INFO'):
             return TestResultPassed(self.test_case_name, result['id'])
 
         # TODO limit who is allowed to waive
@@ -429,7 +433,7 @@ class PackageSpecificRule(Rule):
         'repos': SafeYAMLList(str),
     }
 
-    def check(self, subject_type, subject_identifier, results, waivers):
+    def check(self, subject_type, subject_identifier, results_retriever, waivers):
         """ Check that the subject passes testcase for the given results, but
         only if the subject is a build of a package name configured for
         this rule, specified by "repos".  Any of the repos may be a glob.
@@ -453,7 +457,7 @@ class PackageSpecificRule(Rule):
         rule = PassingTestCaseRule()
         # pylint: disable=attribute-defined-outside-init
         rule.test_case_name = self.test_case_name
-        return rule.check(subject_type, subject_identifier, results, waivers)
+        return rule.check(subject_type, subject_identifier, results_retriever, waivers)
 
     def to_json(self):
         return {
@@ -492,7 +496,7 @@ class Policy(SafeYAMLObject):
                 self._applies_to_product_version(product_version) and
                 subject_type == self.subject_type)
 
-    def check(self, subject_identifier, results, waivers):
+    def check(self, subject_identifier, results_retriever, waivers):
         # If an item is about a package and it is in the blacklist, return RuleSatisfied()
         if self.subject_type == 'koji_build':
             name = subject_identifier.rsplit('-', 2)[0]
@@ -500,7 +504,7 @@ class Policy(SafeYAMLObject):
                 return [BlacklistedInPolicy(subject_identifier) for rule in self.rules]
         answers = []
         for rule in self.rules:
-            response = rule.check(self.subject_type, subject_identifier, results, waivers)
+            response = rule.check(self.subject_type, subject_identifier, results_retriever, waivers)
             if isinstance(response, list):
                 answers.extend(response)
             else:
diff --git a/greenwave/resources.py b/greenwave/resources.py
index e57129c..3713c44 100644
--- a/greenwave/resources.py
+++ b/greenwave/resources.py
@@ -24,6 +24,119 @@ log = logging.getLogger(__name__)
 requests_session = requests.Session()
 
 
+class CachedResults(object):
+    """
+    Results data in cache.
+    """
+    def __init__(self):
+        self.results = []
+        self.can_fetch_more = True
+        self.last_page = -1
+
+
+def results_cache_key(subject_type, subject_identifier, testcase):
+    """
+    Returns cache key for results for given parameters.
+    """
+    return "greenwave.resources:CachedResults|{} {} {}".format(
+        subject_type, subject_identifier, testcase)
+
+
+class ResultsRetriever(object):
+    """
+    Retrieves results from cache or ResultsDB.
+    """
+    def __init__(self, cache, ignore_results):
+        self.cache = cache
+        self.ignore_results = ignore_results
+        self.timeout = current_app.config['REQUESTS_TIMEOUT']
+        self.verify = current_app.config['REQUESTS_VERIFY']
+        self.url = current_app.config['RESULTSDB_API_URL']
+        self.all_results = []
+
+    def all_retrieved_results(self):
+        """
+        Returns all results retrieved from cache or ResultsDB by this instance.
+        """
+        return sorted(self.all_results, key=lambda x: x['id'], reverse=True)
+
+    def retrieve(self, subject_type, subject_identifier, testcase):
+        """
+        Return generator over results.
+        """
+        for result in self._retrieve_helper(subject_type, subject_identifier, testcase):
+            if result['id'] not in self.ignore_results:
+                if result not in self.all_results:
+                    self.all_results.append(result)
+                yield result
+
+    def _retrieve_helper(self, subject_type, subject_identifier, testcase):
+        cache_key = results_cache_key(
+            subject_type, subject_identifier, testcase)
+
+        cached_results = self.cache.get(cache_key)
+        if not isinstance(cached_results, CachedResults):
+            cached_results = CachedResults()
+
+        for result in cached_results.results:
+            yield result
+
+        while cached_results.can_fetch_more:
+            cached_results.last_page += 1
+            results = self._retrieve_page(
+                cached_results.last_page, subject_type, subject_identifier,
+                testcase)
+            cached_results.results.extend(results)
+            cached_results.can_fetch_more = bool(results)
+            self.cache.set(cache_key, cached_results)
+            for result in results:
+                yield result
+
+    def _make_request(self, params):
+        response = requests_session.get(
+            self.url + '/results', params=params, verify=self.verify, timeout=self.timeout)
+        response.raise_for_status()
+        return response.json()['data']
+
+    def _retrieve_page(self, page, subject_type, subject_identifier, testcase):
+        params = {
+            'testcases': testcase,
+            'limit': 1,
+            'page': page,
+        }
+
+        results = []
+        if subject_type == 'bodhi_update':
+            params['type'] = subject_type
+            params['item'] = subject_identifier
+            results = self._make_request(params=params)
+        elif subject_type == 'koji_build':
+            params['type'] = subject_type
+            params['item'] = subject_identifier
+            results = self._make_request(params=params)
+
+            params['type'] = 'brew-build'
+            results.extend(self._make_request(params=params))
+
+            del params['type']
+            del params['item']
+            params['original_spec_nvr'] = subject_identifier
+            results.extend(self._make_request(params=params))
+        elif subject_type == 'compose':
+            params['productmd.compose.id'] = subject_identifier
+            results = self._make_request(params=params)
+
+            del params['productmd.compose.id']
+
+            params['type'] = 'compose'
+            params['item'] = subject_identifier
+            results.extend(self._make_request(params=params))
+        else:
+            raise RuntimeError('Unhandled subject type %r' % subject_type)
+
+        return results
+
+
 @cached
 @greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError)
 def retrieve_scm_from_koji(nvr):
@@ -127,47 +240,6 @@ def retrieve_update_for_build(nvr):
     return None
 
 
-def retrieve_item_results(item):
-    """ Retrieve cached results from resultsdb for a given item. """
-    # XXX make this more efficient than just fetching everything
-
-    params = item.copy()
-    params.update({'limit': '1000'})
-    timeout = current_app.config['REQUESTS_TIMEOUT']
-    verify = current_app.config['REQUESTS_VERIFY']
-    response = requests_session.get(
-        current_app.config['RESULTSDB_API_URL'] + '/results',
-        params=params, verify=verify, timeout=timeout)
-    response.raise_for_status()
-    return response.json()['data']
-
-
-@cached
-def retrieve_results(subject_type, subject_identifier):
-    """
-    Returns all results from ResultsDB which might be relevant for the given
-    decision subject, accounting for all the different possible ways in which
-    test results can be reported.
-    """
-    # Note that the reverse of this logic also lives in the
-    # announcement_subjects() method of the Resultsdb consumer (it has to map
-    # from a newly received result back to the possible subjects it is for).
-    results = []
-    if subject_type == 'bodhi_update':
-        results.extend(retrieve_item_results(
-            {'type': 'bodhi_update', 'item': subject_identifier}))
-    elif subject_type == 'koji_build':
-        results.extend(retrieve_item_results({'type': 'koji_build', 'item': subject_identifier}))
-        results.extend(retrieve_item_results({'type': 'brew-build', 'item': subject_identifier}))
-        results.extend(retrieve_item_results({'original_spec_nvr': subject_identifier}))
-    elif subject_type == 'compose':
-        results.extend(retrieve_item_results({'productmd.compose.id': subject_identifier}))
-        results.extend(retrieve_item_results({'type': 'compose', 'item': subject_identifier}))
-    else:
-        raise RuntimeError('Unhandled subject type %r' % subject_type)
-    return results
-
-
 # NOTE - not cached, for now.
 @greenwave.utils.retry(wait_on=urllib3.exceptions.NewConnectionError)
 def retrieve_waivers(product_version, subject_type, subject_identifiers):
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index 89badbc..31bba4a 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -20,6 +20,25 @@ from greenwave.utils import load_policies
 from greenwave.safe_yaml import SafeYAMLError
 
 
+class DummyResults(object):
+    def __init__(self, subject_identifier=None, testcase=None, outcome='PASSED'):
+        self.subject_identifier = subject_identifier
+        self.testcase = testcase
+        self.outcome = outcome
+
+    def retrieve(self, subject_type, subject_identifier, testcase):
+        if subject_identifier == self.subject_identifier and testcase == self.testcase:
+            yield {
+                'id': 123,
+                'data': {
+                    'item': self.subject_identifier,
+                    'type': 'koji_build',
+                },
+                'testcase': {'name': self.testcase},
+                'outcome': self.outcome,
+            }
+
+
 def test_summarize_answers():
     assert summarize_answers([RuleSatisfied()]) == \
         'all required tests passed'
@@ -56,7 +75,7 @@ rules:
     policy = policies[0]
 
     # Ensure that absence of a result is failure.
-    item, results, waivers = {}, [], []
+    item, results, waivers = {}, DummyResults(), []
     decision = policy.check(item, results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
@@ -137,94 +156,52 @@ rules:
     policy = policies[0]
 
     # Ensure that we fail with no results
-    results, waivers = [], []
+    results, waivers = DummyResults(), []
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
 
     # That a matching, failing result can fail
-    results = [{
-        'id': 123,
-        'data': {
-            'item': 'nethack-1.2.3-1.el9000',
-            'type': 'koji_build',
-        },
-        'testcase': {'name': 'sometest'},
-        'outcome': 'FAILED',
-    }]
+    results = DummyResults('nethack-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultFailed)
 
     # That a matching, passing result can pass
-    results = [{
-        'id': 123,
-        'data': {
-            'item': 'nethack-1.2.3-1.el9000',
-            'type': 'koji_build',
-        },
-        'testcase': {'name': 'sometest'},
-        'outcome': 'PASSED',
-    }]
+    results = DummyResults('nethack-1.2.3-1.el9000', 'sometest')
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)
 
     # That a non-matching passing result is ignored.
-    results = [{
-        'id': 123,
-        'item': 'foobar-1.2.3-1.el9000',
-        'testcase': {'name': 'sometest'},
-        'outcome': 'PASSED',
-    }]
+    results = DummyResults('foobar-1.2.3-1.el9000', 'sometest')
     decision = policy.check('foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)
 
     # That a non-matching failing result is ignored.
-    results = [{
-        'id': 123,
-        'item': 'foobar-1.2.3-1.el9000',
-        'testcase': {'name': 'sometest'},
-        'outcome': 'FAILED',
-    }]
+    results = DummyResults('foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)  # ooooh.
 
     # Ensure that fnmatch globs work in absence
-    results, waivers = [], []
+    results, waivers = DummyResults(), []
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
 
     # Ensure that fnmatch globs work in the negative.
-    results = [{
-        'id': 123,
-        'data': {
-            'item': 'nethack-1.2.3-1.el9000',
-            'type': 'koji_build',
-        },
-        'testcase': {'name': 'sometest'},
-        'outcome': 'FAILED',
-    }]
+    results = DummyResults('python-foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultFailed)
 
     # Ensure that fnmatch globs work in the positive.
-    results = [{
-        'id': 123,
-        'data': {
-            'item': 'nethack-1.2.3-1.el9000',
-            'type': 'koji_build',
-        },
-        'testcase': {'name': 'sometest'},
-        'outcome': 'SUCCESS',
-    }]
+    results = DummyResults('python-foobar-1.2.3-1.el9000', 'sometest')
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
-    assert isinstance(decision[0], TestResultFailed)
+    assert isinstance(decision[0], RuleSatisfied)
 
 
 def test_load_policies():
@@ -351,29 +328,19 @@ rules:
                 waivers = []
 
                 # Ensure that presence of a result is success.
-                results = [{
-                    "id": 12345,
-                    "data": {"original_spec_nvr": [nvr]},
-                    "testcase": {"name": "dist.upgradepath"},
-                    "outcome": "PASSED"
-                }]
+                results = DummyResults(nvr, 'dist.upgradepath')
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], RuleSatisfied)
 
                 # Ensure that absence of a result is failure.
-                results = []
+                results = DummyResults()
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], TestResultMissing)
 
                 # And that a result with a failure, is a failure.
-                results = [{
-                    "id": 12345,
-                    "data": {"original_spec_nvr": [nvr]},
-                    "testcase": {"name": "dist.upgradepath"},
-                    "outcome": "FAILED"
-                }]
+                results = DummyResults(nvr, 'dist.upgradepath', 'FAILED')
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], TestResultFailed)

From ffc32d138abd98ff32a50a99cc21701f27166485 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <lholecek@redhat.com>
Date: Aug 16 2018 10:18:55 +0000
Subject: [PATCH 2/5] Fix possible race condition when invalidating cache


---

diff --git a/functional-tests/consumers/test_resultsdb.py b/functional-tests/consumers/test_resultsdb.py
index 167c254..a1b8f98 100644
--- a/functional-tests/consumers/test_resultsdb.py
+++ b/functional-tests/consumers/test_resultsdb.py
@@ -442,7 +442,8 @@ def test_invalidate_new_result_with_no_preexisting_cache(
         #'topic_prefix.environment.waiver.new',
     ]
     handler.consume(message)
-    handler.cache.delete.assert_not_called()
+    cache_key = 'greenwave.resources:CachedResults|koji_build {} dist.rpmdeplint'.format(nvr)
+    handler.cache.delete.assert_called_once_with(cache_key)
 
 
 @mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')
diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py
index 5cf4d5d..fbc2d10 100644
--- a/greenwave/consumers/resultsdb.py
+++ b/greenwave/consumers/resultsdb.py
@@ -31,11 +31,13 @@ def _invalidate_results_cache(
     """
     key = greenwave.resources.results_cache_key(
         subject_type, subject_identifier, testcase)
-    if not cache.get(key):
-        log.debug("No cache value found for %r", key)
-    else:
-        log.debug("Invalidating cache for %r", key)
+
+    log.debug("Invalidating cache for %r", key)
+
+    try:
         cache.delete(key)
+    except KeyError:
+        log.debug("No cache value found for %r", key)
 
 
 class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):

From 287949579bb28c2efc9ad26844b85b3a29eca127 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Aug 16 2018 10:18:55 +0000
Subject: [PATCH 3/5] Fix failing test


---

diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py
index 6682397..e5fafde 100644
--- a/greenwave/api_v1.py
+++ b/greenwave/api_v1.py
@@ -327,7 +327,12 @@ def make_decision():
                 subject_type, decision_context, product_version))
 
     answers = []
-    results_retriever = ResultsRetriever(current_app.cache, ignore_results)
+    results_retriever = ResultsRetriever(
+        cache=current_app.cache,
+        ignore_results=ignore_results,
+        timeout=current_app.config['REQUESTS_TIMEOUT'],
+        verify=current_app.config['REQUESTS_VERIFY'],
+        url=current_app.config['RESULTSDB_API_URL'])
     waivers = retrieve_waivers(product_version, subject_type, [subject_identifier])
     waivers = [w for w in waivers if w['id'] not in ignore_waivers]
 
diff --git a/greenwave/resources.py b/greenwave/resources.py
index 3713c44..f385d1a 100644
--- a/greenwave/resources.py
+++ b/greenwave/resources.py
@@ -46,12 +46,12 @@ class ResultsRetriever(object):
     """
     Retrieves results from cache or ResultsDB.
     """
-    def __init__(self, cache, ignore_results):
+    def __init__(self, cache, ignore_results, timeout, verify, url):
         self.cache = cache
         self.ignore_results = ignore_results
-        self.timeout = current_app.config['REQUESTS_TIMEOUT']
-        self.verify = current_app.config['REQUESTS_VERIFY']
-        self.url = current_app.config['RESULTSDB_API_URL']
+        self.timeout = timeout
+        self.verify = verify
+        self.url = url
         self.all_results = []
 
     def all_retrieved_results(self):
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index 31bba4a..257c56d 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -16,27 +16,40 @@ from greenwave.policies import (
     TestResultFailed,
     InvalidGatingYaml
 )
+from greenwave.resources import ResultsRetriever
 from greenwave.utils import load_policies
 from greenwave.safe_yaml import SafeYAMLError
 
 
-class DummyResults(object):
-    def __init__(self, subject_identifier=None, testcase=None, outcome='PASSED'):
+class DummyResultsRetriever(ResultsRetriever):
+    def __init__(
+            self, subject_identifier=None, testcase=None, outcome='PASSED',
+            subject_type='koji_build'):
+        super(DummyResultsRetriever, self).__init__(
+            cache=mock.Mock(),
+            ignore_results=[],
+            timeout=0,
+            verify=False,
+            url='')
         self.subject_identifier = subject_identifier
+        self.subject_type = subject_type
         self.testcase = testcase
         self.outcome = outcome
 
-    def retrieve(self, subject_type, subject_identifier, testcase):
-        if subject_identifier == self.subject_identifier and testcase == self.testcase:
-            yield {
+    def _make_request(self, params):
+        if (params.get('item') == self.subject_identifier and
+                params.get('type') == self.subject_type and
+                params.get('testcases') == self.testcase):
+            return [{
                 'id': 123,
                 'data': {
-                    'item': self.subject_identifier,
-                    'type': 'koji_build',
+                    'item': [self.subject_identifier],
+                    'type': [self.subject_type],
                 },
                 'testcase': {'name': self.testcase},
                 'outcome': self.outcome,
-            }
+            }]
+        return []
 
 
 def test_summarize_answers():
@@ -75,7 +88,7 @@ rules:
     policy = policies[0]
 
     # Ensure that absence of a result is failure.
-    item, results, waivers = {}, DummyResults(), []
+    item, results, waivers = {}, DummyResultsRetriever(), []
     decision = policy.check(item, results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
@@ -108,15 +121,7 @@ rules:
     policies = load_policies(tmpdir.strpath)
     policy = policies[0]
 
-    result = {
-        u'data': {
-            u'item': [u'some_nevr'],
-            u'type': [u'brew-build'],
-        },
-        u'id': 6336180,
-        u'outcome': u'FAILED',
-        u'testcase': {u'name': u'sometest'},
-    }
+    results = DummyResultsRetriever('some_nevr', 'sometest', 'FAILED', 'brew-build')
     waiver = {
         u'subject_identifier': u'some_nevr',
         u'subject_type': u'koji_build',
@@ -124,7 +129,7 @@ rules:
         u'waived': True,
     }
 
-    item, results, waivers = 'some_nevr', [result], [waiver]
+    item, waivers = 'some_nevr', [waiver]
     decision = policy.check(item, results, [])
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultFailed)
@@ -156,49 +161,49 @@ rules:
     policy = policies[0]
 
     # Ensure that we fail with no results
-    results, waivers = DummyResults(), []
+    results, waivers = DummyResultsRetriever(), []
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
 
     # That a matching, failing result can fail
-    results = DummyResults('nethack-1.2.3-1.el9000', 'sometest', 'FAILED')
+    results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultFailed)
 
     # That a matching, passing result can pass
-    results = DummyResults('nethack-1.2.3-1.el9000', 'sometest')
+    results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest')
     decision = policy.check('nethack-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)
 
     # That a non-matching passing result is ignored.
-    results = DummyResults('foobar-1.2.3-1.el9000', 'sometest')
+    results = DummyResultsRetriever('foobar-1.2.3-1.el9000', 'sometest')
     decision = policy.check('foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)
 
     # That a non-matching failing result is ignored.
-    results = DummyResults('foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
+    results = DummyResultsRetriever('foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)  # ooooh.
 
     # Ensure that fnmatch globs work in absence
-    results, waivers = DummyResults(), []
+    results, waivers = DummyResultsRetriever(), []
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultMissing)
 
     # Ensure that fnmatch globs work in the negative.
-    results = DummyResults('python-foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
+    results = DummyResultsRetriever('python-foobar-1.2.3-1.el9000', 'sometest', 'FAILED')
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], TestResultFailed)
 
     # Ensure that fnmatch globs work in the positive.
-    results = DummyResults('python-foobar-1.2.3-1.el9000', 'sometest')
+    results = DummyResultsRetriever('python-foobar-1.2.3-1.el9000', 'sometest')
     decision = policy.check('python-foobar-1.2.3-1.el9000', results, waivers)
     assert len(decision) == 1
     assert isinstance(decision[0], RuleSatisfied)
@@ -328,19 +333,19 @@ rules:
                 waivers = []
 
                 # Ensure that presence of a result is success.
-                results = DummyResults(nvr, 'dist.upgradepath')
+                results = DummyResultsRetriever(nvr, 'dist.upgradepath')
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], RuleSatisfied)
 
                 # Ensure that absence of a result is failure.
-                results = DummyResults()
+                results = DummyResultsRetriever()
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], TestResultMissing)
 
                 # And that a result with a failure, is a failure.
-                results = DummyResults(nvr, 'dist.upgradepath', 'FAILED')
+                results = DummyResultsRetriever(nvr, 'dist.upgradepath', 'FAILED')
                 decision = policy.check(nvr, results, waivers)
                 assert len(decision) == 1
                 assert isinstance(decision[0], TestResultFailed)

From 5b667006a5f753d32693064659c84eac6a4eaaeb Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Aug 16 2018 10:47:27 +0000
Subject: [PATCH 4/5] Add test for waiving bodhi update


---

diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index 257c56d..5362b6e 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -145,6 +145,51 @@ rules:
     assert isinstance(decision[0], TestResultFailed)
 
 
+def test_waive_bodhi_update(tmpdir):
+    """ Ensure that a koji_build waiver can match a brew-build result
+
+    Note that 'brew-build' in the result does not match 'koji_build' in the
+    waiver.  Even though these are different strings, this should work.
+    """
+
+    p = tmpdir.join('fedora.yaml')
+    p.write("""
+--- !Policy
+id: some_id
+product_versions:
+- irrelevant
+decision_context: test
+subject_type: bodhi_update
+rules:
+  - !PassingTestCaseRule {test_case_name: sometest}
+        """)
+    policies = load_policies(tmpdir.strpath)
+    policy = policies[0]
+
+    item = 'some_bodhi_update'
+    results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'bodhi_update')
+    waivers = [{
+        u'subject_identifier': item,
+        u'subject_type': 'bodhi_update',
+        u'testcase': 'sometest',
+        u'waived': True,
+    }]
+
+    decision = policy.check(item, results, [])
+    assert len(decision) == 1
+    assert isinstance(decision[0], TestResultFailed)
+
+    decision = policy.check(item, results, waivers)
+    assert len(decision) == 1
+    assert isinstance(decision[0], RuleSatisfied)
+
+    # Also, be sure that negative waivers work.
+    waivers[0]['waived'] = False
+    decision = policy.check(item, results, waivers)
+    assert len(decision) == 1
+    assert isinstance(decision[0], TestResultFailed)
+
+
 def test_package_specific_rule(tmpdir):
     p = tmpdir.join('fedora.yaml')
     p.write("""

From e41a2fead1991afd618dc0b62bbc0f2b8ba7ef8b Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Aug 16 2018 13:14:39 +0000
Subject: [PATCH 5/5] Add test for resultsdb cache


---

diff --git a/greenwave/tests/test_cache.py b/greenwave/tests/test_cache.py
new file mode 100644
index 0000000..2e5aaa5
--- /dev/null
+++ b/greenwave/tests/test_cache.py
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+import mock
+
+import greenwave.resources
+
+
+def test_resultsdb_cache():
+    subject_type = 'koji_build'
+    subject_identifier = 'nethack-1.2.3-1.el9000'
+    testcase = ''
+
+    cache = mock.Mock()
+
+    with mock.patch('greenwave.resources.ResultsRetriever._make_request') as retrieve_method:
+        retrieve_method.return_value = []
+
+        results_retriever = greenwave.resources.ResultsRetriever(
+            cache=cache,
+            ignore_results=[],
+            timeout=0,
+            verify=False,
+            url='')
+
+        results = results_retriever.retrieve(subject_type, subject_identifier, testcase)
+
+        assert retrieve_method.call_count == 0
+        assert cache.set.call_count == 0
+
+        assert len(list(results)) == 0
+
+        assert retrieve_method.call_count > 0
+        assert cache.set.call_count == 1
+
+    key = greenwave.resources.results_cache_key(
+        subject_type, subject_identifier, testcase)
+    actual_key, actual_cached_results = cache.set.call_args[0]
+    assert actual_key == key
+
+    cache.get.return_value = actual_cached_results
+
+    with mock.patch('greenwave.resources.ResultsRetriever._make_request') as retrieve_method:
+        retrieve_method.return_value = []
+
+        results_retriever = greenwave.resources.ResultsRetriever(
+            cache=cache,
+            ignore_results=[],
+            timeout=0,
+            verify=False,
+            url='')
+
+        results = results_retriever.retrieve(subject_type, subject_identifier, testcase)
+
+        assert retrieve_method.call_count == 0
+        assert cache.set.call_count == 1
+
+        assert len(list(results)) == 0
+
+        assert retrieve_method.call_count == 0
+        assert cache.set.call_count == 1