From 70279a2b30db0cb924dcf6a727332910c7fe915f Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Dec 05 2018 19:05:29 +0000 Subject: Don't attempt to make decisions from old-style compose fedmsgs This is effectively a reversion of 414d392 (PR #262). That attempted to 'fix' greenwave to make compose decisions based on the old-style taskotron.result.new messages with type 'compose'. In fact, however, it was *intentional* that greenwave does not do this, because it is not possible to do it reliably. These references have the back story: https://pagure.io/greenwave/issue/122 https://pagure.io/taskotron/resultsdb/issue/92 https://pagure.io/taskotron/resultsdb/pull-request/101 https://pagure.io/greenwave/pull-request/262#comment-70350 For compose tests, Autocloud reports the 'item' as the image it is testing. Some openQA tests report the 'item' as the image under test, and some report the 'item' as the compose ID. Thus you can never actually find all the results for a given compose by searching for a given 'item'. In order to find all results for a given compose, you must search for results with the extra data key 'productmd.compose.id' set to the compose ID, because *all* compose results *do* have that. Old-style 'taskotron.result.new' fedmsgs, however, do not have the extra data keys; they only include the 'item', 'type' and 'name', so far as identifying information goes. For a message like the one added as a test in the initial commit (and kept as a test here, with a different expected outcome), where the item is not the compose ID, we simply cannot possibly discover all the other relevant results. If the 'item' *happens* to be the compose ID, we could use it to find all relevant results with a ResultsDB query for `productmd.compose.id` (though we still couldn't find all relevant results in the message cache), but that doesn't really seem worth trying to support. The new-style 'resultsdb.result.new' fedmsgs that were added by https://pagure.io/taskotron/resultsdb/pull-request/101 *do* always include the extra data. So we *can* reliably find all relevant results if we're working with that message format, and that's what we were actually intending to do. We just never, it seems, flipped the Fedora greenwave instance over to using the new message format instead of the old one. So in the end we should just revert the change, which restores the correct behaviour of only attempting to handle compose decisions when we have productmd.compose.id, and add tests for the intended behaviour and better comments, so no-one ever has to go through this again! I will also try and get the Fedora deployment's config changed so it works from new-style messages instead of old-style messages; if we do this, it'll actually publish *useful* compose decisions. Signed-off-by: Adam Williamson --- diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py index dd563a1..07ad90a 100644 --- a/greenwave/consumers/resultsdb.py +++ b/greenwave/consumers/resultsdb.py @@ -160,9 +160,18 @@ class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer): return value _type = _decode(data.get('type')) - if _type in ['bodhi_update', 'compose', 'component-version', 'redhat-module'] and ( + if _type in ['bodhi_update', 'component-version', 'redhat-module'] and ( 'item' in data): yield (_type, _decode(data['item'])) + # note: it is *intentional* that we do not handle old format + # compose-type messages, because it is impossible to reliably + # produce a decision from these. compose decisions can only be + # reliably made from new format messages, where we can rely on + # productmd.compose.id being available. See: + # https://pagure.io/greenwave/issue/122 + # https://pagure.io/taskotron/resultsdb/issue/92 + # https://pagure.io/taskotron/resultsdb/pull-request/101 + # https://pagure.io/greenwave/pull-request/262#comment-70350 if 'productmd.compose.id' in data: yield ('compose', _decode(data['productmd.compose.id'])) if (_type == 'koji_build' and 'item' in data or diff --git a/greenwave/resources.py b/greenwave/resources.py index 621bb44..eb2caf8 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -119,12 +119,6 @@ class ResultsRetriever(object): 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)) elif subject_type == 'component-version' or subject_type == 'redhat-module': params['type'] = subject_type params['item'] = subject_identifier diff --git a/greenwave/tests/test_resultsdb_consumer.py b/greenwave/tests/test_resultsdb_consumer.py index 5397565..f5daaf6 100644 --- a/greenwave/tests/test_resultsdb_consumer.py +++ b/greenwave/tests/test_resultsdb_consumer.py @@ -31,8 +31,44 @@ def test_announcement_subjects_for_brew_build(): assert subjects == [('koji_build', 'glibc-1.0-3.fc27')] +def test_announcement_subjects_for_new_compose_message(): + """Ensure we are producing the right subjects for compose decisions + as this has caused a lot of confusion in the past. The only + reliable way to make a compose decision is by looking for the key + productmd.compose.id with value of the compose ID. This is only + possible with new-style 'resultsdb' fedmsgs, like this one. + """ + cls = greenwave.consumers.resultsdb.ResultsDBHandler + message = { + 'msg': { + 'data': { + "scenario": ["fedora.universal.x86_64.64bit"], + "source": ["openqa"], + "productmd.compose.name": ["Fedora"], + "firmware": ["bios"], + "meta.conventions": ["result productmd.compose fedora.compose"], + "productmd.compose.respin": ["0"], + "item": ["Fedora-Rawhide-20181205.n.0"], + "productmd.compose.id": ["Fedora-Rawhide-20181205.n.0"], + "type": ["compose"], + "productmd.compose.date": ["20181205"], + "productmd.compose.version": ["Rawhide"], + "arch": ["x86_64"], + "productmd.compose.type": ["nightly"], + "productmd.compose.short": ["Fedora"], + } + } + } + subjects = list(cls.announcement_subjects(message)) -def test_announcement_subjects_for_autocloud_compose(): + assert subjects == [('compose', 'Fedora-Rawhide-20181205.n.0')] + +def test_no_announcement_subjects_for_old_compose_message(): + """With an old-style 'taskotron' fedmsg like this one, it is not + possible to reliably make a compose decision - see + https://pagure.io/greenwave/issue/122 etc. So we should NOT + produce any subjects for this kind of message. + """ cls = greenwave.consumers.resultsdb.ResultsDBHandler message = { 'msg': { @@ -52,7 +88,7 @@ def test_announcement_subjects_for_autocloud_compose(): } subjects = list(cls.announcement_subjects(message)) - assert subjects == [('compose', 'Fedora-AtomicHost-28_Update-20180723.1839.x86_64.qcow2')] + assert subjects == [] @mock.patch('greenwave.resources.ResultsRetriever.retrieve')