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: stop scale loop for pause Scaledbject #4470

Closed
wants to merge 0 commits into from

Conversation

tobotg
Copy link
Contributor

@tobotg tobotg commented Apr 24, 2023

In the controller in the reconcile loop, when a scaledObject is paused (annotated with ), this fix stops the scaling logic (to prevent attempt to connect the to scalers), and deletes the generated HPA (so it is not querying KEDA Metric Server for metrics)

Checklist

Fixes #4253

@tobotg tobotg requested a review from a team as a code owner April 24, 2023 16:00
@JorTurFer
Copy link
Member

The new test is failing, could you take a look?

@tobotg
Copy link
Contributor Author

tobotg commented Apr 24, 2023

@JorTurFer sure thing. I'll take a look.

@tomkerkhove tomkerkhove changed the title chore: stop scale loop for pause Scaledbject #4253 fix: stop scale loop for pause Scaledbject Apr 25, 2023
@JorTurFer
Copy link
Member

JorTurFer commented Apr 25, 2023

/run-e2e internals
Update: You can check the progress here

@tobotg
Copy link
Contributor Author

tobotg commented Apr 27, 2023

@JorTurFer is there anything expected from me wrt the FOSS / build failures? Just checking.

@zroubalik
Copy link
Member

@JorTurFer is there anything expected from me wrt the FOSS / build failures? Just checking.

no, ignore this for now

@zroubalik
Copy link
Member

zroubalik commented Apr 27, 2023

/run-e2e pause
Update: You can check the progress here

@zroubalik
Copy link
Member

@tobotg
Copy link
Contributor Author

tobotg commented Apr 28, 2023

@tobotg seems like the e2e test is failing, could you please double check?

https://github.com/kedacore/keda/blob/main/tests/internals/pause_scaling/pause_scaling_test.go

sure thing. I'll have a look during the weekend.

@tobotg
Copy link
Contributor Author

tobotg commented Apr 29, 2023

@zroubalik I tried to have a look, but can't find the detail of the test run that failed. Would you be able to help? Thanks

@JorTurFer
Copy link
Member

You can see the execution log, clicking the link in the comment:
image
but in this case, the error is here: https://github.com/kedacore/keda/actions/runs/4819967110/jobs/8584143929#step:8:378
The scaling out test is failing

@tobotg
Copy link
Contributor Author

tobotg commented May 2, 2023

@JorTurFer @zroubalik Thank you.
I looked and tested, and found that the tests fail consistently with a wait time of 1 min for WaitForDeploymentReplicaReadyCount, but succeed when I increase that wait time to 2 or 3 min. The behaviour seems fragile when the load (all the e2e tests launched) increases.
Would you agree to increase the wait time periods in the pause_scaling_test.go to fix this test?

@JorTurFer
Copy link
Member

Would you agree to increase the wait time periods in the pause_scaling_test.go to fix this test?

Yes, go ahead.
Other tests have different timeouts, depending on the test internals. I guess that after the changes, the test needs more time to recover the scaling loop. 2-3 mins is something feasible

@JorTurFer
Copy link
Member

Could you rebase main too? There are changes required for executing e2e tests

@tobotg
Copy link
Contributor Author

tobotg commented May 3, 2023

@JorTurFer it should be fine now I guess. But this test is failing now. Do you mind triggering the e2e to check what when wrong? Thanks

@zroubalik
Copy link
Member

zroubalik commented May 3, 2023

/run-e2e
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented May 7, 2023

/run-e2e
Update: You can check the progress here

@zroubalik
Copy link
Member

zroubalik commented May 17, 2023

/run-e2e pause_scaling
Update: You can check the progress here

@zroubalik
Copy link
Member

zroubalik commented May 18, 2023

/run-e2e
Update: You can check the progress 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.

@tobotg could you please make sure that the PR contain only your commits?

@tobotg
Copy link
Contributor Author

tobotg commented May 18, 2023

I ended up closing this and resubmitting a new PR.

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.

The paused-replicas annotation not fully respected
4 participants