Skip to content

add InitialCooldownPeriod for ScaledObjects #5478

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

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

@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
…f CooldownPeriod and e2e

Signed-off-by: wangxingrui <xrwang8@gamil.com>
Signed-off-by: wangxingrui <xrwang8@gamil.com>
@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 and others added 3 commits March 26, 2024 09:16
…eter to ScaledObject

Signed-off-by: wangxingrui <xrwang8@gamil.com>
Signed-off-by: wangxingrui <xrwang8@gamil.com>
@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

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@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

@JorTurFer JorTurFer requested a review from zroubalik March 26, 2024 19:30
@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

xrwang8 and others added 2 commits April 11, 2024 09:00
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: xrwang <68765051+xrwang8@users.noreply.github.com>
Signed-off-by: wangxingrui <xrwang8@gmail.com>
@xrwang8
Copy link
Contributor Author

xrwang8 commented Apr 11, 2024

@zroubalik done

Signed-off-by: xrwang <68765051+xrwang8@users.noreply.github.com>
@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
xrwang8 added 2 commits April 11, 2024 14:31
Signed-off-by: wangxingrui <xrwang8@gmail.com>
Signed-off-by: wangxingrui <xrwang8@gmail.com>
@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
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.

Respect to cooldownPeriod for the first deployment and let service is up and and running based on replica number for the first time.
5 participants