core, schedulers: fix balance-region churn on near-empty clusters - #11137
core, schedulers: fix balance-region churn on near-empty clusters#11137bufferflies wants to merge 3 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe balance-region scheduler now tracks non-empty region sizes separately from overall region sizes. It uses this data for tolerant resource calculations and target scoring. Tests update influence thresholds and verify that a small data-bearing region is not migrated between large mostly empty stores. ChangesRegion balance scoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/schedule/schedulers/balance_region_test.go`:
- Around line 121-151: Update the test fixture around AddLeaderRegion so region
1 exists only on store 1 and store 2 does not already host its peer; add one
additional empty region to store 2 to preserve ten regions per store. Keep
solver target selection and the shouldBalance assertion unchanged so the test
validates a schedulable target.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b1815c2-ed3a-4df9-be53-0059ac28fab7
📒 Files selected for processing (2)
pkg/schedule/schedulers/balance_region_test.gopkg/schedule/schedulers/utils.go
targetStoreScore() only reflected already-pending operators' influence, never the size of the region currently being evaluated for the move. On a cluster where most regions are near-empty (e.g. freshly pre-split, mostly unwritten), the average region size collapses, tolerantResource shrinks with it, and a target store can look artificially light right up until the move lands — letting balance-region pick a target that becomes overloaded the instant the pending region is counted, which triggers a follow-up move to shed it again. Add the candidate region's own approximate size to the target delta (unamplified, since it is not "other pending influence" but the exact size about to be received) so a target's projected post-move score is what actually decides whether it's picked. Recalibrate TestInfluenceAmp's boundary counts: the new term shifted the pre-existing count+size boundary this test pins down by exactly one region-size step. Signed-off-by: bufferflies <1045931706@qq.com>
0867c8d to
61a5934
Compare
ti-chi-bot
Bot
added
release-note
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11137 +/- ##
=======================================
Coverage 79.43% 79.43%
=======================================
Files 542 542
Lines 77117 77117
=======================================
+ Hits 61259 61260 +1
+ Misses 11571 11560 -11
- Partials 4287 4297 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // look artificially light just because this move hasn't landed yet. | ||
| // Unlike opInfluence (other, already-pending operators), this is not | ||
| // amplified: it is the literal size the target is about to receive. | ||
| targetDelta := influence*influenceAmp + tolerantResource + p.Region.GetApproximateSize() |
There was a problem hiding this comment.
In the added 10 MiB/6 TiB regression scenario, GetAverageRegionSize() truncates to zero; the populated source's v2 score is slightly above 10 while an empty target's projected score is exactly 10, so this comparison still schedules the peer. After it lands on any empty store, another empty target recreates the same state, allowing the peer to rotate indefinitely and leaving the reported churn unresolved.
There was a problem hiding this comment.
Fixed in 8532b76. getTolerantResource() now uses a new GetNonEmptyAverageRegionSize() (excludes empty regions from the average instead of GetAverageRegionSize()), so it no longer collapses to zero in this scenario. Re-verified the exact case you described: tolerantResource computes to the candidate region's own size instead of truncating to zero, and the target's projected score now exceeds the source's, so shouldBalance() returns false — the region stays put instead of rotating. TestSingleRegionOnLargeEmptyDiskDoesNotMigrate (renamed/inverted from the original test) asserts this directly.
…t margin getTolerantResource() derived its margin from GetAverageRegionSize(), which averages over every region in the cluster. On a cluster with many freshly-split, unwritten regions, that average collapses toward zero, so the tolerant margin stops damping marginal score differences between otherwise-equivalent stores. Add RegionsInfo.GetNonEmptyAverageRegionSize(), backed by a second pair of incrementally-maintained totals on regionTree (nonEmptyTotalSize / nonEmptyRegionsCnt) alongside the existing totalSize/length(), so it stays O(1). GetAverageRegionSize() itself is untouched; the new method is plumbed through the RegionSetInformer interface and rangeCluster's pass-through wrapper, and getTolerantResource() is switched to use it. Rework TestSingleRegionOnLargeEmptyDiskCanMigrate into TestSingleRegionOnLargeEmptyDiskDoesNotMigrate: with the tolerant margin no longer diluted, a single non-empty region isolated among many empty ones and stores correctly stays put — moving it would not fix a real imbalance and would just relocate the same "which store holds the only real data" state onto a different empty store, inviting the churn reported in tikv#11135. Also drops the region's phantom peer on the target store from the fixture, per review feedback on the prior version of this test. Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
size/L
| // look artificially light just because this move hasn't landed yet. | ||
| // Unlike opInfluence (other, already-pending operators), this is not | ||
| // amplified: it is the literal size the target is about to receive. | ||
| targetDelta := influence*influenceAmp + tolerantResource + p.Region.GetApproximateSize() |
There was a problem hiding this comment.
tolerantResource already contributes one average region to the target margin. Adding the candidate again rejects legitimate moves when the candidate equals the average: with three 96 MiB regions on the source and an empty target (v1, ratio=1), this head compares 192 vs 192 and schedules nothing, although moving one region leaves 192 vs 96. The base head schedules this move, so this introduces a balance-region regression for ordinary equal-sized regions.
There was a problem hiding this comment.
Confirmed and fixed in dafffd2. You're right that tolerantResource already represents about one region's worth of margin, so adding the candidate's size on top double-counted it. Switched to max(tolerantResource, p.Region.GetApproximateSize()) instead of summing them — this falls back to the candidate's real size only when it exceeds the average-based margin, matching the (previously unimplemented) intent in shouldBalance()'s own comment about max(regionSize, averageRegionSize). Re-ran your exact reproduction (three 96MiB regions on the source, empty target, v1, ratio=1): now scores 192 vs 96 and schedules the move, matching pre-PR/base behavior. Added TestBalanceRegionOrdinaryMoveNotBlockedByCandidateSize as a permanent regression test for this case, since no existing test previously covered balancing between several ordinary, similarly-sized regions.
…ntResource
targetStoreScore added the candidate region's own approximate size on
top of tolerantResource, but tolerantResource (averageRegionSize *
ratio) already represents roughly one region's worth of margin. In any
ordinary cluster where the candidate is close to the average region
size, this summed to about two regions' worth, silently doubling the
score gap required before balance-region would act and rejecting
legitimate moves between equal-sized regions (reported by rleungx:
three 96MiB regions on a source vs an empty target, 192 vs 96 after
the move, no longer scheduled).
Take the larger of the two instead of adding them. This matches the
long-standing but never-implemented intent documented in
shouldBalance()'s own comment ("we use max(regionSize,
averageRegionSize)"): fall back to the candidate's real size only when
it exceeds the average-based margin. Verified against both the
regression case above (now matches pre-PR behavior) and the
originally-motivating churn scenario in
TestSingleRegionOnLargeEmptyDiskDoesNotMigrate (still correctly stays
put). TestInfluenceAmp's boundary counts revert to their original,
pre-PR values, since max() is behaviorally identical to the base
formula whenever the candidate doesn't exceed tolerantResource.
Add TestBalanceRegionOrdinaryMoveNotBlockedByCandidateSize as a
permanent regression test for this, since no existing test previously
covered balancing between several ordinary, similarly-sized regions.
Signed-off-by: bufferflies <1045931706@qq.com>
bufferflies
changed the title
|
@bufferflies: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: Close #11135
balance-regioncould churn/thrash on a cluster with many near-empty regions (e.g. freshly pre-split tables, mostly unwritten) alongside a few regions that actually hold data — observed in production as repeated, low-value peer moves between otherwise-empty stores.Two contributing gaps:
targetStoreScore()'s delta never accounted for the specific region being evaluated for the move, so a target store's score didn't reflect what it was about to receive until after the move landed.getTolerantResource()'s margin is derived fromGetAverageRegionSize(), which averages over every region in the cluster. On a cluster full of empty regions, that average collapses toward zero, so the margin stops damping marginal score differences between otherwise-equivalent stores.What is changed and how does it work?
Check List
Tests
Release note