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 InitialCooldownPeriod for ScaledObjects #5478

Merged
merged 13 commits into from
Apr 12, 2024
Merged

add InitialCooldownPeriod for ScaledObjects #5478

merged 13 commits into from
Apr 12, 2024

Conversation

xrwang8
Copy link
Contributor

@xrwang8 xrwang8 commented Feb 4, 2024

… of CooldownPeriod

·

Add a new field InitialDelayCooldownPeriod The purpose of adding a new field InitialDelayCooldownPeriod is to delay the effective time of the CooldownPeriod, so that when the service is started for the first time, it needs to wait for the time of InitialDelayCooldownPeriod to expire before destroying the service.

Checklist

Fixes #5008

Relates to #

@xrwang8 xrwang8 requested a review from a team as a code owner February 4, 2024 09:31
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.

Thanks for the contribution!
Looking good! I've kept some comments inline, but as global comment, could you add some coverage to the change? Maybe an e2e test or maybe unit test

pkg/scaling/executor/scale_executor.go Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
@shubham-kaushal
Copy link

Hi, @JorTurFer is this PR getting merged anytime soon? Our current workaround for handling the delay at the time of deployment is causing challenges. This feature will be very helpful for many of our use cases.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 26, 2024

Hi, @JorTurFer is this PR getting merged anytime soon? Our current workaround for handling the delay at the time of deployment is causing challenges. This feature will be very helpful for many of our use cases.

I hope so, but the feature isn't ready to merge yet because test coverage is required. IDK the state of it, @xrwang8 ?

@xrwang8
Copy link
Contributor Author

xrwang8 commented Feb 28, 2024

I've added e2e testing. @JorTurFer

@xrwang8
Copy link
Contributor Author

xrwang8 commented Mar 4, 2024

What's wrong with that now @JorTurFer

@shubham-kaushal
Copy link

Hi @JorTurFer , could you plz. check this PR. E2E test has been added by @xrwang8

@JorTurFer
Copy link
Member

JorTurFer commented Mar 12, 2024

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

@JorTurFer
Copy link
Member

Hi @JorTurFer , could you plz. check this PR. E2E test has been added by @xrwang8

Sorry, I've been quite disconnected :( I have left some comments inline. Apart from them ,we need to fix DCO check for merging the PR, you can find the steps to fix it in the check itself
image

@xrwang8 xrwang8 closed this Mar 13, 2024
@xrwang8 xrwang8 reopened this Mar 13, 2024
wangxingrui and others added 2 commits March 13, 2024 16:15
@xrwang8
Copy link
Contributor Author

xrwang8 commented Mar 13, 2024

I have simplified e2e @JorTurFer

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.

LGTM!
Thanks for the improvement!

@JorTurFer
Copy link
Member

JorTurFer commented Mar 15, 2024

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

@JorTurFer
Copy link
Member

Could you open a PR against docs repo too, adding this new parameter to the ScaledObject docs?

@xrwang8
Copy link
Contributor Author

xrwang8 commented Mar 17, 2024

Could you open a PR against docs repo too, adding this new parameter to the ScaledObject docs?

ok

@xrwang8
Copy link
Contributor Author

xrwang8 commented Mar 18, 2024

I have modified the document kedacore/keda-docs#1337 @JorTurFer

@JorTurFer
Copy link
Member

I have modified the document kedacore/keda-docs#1337 @JorTurFer

Let me check them, sorry for the delay... last week was KubeCon EU

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.

LGTM!
Could you add a record in changelog.md file too? I think that #New section is the best place for this feature

@xrwang8
Copy link
Contributor Author

xrwang8 commented Mar 26, 2024

LGTM! Could you add a record in changelog.md file too? I think that #New section is the best place for this feature

i hava already done @JorTurFer

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Jorge Turrado Ferrero <[email protected]>
@JorTurFer
Copy link
Member

JorTurFer commented Mar 26, 2024

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

@JorTurFer
Copy link
Member

@zroubalik PTAL

@xrwang8
Copy link
Contributor Author

xrwang8 commented Apr 2, 2024

What is the current status of this PR? @JorTurFer

@JorTurFer
Copy link
Member

What is the current status of this PR?

It's ready to merge IMHO, but I'd like to read @zroubalik thoughts too :)

@aj-ya
Copy link

aj-ya commented Apr 8, 2024

Hey! @zroubalik , could you please take a look at this. This will save a bit of development for us. Thank you.
cc: @JorTurFer

CHANGELOG.md Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Outdated Show resolved Hide resolved
xrwang8 and others added 2 commits April 11, 2024 09:00
@xrwang8
Copy link
Contributor Author

xrwang8 commented Apr 11, 2024

@zroubalik done

@xrwang8 xrwang8 changed the title add new a filed InitialDelayCooldownPeriod delay the effective time… add new a filed InitialCooldownPeriod delay the effective time… Apr 11, 2024
@zroubalik
Copy link
Member

zroubalik commented Apr 11, 2024

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

@xrwang8
Copy link
Contributor Author

xrwang8 commented Apr 12, 2024

@JorTurFer can it be merged ?

@zroubalik zroubalik changed the title add new a filed InitialCooldownPeriod delay the effective time… add InitialCooldownPeriod for ScaledObjects Apr 12, 2024
@zroubalik zroubalik merged commit 80806a7 into kedacore:main Apr 12, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants