*: reset prepare checker once the cache is reset#8860
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8860 +/- ##
==========================================
- Coverage 79.26% 79.21% -0.06%
==========================================
Files 541 541
Lines 75674 75751 +77
==========================================
+ Hits 59985 60005 +20
- Misses 11467 11505 +38
- Partials 4222 4241 +19
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
LGTM. Do we need a unit test for it?
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
ti-chi-bot
Bot
added
size/L
rleungx
force-pushed
the
reset-prepared
branch
2 times, most recently
from
9101749 to
be95f0c
Compare
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
BTW, do we need pick it to 8.5?
| } | ||
|
|
||
| // NewController create a new Controller. | ||
| func NewController(ctx context.Context, cluster sche.CheckerCluster, conf config.CheckerConfigProvider, ruleManager *placement.RuleManager, labeler *labeler.RegionLabeler, opController *operator.Controller) *Controller { | ||
| func NewController(ctx context.Context, cluster sche.CheckerCluster, conf config.CheckerConfigProvider, ruleManager *placement.RuleManager, labeler *labeler.RegionLabeler, opController *operator.Controller, prepareChecker *sche.PrepareChecker) *Controller { |
There was a problem hiding this comment.
BTW, this function has too many parameters. Do we have any good way to solve it?
| if c.ShouldRun(collectWaitTime...) { | ||
| log.Info("coordinator has finished cluster information preparation") | ||
| break | ||
| } | ||
| select { | ||
| case <-ticker.C: | ||
| case <-c.ctx.Done(): | ||
| log.Info("coordinator stops running") | ||
| return | ||
| } |
ti-chi-bot
Bot
added
size/XL
rleungx
force-pushed
the
reset-prepared
branch
2 times, most recently
from
c3f0411 to
ad5fe05
Compare
|
/retest |
ti-chi-bot
Bot
added
size/L
rleungx
force-pushed
the
reset-prepared
branch
from
9c2b83a to
a639570
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, okJiang 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 |
|
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 prepare checker is extracted into a dedicated ChangesPrepare checker gating and cache-reset wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Admin API
participant Cluster
participant Coordinator
participant Checker as preparecheck.Checker
participant SchedulerController
participant CheckerController
Coordinator->>Checker: NewChecker(...)
Coordinator->>SchedulerController: NewController(..., Checker)
Coordinator->>CheckerController: NewController(..., Checker)
loop every 3s
Coordinator->>Checker: Check(cluster)
end
Admin->>Cluster: DeleteAllRegionCache
Cluster->>Coordinator: ResetPrepared()
Coordinator->>Checker: ResetPrepared()
Cluster->>Cluster: ResetRegionCache()
SchedulerController->>Checker: RunIfPrepared(scheduleFn)
CheckerController->>Checker: RunIfPrepared(patrolFn)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/unsaferecovery/unsafe_recovery_controller.go (1)
587-592: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
preparedbefore clearing the region cache
ResetRegionCache()can run whilepreparedis still true, so a scheduler tick may see an empty cache and keep going. Swap these two calls here and inserver/api/admin.go(the same order is also used inpkg/mcs/scheduling/server/apis/v1/api.go).🤖 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/unsaferecovery/unsafe_recovery_controller.go` around lines 587 - 592, The Finished-state cleanup in UnsafeRecoveryController is resetting the region cache before clearing the prepared flag, which can let scheduling observe an empty cache while `prepared` is still true. Update the `Finished` branch in `UnsafeRecoveryController` to call `ResetPrepared()` before `ResetRegionCache()`, and make the same call-order swap in the matching admin recovery path in `server/api/admin.go` and the API implementation in `pkg/mcs/scheduling/server/apis/v1/api.go` so all recovery flows stay consistent.
🧹 Nitpick comments (2)
pkg/schedule/preparecheck/prepare_checker.go (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the misleading "for test purpose" doc on
ResetPrepared.
ResetPreparedis the load-bearing hook for this PR: it's invoked from production cache-reset paths (e.g. the admin/APIdeleteAllRegionCachehandler →Cluster.ResetPrepared()→coordinator.GetPrepareChecker().ResetPrepared()), not only from tests. The comment risks a future dev treating this as test-only and altering/removing it, breaking the delay-rebalancing behavior.📝 Proposed doc fix
-// ResetPrepared is for test purpose +// ResetPrepared resets the checker so that scheduling pauses until cluster +// information is re-collected. It is invoked when the region cache is reset +// (admin/API cache-reset and unsafe-recovery completion). func (checker *Checker) ResetPrepared() {🤖 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/preparecheck/prepare_checker.go` around lines 102 - 108, The doc comment on Checker.ResetPrepared is misleading because this method is used by production cache-reset flows, not just tests. Update the comment to describe its real role in resetting prepared state for cache/prepare workflows, and keep the method name ResetPrepared as the key reference so future changes don’t treat it as test-only. Make sure the wording reflects its usage from Cluster.ResetPrepared and coordinator.GetPrepareChecker().ResetPrepared rather than implying it’s only a test hook.tests/server/api/api_test.go (1)
1758-1773: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify the "pause immediately after reset" behavior.
After calling the delete-cache API, the test manually calls
RemoveOperators()then only checks eventual resumption viatestutil.Eventually. It never asserts that scheduling is actually blocked right afterResetPrepared()runs (e.g.IsPrepared()returns false immediately, or the operator count stays at 0 before the readiness threshold is met). Given the tiny region set here, readiness could be re-achieved almost instantly on the next heartbeat regardless of whetherResetPrepared()works, so this test would likely still pass even if the reset had no effect — weakening its value as a regression guard for the PR's actual intent (delaying rebalancing until region data refreshes).Consider adding an assertion right after the delete call that
rc.GetCoordinator().GetPrepareChecker().IsPrepared()isfalse(or that operator count is 0) before triggering the next heartbeat.♻️ Suggested strengthening of the assertion
output := sendRequest(re, addr, http.MethodDelete, http.StatusOK) re.Contains(string(output), "All regions are removed from server cache") + re.False(rc.GetCoordinator().GetPrepareChecker().IsPrepared()) rc.GetOperatorController().RemoveOperators() re.Equal(0, int(rc.GetOperatorController().OperatorCount(operator.OpSplit)))🤖 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 `@tests/server/api/api_test.go` around lines 1758 - 1773, The delete-cache test in api_test.go only checks eventual recovery and does not verify that scheduling is blocked immediately after the cache reset. In the test around the delete API call and the subsequent heartbeat, add an assertion using rc.GetCoordinator().GetPrepareChecker().IsPrepared() (or the operator count via rc.GetOperatorController()) right after the delete response and before rc.HandleRegionHeartbeat(regionInfo) to confirm the reset takes effect immediately, then keep the existing eventual resumption check as the follow-up.
🤖 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 `@server/api/admin.go`:
- Around line 157-158: The reset sequence in the admin handler should clear the
prepared state before the region cache so a patrol tick cannot run against a
cleared cache while prepared is still true. Update the reset logic in the admin
API flow around rc.ResetRegionCache and rc.ResetPrepared to either swap their
order or guard both operations with the same lock, keeping the two state changes
atomic.
---
Outside diff comments:
In `@pkg/unsaferecovery/unsafe_recovery_controller.go`:
- Around line 587-592: The Finished-state cleanup in UnsafeRecoveryController is
resetting the region cache before clearing the prepared flag, which can let
scheduling observe an empty cache while `prepared` is still true. Update the
`Finished` branch in `UnsafeRecoveryController` to call `ResetPrepared()` before
`ResetRegionCache()`, and make the same call-order swap in the matching admin
recovery path in `server/api/admin.go` and the API implementation in
`pkg/mcs/scheduling/server/apis/v1/api.go` so all recovery flows stay
consistent.
---
Nitpick comments:
In `@pkg/schedule/preparecheck/prepare_checker.go`:
- Around line 102-108: The doc comment on Checker.ResetPrepared is misleading
because this method is used by production cache-reset flows, not just tests.
Update the comment to describe its real role in resetting prepared state for
cache/prepare workflows, and keep the method name ResetPrepared as the key
reference so future changes don’t treat it as test-only. Make sure the wording
reflects its usage from Cluster.ResetPrepared and
coordinator.GetPrepareChecker().ResetPrepared rather than implying it’s only a
test hook.
In `@tests/server/api/api_test.go`:
- Around line 1758-1773: The delete-cache test in api_test.go only checks
eventual recovery and does not verify that scheduling is blocked immediately
after the cache reset. In the test around the delete API call and the subsequent
heartbeat, add an assertion using
rc.GetCoordinator().GetPrepareChecker().IsPrepared() (or the operator count via
rc.GetOperatorController()) right after the delete response and before
rc.HandleRegionHeartbeat(regionInfo) to confirm the reset takes effect
immediately, then keep the existing eventual resumption check as the follow-up.
🪄 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: 5399764e-2f20-478e-bff8-4c2bd732121b
📒 Files selected for processing (19)
pkg/btree/btree_generic_test.gopkg/mcs/scheduling/server/apis/v1/api.gopkg/mcs/scheduling/server/cluster.gopkg/mcs/scheduling/server/server.gopkg/mock/mockcluster/mockcluster.gopkg/schedule/checker/checker_controller.gopkg/schedule/coordinator.gopkg/schedule/preparecheck/prepare_checker.gopkg/schedule/schedulers/scheduler_controller.gopkg/unsaferecovery/unsafe_recovery_controller.goserver/api/admin.goserver/cluster/cluster_test.goserver/cluster/scheduling_controller.gotests/server/api/api_test.gotests/server/api/diagnostic_test.gotests/server/api/region_test.gotests/server/cluster/cluster_test.gotools/pd-ctl/pdctl/command/operator_command.gotools/pd-ctl/tests/store/store_test.go
💤 Files with no reviewable changes (1)
- pkg/btree/btree_generic_test.go
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com> # Conflicts: # pkg/unsaferecovery/unsafe_recovery_controller.go
rleungx
removed
the
do-not-merge/hold
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/schedule/operator/operator_controller.go (1)
661-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent metric accounting vs.
RemoveOperators.For waiting operators, this loop explicitly increments
operatorCounter.WithLabelValues(op.Desc(), "remove")in addition to whateverburyOperatorrecords based on the resulting status (e.g. "cancel"). The running-operator path inRemoveOperators(Lines 634-649) does not do this extra "remove" increment for the same cancel-then-bury flow. This produces asymmetric metrics between the two operator classes now merged into one purge operation.🤖 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/operator/operator_controller.go` around lines 661 - 673, The waiting-operator branch in the purge loop is double-counting by incrementing operatorCounter with "remove" before calling buryOperator, while the running-operator path in RemoveOperators only relies on buryOperator for its final status. Make the metric accounting symmetric by removing the extra "remove" increment here or applying the same cancel-then-bury metric flow used in RemoveOperators, using the waiting-operator handling in operator_controller.go as the place to align the behavior.
🤖 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/operator/operator_controller.go`:
- Around line 651-674: RemoveAllOperators currently races with
PromoteWaitingOperator because RemoveOperators(), wop.Clear(), and later
promotion can interleave, letting a waiting operator be re-added after bulk
removal starts. Add a controller-level guard or shared lock around the full
remove-all flow and the promotion path in Controller.RemoveAllOperators and
Controller.PromoteWaitingOperator so dequeueing from wop and moving into
oc.operators cannot happen during a reset. Use the existing Controller, wop,
RemoveOperators, and PromoteWaitingOperator symbols to keep the atomicity
consistent.
---
Nitpick comments:
In `@pkg/schedule/operator/operator_controller.go`:
- Around line 661-673: The waiting-operator branch in the purge loop is
double-counting by incrementing operatorCounter with "remove" before calling
buryOperator, while the running-operator path in RemoveOperators only relies on
buryOperator for its final status. Make the metric accounting symmetric by
removing the extra "remove" increment here or applying the same cancel-then-bury
metric flow used in RemoveOperators, using the waiting-operator handling in
operator_controller.go as the place to align the behavior.
🪄 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: d2ef4710-8e10-4217-9b87-36ede354a8b8
📒 Files selected for processing (6)
pkg/mcs/scheduling/server/cluster.gopkg/schedule/operator/operator_controller.gopkg/schedule/operator/operator_controller_test.gopkg/schedule/operator/waiting_operator.goserver/cluster/scheduling_controller.gotests/server/api/api_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server/api/api_test.go
This reverts commit c4b3916. Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/schedule/checker/checker_controller.go (1)
103-111: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConsolidate duplicated prepare-checker resolution logic.
The same "use override if provided, else build a default and
SetPrepared()" block is copy-pasted here and inscheduler_controller.go(lines 66-71). Extracting it into a small helper in thepreparecheckpackage would keep both call sites in sync as this logic evolves.♻️ Suggested helper
// in pkg/schedule/preparecheck func Resolve(totalRegionCountFn func() (int, error), checkers ...*Checker) *Checker { if len(checkers) > 0 && checkers[0] != nil { return checkers[0] } c := NewChecker(totalRegionCountFn) c.SetPrepared() return c }- prepareChecker := preparecheck.NewChecker(cluster.GetPrepareRegionCount) - if len(prepareCheckers) > 0 && prepareCheckers[0] != nil { - prepareChecker = prepareCheckers[0] - } else { - prepareChecker.SetPrepared() - } + prepareChecker := preparecheck.Resolve(cluster.GetPrepareRegionCount, prepareCheckers...)🤖 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/checker/checker_controller.go` around lines 103 - 111, The prepare-checker selection logic in NewController is duplicated and should be consolidated so both controller call sites stay consistent. Move the “use the provided checker if present, otherwise create a default Checker and call SetPrepared()” behavior into a small helper in the preparecheck package, such as a Resolve-style function, and then update NewController to use that helper instead of inlining the override/default logic.
🧹 Nitpick comments (1)
pkg/schedule/preparecheck/prepare_checker_test.go (1)
15-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
Check()readiness path.This new test file only exercises
RunIfPrepared/SetPrepared/ResetPrepared.Check()(the heartbeat-count/timeout-driven readiness transition referenced in the AI summary) isn't tested here, yet it's the primary mechanism by whichpreparedbecomestruein production (vs. the test-onlySetPrepared).🤖 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/preparecheck/prepare_checker_test.go` around lines 15 - 47, The new test only covers the manual prepared-state helpers in Checker, so add coverage for the readiness transition driven by Check() as well. Extend prepare_checker_test.go with a test that exercises NewChecker and Check() using the heartbeat-count/timeout path from the implementation, asserting that prepared becomes true only after the expected conditions are met and that RunIfPrepared behavior reflects that state. Keep the focus on the Checker methods Check, RunIfPrepared, SetPrepared, and ResetPrepared so the production readiness flow is covered, not just the test-only helper path.
🤖 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.
Outside diff comments:
In `@pkg/schedule/checker/checker_controller.go`:
- Around line 103-111: The prepare-checker selection logic in NewController is
duplicated and should be consolidated so both controller call sites stay
consistent. Move the “use the provided checker if present, otherwise create a
default Checker and call SetPrepared()” behavior into a small helper in the
preparecheck package, such as a Resolve-style function, and then update
NewController to use that helper instead of inlining the override/default logic.
---
Nitpick comments:
In `@pkg/schedule/preparecheck/prepare_checker_test.go`:
- Around line 15-47: The new test only covers the manual prepared-state helpers
in Checker, so add coverage for the readiness transition driven by Check() as
well. Extend prepare_checker_test.go with a test that exercises NewChecker and
Check() using the heartbeat-count/timeout path from the implementation,
asserting that prepared becomes true only after the expected conditions are met
and that RunIfPrepared behavior reflects that state. Keep the focus on the
Checker methods Check, RunIfPrepared, SetPrepared, and ResetPrepared so the
production readiness flow is covered, not just the test-only helper path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c19fa5f0-984a-4817-bc51-e09c09e804e6
📒 Files selected for processing (8)
pkg/mcs/scheduling/server/cluster.gopkg/schedule/checker/checker_controller.gopkg/schedule/checker/checker_controller_test.gopkg/schedule/preparecheck/prepare_checker.gopkg/schedule/preparecheck/prepare_checker_test.gopkg/schedule/schedulers/scheduler_controller.goserver/cluster/scheduling_controller.gotests/server/api/api_test.go
💤 Files with no reviewable changes (2)
- server/cluster/scheduling_controller.go
- pkg/mcs/scheduling/server/cluster.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/schedule/preparecheck/prepare_checker.go
- tests/server/api/api_test.go
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XXL
|
@rleungx: The following test 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. |
|
/test pull-unit-test-next-gen-3 |
3 participants
What problem does this PR solve?
Issue Number: Close #8634, might fix #8689
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit