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

Add Elasticsearch Scaler based on search template #2311

Merged
merged 25 commits into from
Nov 23, 2021

Conversation

orphaner
Copy link
Contributor

I am pleased to propose you the implementation of an Elasticsearch scaler based on a search template. Using a search template make things easy on the scaler configuration. The user must provide a search template name and eventually its params. The search result can be parsed using the gjson library like the metrics api scaler does.
For now, the authentication is based on username/password ; on the future we could imagine provide a CA certificate to validate a self signed certificate or ECE credentials.

I had a great time working on this scaler and I would be very pleased to get your code review's feedback.

Checklist

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

Fixes #1756

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Hi,
another small nits:
Could you review the comment about the metric name and add atl least 1 test with ScalerIndex > 0?
Thanks! ❤️

pkg/scalers/elasticsearch_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

/run-e2e elasticsearch*

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
@orphaner
Copy link
Contributor Author

/run-e2e elasticsearch*

looks like the e2e test is KO; how do you get access to the logs?

@orphaner orphaner requested a review from JorTurFer November 22, 2021 10:02
@JorTurFer
Copy link
Member

/run-e2e elasticsearch*

looks like the e2e test is KO; how do you get access to the logs?

https://github.com/kedacore/keda/runs/4284418478?check_suite_focus=true#step:7:1

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
@orphaner
Copy link
Contributor Author

/run-e2e elasticsearch*

looks like the e2e test is KO; how do you get access to the logs?

https://github.com/kedacore/keda/runs/4284418478?check_suite_focus=true#step:7:1

https://github.com/kedacore/keda/runs/4284418478?check_suite_focus=true#step:7:94
it timeouts again. I've increased the timeout to 600s

@JorTurFer
Copy link
Member

/run-e2e elasticsearch*

@JorTurFer
Copy link
Member

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
@orphaner
Copy link
Contributor Author

I've add a kubectl describe command to help me understand why does the sts does not start

@orphaner
Copy link
Contributor Author

write permissions are required to run /run-e2e elasticsearch*

ok ; could you run it for me please?

@JorTurFer
Copy link
Member

/run-e2e elasticsearch*

@JorTurFer
Copy link
Member

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
@orphaner
Copy link
Contributor Author

hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff.
Can you relaunch e2e tests please?
Thanks :)

@JorTurFer
Copy link
Member

hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff. Can you relaunch e2e tests please? Thanks :)

Sure, all the times that you need :)

@JorTurFer
Copy link
Member

JorTurFer commented Nov 23, 2021

/run-e2e elasticsearch*
Update: You can check the progres here

@orphaner
Copy link
Contributor Author

hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff. Can you relaunch e2e tests please? Thanks :)

Sure, all the times that you need :)

many thanks!
BTW, now I've figured out how to get access to the e2e job logs ;)

@JorTurFer
Copy link
Member

JorTurFer commented Nov 23, 2021

hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff. Can you relaunch e2e tests please? Thanks :)

Sure, all the times that you need :)

many thanks! BTW, now I've figured out how to get access to the e2e job logs ;)

Nice!! but it's not needed xD. Today we have merged a PR which edit the triggering comment with the execution url automatically:
image

it's the GitHub Action by itself who edit the comment with its own execution url :)
On-demand e2e test is a recently added feature, and we still be working on improving it xD

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
@JorTurFer
Copy link
Member

JorTurFer commented Nov 23, 2021

/run-e2e elasticsearch*
Update: You can check the progres here

@orphaner
Copy link
Contributor Author

hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff. Can you relaunch e2e tests please? Thanks :)

Sure, all the times that you need :)

many thanks! BTW, now I've figured out how to get access to the e2e job logs ;)

Nice!! but it's not needed xD. Today we have merged a PR which edit the triggering comment with the execution url automatically: image

it's the GitHub Action by itself who edit the comment with its own execution url :) On-demand e2e test is a recently added feature, and we still be working on improving it xD

this is a nice improvement 👍

I've understood the problem. Elasticsearch use memory mapping - mmap. The sysctl vm.max_map_count parameter needs to be set to 262144 which is not the case on the github runner.
https://github.com/kedacore/keda/runs/4296384285?check_suite_focus=true#step:8:318

Can you run the test again? I've disabled mmap on the elasticsearch sts.

@JorTurFer
Copy link
Member

the execution is in proges: #2311 (comment)
:)

@orphaner
Copy link
Contributor Author

/run-e2e elasticsearch* Update: You can check the progres here

good new, the e2e tests are OK: https://github.com/kedacore/keda/runs/4296798019?check_suite_focus=true#step:8:414 👍
let me cleanup the debug outputs

orphaner and others added 2 commits November 23, 2021 10:00
@JorTurFer
Copy link
Member

JorTurFer commented Nov 23, 2021

/run-e2e elasticsearch*
Update: You can check the progres here

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
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 a very minor nits

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/elasticsearch_scaler.go Outdated Show resolved Hide resolved
@zroubalik zroubalik changed the title Add an elasticsearch scaler based on search template Add Elasticsearch Scaler based on search template Nov 23, 2021
@JorTurFer
Copy link
Member

JorTurFer commented Nov 23, 2021

/run-e2e elasticsearch*
Update: You can check the progres here

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 for the contribution!

@zroubalik zroubalik merged commit 81e7d3b into kedacore:main Nov 23, 2021
@zroubalik zroubalik added this to the v2.5.0 milestone Nov 23, 2021
@orphaner
Copy link
Contributor Author

thanks for the merge 👍

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.

Provide scaler for Elasticsearch
3 participants