PR#3121: Start setting expiry on sent protonmsg messages - koji

koji

#3121 Start setting expiry on sent protonmsg messages

Closed 2 years ago by tkopecek. Opened 2 years ago by ralph.

ralph/koji protonmsg-ttl into master

Start setting expiry on sent protonmsg messages

Ralph Bean • 2 years ago

c1445b6

docs/source/plugins.rst

file modified

+4 -2

		`@@ -203,12 +203,14 @@`
		`before timing out`

		The ``[message]`` section sets parameters for how messages are formed.
		`- Currently only one field is understood:`

		* ``extra_limit`` -- the maximum allowed size for ``build.extra`` fields that
		- appear in messages. If the ``build.extra`` field is longer (in terms of
		+ appear in messages. If the ``build.extra`` field is longer (in terms of
		json-encoded length), then it will be omitted. The default value is ``0``
		`which means no limit.`
		+ * ``ttl`` -- the time in seconds before the broker should consider sent
		`+ messages to have expired. The broker may drop messages that have not been`
		+ delivered before the ``ttl`` is up. The default value is ``86400`` (24 hours).

		The ``[queue]`` section controls how (or if) the plugin will use the database
		`to queue messages when they cannot be immediately sent.`

plugins/hub/protonmsg.conf

file modified

		`@@ -11,6 +11,8 @@`
		`# if field is longer (json.dumps), ignore it`
		`# default value is 0 - unlimited size`
		`extra_limit = 0`
		`+ # Time after which the broker should expire messages`
		`+ ttl = 86400`

		`[queue]`
		`# enable persistent database queue`

plugins/hub/protonmsg.py

file modified

		`@@ -75,6 +75,9 @@`
		`self.send_msgs(event)`

		`def send_msgs(self, event):`
		`+ ttl = 86400 # a 24 hour default for message expiry`
		`+ if self.conf.has_option('message', 'ttl'):`
		`+ ttl = self.conf.getint('message', 'ttl')`
		`prefix = self.conf.get('broker', 'topic_prefix')`
		`for msg in self.msgs:`
		`address = 'topic://' + prefix + '.' + msg['address']`
		`@@ -86,6 +89,7 @@`
		`self.log.debug('created new sender for %s', address)`
		`self.senders[address] = sender`
		`pmsg = Message(properties=msg['props'], body=msg['body'])`
		`+ pmsg.ttl = ttl`
		`delivery = sender.send(pmsg)`
		`self.log.debug('sent message: %s', msg['props'])`
		`self.pending[delivery] = msg`

ralph commented 2 years ago

Maybe fixing #3120.

ktdreyer commented 2 years ago

deafult -> default

ktdreyer commented 2 years ago

I've tested this with a local RabbitMQ setup and it works. Messages are expired in the queue after the configured time.

rebased onto c1445b6

2 years ago

ralph commented 2 years ago

deafult -> default

Fixed in c1445b6.

tkopecek commented 2 years ago

Does it make to expand it a bit and add a value which is telling proton to not set ttl at all? e.g. ttl = None? Otherwise we are always enforcing some value indepently on broker's configuration.

ktdreyer commented 2 years ago

Koji sends no ttl value. It just uses Proton's defaults. On the wire, Proton sends no expiry header at all.

Also, since Koji sends to a topic://, those messages should introduce no load on a simple broker. (When I tested this PR with RabbitMQ, I hacked the plugin to use a queue:// prefix instead, just to see if the messages would expire, and they do with this PR.)

The problem is that broker administrators set up VirtualTopics to start queuing arbitrary messages in queues that anyone can create and abandon. Fixing all producers is whack-a-mole and complicates producer implementations. The QE cost of adding this to Koji means we'd need to set up ActiveMQ and configure VirtualTopics.

Given RH IT's recent improvements with the Network of Brokers, I think this problem is solved now for Red Hat. Right Ralph?

I do think it's worth updating Proton's API documentation to explain what "0" and "None" imply on the wire. I'll submit a PR for that to https://github.com/apache/qpid-proton

ktdreyer commented 2 years ago

The reason why I am hesitant to add another knob here is that I want this plugin to be simpler to deploy, to give users fewer options, making Koji easier to use.

(There are already so many "koji event" plugins floating around - Fedora, CentOS, and Rocky Linux all have written their own that are independent from the protonmsg one in-tree.)

ktdreyer commented 2 years ago

Proton does not accept None, it bails with TypeError.

I've documented the default proton behavior here: https://github.com/apache/qpid-proton/pull/348