Skip to content

schedule: preserve isolation across target layouts - #11284

Draft
YuhaoZhang00 wants to merge 1 commit into
tikv:masterfrom
YuhaoZhang00:fix/placement-isolation-level-20260918
Draft

YuhaoZhang00 wants to merge 1 commit into
tikv:masterfrom
YuhaoZhang00:fix/placement-isolation-level-20260918

Conversation

@YuhaoZhang00

@YuhaoZhang00 YuhaoZhang00 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #11278, ref #11275

A higher weighted isolation score does not prove that a configured IsolationLevel remains satisfied. With LocationLabels=[zone,host] and host isolation, a four-peer move can increase the score from 303 to 401 while placing two peers on the same host. Automatic merge can also align a valid source Region to a target that is fully replicated but violates its isolation requirement.

The scope also includes already-degraded sources tracked in #11278: A/a, A/a, A/b, A/c, B/d can become A/a, A/a, A/b, B/d, B/d, increasing the score from 405 to 602 and same-host peer pairs from one to two. This case is not fixed by the current draft: the peer-move guard below only preserves an already-satisfied isolation level.

What is changed and how does it work?

Add RuleFit.IsIsolationSatisfied to check each pair of stores against the
configured hierarchy prefix, using the existing location comparison.

Preserve an already-satisfied isolation level in RegionFit.Replace,
independently of the weighted score. Leave unset isolation levels and
incremental repair of already-degraded layouts under existing safeguards.

Reject automatic merge targets that violate configured isolation levels
before aligning the source Region to the target's peers.

Isolation is evaluated per effective rule. The change adds no default host requirement or cross-rule host-concentration policy. The accumulated-layout fix in #11204 may merge first. This PR owns the subsequent isolation-validation integration with scatter.

Check List

Tests

  • 303→401 is rejected with host isolation and remains eligible without it; a legal alternative still passes.
  • Tests cover configured topology levels, hierarchy-relative host names, missing/case-different labels, singleton rules, degraded-layout repair and automatic merge.
  • make gotest PACKAGES='./pkg/schedule/placement ./pkg/schedule/filter ./pkg/schedule/checker' GOTEST_ARGS='./pkg/schedule/placement ./pkg/schedule/filter ./pkg/schedule/checker -count=1': passed.
  • Full pkg/schedule/schedulers tests: passed on retry. The first run failed the existing TestEvictSlowTrendBasicFuncs timestamp-order assertion; no placement code is involved in that assertion.
  • Scoped make check for the touched packages: passed.

Before ready for review

  • Extend configured-isolation protection to already-degraded sources while permitting incremental repair. Evaluate per-rule co-located peer-pair counts and maximum domain occupancy; add the 405→602 regression and non-regressing/improving cases. Resolve missing labels, rule reassignment, final versus temporary layouts and replica-count-changing repair paths before broad reuse. This work remains unimplemented.
  • Integrate final isolation validation into scatter after scatter: check peer targets against the updated region layout #11204, including per-rule IsolationScore non-regression and the rules-disabled pairwise distinct-score comparison. These complete-target checks are not implemented in this draft.
  • Carry over scatter regressions for configured/unset isolation, per-rule grouping, learners, degraded layouts and metadata consistency. Preserve established missing-label semantics. Label-case isolation statistics are handled independently in statistics: compare isolation labels case-insensitively #11294 and are not a prerequisite for this PR.
  • Coordinate target-layout inputs and fit reuse with schedule: validate complete roles when selecting a leader #11283. Candidate checks and complete-target checks remain distinct; no default host or cross-rule P/M policy is introduced.
  • Validate caller and performance boundaries together with the shared role-matching work. Candidate checks remain scoped to the source rule; combined layouts require their own final validation.

Release note

Fix peer moves and automatic merges that could violate a configured placement-rule isolation level.

Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-triage-completed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. labels Sep 18, 2026
@ti-chi-bot

ti-chi-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign qiuyesuifeng for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Sep 18, 2026
@ti-chi-bot

ti-chi-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Hi @YuhaoZhang00. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.85%. Comparing base (7655706) to head (bbf4cd0).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11284      +/-   ##
==========================================
+ Coverage   79.78%   79.85%   +0.06%     
==========================================
  Files         546      546              
  Lines       79537    79565      +28     
==========================================
+ Hits        63461    63535      +74     
+ Misses      11690    11667      -23     
+ Partials     4386     4363      -23     
Flag Coverage Δ
unittests 79.85% <84.61%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YuhaoZhang00 YuhaoZhang00 changed the title schedule: preserve configured placement isolation levels schedule: preserve isolation across target layouts Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/needs-triage-completed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant