The Greenwave message consumer will cause Nackloops

[bowlofeggs]

I saw that #3200 got merged today, and while looking over the final patch I noticed that it raises BodhiExceptions when it receives a message it cannot process. This will cause the Bodhi consumer to send an error e-mail to the bodhi-admin list and then Nack the message to the broker, which will put the message back into the queue. This will cause the message to loop in the queue forever:

bodhi/bodhi/server/consumers/__init__.py

Lines 100 to 104 in fde8eef

	self.greenwave_handler(msg)
	except Exception as e:
	error_msg = f'{str(e)}: Unable to handle message: {msg}'
	log.exception(error_msg)
	raise fedora_messaging.exceptions.Nack(error_msg)

I believe that messages looping on the queue forever will also lead to a halt of message processing, because eventually the head of the queue will only be messages that Bodhi cannot process and all other messages will not be delivered to the consumer.

If we receive a message we cannot process, we should log it and discard (and in this case, probably not at error if messages actually get received with these sorts of defects, because that will lead to error e-mails that are in-actionable).

This is critical to fix - we cannot release in the current state.

[tdawson]

Should be fixed in this pull request: #3303

[cverna]

If I understand this correctly we should not raise any exceptions in the consumers, then we also need to fix the following consumers :
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/consumers/automatic_updates.py
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/consumers/updates.py
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/consumers/composer.py

Also maybe we could add a comment about that in each consumers.

[bowlofeggs]

On Sat, 2019-06-08 at 10:23 -0700, Clement Verna wrote: If I understand this correctly we should not raise any exceptions in the consumers

I don't think that's generally true. It's that we don't want to raise Exceptions due to issues that will never be fixed by retrying. The problem with the patch that got merged was that it raised Exceptions when receiving messages about builds that Bodhi didn't have updates for, or messages that were missing an expected field. Retrying on either of those conditions is not going to fix it. An example of a retry that is sensible is a failure to connect to Bugzilla - if we can't connect to Bugzilla now, it is likely that retrying again later will be successful, and we do want to make sure we do whatever we were going to do in Bugzilla. Basically, permanent failures should not raise an Exception, and temporary failures should.

[cverna]

Oh yes that makes sense thanks for the clarification 😄. So I think the automatic_updates will need to be modified too, since it is raising exception if there are missing info from the message.

[bowlofeggs]

Good catch @cverna - I filed #3302 for this.

[cverna]

This issue was fixed by #3303 let's close it