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.

Sign up for GitHub

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

Jump to bottom

Add support for initialCooldownPeriod On httpScaledObjects #1212

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add support for initialCooldownPeriod On httpScaledObjects #1212

wants to merge 5 commits into from
+15 −4

Conversation

shayrybak
Copy link

@shayrybak shayrybak commented Dec 11, 2024 •
edited
Loading

Add support for initialCooldownPeriod On httpScaledObjects

Checklist

Fixes #1213

@shayrybak shayrybak requested a review from a team as a code owner December 11, 2024 11:05
@shayrybak shayrybak mentioned this pull request Dec 11, 2024
Open
wozniakjan
wozniakjan reviewed Dec 13, 2024
View reviewed changes
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you! Can you please also re-generate the CRDs?

make manifests

@shayrybak
Copy link
Author

@wozniakjan added, I always forget that part.

@wozniakjan
Copy link
Member

perfect, thank you. One more thing, can you please sign the last commit?

@zroubalik
Copy link
Member

Should we really do this? We could easily end up in 1-1 relation with ScaledObject's field, I wonder whether we should users encourage to specify this on the SO directly 🤔

@shayrybak
update changelog
b0c5349 
Signed-off-by: Shay Rybak <[email protected]>
@shayrybak
make manifests
1e93b08 
Signed-off-by: Shay Rybak <[email protected]>
@wozniakjan
Copy link
Member

Should we really do this?

given this can affect scaling right after SO creation, before metrics had a chance to get to KEDA and can lead to an immediate scale to 0 on an active deployment that was just onboarded to autoscaling, I think there is a stronger case for having initialCooldownPeriod in HSO than cooldownPeriod which is already there. The cooldownPeriod could be easily changed after HSO creation directly in SO without any negative effects.

@shayrybak
Copy link
Author

updated.`
@zroubalik this is what we've done temporarily to work around this, but it seems like a lot of effort for such a small change. I understand the use case to have my own SO if I mix other scaler tiriggers, but for a simple use case it's nice to have this.

Like I said in the issue, the use case is starting up sandbox environments so it's nice to have them up before they automatically scale down based on usage.

@zroubalik
Copy link
Member

Makes sense, no objections from my side.

zroubalik
zroubalik reviewed Dec 13, 2024
View reviewed changes
operator/apis/http/v1alpha1/httpscaledobject_types.go Outdated
@@ -102,6 +102,9 @@ type HTTPScaledObjectSpec struct {
// (optional) Cooldown period value
// +optional
CooldownPeriod *int32 `json:"scaledownPeriod,omitempty" description:"Cooldown period (seconds) for resources to scale down (Default 300)"`
// (optional) Initial period before scaling
// +optional
InitialCooldownPeriod int32 `json:"initialCooldownPeriod,omitempty" description:"Initial period (seconds) before scaling (Default 0)"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
InitialCooldownPeriod int32 `json:"initialCooldownPeriod,omitempty" description:"Initial period (seconds) before scaling (Default 0)"`
InitialCooldownPeriod *int32 `json:"initialCooldownPeriod,omitempty" description:"Initial period (seconds) before scaling (Default 0)"`

I think this is better

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this, however the scaledObject parameter is not a pointer, which means we'll have to add a nil check.
let me know if you think it's worth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that was an oversight on the upstream KEDA. Let's use pointers here, I will see if I can safely change the upstream as well.

Copy link
Member

@wozniakjan wozniakjan Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that seems to be correct. In KEDA the CooldownPeriod is a pointer while InitialCooldownPeriod is not a pointer. I think it's because default for CooldownPeriod is 300 and user setting 0 must be distinguishable from user not setting anything and expecting KEDA to set the default. For InitialCooldownPeriod, the default is 0 so setting nil would be semantically equivalent, hence no need for a pointer.

I'd actually vote for keeping the API consistent with KEDA, CooldownPeriod as a pointer and InitialCooldownPeriod not a pointer. But I'm easy to convince otherwise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine either way, just let me know what to do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking here: kedacore/keda#6423

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zroubalik I see the upstream was merged. should I update this one accordingly? do we want to set default values here or just pass whatever we get to the scaled object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please also use pointer here and I think that we should keep the defaults to the core, CC @wozniakjan

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we will probably have to wait for kedacore to cut a release version we can reference with the new type. unless you want to point to a master commit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code for now.

@zero101010
Copy link

Should we really do this? We could easily end up in 1-1 relation with ScaledObject's field, I wonder whether we should users encourage to specify this on the SO directly 🤔

Yes. it's really useful. Yesterday I need to do a workaround to use this. I'm really excited to have this on keda.

@zroubalik
Copy link
Member

Yesterday I need to do a workaround to use this

Just out of curiosity, what workaround that was?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wozniakjan wozniakjan wozniakjan left review comments

@zroubalik zroubalik zroubalik left review comments

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for InitialCooldownPeriod
5 participants
@shayrybak @wozniakjan @zroubalik @zero101010 @JorTurFer