PR#349: Don't try and make a decision for pipeline msgs with empty NVR - greenwave

greenwave

#349 Don't try and make a decision for pipeline msgs with empty NVR

Merged 5 years ago by gnaponie. Opened 5 years ago by adamwill.

adamwill/greenwave no-empty-nvr into master

Don't try and make a decision for pipeline msgs with empty NVR

Adam Williamson • 5 years ago

45c2de4

greenwave/consumers/resultsdb.py

file modified

+5 -1

		`@@ -181,7 +181,11 @@`
		`nvr = _decode(data['item'])`
		`else:`
		`nvr = _decode(data['original_spec_nvr'])`
		`- yield ('koji_build', nvr)`
		`+ # when the pipeline ignores a package, which happens`
		`+ # a lot, we get a message with an 'original_spec_nvr'`
		`+ # key with an empty value; let's not try and handle this`
		`+ if nvr:`
		`+ yield ('koji_build', nvr)`

		`def consume(self, message):`
		`"""`

greenwave/tests/test_resultsdb_consumer.py

file modified

+16

		`@@ -19,6 +19,22 @@`
		`assert subjects == [('koji_build', 'glibc-1.0-1.fc27')]`


		`+ def test_no_announcement_subjects_for_empty_nvr():`
		`+ """The CI pipeline submits a lot of results for the test`
		`+ 'org.centos.prod.ci.pipeline.allpackages-build.package.ignored'`
		`+ with the 'original_spec_nvr' key present, but the value just an`
		`+ empty string. To avoid unpredictable consequences, we should not`
		`+ return any announcement subjects for such a message.`
		`+ """`
		`+ cls = greenwave.consumers.resultsdb.ResultsDBHandler`
		`+ message = {'msg': {'data': {`
		`+ 'original_spec_nvr': [""],`
		`+ }}}`
		`+ subjects = list(cls.announcement_subjects(message))`
		`+`
		`+ assert subjects == []`
		`+`
		`+`
		`def test_announcement_subjects_for_brew_build():`
		`# The 'brew-build' type appears internally within Red Hat. We treat it as an`
		`# alias of 'koji_build'.`

adamwill commented 5 years ago

While working on another PR, I noticed that the pipeline reports
a lot of results for testcase
'ci.pipeline.allpackages-build.package.ignored'. In all these
results, the 'original_spec_nvr' key is present, but its value
is an empty string. It looks to me like this code, when given
one of these messages, will try to publish a decision for a
package with the NVR "". I'm not sure how things go from there,
but I'm guessing either something later just bails on an empty
subject_identifier (best case) or it actually tries and comes
up with a nonsensical decision (medium case) or it causes some
kind of error or crash (worst case). Whichever of those it is,
it seems sensible to just avoid this by not yielding a subject
unless we actually have some content in the nvr field.

Here's an example of the fedmsg for one of these results:
https://apps.fedoraproject.org/datagrepper/id?id=2018-7d336210-10f9-4afa-88a2-2dcb8f32fb11&is_raw=true&size=extra-large

Signed-off-by: Adam Williamson awilliam@redhat.com

lucarval commented 5 years ago

:thumbsup: looks reasonable

adamwill commented 5 years ago

Note, this isn't happening in Real Life yet as both Fedora instances are currently set to consume the old taskotron.result.new messages, not the new resultsdb.result.new ones like this. But we need to switch that over, and I want to avoid this doing something bad when we do that. :)