fix: skip redundant lastActiveTime updates to prevent cascading cooldown reset#7614
fix: skip redundant lastActiveTime updates to prevent cascading cooldown reset#7614Fedosin wants to merge 1 commit intokedacore:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
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. |
![semgrep-code-kedacore[bot]](https://avatars.githubusercontent.com/u/49917779?s=60&v=4){kind=small}
| client.EXPECT().Status().Return(statusWriter).Times(3) | ||
| statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) | ||
|
|
||
| scaleExecutor.RequestScale(context.TODO(), &scaledObject, true, false, &ScaleExecutorOptions{}) |
There was a problem hiding this comment.
Consider to use well-defined context
🚀 Fixed in commit e388b98 🚀
|
Semgrep found 1 Consider to use well-defined context |
Fedosin
force-pushed
the
fix-lastactivetime-redundant-update
branch
2 times, most recently
from
38a4dee to
e388b98
Compare
wozniakjan
added
the
waiting-on-other-pr
|
is it ok if we wait for #7610 to merge first? I'll review the logic and guarantees promised by |
|
@wozniakjan sure thing! no rush 👍 |
|
Just for your information, the PR we were waiting for has merged. |
rickbrouwer
added
merge-conflict
Fedosin
force-pushed
the
fix-lastactivetime-redundant-update
branch
from
e388b98 to
4fe1d05
Compare
Every polling cycle where triggers are active, RequestScale unconditionally sets result.LastActiveTime to time.Now(). Since the timestamp changes every time, handleResult's DeepEqual check never short-circuits, producing a status PATCH on every cycle for every active ScaledObject — even when lastActiveTime only moved by a few seconds. Add a guard that skips the lastActiveTime write when the existing timestamp is still within the current polling interval. When nothing else changed, DeepEqual detects no diff and the PATCH is skipped entirely, reducing unnecessary API server load at scale. Note: the original race condition described in kedacore#7613 (where delayed PATCHes could reset lastActiveTime after triggers went inactive) was fixed by kedacore#7610, which serialized evaluations under a per-object mutex with a single atomic status PATCH. This guard is now a performance optimization rather than a correctness fix. Signed-off-by: Mikhail Fedosin <mfedosin@redhat.com>
Fedosin
force-pushed
the
fix-lastactivetime-redundant-update
branch
from
4fe1d05 to
7781c96
Compare
Fedosin
changed the title
Fedosin
changed the title
|
Closing this PR. The underlying race condition (cascading cooldown reset caused by multiple inline status PATCHes under API throttling) was fixed by #7610, which refactored the executor to return a After retesting on the rebased branch, I can no longer reproduce the issue — evaluations are fully serialized and The guard proposed here would also introduce a minor correctness flaw: if the skip fires due to timing jitter and triggers go inactive before the next cycle, the cooldown could expire up to one polling interval early. |
3 participants
Summary
RequestScalethat skips redundantlastActiveTimewrites when the existing timestamp is still within the current polling intervalActive=Truestatus causesupdateLastActiveTimeto refresh the timestamp even though the trigger is already inactive, resetting the cooldown timer and delaying scale-to-zero indefinitelySee also: #7610
Fixes: #7613
Test plan
TestSkipRedundantLastActiveTimeUpdateverifies thatlastActiveTimeis not updated when it was already set within the polling interval (5s < 30s default)TestUpdateLastActiveTimeWhenExpiredverifies thatlastActiveTimeIS updated when enough time has elapsed (60s > 30s default)