Extend RabbitMQ scaler to support count unacked messages #700

holyketzer · 2020-03-25T10:55:50Z

Add settings which allows to count total messages count in queue (ready + unacked) to do not scale worker down if some long time processing is going.

By default countUnacked = false and it keeps old behaviour via AMQP protocol, if countUnacked = true it requires also apiHost to be set and uses HTTP management API to get queue info.

AMQP protocol doesn't allow to fetch unacked messages count, that is we need management API here. Technically we an completely switch to management API but as I understand not all servers have it enabled, that is why backward compatibility is saved and countUnacked is optional.

Checklist

A PR is opened to update the documentation on Extend RabbitMQ scaler to support HTTM management API protocol keda-docs#123
Tests have been added

It was tested in our Kubernetes cluster

Fixes #356

Signed-off-by: Alex Emelyanov <[email protected]>

tomkerkhove · 2020-03-25T11:34:03Z

Docs are saying this:

By default countUnacked is false in this case scaler uses AMQP protocol, requires host and only counts messages in the queue and ignores unacked messages.
If countUnacked is true then host is not required but apiHost is required in this case scaler uses HTTP management API and counts messages in the queue + unacked messages count.

Which requires following URLs:

Host: amqp://guest:password@localhost:5672/
Api Host: https://guest:password@localhost:443/vhostname

One could argue that we should always require guest:password@localhost:5672 to be configured and change protocol based on the flags that are turned on?

Certainly since vhost is being added via kedacore/keda-docs#70 (pending review) which is also the same.

pkg/scalers/rabbitmq_scaler.go

holyketzer · 2020-03-25T13:56:30Z

One could argue that we should always require guest:password@localhost:5672 to be configured and change protocol based on the flags that are turned on?

Sure I tried it first (parse AMQP connection string, replace protocol, port, extract vhost) and suddenly discovered that our credentials for AMQP protocol has different permissions level and can't be used to access management API. That is why I introduced new connection string.

We can use all different fields for like host, port, user, password and vhost and share them, or maybe yes, we can use one host setting in different contexts as AMQP or HTTP protocol, but i'm not sure what is more clear, maybe one host is better than host and apiHost

Certainly since vhost is being added via kedacore/keda-docs#70 (pending review) which is also the same.

Hm, we have been using vhost in AMQP connection string since we started to use Keda and it works ). You mean it wasn't documented?

ahmelsayed · 2020-03-26T00:21:20Z

pkg/scalers/rabbitmq_scaler.go

