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

Improve error logging by skipping logging of errors related to paused scalers #4254

Closed
wants to merge 2 commits into from

Conversation

aharonh
Copy link

@aharonh aharonh commented Feb 20, 2023

A small addition to the Reconciliation loop to allow for skipping the reconciliation of the paused scaled objects. This addition is beneficial as without it, as it is now, many log messages are caused by a paused scaledobject whose scaler's running pre-conditions are not satisfied. For an example a database mysql scaler depends on may be down while the scaledobject is paused. There is no value in those logs so better to remove them to keep the logs useful, clean of noise, and efficient and easy to use.

Checklist

Fixes #

Relates to #

@aharonh aharonh requested a review from a team as a code owner February 20, 2023 15:06
@aharonh aharonh changed the title fix https://github.com/kedacore/keda/issues/4253 fix "The paused-replicas annotation not fully respected" https://github.com/kedacore/keda/issues/4253 Feb 21, 2023
@aharonh aharonh changed the title fix "The paused-replicas annotation not fully respected" https://github.com/kedacore/keda/issues/4253 fix "The paused-replicas annotation not fully respected" #4253 Feb 21, 2023
@aharonh aharonh changed the title fix "The paused-replicas annotation not fully respected" #4253 fix "The paused-replicas annotation not fully respected" Feb 21, 2023
@aharonh aharonh changed the title fix "The paused-replicas annotation not fully respected" Improve error logging by skipping logging of errors related to paused scalers Feb 21, 2023
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.

Hi, did you actually test this change? I don't think it will help with your issue, because majority of the scaling logic is being driven by scaler handler: https://github.com/kedacore/keda/blob/main/pkg/scaling/scale_handler.go

I think that we should have a different approach.

@aharonh
Copy link
Author

aharonh commented Feb 21, 2023

Hi, did you actually test this change? I don't think it will help with your issue, because majority of the scaling logic is being driven by scaler handler: https://github.com/kedacore/keda/blob/main/pkg/scaling/scale_handler.go

I think that we should have a different approach.

Hi @zroubalik ,

I just re-did the changes with the gpg signature that was missing. Yep, I tested it, looked like it works as expected:

  • There were no errors when the ScaledObject was paused
  • When unpaused, it worked as expected

Can you think of any further tests that would be useful to perform?

Thanks,
Aharon

Signed-off-by: aharonh <aharon.haravon@gmail.com>
Signed-off-by: aharonh <aharon.haravon@gmail.com>
@zroubalik
Copy link
Member

Hi, did you actually test this change? I don't think it will help with your issue, because majority of the scaling logic is being driven by scaler handler: https://github.com/kedacore/keda/blob/main/pkg/scaling/scale_handler.go
I think that we should have a different approach.

Hi @zroubalik ,

I just re-did the changes with the gpg signature that was missing. Yep, I tested it, looked like it works as expected:

* There were no errors when the ScaledObject was paused

* When unpaused, it worked as expected

Can you think of any further tests that would be useful to perform?

Thanks, Aharon

Thanks!

Let's keep the conversation in the issue.

@zroubalik
Copy link
Member

This PR fixes the problem, instead of skipping the loggin it stops the whole scaler. So closing this in favour of that PR.

@zroubalik zroubalik closed this Jul 7, 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.

2 participants