Repository navigation
resource_group client: prevent NaN in calcAvg when elapsed duration is zero - #11023
Conversation
…s zero A freshly created groupCostController can be published between the updateRunState and updateAvgRequestResourcePerSec passes of the same tick, in which case gc.run.now still equals the avgLastTime set by initRunState and calcAvg computes (0-0)/0 = NaN, permanently poisoning avgRUPerSec and making every subsequent token-bucket request carry required-token=NaN until the idle cleanup deletes the controller. Skip the sample when the elapsed duration is non-positive, and update the burstable flag before sampling so a skipped sample does not delay it. Add a regression test covering the exact scenario. Signed-off-by: JmPotato <github@ipotato.me>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe resource group controller now records burstability before average calculation and skips RU-per-second sampling when runtime time has not advanced. A regression test verifies no NaN is produced, accounting state remains unchanged, and sampling resumes after time advances. ChangesResource average sampling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@YuhaoZhang00: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11023 +/- ##
==========================================
+ Coverage 79.23% 79.25% +0.01%
==========================================
Files 541 541
Lines 76035 76037 +2
==========================================
+ Hits 60245 60261 +16
+ Misses 11545 11523 -22
- Partials 4245 4253 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, lhy1024, rleungx, YuhaoZhang00 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test pull-unit-test-next-gen-3 |
2 similar comments
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-3 |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: JmPotato <github@ipotato.me>
…s zero (#11023) (#11028) ref #9455, close #11022\n\nA freshly created `groupCostController` can be published to the controller map between the `updateRunState` and `updateAvgRequestResourcePerSec` passes of the same tick. In that case `gc.run.now` still equals the `avgLastTime` set by `initRunState` (the same `time.Now()` value), so `calcAvg` computes `(0-0)/0 = NaN`, permanently poisoning `avgRUPerSec` (the moving average keeps NaN forever) and making every subsequent token-bucket request carry `required-token=NaN` until the idle cleanup deletes the controller. Skip the sample when the elapsed duration is non-positive, and update the `burstable` flag before sampling so a skipped sample does not delay it. A regression test reproduces the exact scenario and verifies `avgRUPerSec` stays finite and sampling resumes once the run state advances.\n\nSigned-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>\nSigned-off-by: JmPotato <github@ipotato.me>\n\nCo-authored-by: JmPotato <ghzpotato@gmail.com>\nCo-authored-by: JmPotato <github@ipotato.me>
What problem does this PR solve?
Issue Number: Close #11022, ref #9455
What is changed and how does it work?
Check List
Tests
Related changes
Release note
Summary by CodeRabbit
Bug Fixes
Tests