-	if err != nil {
-		rabbitmqLog.Error(err, "Error closing rabbitmq connection")
-		return err
+	if s.metadata.countUnacked == false {


how about s.connection != nil so it'd make sure to dispose the connection if it's at all created.

ahmelsayed

Thanks @holyketzer, and appreciate the thorough test. I have one minor comment.

ahmelsayed · 2020-03-26T00:30:33Z

I'm not very familiar with rabbitmq, so not sure I have anything to add to the discussion. I think having a single host as opposed to apiHost and host but from the description it doesn't seem that they are always compatible? could they be different? can the same user account work with both given the right permissions?

tomkerkhove · 2020-03-26T06:58:02Z

One could argue that we should always require guest:password@localhost:5672 to be configured and change protocol based on the flags that are turned on?

Sure I tried it first (parse AMQP connection string, replace protocol, port, extract vhost) and suddenly discovered that our credentials for AMQP protocol has different permissions level and can't be used to access management API. That is why I introduced new connection string.

We can use all different fields for like host, port, user, password and vhost and share them, or maybe yes, we can use one host setting in different contexts as AMQP or HTTP protocol, but i'm not sure what is more clear, maybe one host is better than host and apiHost

I think using host would be better but that would be a breaking change, so let's stick with seperation between the 2 and we'll open an issue to change this for 2.0 - What do you think @zroubalik @ahmelsayed ?

Certainly since vhost is being added via kedacore/keda-docs#70 (pending review) which is also the same.

Hm, we have been using vhost in AMQP connection string since we started to use Keda and it works ). You mean it wasn't documented?

Yes, I've merged it now

zroubalik · 2020-03-26T17:21:56Z

I'm not very familiar with rabbitmq, so not sure I have anything to add to the discussion. I think having a single host as opposed to apiHost and host but from the description it doesn't seem that they are always compatible? could they be different? can the same user account work with both given the right permissions?

+1 good questions

I agree with @tomkerkhove point on opening a new issue targeted for 2.0 for the breaking change (in case it makes sense to implement it that way wrt the questions raised by @ahmelsayed).

holyketzer · 2020-03-27T14:30:25Z

Ok, I got all the points and going to fix all of them.

Signed-off-by: Alex Emelyanov <[email protected]>

holyketzer · 2020-03-28T19:59:50Z

Feedback is accepted I pushed fixes + fix for doc PR

tomkerkhove

LGTM from a config/semantics approach

zroubalik

LGTM

zroubalik · 2020-04-08T18:14:19Z

@ahmelsayed should we merge this one? WDYT?

ahmelsayed

LGTM too

ahmelsayed · 2020-04-08T18:49:16Z

Thanks @holyketzer for your contributions!

holyketzer · 2020-04-08T20:39:48Z

Cool thank you guys!

* Extend RabbitMQ scaler to support count unacked messages Signed-off-by: Alex Emelyanov <[email protected]> * Fix code review feedback Signed-off-by: Alex Emelyanov <[email protected]>

* Extend RabbitMQ scaler to support count unacked messages Signed-off-by: Alex Emelyanov <[email protected]> * Fix code review feedback Signed-off-by: Alex Emelyanov <[email protected]> Signed-off-by: Amith Ganesh <[email protected]>

holyketzer requested review from ahmelsayed and zroubalik as code owners March 25, 2020 10:55

Extend RabbitMQ scaler to support count unacked messages

6a2e827

Signed-off-by: Alex Emelyanov <[email protected]>

holyketzer force-pushed the extend-rabbit-scaler branch from 3d6e809 to 6a2e827 Compare March 25, 2020 10:56

holyketzer mentioned this pull request Mar 25, 2020

Extend RabbitMQ scaler to support HTTM management API protocol kedacore/keda-docs#123

Merged

tomkerkhove reviewed Mar 25, 2020

View reviewed changes

pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved

ahmelsayed reviewed Mar 26, 2020

View reviewed changes

Christoph-Raab mentioned this pull request Mar 26, 2020

RabbitMQ ignores unacknowledged messages #638

Closed

Fix code review feedback

ce4c385

Signed-off-by: Alex Emelyanov <[email protected]>

holyketzer force-pushed the extend-rabbit-scaler branch from 5f8d644 to ce4c385 Compare March 28, 2020 19:48

tomkerkhove approved these changes Mar 30, 2020

View reviewed changes

tomkerkhove mentioned this pull request Mar 31, 2020

Consolidate apiHost & host for RabbitMQ scaler #711

Closed

tomkerkhove assigned ahmelsayed and zroubalik Mar 31, 2020

zroubalik approved these changes Apr 6, 2020

View reviewed changes

ahmelsayed approved these changes Apr 8, 2020

View reviewed changes

ahmelsayed merged commit ef7e4e9 into kedacore:master Apr 8, 2020

quoimec mentioned this pull request Apr 21, 2020

RabbitMQ HTTP API - Unable to provide default VHost value #768

Closed

smoke mentioned this pull request Jan 27, 2022

RabbitMQ ignores unacknowledged messages since >=2.0 #2572

Closed

SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023

Make HTML titles consistent: "<page-title> | KEDA" (kedacore#700)

37e739b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend RabbitMQ scaler to support count unacked messages #700

Extend RabbitMQ scaler to support count unacked messages #700

holyketzer commented Mar 25, 2020

tomkerkhove commented Mar 25, 2020

holyketzer commented Mar 25, 2020 •

edited

Loading

ahmelsayed Mar 26, 2020

ahmelsayed left a comment

ahmelsayed commented Mar 26, 2020

tomkerkhove commented Mar 26, 2020

zroubalik commented Mar 26, 2020 •

edited

Loading

holyketzer commented Mar 27, 2020

holyketzer commented Mar 28, 2020

tomkerkhove left a comment

zroubalik left a comment

zroubalik commented Apr 8, 2020

ahmelsayed left a comment

ahmelsayed commented Apr 8, 2020

holyketzer commented Apr 8, 2020

Extend RabbitMQ scaler to support count unacked messages #700

Extend RabbitMQ scaler to support count unacked messages #700

Conversation

holyketzer commented Mar 25, 2020

Checklist

tomkerkhove commented Mar 25, 2020

holyketzer commented Mar 25, 2020 • edited Loading

ahmelsayed Mar 26, 2020

Choose a reason for hiding this comment

ahmelsayed left a comment

Choose a reason for hiding this comment

ahmelsayed commented Mar 26, 2020

tomkerkhove commented Mar 26, 2020

zroubalik commented Mar 26, 2020 • edited Loading

holyketzer commented Mar 27, 2020

holyketzer commented Mar 28, 2020

tomkerkhove left a comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Apr 8, 2020

ahmelsayed left a comment

Choose a reason for hiding this comment

ahmelsayed commented Apr 8, 2020

holyketzer commented Apr 8, 2020

holyketzer commented Mar 25, 2020 •

edited

Loading

zroubalik commented Mar 26, 2020 •

edited

Loading