Add support for initialCooldownPeriod On httpScaledObjects

[shayrybak]

Add support for initialCooldownPeriod On httpScaledObjects

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Changelog has been updated and is aligned with our changelog requirements
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo

Fixes #1213

[wozniakjan]

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

make manifests

[shayrybak]

@wozniakjan added, I always forget that part.

[wozniakjan]

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

[zroubalik]

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 🤔

[wozniakjan]

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]

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]

Makes sense, no objections from my side.

[zroubalik]

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

[shayrybak]

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.

[zroubalik]

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.

[wozniakjan]

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.

[shayrybak]

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

[zroubalik]

Tracking here: kedacore/keda#6423

[shayrybak]

@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?

[zroubalik]

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

[shayrybak]

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.

[shayrybak]

I've updated the code for now.

[zero101010]

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]

Yesterday I need to do a workaround to use this

Just out of curiosity, what workaround that was?