-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Elasticsearch Scaler based on search template #2311
Conversation
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>
There was a problem hiding this 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! ❤️
/run-e2e elasticsearch* |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
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>
https://github.com/kedacore/keda/runs/4284418478?check_suite_focus=true#step:7:94 |
/run-e2e elasticsearch* |
Same error I think: |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
I've add a |
ok ; could you run it for me please? |
/run-e2e elasticsearch* |
@orphaner , these are the logs: |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
hello @JorTurFer I really need to understand what's going on. I add more kubectl command to debug that stuff. |
Sure, all the times that you need :) |
/run-e2e elasticsearch* |
many thanks! |
Nice!! but it's not needed xD. Today we have merged a PR which edit the triggering comment with the execution url automatically: it's the GitHub Action by itself who edit the comment with its own execution url :) |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
/run-e2e elasticsearch* |
this is a nice improvement 👍 I've understood the problem. Elasticsearch use memory mapping - mmap. The sysctl Can you run the test again? I've disabled mmap on the elasticsearch sts. |
the execution is in proges: #2311 (comment) |
good new, the e2e tests are OK: https://github.com/kedacore/keda/runs/4296798019?check_suite_focus=true#step:8:414 👍 |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
/run-e2e elasticsearch* |
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
There was a problem hiding this 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
Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>
/run-e2e elasticsearch* |
There was a problem hiding this 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!
thanks for the merge 👍 |
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
Fixes #1756