Repository navigation
General: Throttle LastActiveTime status writes on active ScaledObjects - #8260
Sarthak-Pandey wants to merge 2 commits into
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
Sarthak-Pandey
changed the title
855a858 to
79328e2
Compare
| result.LastActiveTime = &metav1.Time{Time: time.Now()} | ||
| if minReplicas <= 0 || scaledObject.Spec.IdleReplicaCount != nil { | ||
| cooldownPeriod := getCooldownPeriod(scaledObject) | ||
| if scaledObject.Status.LastActiveTime == nil || time.Since(scaledObject.Status.LastActiveTime.Time) >= cooldownPeriod/2 { |
There was a problem hiding this comment.
Throttling to cooldownPeriod/2 means status.lastActiveTime can lag real activity by up to half the cooldown, and scaleToZeroOrIdle measures cooldown from that field.
Example: cooldown=300s, refreshed at t=0, active until t=149 → scaled in at t=300, i.e. 151s after the last activity.
That silently halves the guarantee cooldownPeriod gives today. Could we keep the throttle but always stamp LastActiveTime on the active→inactive transition (first inactive poll while the Active condition is still True), and have scaleToZeroOrIdle use that fresh value? That keeps the write reduction without changing cooldown semantics.
| default: | ||
| // triggers are active, but we didn't need to scale (replica count > 0) | ||
| result.LastActiveTime = &metav1.Time{Time: time.Now()} | ||
| if minReplicas <= 0 || scaledObject.Spec.IdleReplicaCount != nil { |
There was a problem hiding this comment.
minReplicas <= 0 || IdleReplicaCount != nil is the cooldown-relevance rule we already have in the webhook (scaledobject_webhook.go:383-387, "CooldownPeriod is only relevant when minReplicaCount = 0 or idleReplicaCount is set").
Could we extract it as ScaledObject.IsCooldownPeriodRelevant() next to IsPollingIntervalRelevant() (#8031) and use it in both places, so they can't drift? Note it's not the same as IsPollingIntervalRelevant(): with useCachedMetrics the loop still polls, but LastActiveTime is still unused.
One edge case: if the user later sets minReplicaCount: 0 while triggers are inactive, LastActiveTime is nil/stale and scaleToZeroOrIdle scales to zero immediately, bypassing cooldown. It's also a user-visible status field people alert on. Please handle that transition (e.g. stamp it when cooldown becomes relevant) or document it.
Skip LastActiveTime updates when minReplicaCount > 0 and idleReplicaCount is nil, throttle updates to cooldownPeriod / 2 for active scale-to-zero/idle workloads to avoid unnecessary status writes on every polling interval, and stamp LastActiveTime on active-to-inactive transition so cooldown begins properly. Centralize cooldown period relevance in ScaledObject.IsCooldownPeriodRelevant() across the executor and admission webhook. Signed-off-by: Sarthak Pandey <sarthak@email.com>
79328e2 to
3bdd941
Compare
![semgrep-code-kedacore[bot]](https://avatars.githubusercontent.com/u/49917779?s=60&v=4){kind=small}
| mockDeploymentGet(mockClient) | ||
| mockHealthyHPA(mockClient) | ||
|
|
||
| result := exec.RequestScale(context.TODO(), &so, false, false, ScaleExecutorOptions{}) |
There was a problem hiding this comment.
🟡 Medium severity issue identified in your code:
Consider to use well-defined context
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by context-todo.
You can view more details about this finding in the Semgrep AppSec Platform.
| mockHealthyHPA(mockClient) | ||
|
|
||
| // isActive remains false | ||
| result := exec.RequestScale(context.TODO(), &so, false, false, ScaleExecutorOptions{}) |
There was a problem hiding this comment.
🟡 Medium severity issue identified in your code:
Consider to use well-defined context
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by context-todo.
You can view more details about this finding in the Semgrep AppSec Platform.
| mockHealthyHPA(mockClient) | ||
|
|
||
| // isActive transitions from true to false | ||
| result := exec.RequestScale(context.TODO(), &so, false, false, ScaleExecutorOptions{}) |
There was a problem hiding this comment.
🟡 Medium severity issue identified in your code:
Consider to use well-defined context
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by context-todo.
You can view more details about this finding in the Semgrep AppSec Platform.
|
Hi @JorTurFer Update on Issue #8256: Throttling
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: kedacore/keda
- Branch: main
- File: .coderabbit.yaml
- Commit: 783c65b
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
bdbd6b5f-f28b-4e55-b70d-d542c5bcc89e
-
apis/keda/v1alpha1/scaledobject_types.goapis/keda/v1alpha1/scaledobject_types_test.goapis/keda/v1alpha1/scaledobject_webhook.gopkg/scaling/executor/scale_scaledobjects.gopkg/scaling/executor/scale_scaledobjects_test.go
🔗 Linked repositories identified
-
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kedacore/keda-docs(manual)kedacore/charts(manual)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
-
There was a problem hiding this comment.
Keep this test out of the minimum-replica correction path.
mockDeploymentGet returns one replica, but this test sets MinReplicaCount to two. With inactive triggers and no idle count, RequestScale calls updateScaleOnScaleTarget. The test does not expect that call, so the executor test suite fails before the timestamp assertion. Set the minimum to one, or mock a target with two replicas.
Description
This PR fixes #8256 where
LastActiveTimewas unconditionally updated totime.Now()on every polling interval when a ScaledObject had active triggers and replicas > 0. Becauseequality.Semantic.DeepEqualdetected a changed timestamp on every loop, KEDA was issuing a statusPATCHto the Kubernetes API server on everypollingIntervalper active workload.Changes implemented:
Rule 1 — Skip
LastActiveTimewhen scale-to-zero/idle is impossible:minReplicaCount > 0andidleReplicaCount == nil, the workload cannot use KEDA's scale-to-zero or idle scaling path (HPA manages replica count between min and max).result.LastActiveTimeis left unset (nil), preventing any statusPATCHduring steady-state active polling.scaleFromZeroOrIdle()remain untouched.Rule 2 — Throttle
LastActiveTimefor scale-to-zero/idle workloads:minReplicas == 0oridleReplicaCount != nil),LastActiveTimeis required for cooldown decisions.LastActiveTime == nil(timestamp initialized for active workloads).time.Since(*LastActiveTime) >= cooldownPeriod / 2(refreshed periodically).Helper & Unit Tests:
getCooldownPeriod(scaledObject)to standardize cooldown duration resolution acrossRequestScale()andscaleToZeroOrIdle().pkg/scaling/executor/scale_scaledobjects_test.gocovering all conditions (Rule 1 skip, nil initialization, throttling younger thancooldownPeriod / 2, refreshing at/aftercooldownPeriod / 2, and idle replica count withminReplicaCount > 0).pkg/scaling/scale_handler_test.goverifying thathandleResult()skipsclient.Status().Patch()whenresult.LastActiveTimeis nil.Checklist
make generate-scalers-schemahas been run to update any outdated generated filesFixes #8256
Relates to #