#398 Omit comparing result_id values for decision change
Merged 5 years ago by lholecek. Opened 5 years ago by lholecek.
lholecek/greenwave fix-decision-change-check  into  master

@@ -156,6 +156,47 @@ 

  

  @mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')

  @mock.patch('greenwave.consumers.resultsdb.fedmsg.publish')

+ def test_consume_unchanged_result(

+         mock_fedmsg, load_config, requests_session, greenwave_server,

+         testdatabuilder):

+     load_config.return_value = {'greenwave_api_url': greenwave_server + 'api/v1.0'}

+     nvr = testdatabuilder.unique_nvr(product_version='fc26')

+ 

+     testdatabuilder.create_result(

+         item=nvr, testcase_name='dist.rpmdeplint', outcome='PASSED')

+     new_result = testdatabuilder.create_result(

+         item=nvr, testcase_name='dist.rpmdeplint', outcome='PASSED')

+ 

+     message = {

+         'body': {

+             'topic': 'resultsdb.result.new',

+             'msg': {

+                 'id': new_result['id'],

+                 'outcome': 'PASSED',

+                 'testcase': {

+                     'name': 'dist.rpmdeplint',

+                 },

+                 'data': {

+                     'item': [nvr],

+                     'type': ['koji_build'],

+                 }

+             }

+         }

+     }

+     hub = mock.MagicMock()

+     hub.config = {

+         'environment': 'environment',

+         'topic_prefix': 'topic_prefix',

+     }

+     handler = resultsdb.ResultsDBHandler(hub)

+     assert handler.topic == ['topic_prefix.environment.taskotron.result.new']

+     handler.consume(message)

+ 

+     assert len(mock_fedmsg.mock_calls) == 0

+ 

+ 

+ @mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')

+ @mock.patch('greenwave.consumers.resultsdb.fedmsg.publish')

  def test_invalidate_new_result_with_mocked_cache(

          mock_fedmsg, load_config, requests_session, greenwave_server,

          testdatabuilder):

@@ -106,6 +106,38 @@ 

              cache, subject_type, subject_identifier, testcase=None)

  

  

+ def _equals_except_keys(lhs, rhs, except_keys):

+     keys = lhs.keys() - except_keys

+     return lhs.keys() == rhs.keys() \

+         and all(lhs[key] == rhs[key] for key in keys)

+ 

+ 

+ def _is_decision_unchanged(old_decision, decision):

+     """

+     Returns true only if new decision is same as old one

+     (ignores result_id values).

+     """

+     if old_decision is None or decision is None:

+         return old_decision == decision

+ 

+     requirements_keys = ('satisfied_requirements', 'unsatisfied_requirements')

+     if not _equals_except_keys(old_decision, decision, requirements_keys):

+         return False

+ 

+     ignore_keys = ('result_id',)

+     for key in requirements_keys:

+         old_requirements = old_decision[key]

+         requirements = decision[key]

+         if len(old_requirements) != len(requirements):

+             return False

+ 

+         for old_requirement, requirement in zip(old_requirements, requirements):

+             if not _equals_except_keys(old_requirement, requirement, ignore_keys):

+                 return False

+ 

+     return True

+ 

+ 

  class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):

      """

      Handle a new result.
@@ -254,7 +286,7 @@ 

                  log.exception('Failed to retrieve decision for data=%s, error: %s', data, e)

                  continue

  

-             if decision == old_decision:

+             if _is_decision_unchanged(old_decision, decision):

                  log.debug('Skipped emitting fedmsg, decision did not change: %s', decision)

              else:

                  decision.update({

When Greenwave receives new result message from ResultsDB it tries to
compare old decision (ignoring the new result) with new one (for all its
policies) so it can publish decision update message only when the
decision changed.

The new decision was seen as "changed" when any of its data differ from
the old decision. The problem is that decision data include result IDs
so it's always seen as "changed" if the new result is part of the new
decision.

Example:

# Decision before new result is available:
{
    "applicable_policies": [
        "taskotron_release_critical_tasks_for_stable"
    ],
    "policies_satisfied": true,
    "satisfied_requirements": [
        {
            "result_id": 27968725,
            "testcase": "dist.abicheck",
            "type": "test-result-passed"
        },
        {
            "result_id": 28015726,
            "testcase": "dist.rpmdeplint",
            "type": "test-result-passed"
        }
    ],
    "summary": "All required tests passed",
    "unsatisfied_requirements": []
}

# Decision after new result with ID=28015899 is available:
{
    "applicable_policies": [
        "taskotron_release_critical_tasks_for_stable"
    ],
    "policies_satisfied": true,
    "satisfied_requirements": [
        {
            "result_id": 27968725,
            "testcase": "dist.abicheck",
            "type": "test-result-passed"
        },
        {
            "result_id": 28015899,
            "testcase": "dist.rpmdeplint",
            "type": "test-result-passed"
        }
    ],
    "summary": "All required tests passed",
    "unsatisfied_requirements": []
}

With this change, the new decision is seen as "changed" when any of its
data changes except result IDs.

Fixes #395

Signed-off-by: Lukas Holecek hluk@email.cz

I find all this block a bit confusing (maybe it's just the variables name...)
Can't you just loop over decision['satisfied_requirements'] and decision['unsatisfied_requirements'] and for each of these elements make del(item['result_id']).

It is similar to what you did, but I find it less messy.

I did this before, but it changes the decision data that goes into the published message later (therefore the decision.copy() above).

There is a definitely smarter way to do this but I failed to find something like deep comparison and all my other code looked worse.

Maybe doing deepcopy and then removing the result_id values, but that seems like overkill (the decision data can be relatively large).

Yeah deepcopy would work. What about something like that? (flake8 needs to be fixed):
https://paste.fedoraproject.org/paste/An9s75xBojrREXR86YjW1w

Is that still confusing? It seems ok to me.

rebased onto b20483c42a336a41f35f91f8d0232ea01bc47115

5 years ago

rebased onto 4e842f5590153e79666b59dbee6ba91662db5489

5 years ago

How about straightforward compare without any copy operation? (Updated.)

What's lhs and rhs supposed to stand for?

What's lhs and rhs supposed to stand for?

Left/right hand side! :raised_hands:

We really only care about comparing the policies_satisfied key, right? Can we be explicit about that instead of potentially comparing some junk?

I cannot find the discussion but I the change in satisfied/unsatisfied requirements was also needed (I also mentioned it here).

I think it follows the order of rules defined in policies, so the order shouldn't change even after setting the ignore_result parameter.

If it's not required for the input variables to be dictionaries, perhaps it makes sense for except_keys to default to None.

If it's not required for the input variables to be dictionaries, perhaps it makes sense for except_keys to default to None.

I'll remove this check and add following to the function below to make it more clear.

    if old_decision is None or decision is None:
        return old_decision == decision

rebased onto 883f179

5 years ago

Commit 54407b0 fixes this pull-request

Pull-Request has been merged by lholecek

5 years ago

Pull-Request has been merged by lholecek

5 years ago