fix(updater): preserve topology when canceling offline stores - #6996
fix(updater): preserve topology when canceling offline stores#6996fgksgf wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
|
/run-pull-e2e-kind-v2 |
There was a problem hiding this comment.
Pull request overview
This PR fixes cancel-offline (partial scale-in cancellation) for TiKV/TiFlash so it restores the topology-best offlining instance rather than arbitrarily restoring the first name-sorted candidate, preserving AZ/rack balance under weighted topology policies.
Changes:
- Extend the topology scheduler (
NextAdd) to optionally score only a provided set of candidate topologies while still using global active counts/weights. - Add a topology-aware cancel-offline prefer policy (
PolicyCancelOffline) and wire it into the updater’s cancel-offline selection path. - Strengthen unit and e2e coverage to deterministically assert the correct cancel-offline candidate is chosen under weighted multi-AZ topology.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/tikv/tikv.go | Makes the partial cancellation scale-in e2e case topology-weighted and deterministic, asserting the topology-correct cancel-offline choice. |
| pkg/utils/topology/scheduler.go | Extends NextAdd with optional candidate restriction and handles negative-score candidate sets safely. |
| pkg/utils/topology/scheduler_test.go | Adds focused tests for candidate-restricted NextAdd behavior. |
| pkg/updater/two_steps_deletion_test.go | Adds a unit test proving cancel-offline can be driven by a custom cancel-offline prefer policy. |
| pkg/updater/policy/topology.go | Implements PolicyCancelOffline and fixes Update scheduler keying to use the outdated instance name. |
| pkg/updater/policy/topology_test.go | Adds unit tests for topology-aware cancel-offline selection and the Update scheduler keying fix. |
| pkg/updater/builder.go | Adds WithCancelOfflinePreferPolicy and constructs a dedicated cancel-offline selector. |
| pkg/updater/actor.go | Uses the cancel-offline selector when scaling out by restoring an offlining instance. |
| pkg/controllers/tikvgroup/tasks/updater.go | Wires PolicyCancelOffline() into TiKVGroup updater construction. |
| pkg/controllers/tikvgroup/tasks/updater_test.go | Adds a unit test ensuring TiKVGroup cancels the topology-correct offlining instance (not lexicographic). |
| pkg/controllers/tiflashgroup/tasks/updater.go | Wires PolicyCancelOffline() into TiFlashGroup updater construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fb0d251 to
58139b1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6996 +/- ##
==========================================
+ Coverage 40.39% 40.50% +0.10%
==========================================
Files 436 436
Lines 25219 25274 +55
==========================================
+ Hits 10188 10237 +49
- Misses 15031 15037 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Update hooks run before KeepName, so the new object's name is still empty when topologyPolicy.Update runs. Keying the in-memory scheduler by update.GetName() corrupted scheduler state across repeated updates within a single updater execution (affects rolling-update topology scoring for all components using TopologyPolicy). Key by the outdated instance's name instead.
Canceling an in-progress TiKV/TiFlash scale-in previously selected the lexicographically-first offlining instance to cancel via a name-sorted fallback in Selector.Choose, ignoring topology. Add a topology-aware cancel-offline PreferPolicy that scores only the still-cancelable candidates (not the global best topology, which may have none), extend the scheduler's NextAdd to accept a candidate restriction, and wire the new policy into the TiKV/TiFlash updaters.
These fmt.Println calls (left over from pingcap#6538) are unrelated to the cancel-offline fix, but this change already touches the file.
58139b1 to
ec7cbab
Compare
Summary
Canceling an in-progress TiKV/TiFlash scale-in currently selects the lexicographically-first offlining instance to restore, instead of the topology-best candidate among the instances that can actually be canceled. This can leave the cluster's zone/rack distribution worse than necessary after a partial cancellation.
Closes #6991
Why
Selector.Choosefalls back to the first name-sorted item when noPreferPolicyprefers a candidate. Cancel-offline never had a topology-awarePreferPolicywired in, so it always fell back to name order regardless of which zone/rack each offlining candidate belonged to.How
pkg/utils/topology/scheduler.go:NextAddnow accepts an optional list of candidate topologies. When given, scoring is restricted to those candidates (using the existing weight/count formula) instead of scoring every configured topology — this lets the cancel-offline path ask "which of these candidates is best" rather than "what's the global best topology" (which may have no cancelable candidate at all).pkg/updater/policy/topology.go: addsPolicyCancelOffline()onTopologyPolicy, backed by a newcancelOfflinePreferPolicythat scores only the allowed (cancelable) candidates via the updatedNextAdd. Also fixesUpdateto key the in-memory scheduler by the outdated instance's name, since update hooks run beforeKeepNamefills in the new object's name.pkg/updater/builder.go/pkg/updater/actor.go: wires acancelOfflineSelector(viaWithCancelOfflinePreferPolicy) used bychooseToCancelOffline, falling back to existing name-order behavior when unconfigured.pkg/controllers/tikvgroup/tasks/updater.goandpkg/controllers/tiflashgroup/tasks/updater.go: registertopoPolicy.PolicyCancelOffline()as the cancel-offline prefer policy.tests/e2e/tikv/tikv.go: extends the partial-cancellation scale-in test to use a two-zone weighted topology and assert that the topology-aware candidate (not the lexicographically-first one) is the one canceled, deterministically regardless of the random instance names PD assigns.Testing
make lint: 0 issues.make unit(full repo, race-enabled): passes, aside from a pre-existing failure inpkg/controllers/tiproxy/tasks/finalizer_test.go(TestTaskDrainPodForDelete*) that reproduces identically onmainwithout this change and has no dependency on any package touched here.make verifysub-targets (tidy, codegen, crd, runtimegen, gengo, overlaygen, doc, license, feature-gates) run clean against the committed tree — no generated-file drift.TestSchedulerNextAddCandidates,TestPolicyCancelOfflinePreferBestAvailableTopology,TestTopologyPolicyUpdateKeysSchedulerByOutdatedName, the newtwo_steps_deletion_test.gocase, andTestTaskUpdaterCancelOfflinePrefersTopologyAwareCandidate.pull-e2e-kind-v2CI job (not run locally, per policy against running E2E outside an approved kind cluster).Risks
tiproxytest failures noted above are unrelated but should be triaged separately./run-pull-e2e-kind-v2and monitor once the PR is open.