affinity: add policy, metrics collection and tests #9999

lhy1024 · 2025-12-04T12:53:36Z

What problem does this PR solve?

Issue Number: Ref #9764

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2025-12-04T12:53:39Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2025-12-04T13:25:08Z

Codecov Report

❌ Patch coverage is 74.28571% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.41%. Comparing base (8c350af) to head (66fb84f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9999      +/-   ##
==========================================
+ Coverage   78.32%   78.41%   +0.09%     
==========================================
  Files         511      511              
  Lines       68323    68419      +96     
==========================================
+ Hits        53514    53651     +137     
+ Misses      10923    10883      -40     
+ Partials     3886     3885       -1

Flag	Coverage Δ
unittests	`78.41% <74.28%> (+0.09%)`	⬆️

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.

Signed-off-by: lhy1024 <[email protected]>

pkg/schedule/affinity/policy.go

liyishuai

Avoid overloading types.
Clarify "kind" instead of "level".

pkg/schedule/affinity/policy.go

ti-chi-bot · 2025-12-09T09:51:51Z

@liyishuai: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Avoid overloading types.
Clarify "kind" instead of "level".

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.

liyishuai

missing part in my previous change request

pkg/schedule/affinity/policy.go

ti-chi-bot · 2025-12-09T09:57:14Z

@liyishuai: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

missing part in my previous change request

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.

lhy1024 · 2025-12-09T13:21:48Z

Avoid overloading types. Clarify "kind" instead of "level".

After discussion with @liyishuai , this is the only place where logEntry is needed, and there are no immediate plans for expansion. If a series of similar entries at the same level are added in the future, we can follow this approach by adding a kind to distinguish them and then move them into logutil.

bufferflies · 2025-12-10T01:59:25Z

pkg/schedule/affinity/policy.go

+func (m *Manager) startAvailabilityCheckLoop() {
+	interval := getAvailabilityCheckInterval()
+	ticker := time.NewTicker(interval)
+	go func() {


Does it need a waitgroup to wait for this goroutine to exit?

This function doesn't perform any critical operations, so could we add a TODO statement for now?

Yes, pls add todolist avoid the running goroutine exist even if the server is closed .

pkg/schedule/affinity/policy.go

bufferflies · 2025-12-10T02:08:43Z

pkg/schedule/affinity/policy.go

+	defer m.RUnlock()
+	for _, storeID := range voterStoreIDs {
+		condition, ok := m.unavailableStores[storeID]
+		if ok && (!condition.affectsLeaderOnly() || storeID == leaderStoreID) {


If store 3 is not the leader but has the evit leader label, it should return an error?

No, that's reasonable. We do not check the evict leader for peers who are not the leader.

pkg/schedule/affinity/policy.go

bufferflies · 2025-12-10T02:17:22Z

pkg/schedule/affinity/policy.go

+	m.RUnlock()
+
+	// Log after releasing lock
+	for _, entry := range logEntries {


If we have 100 nodes that are unavailable, we need to log 100 logs every time.

Logs will only be generated whenever the overall status changes.

We will change the print quantity.

pkg/schedule/affinity/policy.go

okJiang

Can you use switch instead of if-else in this function?

okJiang · 2025-12-10T02:57:46Z

pkg/schedule/affinity/policy.go

+			}
+		}
+		newAvailability := maxCondition.groupAvailability()
+		if newAvailability != groupInfo.GetAvailability() {


Please add some comments to explain why when availability changing, unavailableStore != 0 represents that group has a new unavailableStore, otherwise represents group becomes health.

This contains a great deal of underlying logical relationships. Perhaps modifying the code could make this section simpler and easier to understand.

refactor it

okJiang · 2025-12-10T03:02:23Z

pkg/schedule/affinity/policy.go

+		}
+		newAvailability := maxCondition.groupAvailability()
+		if newAvailability != groupInfo.GetAvailability() {
+			groupAvailabilityChanges[groupInfo.ID] = newAvailability


Changes can contain oldAvailbility and newAvailability. And the log level can be confirmed by compare the oldAvailbility and newAvailability(compare can be wrapped to a function). This makes the code clearer

reduce unnecessary log

pkg/schedule/affinity/manager_test.go

okJiang · 2025-12-10T03:24:58Z

pkg/schedule/affinity/policy.go

+	if !isUnavailableStoresChanged {
+		m.RUnlock()
+		return false, nil


When the store remains unchanged, the group will persist in a degraded state without transitioning to expired.

We always use GetAvailability to retrieve the availability, which contains the relevant processing logic. We don't update it using GetAvailability because it might be called within a read lock.

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot · 2025-12-10T08:01:50Z

[LGTM Timeline notifier]

Timeline:

2025-12-10 07:42:33.545372313 +0000 UTC m=+1026898.359149885: ☑️ agreed by bufferflies.
2025-12-10 08:01:49.223760121 +0000 UTC m=+1028054.037537703: ☑️ agreed by okJiang.

lhy1024 · 2025-12-10T08:06:20Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:15:20Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:25:08Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:29:53Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:34:46Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:40:59Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:44:29Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:47:55Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T08:55:06Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T09:01:32Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T11:05:17Z

/test pull-unit-test-next-gen-2

lhy1024 · 2025-12-10T11:14:32Z

/retest

lhy1024 · 2025-12-10T11:23:17Z

/retest

lhy1024 · 2025-12-10T11:32:20Z

/retest

lhy1024 · 2025-12-10T11:32:42Z

/test pull-integration-realcluster-test

lhy1024 · 2025-12-10T12:05:51Z

/test pull-integration-realcluster-test

ref tikv#9764 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-12-10T13:27:32Z

In response to a cherrypick label: new pull request created to branch release-8.5: #10047.

rleungx · 2025-12-11T07:39:17Z

pkg/schedule/affinity/policy.go

+// which are split into three groups separated by degradedBoundary.
+type storeCondition int
+
+const (


It's better to add a comment about the sequence matter.

rleungx · 2025-12-11T07:40:53Z

pkg/schedule/affinity/policy.go

+
+// GetNewAvailability uses the given unavailableStores to compute a new groupAvailability.
+// Note that this function does not update runtimeGroupInfo.
+func (g *runtimeGroupInfo) GetNewAvailability(unavailableStores map[uint64]storeCondition) groupAvailability {


Can we merge GetNewAvailability and GetAvailability?

rleungx · 2025-12-11T07:41:50Z

pkg/schedule/affinity/policy.go

+// GetNewAvailability uses the given unavailableStores to compute a new groupAvailability.
+// Note that this function does not update runtimeGroupInfo.
+func (g *runtimeGroupInfo) GetNewAvailability(unavailableStores map[uint64]storeCondition) groupAvailability {
+	maxCondition := storeAvailable


maxCondition is not readable

rleungx · 2025-12-11T07:52:13Z

pkg/schedule/affinity/policy.go

+}
+
+// checkStoresAvailability checks the availability status of stores and invalidates groups with unavailable stores.
+func (m *Manager) checkStoresAvailability() {


should be checkGroupAvailability?

rleungx · 2025-12-11T07:52:33Z

pkg/schedule/affinity/policy.go

+	affinityRegionCount.Set(float64(m.affinityRegionCount))
+}
+
+func (m *Manager) generateUnavailableStores() map[uint64]storeCondition {


Suggested change

func (m *Manager) generateUnavailableStores() map[uint64]storeCondition {

func (m *Manager) collectUnavailableStores() map[uint64]storeCondition {

ref tikv#9764 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: 混沌DM <[email protected]>

ref tikv#9764 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: 混沌DM <[email protected]> Signed-off-by: lhy1024 <[email protected]>

lhy1024 mentioned this pull request Dec 4, 2025

Support data affinity schedule for performance optimization #9764

Open

13 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Dec 5, 2025

lhy1024 force-pushed the affinity-pr2C branch from 01afe2f to a463d78 Compare December 9, 2025 07:38

affinity: add policy, metrics collection and tests

6cc401a

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the affinity-pr2C branch from a463d78 to 6cc401a Compare December 9, 2025 07:42

lhy1024 marked this pull request as ready for review December 9, 2025 07:43

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2025

lhy1024 requested review from bufferflies and okJiang December 9, 2025 07:47

liyishuai reviewed Dec 9, 2025

View reviewed changes

pkg/schedule/affinity/policy.go Outdated Show resolved Hide resolved

liyishuai reviewed Dec 9, 2025

View reviewed changes

pkg/schedule/affinity/policy.go Outdated Show resolved Hide resolved

liyishuai suggested changes Dec 9, 2025

View reviewed changes

pkg/schedule/affinity/policy.go Outdated Show resolved Hide resolved

pkg/schedule/affinity/policy.go Outdated Show resolved Hide resolved

liyishuai suggested changes Dec 9, 2025

View reviewed changes

pkg/schedule/affinity/policy.go Outdated Show resolved Hide resolved

bufferflies reviewed Dec 10, 2025

View reviewed changes

okJiang reviewed Dec 10, 2025

View reviewed changes

pkg/schedule/affinity/manager_test.go Outdated Show resolved Hide resolved

okJiang reviewed Dec 10, 2025

View reviewed changes

address comments

544bcfe

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the affinity-pr2C branch from a6bacea to 544bcfe Compare December 10, 2025 04:39

lhy1024 requested review from bufferflies and okJiang December 10, 2025 04:47

Merge branch 'master' of github.com:tikv/pd into affinity-pr2C

66fb84f

ti-chi-bot bot merged commit f0b6d8b into tikv:master Dec 10, 2025
39 of 42 checks passed

lhy1024 deleted the affinity-pr2C branch December 10, 2025 13:27

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Dec 10, 2025

This is an automated cherry-pick of tikv#9999

abdcaf3

ref tikv#9764 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Dec 10, 2025

affinity: add policy, metrics collection and tests (#9999) #10047

Closed

rleungx reviewed Dec 11, 2025

View reviewed changes

HunDunDM added a commit to HunDunDM/pd that referenced this pull request Dec 11, 2025

affinity: add policy, metrics collection and tests (tikv#9999)

d58a060

ref tikv#9764 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: 混沌DM <[email protected]>

	func (m *Manager) generateUnavailableStores() map[uint64]storeCondition {
	func (m *Manager) collectUnavailableStores() map[uint64]storeCondition {

affinity: add policy, metrics collection and tests #9999

affinity: add policy, metrics collection and tests #9999

Uh oh!

Conversation

lhy1024 commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Dec 4, 2025

Uh oh!

codecov bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

liyishuai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot bot commented Dec 9, 2025

Uh oh!

liyishuai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot bot commented Dec 9, 2025

Uh oh!

lhy1024 commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HunDunDM Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

okJiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Dec 10, 2025

[LGTM Timeline notifier]

Uh oh!

lhy1024 commented Dec 10, 2025

Uh oh!

lhy1024 commented Dec 4, 2025 •

edited

Loading

codecov bot commented Dec 4, 2025 •

edited

Loading

HunDunDM Dec 10, 2025 •

edited

Loading