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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak by checking triggers uniqueness properly #1640

Merged
merged 11 commits into from
Mar 1, 2021
Merged

Fix memory leak by checking triggers uniqueness properly #1640

merged 11 commits into from
Mar 1, 2021

Conversation

ycabrer
Copy link
Contributor

@ycabrer ycabrer commented Mar 1, 2021 •
edited by zroubalik
Loading

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • [N/A] Tests have been added
  • [ N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [N/A ] A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #

Potential fix for #1636

Scalers were not being closed properly after use.

ycabrer added 4 commits March 1, 2021 03:00
@ycabrer
add metricName to postgres scaler
c45f41d 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
fix typo in postgres scaler test
69c0db3 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
close scalers after name check
f3ac4aa 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
Update Changelog
fa243e2 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
limit metricName validation to new or changed scaledObjects
009221d 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
zroubalik
zroubalik previously approved these changes Mar 1, 2021
View reviewed changes
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.

LGTM, thanks!

controllers/scaledobject_controller.go Outdated Show resolved Hide resolved
@zroubalik zroubalik dismissed their stale review March 1, 2021 11:03

Might be better to move the uniqueness check

ycabrer added 2 commits March 1, 2021 07:28
@ycabrer
remove validateMetricNameUniqueness
6914f97 
similiar check moved to hpa getScaledObjectMetricSpecs

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
add uniqueness check to getScaledObjectMetricSpecs
eb315d4 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
zroubalik
zroubalik reviewed Mar 1, 2021
View reviewed changes
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.

Looking good, just minor typo in err message

controllers/hpa.go Outdated
if metricSpec.External != nil {
externalMetricName := metricSpec.External.Metric.Name
if util.Contains(externalMetricNames, externalMetricName) {
return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metircName manually", externalMetricName, scaledObject.Name)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metircName manually", externalMetricName, scaledObject.Name)
return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metricName manually", externalMetricName, scaledObject.Name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thank you!

zroubalik
zroubalik reviewed Mar 1, 2021
View reviewed changes
controllers/hpa.go Outdated
@@ -15,6 +15,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

kedav1alpha1 "github.com/kedacore/keda/v2/api/v1alpha1"
"github.com/kedacore/keda/v2/controllers/util"
Copy link
Member

Choose a reason for hiding this comment

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

The same is imported on the line below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is awkward. Fixed! :D

ycabrer added 2 commits March 1, 2021 07:43
@ycabrer
fix typo
d2ec27a 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
remove redundent import
aac7880 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer
Copy link
Contributor Author

ycabrer commented Mar 1, 2021 •
edited
Loading

I'm running pre-commit now locally. It looks like a formating change needs to be done

@ycabrer
formatting change
b819c5e 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@zroubalik zroubalik changed the title Fix/scaler leak Fix memory leak by checking triggers uniqueness properly Mar 1, 2021
zroubalik
zroubalik reviewed Mar 1, 2021
View reviewed changes
CHANGELOG.md Outdated
@@ -31,6 +31,7 @@
- Fix a memory leak in kafka client and close push scalers ([#1565](https://github.com/kedacore/keda/issues/1565))
- Add 'Metadata' header to AAD podIdentity request ([#1566](https://github.com/kedacore/keda/issues/1566))
- KEDA should make sure generate correct labels for HPA ([#1630](https://github.com/kedacore/keda/issues/1630))
- Close scalers after name check; fix memory leak ([#1636](https://github.com/kedacore/keda/issues/1636))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Close scalers after name check; fix memory leak ([#1636](https://github.com/kedacore/keda/issues/1636))
- Fix memory leak by checking triggers uniqueness properly ([#1640](https://github.com/kedacore/keda/pull/1640))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed! Good suggestion

@ycabrer
update changelog
265b737 
Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>
@ycabrer ycabrer requested a review from zroubalik March 1, 2021 15:10
@ycabrer
Copy link
Contributor Author

ycabrer commented Mar 1, 2021

@zroubalik How do I retrigger a check? It looks like the FOSSA check timed out.

@zroubalik
Copy link
Member

@zroubalik How do I retrigger a check? It looks like the FOSSA check timed out.

@ycabrer I did that :)

zroubalik
zroubalik approved these changes Mar 1, 2021
View reviewed changes
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.

LGTM, thanks!

@zroubalik zroubalik merged commit 5a54908 into kedacore:main Mar 1, 2021
@ycabrer ycabrer deleted the fix/scaler-leak branch March 2, 2021 04:07
@zroubalik zroubalik mentioned this pull request Mar 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ycabrer @zroubalik