config: persist default store limit for future stores - #10900
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughThis PR adds persisted schedule-level default store limits. Store-limit read and write paths now use the persisted defaults. Tests cover adjustment, migration, reload, restart, per-store overrides, and future-store initialization. ChangesPersisted default store limit flow
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
|
Hi @King-Dylan. 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. |
ti-chi-bot
Bot
added
the
size/L
b2c0925 to
62c341f
Compare
ti-chi-bot
Bot
added
dco-signoff: yes
ti-chi-bot
Bot
removed
do-not-merge/work-in-progress
![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
🤖 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/config/config.go`:
- Around line 633-641: The GetDefaultStoreLimit method treats any zero value as
unset and replaces it with process defaults, which breaks persistence of
explicitly configured zero values across restart. Replace the zero-value checks
for AddPeer and RemovePeer fields with a proper mechanism to distinguish between
explicitly set zero values and truly unset values (such as using pointers to the
fields or a different storage representation), so that persisted zero values are
respected and not overwritten by process defaults.
🪄 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: cfc073c5-d240-4769-8674-03ba34dcad72
📒 Files selected for processing (7)
pkg/mcs/scheduling/server/config/config.gopkg/mcs/scheduling/server/config/config_test.gopkg/schedule/config/config.goserver/cluster/cluster.goserver/cluster/cluster_test.goserver/config/config_test.goserver/config/persist_options.go
5e152da to
6f224a3
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/config/config.go`:
- Around line 472-475: The migration logic in config.go currently lets
StoreBalanceRate overwrite an explicitly configured
DefaultStoreLimit/DefaultStoreLimitConfig, which clobbers user-specified
add-peer/remove-peer values. Update the backfill path around StoreBalanceRate
handling so it only populates DefaultStoreLimit when the corresponding
default-store-limit fields are unset, or add explicit validation to reject mixed
configuration; use the existing config migration logic in the affected blocks
near the StoreBalanceRate assignments and the
DefaultStoreLimit/DefaultStoreLimitConfig fields to preserve explicit values.
🪄 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: 2749e8c6-be72-4a19-b949-c6a6e8ff6353
📒 Files selected for processing (7)
pkg/mcs/scheduling/server/config/config.gopkg/mcs/scheduling/server/config/config_test.gopkg/schedule/config/config.goserver/cluster/cluster.goserver/cluster/cluster_test.goserver/config/config_test.goserver/config/persist_options.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/config/persist_options.go
- pkg/mcs/scheduling/server/config/config.go
- server/cluster/cluster.go
- server/cluster/cluster_test.go
Close tikv#10898 Signed-off-by: King-Dylan <702299521@qq.com>
6f224a3 to
9a3351f
Compare
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/schedule/config/config.go (1)
509-511: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't use a zero
StoreLimitConfigas the nil-meta sentinel.Line 509 still conflates an explicit
default-store-limit = {add-peer: 0, remove-peer: 0}with “unset”. WhenMigrateDeprecatedFlagshits this nil-metadata path on Line 571,store-balance-ratewill overwrite that explicit default and violate the new precedence contract. This branch needs presence tracking (or mixed-config rejection) too.Also applies to: 571-571
🤖 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/config/config.go` around lines 509 - 511, The DefaultStoreLimit handling in config migration is using an all-zero StoreLimitConfig as the unset sentinel, which incorrectly treats an explicit default-store-limit of zeros as missing. Update the logic in the config migration path around DefaultStoreLimit and MigrateDeprecatedFlags to track whether the field was actually present (or reject mixed configs) instead of relying on zero-value comparison, so store-balance-rate does not overwrite an explicit default and the precedence rules remain intact.
🧹 Nitpick comments (1)
pkg/mcs/scheduling/server/config/config_test.go (1)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRound-trip the config for the restart assertion.
Line 46 says this simulates a restarted process, but Lines 47-49 still read from the same
persistConfiginstance. That only proves the in-memory precedence order; it will not catch a persistence/reload regression indefault-store-limit. RebuildPersistConfigfrom persisted state before the last assertion so this test actually covers the restart contract from the PR objective.🤖 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/mcs/scheduling/server/config/config_test.go` around lines 46 - 49, The restart assertion in the config test is still using the same in-memory PersistConfig instance, so it does not verify the persisted round-trip behavior. Rebuild a new PersistConfig from the saved state before the final GetStoreLimit assertion, using the existing DefaultStoreLimit setup to simulate a real process restart. Keep the assertion against StoreLimitConfig but make it exercise the reload path rather than only the precedence order.
🤖 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/config/config.go`:
- Around line 509-511: The DefaultStoreLimit handling in config migration is
using an all-zero StoreLimitConfig as the unset sentinel, which incorrectly
treats an explicit default-store-limit of zeros as missing. Update the logic in
the config migration path around DefaultStoreLimit and MigrateDeprecatedFlags to
track whether the field was actually present (or reject mixed configs) instead
of relying on zero-value comparison, so store-balance-rate does not overwrite an
explicit default and the precedence rules remain intact.
---
Nitpick comments:
In `@pkg/mcs/scheduling/server/config/config_test.go`:
- Around line 46-49: The restart assertion in the config test is still using the
same in-memory PersistConfig instance, so it does not verify the persisted
round-trip behavior. Rebuild a new PersistConfig from the saved state before the
final GetStoreLimit assertion, using the existing DefaultStoreLimit setup to
simulate a real process restart. Keep the assertion against StoreLimitConfig but
make it exercise the reload path rather than only the precedence order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c6f7271-2f64-4a84-b26c-57c8b9f93e4b
📒 Files selected for processing (7)
pkg/mcs/scheduling/server/config/config.gopkg/mcs/scheduling/server/config/config_test.gopkg/schedule/config/config.goserver/cluster/cluster.goserver/cluster/cluster_test.goserver/config/config_test.goserver/config/persist_options.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/mcs/scheduling/server/config/config.go
- server/cluster/cluster_test.go
- server/cluster/cluster.go
- server/config/persist_options.go
|
@King-Dylan: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bufferflies 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 |
[LGTM Timeline notifier]Timeline:
|
…re-limit Signed-off-by: King-Dylan <702299521@qq.com> # Conflicts: # pkg/mcs/scheduling/server/config/config_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/mcs/scheduling/server/config/config_test.go (1)
123-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse a distinct process default in the reload assertion.
The test persists
RemovePeeras15, then sets the process default to15before reading store 2. Ifdefault-store-limit.remove-peeris omitted or ignored during reload, the test still passes through the process default fallback. Set the process default to a different value after marshaling, while keeping the expected reloaded value at15.Proposed test adjustment
- sc.DefaultStoreLimit.SetDefaultStoreLimit(storelimit.RemovePeer, 15) + sc.DefaultStoreLimit.SetDefaultStoreLimit(storelimit.RemovePeer, 25)This validates the persisted-default-before-process-default precedence required by the PR objectives.
🤖 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/mcs/scheduling/server/config/config_test.go` at line 123, Update the reload assertion setup around SetDefaultStoreLimit so the process-level RemovePeer default is changed to a value different from the persisted 15 after marshaling. Keep the expected reloaded store-2 value at 15, ensuring the assertion verifies persisted configuration precedence over the process default.
🤖 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/mcs/scheduling/server/config/config_test.go`:
- Line 123: Update the reload assertion setup around SetDefaultStoreLimit so the
process-level RemovePeer default is changed to a value different from the
persisted 15 after marshaling. Keep the expected reloaded store-2 value at 15,
ensuring the assertion verifies persisted configuration precedence over the
process default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 780b37df-7e20-4fab-b382-79195b103b50
📒 Files selected for processing (5)
pkg/mcs/scheduling/server/config/config.gopkg/mcs/scheduling/server/config/config_test.goserver/cluster/cluster.goserver/cluster/cluster_test.goserver/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/cluster/cluster_test.go
- pkg/mcs/scheduling/server/config/config.go
- server/config/config_test.go
| AddPeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.AddPeer), | ||
| RemovePeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.RemovePeer), | ||
| } | ||
| limitCfg := cfg.GetDefaultStoreLimit() |
There was a problem hiding this comment.
Config entries written by pre-upgrade PD do not contain default-store-limit; the MCS watcher unmarshals those entries into a zero-valued ScheduleConfig, so this fallback installs {0,0} for an unlisted store. In store-limit v1, zero is treated as unlimited, so a new store whose per-store entry has not been persisted can bypass add/remove-peer throttling after an upgrade.
There was a problem hiding this comment.
Fixed in 96d4b03. Persisted JSON now tracks default-store-limit field presence, and the MCS watcher initializes and migrates the watched schedule config before installing it. Legacy configs without the field now use the built-in default (or store-balance-rate when present), while explicit zero values remain unchanged. TestAdjustScheduleConfigDefaultStoreLimit covers these upgrade cases and the future-store lookup.
| defaultStoreLimit := StoreLimitConfig{AddPeer: c.StoreBalanceRate, RemovePeer: c.StoreBalanceRate} | ||
| DefaultStoreLimit = StoreLimit{AddPeer: defaultStoreLimit.AddPeer, RemovePeer: defaultStoreLimit.RemovePeer} | ||
| if defaultStoreLimitMeta == nil { | ||
| if c.DefaultStoreLimit == (StoreLimitConfig{}) { |
There was a problem hiding this comment.
During reload of a legacy config that still contains store-balance-rate, Adjust(nil, true) has already filled DefaultStoreLimit with 15 before JSON unmarshalling, so this zero-value check never migrates the legacy rate. The deprecated field is then cleared and future stores silently fall back to 15 instead of the persisted rate.
There was a problem hiding this comment.
Fixed in 96d4b03. The persisted-config migration now uses per-field JSON presence instead of a zero-value sentinel, with precedence default-store-limit > store-balance-rate > process default. TestReloadLegacyStoreBalanceRate exercises the real Adjust -> JSON unmarshal -> migrate reload path and verifies that the legacy rate is retained for future stores.
Signed-off-by: King-Dylan <702299521@qq.com>
Signed-off-by: King-Dylan <702299521@qq.com>
3 participants
What problem does this PR solve?
Issue Number: close #10898
Persist the default store limit used for future stores. Today
/pd/api/v1/stores/limitupdates existing stores and mutates the process-local default store limit, but the default for future stores is not persisted. After PD/API/scheduling service restart or leader/primary switch, newly joined TiKV stores can fall back to the built-in default value.What is changed and how does it work?
schedule.default-store-limittoScheduleConfig.SetAllStoresLimitto persist the default limit as well as updating existing per-store entries.schedule.default-store-limitbefore falling back to the process default.schedule.store-limit[storeID]as the highest-priority override.PersistConfig.The resulting priority is:
schedule.store-limit[storeID]schedule.default-store-limitCheck List
Tests
Also attempted the existing MCS integration store-limit test:
It failed during build because dashboard embedded assets were not generated in this checkout:
Code changes
Side effects
Release note
Summary by CodeRabbit
Release Notes