scheduler, evict-slow-store allow draining multiple store from AZ#10743
scheduler, evict-slow-store allow draining multiple store from AZ#10743Tema wants to merge 4 commits into
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @Tema. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
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:
📝 WalkthroughWalkthroughEvict-slow-store moves from single-store to group-aware eviction: config now persists label-driven eviction groups and a set of evicted store IDs; lifecycle and leader marking handle batched evictions; scheduling blocks while any disk eviction is active; algorithm starts, reconciles, expands, or releases multi-store drains based on configured failure-domain labels and topology capacity. ChangesDisk slow store eviction refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant Scheduler
participant Topology
participant PersistedState
Client->>Scheduler: POST /config (group-eviction-labels)
Scheduler->>PersistedState: persist GroupEvictionLabels
Scheduler->>Topology: collect slow stores
Scheduler->>Scheduler: evaluate domain co-location & healthy domains
Scheduler->>PersistedState: setEvictedStoresAndPersist(groupIDs)
Scheduler->>Topology: prepareEvictLeader(cluster, addIDs)
Topology->>Scheduler: leader eviction status / recovery signals
Scheduler->>PersistedState: removeEvictedAndPersist/clearEvictedAndPersist on reconcile/release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ti-chi-bot
Bot
added
the
size/XXL
9b03ea3 to
bfc522d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/response/easyjson-bootstrap4020482302.go (1)
1-4: 💤 Low valueUse modern build constraint syntax and fix typo.
Line 1: Go 1.17+ prefers
//go:build ignoreover the older// +build ignore.
Line 3: Typo "bootstapping" → "bootstrapping".-// +build ignore +//go:build ignore -// TEMPORARY AUTOGENERATED FILE: easyjson bootstapping code to launch +// TEMPORARY AUTOGENERATED FILE: easyjson bootstrapping code to launch🤖 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/response/easyjson-bootstrap4020482302.go` around lines 1 - 4, Replace the old build constraint and fix the typo: change the first-line build tag from "// +build ignore" to the modern "//go:build ignore" and correct the misspelled word "bootstapping" to "bootstrapping" in the comment describing the autogenerated file (the header comment in easyjson-bootstrap4020482302.go).
🤖 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/response/region_easyjson.go`:
- Line 11: The stubbed MarshalJSON implementations (e.g., MetaPeer.MarshalJSON)
currently return (nil, nil) which will silently produce null JSON; change each
stub (MetaPeer.MarshalJSON and the similar stubs for PDPeerStats.MarshalJSON,
RegionInfo.MarshalJSON, RegionsInfo.MarshalJSON, ReplicationStatus.MarshalJSON)
to fail loudly by returning a non-nil error (or panic with a clear message)
indicating that the method is not implemented/auto-generated code is out of date
so callers get an explicit failure rather than silent nulls.
- Line 4: The package declaration has an extra space ("package response");
update it to a single-space package declaration ("package response") in the file
containing region_easyjson.go and run the repository formatter (e.g., make fmt)
to apply and verify formatting; change the package line only (the symbol to
locate is the package declaration at the top of region_easyjson.go).
---
Nitpick comments:
In `@pkg/response/easyjson-bootstrap4020482302.go`:
- Around line 1-4: Replace the old build constraint and fix the typo: change the
first-line build tag from "// +build ignore" to the modern "//go:build ignore"
and correct the misspelled word "bootstapping" to "bootstrapping" in the comment
describing the autogenerated file (the header comment in
easyjson-bootstrap4020482302.go).
🪄 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
Run ID: a69917e9-7ee2-4871-9dcb-61690044e5f9
📒 Files selected for processing (5)
pkg/response/easyjson-bootstrap4020482302.gopkg/response/region_easyjson.gopkg/response/region_easyjson.go.tmppkg/schedule/schedulers/evict_slow_store.gopkg/schedule/schedulers/evict_slow_store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schedule/schedulers/evict_slow_store_test.go
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
bfc522d to
1dd3803
Compare
|
/retest |
|
@Tema: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
/ok-to-test |
|
@Tema: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
|
/retest-required |
|
|
/retest-required |
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10743 +/- ##
==========================================
+ Coverage 79.16% 79.19% +0.02%
==========================================
Files 536 536
Lines 73609 74034 +425
==========================================
+ Hits 58273 58631 +358
- Misses 11235 11280 +45
- Partials 4101 4123 +22
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // groupIsolationKey returns the label key to group slow stores by. Group eviction | ||
| // is only allowed when every placement rule enforces the same non-empty isolation | ||
| // level, so draining a single zone is known to be safe for every region. | ||
| func groupIsolationKey(cluster sche.SchedulerCluster) (string, bool) { | ||
| rules := cluster.GetRuleManager().GetAllRules() // includes default one | ||
| if len(rules) == 0 || rules[0].IsolationLevel == "" { | ||
| return "", false | ||
| } | ||
| level := rules[0].IsolationLevel | ||
| for _, r := range rules[1:] { | ||
| if r.IsolationLevel != level { | ||
| return "", false | ||
| } | ||
| } | ||
| return level, true | ||
| } |
There was a problem hiding this comment.
It is not recommended to use IsolationLevel as a target filter label.
A more appropriate approach is to add a manual configuration that determines whether the slow stores belong to the same availability zone (AZ). And the comparison logic should consider that the configuration value could be a multi-level labels, i.e. zone or zone, rack.
For example, we could name the configuration evicting-label and set it as follows:
evicting-label: zone: Multi-node leader eviction is triggered only when the slow stores share the same zone label.evicting-label: zone, rack: Multi-node leader eviction is triggered only when the slow stores share both the same zone and rack labels.- Empty (default): The default evicting strategy applies, which falls back to the original design — evicting a single slow node.
The comparison logic can be referenced from:
Lines 691 to 703 in 51b1226
Rest LGTM.
There was a problem hiding this comment.
It is not recommended to use IsolationLevel as a target filter label.
I'm curious why not? Just to make it configurable differently? I feel if it does not match isolation expectations, it can lead to issues.
Anyway, I don't have a strong opinion on that and like an option to turn it on/off, so I've added group-eviction-labels into PD scheduler config as you recommended. @LykxSassinator PTAL
There was a problem hiding this comment.
Correct me if I'm wrong. /cc @rleungx
FYI, isolation-level is used to determine the HA level for the entire TiKV cluster, not to determine the AZ of individual TiKV nodes.
- By default, this value is empty. This means that regardless of the AZs in which TiKV nodes are deployed, if some TiKV nodes go down (marked as the
Downstate), PD will add replicas on other alive TiKV nodes to ensure overall HA stability — irrespective of the AZs of the TiKV nodes. - If
isolation-levelis set tozonein a three‑AZ deployment, and one AZ goes down, PD will not trigger operations to add replicas in other AZs, because the isolation level is constrained tozone.
In other words, isolation-level is not the actual tag that reflects which AZ a TiKV node is deployed in. In fact, LocationLabel is used to mark the deployment tag that indicates the location of a given TiKV node.
Regarding my earlier suggestion:
"A more appropriate approach is to add a manual configuration ..."
On one hand, we want to introduce an extra, clear configuration to explicitly enable or disable this newly introduced feature. On the other hand, by allowing users to manually set this configuration based on their requests, users can control the evicting strategy themselves if multiple TiKV nodes exist, exactly as they expect.
There was a problem hiding this comment.
After further discussion, I think we should keep this PR simple and revert to the previous isolation-based direction.
The current group-eviction-labels config feels unnecessary here. It is not well aligned with the semantics of IsolationLevel, and it adds extra config, validation, and operational cost for what should be a focused correctness fix.
But the old implementation still needs one fix: it treated IsolationLevel as a single label key, while PD's actual isolation semantics are based on the LocationLabels prefix up to and including IsolationLevel.
So for this PR, I suggest:
- revert to the previous isolation-based design;
- fix the implementation to compare the
LocationLabelsprefix up toIsolationLevel, instead of onlystore.GetLabelValue(IsolationLevel); - keep the original conservative rule that grouping is allowed only when all placement rules share the same non-empty
IsolationLevel; - add only a simple boolean on/off switch so this feature can be explicitly disabled.
This keeps the change aligned with PD's existing isolation model, fixes the real bug, and avoids turning this PR into a broader config-policy discussion.
@Tema PTAL
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
|
/retest-required |
|
/test pull-unit-test-next-gen-3 |
|
/retest |
|
/retest |
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/cluster/cluster_test.go (1)
3521-3610: 🏗️ Heavy liftExercise actual leader movement in this e2e case.
Every assertion here is against
EvictedAsSlowStore(), so this can stay green even if the coordinator never drains any leaders or if leaders come back before the whole zone has recovered. Since the docstring/comments describe end-to-end eviction and recovery ordering, seed a few real regions and assert leader counts or transfer operators move offz1and only return after all three stores recover.🤖 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 `@server/cluster/cluster_test.go` around lines 3521 - 3610, The test currently only asserts EvictedAsSlowStore(), so add real leader movement checks: seed several regions with leaders pinned to stores 1,2,3 at the start (use the test cluster helper methods that create regions with a specific leader on a store via tc.AddRegion / tc.PutRegion / whatever region-seeding helper exists), then after calling markSlow to trigger group eviction assert that the leader count on zone "z1" (query via tc.RegionStoreLeaderCount or by iterating tc.Regions and counting leaders on store IDs 1,2,3) goes to zero (or that transfer operators off z1 are created) and remains off until all three stores have been recovered (call markSlow(1,0); markSlow(2,0); markSlow(3,0) and then assert leaders are returned to z1), while keeping the existing EvictedAsSlowStore() assertions; reference TestEvictSlowStoreGroupEviction, markSlow, tc.GetStore and EvictedAsSlowStore() to locate where to insert region seeding and leader-count assertions.
🤖 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 `@server/cluster/cluster_test.go`:
- Around line 3521-3610: The test currently only asserts EvictedAsSlowStore(),
so add real leader movement checks: seed several regions with leaders pinned to
stores 1,2,3 at the start (use the test cluster helper methods that create
regions with a specific leader on a store via tc.AddRegion / tc.PutRegion /
whatever region-seeding helper exists), then after calling markSlow to trigger
group eviction assert that the leader count on zone "z1" (query via
tc.RegionStoreLeaderCount or by iterating tc.Regions and counting leaders on
store IDs 1,2,3) goes to zero (or that transfer operators off z1 are created)
and remains off until all three stores have been recovered (call markSlow(1,0);
markSlow(2,0); markSlow(3,0) and then assert leaders are returned to z1), while
keeping the existing EvictedAsSlowStore() assertions; reference
TestEvictSlowStoreGroupEviction, markSlow, tc.GetStore and EvictedAsSlowStore()
to locate where to insert region seeding and leader-count assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ed2246d-13d9-4897-a1ca-fc25103dac20
📒 Files selected for processing (1)
server/cluster/cluster_test.go
|
/retest |
|
/retest-required |
|
@LykxSassinator: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LykxSassinator 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 |
| func hasEnoughHealthyDomains(cluster sche.SchedulerCluster, labels []string, drained *core.StoreInfo) bool { | ||
| reps := make([]*core.StoreInfo, 0, minRemainingHealthyDomains) | ||
| for _, store := range cluster.GetStores() { | ||
| if !store.IsUp() || store.IsSlow() || store.EvictedAsSlowStore() || !store.AllowLeaderTransferIn() { |
There was a problem hiding this comment.
Can we use the same target-store checks as leader transfer here? This predicate can count stores that StoreStateFilter{TransferLeader: true} later rejects (down, disconnected, busy, reject-leader), so group eviction may start without two real healthy target domains.
There was a problem hiding this comment.
FYI, you can use a helper function to take the same filtering condition as "leader transfer" does, for example:
func eligibleHealthyLeaderTargets(cluster sche.SchedulerCluster, stores []*core.StoreInfo) []*core.StoreInfo {
candidates := filter.NewCandidates(stores).
FilterTarget(
cluster.GetSchedulerConfig(),
nil,
nil,
&filter.StoreStateFilter{
ActionScope: types.EvictSlowStoreScheduler.String(),
TransferLeader: true,
OperatorLevel: constant.Urgent,
},
).
PickAll()
healthy := candidates[:0]
// Filter out already slow nodes
for _, store := range candidates {
if store.IsSlow() {
continue
}
healthy = append(healthy, store)
}
return healthy
}
okJiang
left a comment
okJiang
left a comment
There was a problem hiding this comment.
Apart from existing comments, lgtm
4 participants
What problem does this PR solve?
Issue Number: Close #10742
What is changed and how does it work?
Check List
Tests
Use chaos mesh to emulate slow disk
Verify default behavior:
Enabling the feature:
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests