schedule: support placement-constrained hot scheduling - #11057
Conversation
Skip global load expectation checks for peers governed by placement rules with label constraints. Keep target selection within the matched placement rule and preserve the existing behavior for unconstrained rules. Ref tikv#5545 Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
|
[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/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe hot region scheduler now computes placement fits per region, derives placement-scoped load expectations, applies them to source, destination, and revert selection, and propagates fits to safeguards. Supporting placement and store-load tests cover constrained peer and leader scheduling. ChangesPlacement-aware hot region scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HotRegionSolver
participant RegionFit
participant PlacementLoadScope
participant SourceSelection
participant DestinationSelection
HotRegionSolver->>RegionFit: compute current region fit
RegionFit-->>HotRegionSolver: return curFit
HotRegionSolver->>PlacementLoadScope: derive scoped expectations
PlacementLoadScope-->>HotRegionSolver: return curScope
HotRegionSolver->>SourceSelection: validate hot-peer source
SourceSelection-->>HotRegionSolver: return eligible source
HotRegionSolver->>DestinationSelection: validate destination with curFit and curScope
DestinationSelection-->>HotRegionSolver: return scheduling candidate
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Add explicit coverage for write-leader and read-peer scheduling when Placement Rules constrain eligible stores and global load expectations would otherwise block valid operations. Ref tikv#5545 Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11057 +/- ##
==========================================
+ Coverage 79.23% 79.30% +0.06%
==========================================
Files 540 541 +1
Lines 76010 76568 +558
==========================================
+ Hits 60226 60719 +493
- Misses 11533 11574 +41
- Partials 4251 4275 +24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
![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/hot_region_solver.go`:
- Around line 209-233: Remove the engine-level caching of mayUsePlacementScope
results in the source-store loop. Evaluate bs.mayUsePlacementScope(srcStore)
independently for each source store before deciding whether to record
sourceFailure and continue, while preserving the existing placementEnabled and
per-store result handling.
🪄 Autofix (Beta)
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: db294030-6241-4d3b-bb69-01c27750d796
📒 Files selected for processing (4)
pkg/schedule/filter/filters.gopkg/schedule/filter/filters_test.gopkg/schedule/schedulers/hot_region_solver.gopkg/schedule/schedulers/hot_region_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schedule/schedulers/hot_region_test.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/test pull-unit-test-next-gen-3 |
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
This PR unblocks the positive case where a global expectation incorrectly rejects scheduling within a single label-constrained rule. However, the implementation is not placement-aware expectation; it is placement-triggered expectation disablement.
Pairwise ranking does not replace scope-local expectation, five-minute historical stability, or stddev convergence protection. In addition, the actual candidate sets for leader and revert-region scheduling may cross the source peer's rule boundary. Therefore, the current implementation is not yet complete or robust enough to claim general support for placement-constrained hot scheduling.
Before merging, please address the scope-local statistics, cross-rule leader handling, revert-region placement validation, and the full-rule-set scan described in the inline comments, and add the corresponding negative and cross-rule tests.
Per the review request, I did not run tests or inspect CI results.
|
|
||
| func (bs *balanceSolver) enableExpectation() bool { | ||
| return bs.sche.conf.getDstToleranceRatio() > 0 && bs.sche.conf.getSrcToleranceRatio() > 0 | ||
| return !bs.skipLoadExpectation && bs.sche.conf.getDstToleranceRatio() > 0 && bs.sche.conf.getSrcToleranceRatio() > 0 |
There was a problem hiding this comment.
We should not use skipLoadExpectation to disable expectation, historical expectation, and the stddev/uniform checks altogether.
A Placement Rule only means that the global store population may be the wrong population for the calculation. It does not make the convergence safeguards themselves unnecessary. With the current implementation, every constrained scope permanently loses the redundant-scheduling and transient-load protections introduced by #2297, #4912, and #5286.
For example, suppose a rule matches 100 stores: 98 stores have load 100, the source has load 110, and the target has load 90. There is one additional unmatched store with load 100. The normalized stddev of the matched scope is only about 1.4%, so the uniform check should stop scheduling. However, the presence of the single out-of-scope store makes this code skip the stddev check and may still produce an operator.
Please calculate expectation, historical expectation, and stddev for the applicable placement scope instead of disabling all of them with a boolean. At minimum, a scope-local uniform/history guard and a negative test for an already-uniform scope are needed.
| } | ||
| ruleFit := bs.curFit.GetRuleFit(sourcePeer.GetId()) | ||
| if ruleFit != nil { | ||
| bs.skipLoadExpectation = bs.ruleRestrictsStoreLoad(ruleFit.Rule, bs.cur.srcStore) |
There was a problem hiding this comment.
This assumes that the legal leader target set is the label scope of the source peer's rule, which is not true when multiple Voter rules are involved.
RegionFit.Replace permits a role transfer when the source and target belong to different rules with the same Voter role. For example, a region may have rule A as Voter count=1, pool=A and rule B as Voter count=2, pool=B. The leader may currently be in A but can legally transfer to an existing peer in B. Rule A constrains replica placement; it does not constrain the leader to A. Nevertheless, this code disables expectation based on rule A and then allows a target from B.
The leader load scope needs to be derived from the complete legal leader candidate set rather than directly from the source peer's RuleFit. Please add coverage for leader transfers across multiple rules with the same role.
| filter.NewExcludedFilter(bs.sche.GetName(), bs.cur.region.GetStoreIDs(), bs.cur.region.GetStoreIDs()), | ||
| filter.NewSpecialUseFilter(bs.sche.GetName(), filter.SpecialUseHotRegion), | ||
| filter.NewPlacementSafeguard(bs.sche.GetName(), bs.GetSchedulerConfig(), bs.GetBasicCluster(), bs.GetRuleManager(), bs.cur.region, srcStore, nil), | ||
| filter.NewPlacementSafeguard(bs.sche.GetName(), bs.GetSchedulerConfig(), bs.GetBasicCluster(), bs.GetRuleManager(), bs.cur.region, srcStore, bs.curFit), |
There was a problem hiding this comment.
This placement safeguard is only applied to the main region. When rank v2 searches a revert region, the reverse dstStore -> srcStore operation does not run an equivalent safeguard and does not derive the revert region's own placement scope.
This can produce an invalid second operator when the two regions belong to different or overlapping rules. For example, store 2 may match both rule A and rule B while store 1 only matches rule A. The main region can legally move from store 1 to store 2, but a revert-region peer assigned to rule B cannot legally move back to store 1. The revert path may still create that operator.
The operator builder does not replace NewPlacementSafeguard for validating the added peer's label constraints. Please fit and safeguard the main and revert regions independently and use their respective load scopes. A rank-v2 regression test with overlapping placement rules and two generated operators is also needed.
| } | ||
|
|
||
| func (bs *balanceSolver) mayUsePlacementScope(isTiKV bool) bool { | ||
| for _, rule := range bs.GetRuleManager().GetAllRules() { |
There was a problem hiding this comment.
mayUsePlacementScope should not call GetAllRules() on the hot-scheduler path.
GetAllRules() clones and sorts the entire placement-rule set, and Rule.Clone currently performs JSON marshal/unmarshal. This scheduler can run once per second, and read scheduling constructs both leader and peer solvers. In clusters with many table or partition placement rules, this introduces unbounded CPU, sorting, allocation, and GC overhead.
It also scans rules unrelated to any candidate hot region's key range, including rules that will never be effective for these regions, so this precheck can produce many false positives. Please inspect only the effective rules of candidate hot regions, or maintain a lightweight RuleManager cache invalidated by rule/store-label versions.
| hot := scheduler.(*hotScheduler) | ||
| hot.types = []resourceType{resource} | ||
| hot.conf.setHistorySampleDuration(0) | ||
| return tc, hot |
There was a problem hiding this comment.
This shared helper forces history-sample-duration to zero, so none of the new tests exercise the historical-expectation path that this PR also bypasses.
The default configuration keeps five minutes of load history. With the current implementation, a placement-constrained region can be scheduled solely from its current load even when the source and target histories do not support that direction, which can turn transient load changes into scheduling churn.
Please add negative cases for historical load rejecting the move, an already-uniform placement scope, an unconstrained rule retaining the existing behavior, leader transfer across multiple same-role rules, and a revert region belonging to a different rule.
Calculate current, historical, and variance safeguards from each effective placement scope instead of disabling them. Reuse the global fast path, cache single-rule scopes, and avoid scanning or cloning the full rule set in the scheduler loop. Derive leader scopes from all same-role fits and validate revert regions against their own placement fit before creating the reverse operator. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (7)
pkg/schedule/placement/config.go (2)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or name the array indices.
[2]boolwith0 = TiKV/1 = TiFlashis only decipherable by readingadjust()andMayRestrictStoreLoad. A short comment or two named fields (mayRestrictTiKVLoad,mayRestrictTiFlashLoad) removes the positional coupling across files.🤖 Prompt for 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. In `@pkg/schedule/placement/config.go` at line 29, Clarify the positional meaning of Config’s mayRestrictStoreLoad array by documenting index 0 as TiKV and index 1 as TiFlash, or replace it with clearly named fields such as mayRestrictTiKVLoad and mayRestrictTiFlashLoad. Update adjust() and MayRestrictStoreLoad to use the clarified representation consistently.
65-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment for the sentinel and return condition.
The
""value at Line 88 encodes "store with no engine label" and the final condition mixes two distinct notions (non-engine constraints present, or only a subset of engine values matched). Both are non-obvious to the next reader; a two-line comment above the function would make the contract explicit.🤖 Prompt for 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. In `@pkg/schedule/placement/config.go` around lines 65 - 96, Add a two-line doc comment immediately above ruleMayRestrictEngine documenting that the empty-string value represents a store without an engine label, and that the return condition is true when either non-engine constraints exist or only some supplied engine values match.pkg/schedule/placement/rule_manager_test.go (1)
94-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
NotInengine constraints and multi-rule OR-ing.The cases only exercise
Inon the engine key with a single rule. The two behaviors most likely to regress areengine NotIn [tiflash](which should not restrict TiKV load, since it matches both the empty andtikvlabel values) and the||accumulation across two rules inadjust(). Adding those two assertions would lock the intended semantics.🤖 Prompt for 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. In `@pkg/schedule/placement/rule_manager_test.go` around lines 94 - 111, Extend the tests around setConstraints and MayRestrictStoreLoad to cover an engine-key NotIn constraint excluding TiFlash, asserting it does not restrict TiKV load for both flag values. Also add a multi-rule case verifying adjust() ORs the rules correctly, preserving restriction when any applicable rule permits it.pkg/schedule/schedulers/hot_region_test.go (1)
514-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLabel setup is hard to follow.
Lines 515-523 assign
labels, conditionally replace it, then null it again for the exclusive case. Computing the label map per case (or a smalllabelsFor(id)closure per test case) would make the "implicit exclusive labels" scenario self-evident.🤖 Prompt for 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. In `@pkg/schedule/schedulers/hot_region_test.go` around lines 514 - 529, Simplify label construction in the test loop around AddLabelsStore by computing each store’s labels through a per-case helper or equivalent logic. Make the exclusive scenario’s implicit labels explicit, while preserving target labels for stores 1–4, other labels for stores 5–6, and nil labels for exclusive target stores.pkg/schedule/schedulers/hot_region_solver.go (2)
905-913: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMulti-rule scopes bypass the cache and are recomputed per region.
cacheableis only true for a single rule, so thetransferLeaderpath with several same-role rules re-scans everystLoadDetailentry (and re-matches label constraints for each rule) for every candidate hot region. Keying the cache on the full rule set — e.g. a small composite key built from the rule pointers/IDs plusisTiKV— would keep the leader path off the per-region hot path.🤖 Prompt for 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. In `@pkg/schedule/schedulers/hot_region_solver.go` around lines 905 - 913, Update balanceSolver.getPlacementLoadScope to cache scopes for multi-rule inputs as well as single-rule inputs. Build a stable composite placementScopeKey from the complete rules set and isTiKV, use it for lookup and storage, and preserve correct distinction between different rule combinations while avoiding per-region recomputation.
299-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the revert fit explicit rather than harvesting
bs.curFit.
revertFitis obtained by relying ongetRegion→isRegionAvailableoverwritingbs.curFit, then immediately restoring the main fit. It is correct as written, but any future statement inserted between Lines 301 and 303 would leavebs.curFitpointing at the revert region while the main region's operators are built. HavinggetRegion(or a small wrapper) return the fit alongside the region would remove the hidden mutation.🤖 Prompt for 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. In `@pkg/schedule/schedulers/hot_region_solver.go` around lines 299 - 312, Update the revert-region lookup in the hot-region solver to return the region’s fit explicitly alongside the region, rather than relying on getRegion/isRegionAvailable mutating bs.curFit. Use the returned fit for revertFit, then restore bs.curFit to mainFit before subsequent main-region operator construction, preserving existing validation and revert behavior.pkg/statistics/store_load.go (1)
111-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplicit contract between
AddandResultis easy to break.
Resultdivides by the count accumulated throughAddbut computes variance from the caller-supplieddetails. If the two populations ever diverge, the stddev is silently wrong with no error. Consider havingAddtake*StoreLoadDetailand retain the slice internally soResultneeds no argument, or at minimum document the requirement thatdetailsmust be exactly the loads previously added.🤖 Prompt for 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. In `@pkg/statistics/store_load.go` around lines 111 - 115, The StoreLoadSummary Add/Result contract can silently produce incorrect standard deviation when the supplied populations differ. Update StoreLoadSummary to retain the details accepted by Add and make Result compute from that internal collection without a caller-supplied slice; otherwise document and enforce that Result’s details exactly match those previously added.
🤖 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.
Nitpick comments:
In `@pkg/schedule/placement/config.go`:
- Line 29: Clarify the positional meaning of Config’s mayRestrictStoreLoad array
by documenting index 0 as TiKV and index 1 as TiFlash, or replace it with
clearly named fields such as mayRestrictTiKVLoad and mayRestrictTiFlashLoad.
Update adjust() and MayRestrictStoreLoad to use the clarified representation
consistently.
- Around line 65-96: Add a two-line doc comment immediately above
ruleMayRestrictEngine documenting that the empty-string value represents a store
without an engine label, and that the return condition is true when either
non-engine constraints exist or only some supplied engine values match.
In `@pkg/schedule/placement/rule_manager_test.go`:
- Around line 94-111: Extend the tests around setConstraints and
MayRestrictStoreLoad to cover an engine-key NotIn constraint excluding TiFlash,
asserting it does not restrict TiKV load for both flag values. Also add a
multi-rule case verifying adjust() ORs the rules correctly, preserving
restriction when any applicable rule permits it.
In `@pkg/schedule/schedulers/hot_region_solver.go`:
- Around line 905-913: Update balanceSolver.getPlacementLoadScope to cache
scopes for multi-rule inputs as well as single-rule inputs. Build a stable
composite placementScopeKey from the complete rules set and isTiKV, use it for
lookup and storage, and preserve correct distinction between different rule
combinations while avoiding per-region recomputation.
- Around line 299-312: Update the revert-region lookup in the hot-region solver
to return the region’s fit explicitly alongside the region, rather than relying
on getRegion/isRegionAvailable mutating bs.curFit. Use the returned fit for
revertFit, then restore bs.curFit to mainFit before subsequent main-region
operator construction, preserving existing validation and revert behavior.
In `@pkg/schedule/schedulers/hot_region_test.go`:
- Around line 514-529: Simplify label construction in the test loop around
AddLabelsStore by computing each store’s labels through a per-case helper or
equivalent logic. Make the exclusive scenario’s implicit labels explicit, while
preserving target labels for stores 1–4, other labels for stores 5–6, and nil
labels for exclusive target stores.
In `@pkg/statistics/store_load.go`:
- Around line 111-115: The StoreLoadSummary Add/Result contract can silently
produce incorrect standard deviation when the supplied populations differ.
Update StoreLoadSummary to retain the details accepted by Add and make Result
compute from that internal collection without a caller-supplied slice; otherwise
document and enforce that Result’s details exactly match those previously added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0afff88d-8c5b-47c8-b771-f3e1a86631ca
📒 Files selected for processing (9)
pkg/schedule/placement/config.gopkg/schedule/placement/label_constraint.gopkg/schedule/placement/rule_manager.gopkg/schedule/placement/rule_manager_test.gopkg/schedule/schedulers/hot_region_solver.gopkg/schedule/schedulers/hot_region_solver_test.gopkg/schedule/schedulers/hot_region_test.gopkg/statistics/store_hot_peers_infos.gopkg/statistics/store_load.go
Keep rank v1 on the existing global expectation path. Cache multi-rule placement scopes and return revert-region fits explicitly. Add coverage for overlapping rules, v1 isolation, engine constraints, and multi-rule cache reuse. Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
Thanks for the update. The scoped expectation, history, standard-deviation, revert-fit, and hot-path changes address most of my previous concerns. I still see the following compatibility and coverage gaps.
| // Init store load detail according to the type. | ||
| bs.stLoadDetail = bs.sche.stLoadInfos[bs.resourceTy] | ||
| if placementState == nil { | ||
| bs.placementV2Enabled = bs.GetSchedulerConfig().IsPlacementRulesEnabled() && rankFormulaVersion == "v2" |
There was a problem hiding this comment.
[P1] Do not leave upgraded rank-v1 clusters on the broken global population
Placement scoping is orthogonal to ranking: rank v1 uses the same source, destination, history, and uniformity safeguards. Persisted configurations from clusters created before v6.4 can still resolve to v1 because a missing field remains empty and an empty value maps to v1. This gate therefore leaves the reported bug unfixed for supported upgraded clusters, while the release note states that the issue is fixed. Please enable placement scopes for both rank formulas, or provide an explicit migration/deprecation path and narrow the release note accordingly.
| addRegionInfo(tc, utils.Write, []testRegionInfo{ | ||
| {1, []uint64{1, 2, 3}, 2.5 * units.MiB, 0, 0}, | ||
| {2, []uint64{1, 2, 3}, 17.5 * units.MiB, 0, 0}, | ||
| {3, []uint64{2, 1, 3}, 0, 0, 0}, |
There was a problem hiding this comment.
[P2] Make this test distinguish scoped expectation from global expectation
These regions produce leader byte loads of [20, 0, 0, 0, 0, 0]. The global expectation is 20/6, while the placement-scoped expectation is 20/4; source 20 and destination 0 pass both paths. Consequently this test still succeeds if write-leader scheduling silently falls back to the old global calculation. Please use a destination that is above the global expectation but below the scoped expectation, and preferably add a v1/global-path control assertion.
| re.NoError(err) | ||
| hb := sche.(*hotScheduler) | ||
| hb.types = []resourceType{writePeer} | ||
| hb.conf.setDstToleranceRatio(0.0) |
There was a problem hiding this comment.
[P2] Exercise the scoped revert safeguards instead of disabling them
Setting both tolerance ratios to zero and disabling history bypasses the newly added revert-scope expectation, history, and uniformity checks. This test currently validates only the placement safeguard. Please keep the default safeguards in at least one valid/invalid revert case, and add a case where current load passes but scoped history rejects the candidate.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
2 participants
What problem does this PR solve?
Issue Number: ref #5545
The hot Region scheduler calculates load expectations and variance across all stores. When Placement Rules constrain a Region's peers to a subset of stores, loads from other store groups can prevent valid hot Region scheduling within the constrained group.
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit