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

Fix scalers leaking #684

Merged
merged 2 commits into from
Mar 16, 2020
Merged

Conversation

holyketzer
Copy link
Contributor

@holyketzer holyketzer commented Mar 16, 2020

We are using Keda with RabbitMQ, after 30 days running of keda-operator pod we have more than 1000 open connections in RabbitMQ console. It wasn't our app or something else, we checked it by restarting step by step. Connections were dropped only after keda-operator restart.

Fixes:

In case of error GetDeploymentScalers and getJobScalers returns acquired scalers, but callers of these methods doesn't use them or close them. So it doesn't make sense to return scalers at all in case of error, and it should close all already opened scalers to avoid leaking.

@msftclas
Copy link

msftclas commented Mar 16, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ahmelsayed ahmelsayed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks @holyketzer for the fix! There is one minor comment about fixing the existing error message as well that would be good to fix in this change as well.
Other than that, the change LGTM

Signed-off-by: Alex Emelyanov <holyketzer@gmail.com>
Signed-off-by: Alex Emelyanov <holyketzer@gmail.com>
@holyketzer
Copy link
Contributor Author

I renamed scalers to scalersRes because there was a name clash between variable scalers which overwrites namespace (or how it's called in Go) scalers and I wasn't able to use scalers.Scaler struct inside function with error:

pkg/handler/scale_handler.go:257:20: scalers.Scaler undefined 
(type []scalers.Scaler has no field or method Scaler)

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

@ahmelsayed ahmelsayed merged commit b919a4d into kedacore:master Mar 16, 2020
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants