schedule: respect limits in concurrent patrol workers#10925
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/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 checker controller's patrol-region loop now tracks in-flight work with an atomic pending count instead of channel length, gating batch starts and worker reconfiguration on idleness. Operator admission is refactored around a new ChangesPatrol Region Concurrency Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PatrolRegions
participant PatrolRegionContext
participant WorkerGoroutine
participant Controller
PatrolRegions->>PatrolRegionContext: submit(region)
PatrolRegionContext->>WorkerGoroutine: send region on regionChan
WorkerGoroutine->>Controller: tryAddOperators(region)
Controller->>Controller: buildOperatorCandidate(region)
Controller->>Controller: admitWaitingOperators(candidate) under operatorMu
Controller->>Controller: check per-op-kind limit / halt state
alt limit reached or halted
Controller->>Controller: defer region into pendingProcessedRegions
else admitted
Controller->>Controller: AddWaitingOperator(ops)
end
WorkerGoroutine->>PatrolRegionContext: pendingCount--
PatrolRegions->>PatrolRegionContext: waitIdle()
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/schedule/checker/checker_controller.go (1)
490-501: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a comment or warning for the default case.
The default returning
0means any operator with an unhandledOpKindwould be silently blocked (sincecount >= 0is always true). While current code paths only use handled kinds, this could cause subtle bugs if new checker types are added.A comment explaining this defensive behavior would help maintainers:
📝 Suggested documentation
func (c *Controller) getOperatorLimit(kind operator.OpKind) uint64 { switch kind { case operator.OpReplica: return c.conf.GetReplicaScheduleLimit() case operator.OpMerge: return c.conf.GetMergeScheduleLimit() case operator.OpAffinity: return c.conf.GetAffinityScheduleLimit() default: + // Unknown kinds return 0, which blocks all such operators. + // This is defensive - all callers should use known kinds with limits. return 0 } }🤖 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 490 - 501, Add a comment to the default case in the getOperatorLimit function that explains why returning 0 is intentional defensive behavior. The comment should clarify that returning 0 will silently block any operator with an unhandled OpKind, and note that this behavior should be considered when adding new operator types in the future to prevent subtle bugs.
🤖 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/checker/checker_controller.go`:
- Around line 490-501: Add a comment to the default case in the getOperatorLimit
function that explains why returning 0 is intentional defensive behavior. The
comment should clarify that returning 0 will silently block any operator with an
unhandled OpKind, and note that this behavior should be considered when adding
new operator types in the future to prevent subtle bugs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e463cd2e-aa38-450e-b0e1-52dd1b762a36
📒 Files selected for processing (2)
pkg/schedule/checker/checker_controller.goserver/cluster/cluster_test.go
ti-chi-bot
Bot
added
release-note
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
patrol-region-worker-count-master-fix
branch
from
7f5eeae to
e48894e
Compare
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
patrol-region-worker-count-master-fix
branch
from
13907a2 to
16c8144
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
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 `@tests/server/apiv2/handlers/affinity_test.go`:
- Around line 142-157: The waitForAffinityGroups helper is using require-based
assertions inside the testutil.Eventually callback, which can abort retries on
transient failures. Update the polling logic in waitForAffinityGroups to use
EventuallyWithT or move the ReadGetJSON/response checks outside the retry
closure so failures are handled without stopping the retry loop. Keep the fix
localized to waitForAffinityGroups and its testutil.Eventually usage.
🪄 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: afcb1072-a6af-4db5-92a8-482c5ade043b
📒 Files selected for processing (1)
tests/server/apiv2/handlers/affinity_test.go
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-2 |
|
/test pull-integration-realcluster-test |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10925 +/- ##
==========================================
+ Coverage 79.15% 79.17% +0.02%
==========================================
Files 540 541 +1
Lines 74801 75753 +952
==========================================
+ Hits 59212 59981 +769
- Misses 11405 11521 +116
- Partials 4184 4251 +67
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>
| if !c.patrolRegionContext.isIdle() { | ||
| continue | ||
| } | ||
| c.metrics.patrolRegionChannelSize.Set(float64(len(c.patrolRegionContext.regionChan))) |
There was a problem hiding this comment.
This metric now effectively only records 0. The code checks isIdle() before setting patrolRegionChannelSize, so by the time we reach this line there should be no queued or in-flight patrol work, and len(regionChan) no longer reflects patrol backlog.
Could we either sample before the idle check, or change the metric to report the pending/in-flight count instead? phaseWaitForChannel also no longer gets observed after this change, so it should either be restored or removed to avoid misleading metrics.
| return &result | ||
| } | ||
|
|
||
| func waitForAffinityGroups(re *require.Assertions, url string, groupIDs ...string) { |
There was a problem hiding this comment.
Could we drop the changes in this file, or rebase and keep the current master version? This PR now has a content conflict with latest master in tests/server/apiv2/handlers/affinity_test.go, and master already contains an equivalent tryGetAffinityGroups / waitForAffinityGroups polling helper.
The affinity test stabilization is also unrelated to the patrol-worker schedule-limit fix, so keeping it in this PR adds conflict surface and review noise.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
| }) | ||
| c.updateTickerIfNeeded(ticker) | ||
| pendingRegionCount := c.patrolRegionContext.pendingRegionCount() | ||
| c.metrics.patrolRegionChannelSize.Set(float64(pendingRegionCount)) |
There was a problem hiding this comment.
This gauge now reports the pending patrol work count (queued plus in-flight regions), not the raw channel length anymore. Could we update the metric help/comment in metrics.go to match the new semantics? Keeping the existing metric name for compatibility seems fine, but the current help text still says "Size of patrol region channel", which is no longer accurate.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
| } | ||
|
|
||
| func (l operatorLimit) reached(opController *operator.Controller) bool { | ||
| return l.getLimit != nil && opController.OperatorCount(l.kind) >= l.getLimit() |
There was a problem hiding this comment.
I think the schedule-limit recheck is still incomplete for merge / affinity-merge candidates.
operatorLimit.reached() only checks OperatorCount(kind) >= limit before admission, but a candidate can contain multiple operators. Both MergeChecker.Check() and affinity merge return a pair, and AddWaitingOperator() can admit both operators together. For example, with merge-schedule-limit = 3 and current OperatorCount(OpMerge) = 2, this check still passes, then the next merge pair is admitted and the final count becomes 4.
This seems within this PR's stated scope because the PR says it re-checks replica, merge, and affinity schedule limits immediately before AddWaitingOperator.
Could we either account for the candidate size when checking the limit, e.g. reject current + candidateCount > limit, or clarify that merge / affinity-merge limits may still exceed the configured limit by one pair? It would also be good to add a regression test for this boundary case.
There was a problem hiding this comment.
Addressed in 8e7ec80 with candidate-count-aware capacity checks for both merge and affinity candidates, plus the odd-limit merge regression test.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/M
| c.RemovePendingProcessedRegion(id) | ||
| return | ||
| } | ||
| c.operatorMu.Lock() |
There was a problem hiding this comment.
The new coarse-grained operatorMu does fix the stale-count race between patrol workers, but I think the merge / affinity-merge boundary case is still not covered.
CheckRegion() still only checks OperatorCount(OpMerge) < GetMergeScheduleLimit() before returning a merge pair. Since a merge candidate contains two operators, merge-schedule-limit = 3 and current OperatorCount(OpMerge) = 2 can still admit another pair and end up with 4 merge operators.
I verified this on the latest head with the following focused test, which fails because the final merge count becomes 4:
package checker
import (
"context"
"testing"
"github.com/stretchr/testify/require"
"github.com/tikv/pd/pkg/mock/mockcluster"
"github.com/tikv/pd/pkg/mock/mockconfig"
"github.com/tikv/pd/pkg/schedule/hbstream"
"github.com/tikv/pd/pkg/schedule/operator"
)
func TestTryAddOperatorsRejectsMergePairPastLimit(t *testing.T) {
re := require.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
opt := mockconfig.NewTestOptions()
tc := mockcluster.NewCluster(ctx, opt)
tc.SetMergeScheduleLimit(3)
tc.SetSplitMergeInterval(0)
for storeID := uint64(1); storeID <= 3; storeID++ {
tc.AddRegionStore(storeID, 0)
}
tc.AddLeaderRegionWithRange(1, "", "a", 1, 2, 3)
tc.AddLeaderRegionWithRange(2, "a", "b", 1, 2, 3)
tc.AddLeaderRegionWithRange(3, "b", "c", 1, 2, 3)
tc.AddLeaderRegionWithRange(4, "c", "d", 1, 2, 3)
stream := hbstream.NewTestHeartbeatStreams(ctx, tc, false)
defer stream.Close()
oc := operator.NewController(ctx, tc.GetBasicCluster(), tc.GetSharedConfig(), stream)
controller := NewController(ctx, tc, tc.GetCheckerConfig(), oc)
controller.tryAddOperators(tc.GetRegion(1))
re.Equal(uint64(2), oc.OperatorCount(operator.OpMerge))
controller.tryAddOperators(tc.GetRegion(3))
re.LessOrEqual(oc.OperatorCount(operator.OpMerge), uint64(3))
}Could we either account for the candidate size when checking merge / affinity limits, or explicitly document that these limits may be exceeded by one pair?
There was a problem hiding this comment.
Fixed in 8e7ec80. Merge and affinity candidates now check their full candidate count against the remaining schedule-limit capacity (candidateCount <= limit - current) before admission, so a pair is rejected when only one slot remains. I added the provided merge boundary regression plus focused capacity cases. Verified with the full checker package, the existing patrol replica-limit regression, focused race tests, and make static.
There was a problem hiding this comment.
I want to clarify my earlier suggestion here. The concern about merge candidates being different from single-operator candidates is still valid, but suggesting a strict current + candidateCount <= limit check was too strict if we want to preserve the existing merge-pair semantics.
The original issue this PR is fixing is the stale-count race between concurrent patrol workers. It is not necessarily trying to change the existing single-threaded behavior where one merge schedule produces two operators. Existing behavior/tests seem to allow one merge pair to be created when merge-schedule-limit = 1, and existing TestPatrolRegionConcurrency uses mergeScheduleLimit = 15 and waits for at least 15 operators.
With the current len(ops)-based capacity check, an odd merge limit can stop below the configured limit. For example, with merge-schedule-limit = 15, the merge checker can stop at 14 because the next merge pair has 2 operators. I verified this on the latest head by running:
make gotest GOTEST_ARGS='./server/cluster -run TestPatrolRegionConcurrency -count=1 -timeout=2m'and TestPatrolRegionConcurrency fails because the len(oc.GetOperators()) >= mergeScheduleLimit condition is never satisfied.
Could we preserve the existing merge-pair semantics while still serializing admission to avoid the concurrent stale-count overshoot? In other words, the admission lock should prevent multiple workers from racing on the same old count, but we probably should not make odd merge limits reject a whole pair before reaching the configured limit.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/L
|
@rleungx: 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 #10926
When
schedule.patrol-region-worker-countis greater than 1, multiple patrol workers can check operator counts concurrently and generate operators before any worker adds its operator to the operator controller. This can make patrol region checking exceed schedule limits such asreplica-schedule-limit.What is changed and how does it work?
This PR:
CheckRegion, then enqueues the generated operators while still holding the same admission mutexreplica-schedule-limitCheck List
Tests
Code changes
Side effects
Related changes
Release note