Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RabbitMQ] vHost is ignored when using regex #2498

Closed
crisp2u opened this issue Jan 21, 2022 · 11 comments · Fixed by #2591
Closed

[RabbitMQ] vHost is ignored when using regex #2498

crisp2u opened this issue Jan 21, 2022 · 11 comments · Fixed by #2591
Labels
bug Something isn't working

Comments

@crisp2u
Copy link

crisp2u commented Jan 21, 2022

Report

We have a shared RabbitMQ instance with multiple vhosts and multiple deployments connecting to them and we get "Error":"error inspecting rabbitMQ: regex matches more queues than can be recovered at once"}
We use regex to scale our workers and looking at the code it seems that if useRegex is turned on the http call is not made using the vhost.

Relevant code is here

getQueueInfoManagementURI = fmt.Sprintf("%s/api/queues?page=1&use_regex=true&pagination=false&name=%s&page_size=%d", parsedURL.String(), url.QueryEscape(s.metadata.queueName), s.metadata.pageSize)

vs
getQueueInfoManagementURI = fmt.Sprintf("%s/api/queues%s/%s", parsedURL.String(), vhost, url.QueryEscape(s.metadata.queueName))

Because we run multiple instances of the same app the regex matches all the queues from all the vhost and the result exceeds the default pageSize of 100

The RabbitMQ Console is running the same regex filter using /api/queues/<<vHost>>?

Any workarounds for this ? I'm thinking increasing the pageSize but that will trigger all workers regarding of vhost. The other option would be to move to a dedicated instance.

Expected Behavior

Deployments should scale when using regex with multiple queues on a certain vhost

Actual Behavior

Deployments cannot scale because Scaler ignores vhost and the result match exceeds pageSize

Steps to Reproduce the Problem

See description

Logs from KEDA operator

{"level":"debug","ts":1642770893.2876174,"logger":"scalehandler","msg":"Error getting scale decision","Error":"error inspecting rabbitMQ: regex matches more queues than can be recovered at once"}

KEDA Version

2.5.0

Kubernetes Version

1.21

Platform

Amazon Web Services

Scaler Details

RabbitMQ

Anything else?

No response

@crisp2u crisp2u added the bug Something isn't working label Jan 21, 2022
@crisp2u
Copy link
Author

crisp2u commented Jan 31, 2022

Can anyone give any feedback on this ? @JorTurFer ? This is blocking us and we only have the option of dropping Keda or forking it :(
I think we can also provide a PR for this but a feedback on a possible solution would be most welcomed. Also as far as I see Keda does not have bug fix releases...

@JorTurFer
Copy link
Member

Hi,
I have to apologize because I didn't see this...
I'm AFK now, in 20-30 min I'll review this

@JorTurFer
Copy link
Member

Hi,
I have review your issue, You could increase the pageSize in trigger.metadata section up to your server max pageSize.
Related with doing the queries using regex and vhots, IDK, I have to test it. I'll do during the day

@crisp2u
Copy link
Author

crisp2u commented Jan 31, 2022

Already did that to get rid of the error. The problem is that I want to deploy another client on the same MQ cluster and the scaler will pick up the wrong queues without the vhost.

@JorTurFer
Copy link
Member

That depends on the rabbit management plugin itself.
Did you try if it's possible using vhost? (just to save some time)

@crisp2u
Copy link
Author

crisp2u commented Jan 31, 2022

yes. I've mentioned in the issue. The easiest way to check is if you go to the RabbitMQ Management UI and filter queues using regex on a vhost. It adds /api/queues/<<vHost>> to the request

@JorTurFer
Copy link
Member

JorTurFer commented Jan 31, 2022

I think that supporting this new feature if we maintain the current behavior also is worth. Maybe something like: if useRegex and vhost, then /api/queues/<<vHost>> and if not, current behavior.
You said that you are open to open a PR, right? 😄

Photonios added a commit to SectorLabs/keda that referenced this issue Feb 2, 2022
The vhost is included when you don't use `useRegex: true`. It makes
sense to have consistent behaviour between the two modes.

Fixes kedacore#2498
Photonios added a commit to SectorLabs/keda that referenced this issue Feb 2, 2022
The vhost is included when you don't use `useRegex: true`. It makes
sense to have consistent behaviour between the two modes.

Fixes kedacore#2498

Signed-off-by: Swen Kooij <swen@sectorlabs.ro>
Photonios added a commit to SectorLabs/keda that referenced this issue Feb 2, 2022
The vhost is included when you don't use `useRegex: true`. It makes
sense to have consistent behaviour between the two modes.

Fixes kedacore#2498

Signed-off-by: Swen Kooij <swen@sectorlabs.ro>
@crisp2u
Copy link
Author

crisp2u commented Feb 4, 2022

@JorTurFer PR ready, sorry for being pushy but we're close to our release data

@JorTurFer
Copy link
Member

@JorTurFer PR ready, sorry for being pushy but we're close to our release data

Don't worry, but just for sharing information, we released v2.6.0 last week and according to our governance repo, next release will be in 3 months. The milestone v2.7 ends on May.
Probably we will do another release with bugfixes, but we don't plan to include new features on.

I'm explaining this because you said that you are close to your release date and maybe you expect that this feature will be released ASAP. Once it's merged, it will be available in main tag, but main is not stable and we don't recommend it in prod

@crisp2u
Copy link
Author

crisp2u commented Feb 4, 2022

Yeah, I missed the release, I just saw it today. I'm also aware you are not doing bug fix releases, figured that out from the change log. We will either run from our fork or take the risk of running from keda master. Thanks for you help and I appreciate what you guys are doing.

@JorTurFer
Copy link
Member

one tip, if you use KEDA from main tag, pull it and then push it into other registry and use it from that other registry. Doing that, you can "freeze" the version till next release

valentindeaconu pushed a commit to SectorLabs/keda that referenced this issue Mar 3, 2022
The vhost is included when you don't use `useRegex: true`. It makes
sense to have consistent behaviour between the two modes.

Fixes kedacore#2498

Signed-off-by: Swen Kooij <swen@sectorlabs.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants