schedule, server: fix replicated state semantics - #11056
Conversation
|
[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 |
|
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:
📝 WalkthroughWalkthroughChangesPlacement state reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CheckRegionsReplicated
participant RuleChecker
participant ReplicaStrategy
Client->>CheckRegionsReplicated: Request replicated status
CheckRegionsReplicated->>CheckRegionsReplicated: Validate range coverage
CheckRegionsReplicated->>RuleChecker: Evaluate placement state
RuleChecker->>ReplicaStrategy: Find eligible placement candidates
ReplicaStrategy-->>RuleChecker: Candidate stores
RuleChecker-->>CheckRegionsReplicated: Placement state
CheckRegionsReplicated-->>Client: Return API status
Possibly related issues
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (7)
pkg/schedule/checker/rule_checker_state.go (1)
152-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
contextparameter.
contextshadows the stdlib package name and reads like acontext.Context, which it isn't. The sibling methods already usectx *regionPlacementStateContext; something likestateCtx(or matchingctx) keeps it consistent and unambiguous.🤖 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/rule_checker_state.go` around lines 152 - 155, Rename the context parameter in RuleChecker.evaluateRegionPlacementState from context to ctx or stateCtx, and update all references within the method body to match. Keep its type and behavior unchanged, aligning with the sibling methods’ naming convention.server/api/region.go (1)
139-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegenerate the Swagger spec after changing these annotations. Both endpoints'
@Summarytext changed, so the committed spec is now stale.
server/api/region.go#L139-L139: runmake swagger-spec(SWAGGER=1) and commit the regenerated output for this summary change.pkg/mcs/scheduling/server/apis/v1/api.go#L1469-L1469: ensure the scheduling microservice spec is regenerated with the same wording.As per coding guidelines: "Regenerate Swagger spec with
make swagger-spec(SWAGGER=1) when API annotations change".🤖 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/api/region.go` at line 139, Regenerate and commit the Swagger specifications after the summary annotation changes: run make swagger-spec with SWAGGER=1 for server/api/region.go:139-139 and update the generated output, ensuring pkg/mcs/scheduling/server/apis/v1/api.go:1469-1469 reflects the same wording; no direct source change is required beyond the annotation updates.Source: Coding guidelines
pkg/schedule/checker/rule_checker_state_test.go (1)
445-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCryptic zone labels in the benchmark.
string(rune(i))yields control characters and makes the intentional collision between store 1 and store 1001 hard to see.fmt.Sprintf("z%d", i)(with"z1"for 1001) would express the intent directly.🤖 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/rule_checker_state_test.go` around lines 445 - 451, Update the benchmark setup around AddLabelsStore to use readable zone labels formatted as “z” plus the store ID, and assign store 1001 the intentional colliding label “z1” instead of converting IDs to runes. Preserve the existing store and leader-region relationships.pkg/schedule/placement/fit.go (1)
152-166: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueExported method can panic on nil
Storesentries.
Storesmay containnil(e.g.cloneRegionFitWithStoresinrule_manager.gofills viagetStoreByID, which can miss). The currentRuleCheckercaller pre-validates non-nil stores, but this is an exported method; a cheap guard makes it robust.🛡️ Proposed guard
highestLevel := f.Rule.LocationLabels[0] for i, store := range f.Stores { + if store == nil { + continue + } labelValue := store.GetLabelValue(highestLevel) for _, other := range f.Stores[i+1:] { - if labelValue == other.GetLabelValue(highestLevel) { + if other != nil && labelValue == other.GetLabelValue(highestLevel) { return false } } }🤖 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/fit.go` around lines 152 - 166, Update RuleFit.IsolatedAtHighestLevel to safely handle nil entries in f.Stores before calling GetLabelValue; return a non-panicking result consistent with invalid or incomplete store data, while preserving the existing label and peer-count behavior for valid stores.pkg/schedule/placement/rule_manager_test.go (1)
511-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertion can pass vacuously.
If no
RuleFitpeer matchesrefreshedStore, the loop body never executes and the test still passes. Track a flag and require it.♻️ Proposed tightening
readFit = manager.FitRegionForRead(stores, region) + checked := false for _, ruleFit := range readFit.RuleFits { for i, peer := range ruleFit.Peers { if peer.GetStoreId() == refreshedStore.GetID() { re.Same(refreshedStore, ruleFit.Stores[i]) + checked = true } } } + re.True(checked)🤖 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 511 - 518, Update the assertions in the FitRegionForRead test around readFit.RuleFits to track whether any peer matches refreshedStore.GetID(), require that a match was found after the loops, and retain the existing re.Same assertion for the corresponding ruleFit.Stores entry.pkg/schedule/checker/replica_strategy.go (1)
240-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
selectStoreToRemoveForStateduplicatesselectStoreToRemoveWithTempStatesemantics.Two independent implementations of "which store would be removed" will drift; the parity test only pins two scenarios. Consider extracting the shared selection into one helper used by both the operator path and the read-only state path (e.g. a function returning permissive/strict candidates that the existing method wraps).
🤖 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/replica_strategy.go` around lines 240 - 281, Unify the duplicate store-selection logic used by selectStoreToRemoveForState and selectStoreToRemoveWithTempState by extracting their shared permissive/strict candidate selection into one helper. Update both callers to use that helper while preserving each method’s existing return behavior and filtering semantics.pkg/schedule/handler/handler.go (1)
1316-1335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a GoDoc comment for the range-coverage contract.
The empty-end-key semantics (unbounded range requires an unbounded last region) are subtle enough to deserve a one-line comment above the helper.
🤖 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/handler/handler.go` around lines 1316 - 1335, Add a concise GoDoc comment immediately above regionsCoverRange documenting its range-coverage contract, including that an empty endKey denotes an unbounded range and therefore requires the final region to have an empty end key.
🤖 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/replica_strategy.go`:
- Around line 240-281: Unify the duplicate store-selection logic used by
selectStoreToRemoveForState and selectStoreToRemoveWithTempState by extracting
their shared permissive/strict candidate selection into one helper. Update both
callers to use that helper while preserving each method’s existing return
behavior and filtering semantics.
In `@pkg/schedule/checker/rule_checker_state_test.go`:
- Around line 445-451: Update the benchmark setup around AddLabelsStore to use
readable zone labels formatted as “z” plus the store ID, and assign store 1001
the intentional colliding label “z1” instead of converting IDs to runes.
Preserve the existing store and leader-region relationships.
In `@pkg/schedule/checker/rule_checker_state.go`:
- Around line 152-155: Rename the context parameter in
RuleChecker.evaluateRegionPlacementState from context to ctx or stateCtx, and
update all references within the method body to match. Keep its type and
behavior unchanged, aligning with the sibling methods’ naming convention.
In `@pkg/schedule/handler/handler.go`:
- Around line 1316-1335: Add a concise GoDoc comment immediately above
regionsCoverRange documenting its range-coverage contract, including that an
empty endKey denotes an unbounded range and therefore requires the final region
to have an empty end key.
In `@pkg/schedule/placement/fit.go`:
- Around line 152-166: Update RuleFit.IsolatedAtHighestLevel to safely handle
nil entries in f.Stores before calling GetLabelValue; return a non-panicking
result consistent with invalid or incomplete store data, while preserving the
existing label and peer-count behavior for valid stores.
In `@pkg/schedule/placement/rule_manager_test.go`:
- Around line 511-518: Update the assertions in the FitRegionForRead test around
readFit.RuleFits to track whether any peer matches refreshedStore.GetID(),
require that a match was found after the loops, and retain the existing re.Same
assertion for the corresponding ruleFit.Stores entry.
In `@server/api/region.go`:
- Line 139: Regenerate and commit the Swagger specifications after the summary
annotation changes: run make swagger-spec with SWAGGER=1 for
server/api/region.go:139-139 and update the generated output, ensuring
pkg/mcs/scheduling/server/apis/v1/api.go:1469-1469 reflects the same wording; no
direct source change is required beyond the annotation updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad920e4c-eed3-4e1b-bbe4-6a2191ce00e9
📒 Files selected for processing (12)
pkg/mcs/scheduling/server/apis/v1/api.gopkg/schedule/checker/replica_strategy.gopkg/schedule/checker/rule_checker_state.gopkg/schedule/checker/rule_checker_state_test.gopkg/schedule/handler/handler.gopkg/schedule/handler/handler_test.gopkg/schedule/placement/fit.gopkg/schedule/placement/fit_test.gopkg/schedule/placement/rule_manager.gopkg/schedule/placement/rule_manager_test.goserver/api/region.gotests/server/api/region_test.go
|
/retest |
Treat the RuleChecker pending list as a hint and revalidate missing placement-rule peers against the current Store topology. Report PENDING only when no add or swap target exists; otherwise report INPROGRESS. Also avoid reporting uncovered Region ranges as replicated. Signed-off-by: Ryan Leung <rleungx@gmail.com>
5dca009 to
9a4df95
Compare
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/handler/handler.go`:
- Around line 1304-1318: The topology-based PENDING classification in
pkg/schedule/handler/handler.go lines 1304-1318 must evaluate
GetRuleChecker().GetRegionPlacementState(region) independently of
co.IsPendingRegion; retain completed-Region scan avoidance through a separate
fast path. Add a test in tests/server/api/region_test.go lines 211-240 with
mockPending false or disabled and a no-legal-target offline peer, asserting the
region is reported as PENDING.
🪄 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: 341f8b5f-36f6-461d-a653-a8198a71ac6e
📒 Files selected for processing (6)
pkg/mcs/scheduling/server/apis/v1/api.gopkg/schedule/checker/rule_checker.gopkg/schedule/checker/rule_checker_test.gopkg/schedule/handler/handler.goserver/api/region.gotests/server/api/region_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/region.go
- pkg/mcs/scheduling/server/apis/v1/api.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11056 +/- ##
==========================================
+ Coverage 79.23% 79.32% +0.08%
==========================================
Files 540 541 +1
Lines 76010 76635 +625
==========================================
+ Hits 60226 60788 +562
- Misses 11533 11569 +36
- Partials 4251 4278 +27
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>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/schedule/handler/handler.go (1)
1305-1319: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPending classification is still gated by RuleChecker's internal pending-list cache.
Regions where
co.IsPendingRegionreturnsfalsebypassGetRegionsPlacementStateentirely and fall back to the oldfilter.IsRegionReplicatedcheck, which cannot reportPENDING. So a Region with a genuinely unfixable placement problem that RuleChecker's patrol hasn't yet flagged as pending will be reportedINPROGRESSinstead ofPENDING, contradicting the objective thatPENDINGshould be based on the current topology, not on RuleChecker's cache membership.This mirrors the concern raised on an earlier commit of this PR: evaluate placement feasibility from the current topology independently of
co.IsPendingRegion, and use the pending-list only as an optimization to skip already-known-fine Regions (e.g., short-circuit only afterfilter.IsRegionReplicated/rule-fit succeeds).🤖 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/handler/handler.go` around lines 1305 - 1319, Update the placement-state logic around co.IsPendingRegion so PENDING classification is determined from current topology feasibility rather than requiring cached pending membership. Use co.IsPendingRegion only as an optimization for already-known-fine regions, while ensuring regions that fail filter.IsRegionReplicated or the relevant rule-fit check are evaluated and reported as PENDING when unfixable; preserve the existing INPROGRESS behavior for regions with recoverable placement work.
🤖 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.
Duplicate comments:
In `@pkg/schedule/handler/handler.go`:
- Around line 1305-1319: Update the placement-state logic around
co.IsPendingRegion so PENDING classification is determined from current topology
feasibility rather than requiring cached pending membership. Use
co.IsPendingRegion only as an optimization for already-known-fine regions, while
ensuring regions that fail filter.IsRegionReplicated or the relevant rule-fit
check are evaluated and reported as PENDING when unfixable; preserve the
existing INPROGRESS behavior for regions with recoverable placement work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28e0f013-6f89-4062-a4b8-86e33dde6992
📒 Files selected for processing (5)
pkg/schedule/checker/checker_controller.gopkg/schedule/checker/replica_strategy.gopkg/schedule/checker/rule_checker.gopkg/schedule/checker/rule_checker_test.gopkg/schedule/handler/handler.go
Evaluate placement states without relying on the RuleChecker pending LRU, distinguish retryable work from unschedulable topology, and keep completed-range checks on the fast path. Reuse Store candidates across incomplete Regions and cover active replica operators and missing Region metadata. Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/schedule/operator/waiting_operator.go (1)
162-178: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNil the vacated slot before truncating.
The swap-remove leaves the removed operator referenced by the backing array past
len(ops), keeping it (and its region snapshot) alive. The bucket code inGetOperatorexplicitly nils entries for this reason.♻️ Proposed change
ops[i] = ops[len(ops)-1] + ops[len(ops)-1] = nil // avoid memory leak ops = ops[:len(ops)-1] break🤖 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/waiting_operator.go` around lines 162 - 178, Update randBuckets.removeOperatorFromRegion to clear the vacated final slot in ops before truncating the slice, matching the cleanup behavior used by GetOperator. Preserve the existing swap-remove logic and region-map deletion behavior.pkg/schedule/checker/checker_controller.go (1)
269-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso drop the pending entry for vanished Regions.
A merged/removed Region keeps its ID in
pendingProcessedRegionsuntil TTL expiry, andevaluateRegionPlacementStatetreats membership asInProgress. Removing it here matches the new "remove before check" policy.♻️ Proposed change
if region == nil { + c.RemovePendingProcessedRegion(id) continue }🤖 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 269 - 274, Update the region iteration in the checker controller so a nil or vanished region also removes its ID from pendingProcessedRegions before continuing. Apply RemovePendingProcessedRegion(id) before the region == nil branch, while preserving the existing removal-before-check behavior for non-nil regions.tests/server/api/region_test.go (1)
219-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
testutil.Eventuallyfor these status assertions, and document the restore block.Every other
/regions/replicatedassertion in this test polls; these two read once right after mutating store state throughPutStoreon both the PD and scheduling clusters, which is more exposed to propagation timing in the scheduling-server variant. Lines 236-240 also silently reset store 1 to serving and store 2 to offline so the laterPENDINGexpectation holds — worth a one-line comment.🤖 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/region_test.go` around lines 219 - 240, Update the two status checks around the mutations to targetStore and availableTarget in the replicated-region test to use testutil.Eventually, polling until the expected PENDING and INPROGRESS values are observed. Add a concise comment before the final putStore calls explaining that they restore store 1 to serving and store 2 to offline for the subsequent PENDING assertion.pkg/schedule/checker/rule_checker.go (1)
317-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
hasOrphanPeerActionduplicatesfixOrphanPeerspolicy; consider a shared decision helper.This re-implements the orphan-handling decision tree (first-orphan preference, pin-down replacement, joint-consensus role rules, extra-count heuristics) that
fixOrphanPeersencodes. Any future change to one side silently desynchronizes the reported placement state from what the checker actually does. Extracting a common "which orphan action applies" step that both the operator path and the state path consume would keep them in lockstep.🤖 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/rule_checker.go` around lines 317 - 421, The orphan-handling policy is duplicated between hasOrphanPeerAction and fixOrphanPeers, risking inconsistent checker results and operator behavior. Extract a shared decision helper that determines the applicable orphan action, including first-orphan selection, pin-down replacement, role handling, joint-consensus checks, and extra-count heuristics; update both hasOrphanPeerAction and fixOrphanPeers to consume that helper while preserving their existing outcomes.
🤖 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 269-274: Update the region iteration in the checker controller so
a nil or vanished region also removes its ID from pendingProcessedRegions before
continuing. Apply RemovePendingProcessedRegion(id) before the region == nil
branch, while preserving the existing removal-before-check behavior for non-nil
regions.
In `@pkg/schedule/checker/rule_checker.go`:
- Around line 317-421: The orphan-handling policy is duplicated between
hasOrphanPeerAction and fixOrphanPeers, risking inconsistent checker results and
operator behavior. Extract a shared decision helper that determines the
applicable orphan action, including first-orphan selection, pin-down
replacement, role handling, joint-consensus checks, and extra-count heuristics;
update both hasOrphanPeerAction and fixOrphanPeers to consume that helper while
preserving their existing outcomes.
In `@pkg/schedule/operator/waiting_operator.go`:
- Around line 162-178: Update randBuckets.removeOperatorFromRegion to clear the
vacated final slot in ops before truncating the slice, matching the cleanup
behavior used by GetOperator. Preserve the existing swap-remove logic and
region-map deletion behavior.
In `@tests/server/api/region_test.go`:
- Around line 219-240: Update the two status checks around the mutations to
targetStore and availableTarget in the replicated-region test to use
testutil.Eventually, polling until the expected PENDING and INPROGRESS values
are observed. Add a concise comment before the final putStore calls explaining
that they restore store 1 to serving and store 2 to offline for the subsequent
PENDING assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90aec105-7972-48c4-ada0-50c2f55eaaed
📒 Files selected for processing (8)
pkg/schedule/checker/checker_controller.gopkg/schedule/checker/rule_checker.gopkg/schedule/checker/rule_checker_test.gopkg/schedule/handler/handler.gopkg/schedule/operator/operator_controller.gopkg/schedule/operator/waiting_operator.gopkg/schedule/operator/waiting_operator_test.gotests/server/api/region_test.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@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. |
What problem does this PR solve?
The
/regions/replicatedendpoint treats the RuleChecker pending list as thefinal state. That list records a failed target selection and can become stale
after the Store topology changes, so schedulable work may still be reported as
PENDING. It can also reportREPLICATEDwhen Region metadata does not coverthe requested range.
Issue Number: Close #11055
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
reuse the Store list within the range check and stop candidate scans at the
first legal target.
waiting-operator index.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
/regions/replicatedto match the refined status meanings.Tests