mcs: fix rule sync when meet "no rule left" and concurrency #7481

lhy1024 · 2023-11-29T11:47:37Z

What problem does this PR solve?

Issue Number: Close #7418

What is changed and how does it work?

To address scenarios where deletion precedes an update, we've introduced a pendingDeletion structure. This structure tracks rules or rule groups marked for deletion. If there's an inability to delete immediately due to a lack of rules, it's temporarily stored. Subsequently, during future rule or rule group placement events, the system allows their removal once it becomes feasible due to the availability of required rules.
To solve concurrent issues, I merged two watchers that operate on a shared path but handle rules and groups differently. The new watcher now collectively manages the handling of rules and groups, ensuring coordinated and consistent operations in the face of concurrent access.
add more tests.

Check List

Tests

Unit test
Integration test

Release note

None.

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2023-11-29T11:47:40Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov · 2023-11-29T14:47:19Z

Codecov Report

Merging #7481 (4087916) into master (f51f913) will decrease coverage by 0.01%.
The diff coverage is 81.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7481      +/-   ##
==========================================
- Coverage   74.65%   74.64%   -0.01%     
==========================================
  Files         454      454              
  Lines       50142    50177      +35     
==========================================
+ Hits        37432    37454      +22     
- Misses       9385     9399      +14     
+ Partials     3325     3324       -1

Flag	Coverage Δ
unittests	`74.64% <81.37%> (-0.01%)`	⬇️

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

rleungx · 2023-11-30T02:42:05Z

tests/server/api/rule_test.go

+ env.RunTestInTwoModes(suite.checkDeleteAndUpdate)
+}
+
+func (suite *ruleTestSuite) checkDeleteAndUpdate(cluster *tests.TestCluster) {


Will it fail in the previous implementation?

JmPotato · 2023-11-30T06:11:03Z

pkg/mcs/scheduling/server/apis/v1/api.go

@@ -1330,5 +1330,5 @@ func checkRegionsReplicated(c *gin.Context) {
 c.String(http.StatusBadRequest, err.Error())
 return
 }
- c.String(http.StatusOK, state)
+ c.IndentedJSON(http.StatusOK, state)


What is the reason for this modification? Do we need to modify the corresponding API interface for the non-API mode?

Avoid failure in JSON unmarshalling and maintain consistency with PD mode.

rleungx · 2023-11-30T07:47:46Z

pkg/mcs/scheduling/server/rule/watcher.go

 ruleWatcher *etcdutil.LoopWatcher
 groupWatcher *etcdutil.LoopWatcher


I still worry about whether two watchers will get the event disordered and overwrite the same rule.

I will add a test about it. If necessary, I will merge these watchers.

Signed-off-by: lhy1024 <admin@liudos.us>

pkg/mcs/scheduling/server/rule/watcher.go

Signed-off-by: lhy1024 <admin@liudos.us>

pkg/mcs/scheduling/server/rule/watcher.go

Signed-off-by: lhy1024 <admin@liudos.us>

CabinfeverB

Mostly LGTM. Any manual test?

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2023-12-08T08:32:53Z

PR needs rebase.

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/test-infra repository.

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-12-12T08:54:21Z

tests/server/api/rule_test.go

 return false
 }
- }
- return true
+ log.Info("respBundle", zap.Any("respBundle", respBundle), zap.Any("bundle", bundle))


Will it log too much?

rleungx · 2023-12-12T09:53:58Z

pkg/mcs/scheduling/server/rule/watcher.go

+ rw.pendingDeletion.kvs[path] = [2]string{groupID, ruleID}
+}
+
+func (rw *Watcher) tryFinishPendingDeletion() {


I am worried about if the put and delete can be disordered. If so, the newly added rule might be deleted unexpectedly.

lhy1024 · 2023-12-14T14:47:28Z

We will use replace this pr with txn.

lhy1024 added 2 commits November 29, 2023 18:27

mcs: fix rule sync when meet "no rule left"

7866780

Signed-off-by: lhy1024 <admin@liudos.us>

add some test

10af033

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 requested review from JmPotato, CabinfeverB and rleungx November 29, 2023 11:47

rleungx reviewed Nov 30, 2023

View reviewed changes

JmPotato reviewed Nov 30, 2023

View reviewed changes

rleungx reviewed Nov 30, 2023

View reviewed changes

rleungx mentioned this pull request Nov 30, 2023

Separate scheduling into an independent service #5839

Closed

lhy1024 added 2 commits November 30, 2023 18:44

add TestConcurrency

9e006d3

Signed-off-by: lhy1024 <admin@liudos.us>

merge two watcher

d785df3

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 changed the title ~~mcs: fix rule sync when meet "no rule left"~~ mcs: fix rule sync when meet "no rule left" and concurrency Dec 4, 2023

rleungx reviewed Dec 5, 2023

View reviewed changes

pkg/mcs/scheduling/server/rule/watcher.go Show resolved Hide resolved

lhy1024 added 2 commits December 5, 2023 11:39

Merge branch 'master' into fix-rule-sync

23d6032

fix init

bd5acaa

Signed-off-by: lhy1024 <admin@liudos.us>

CabinfeverB reviewed Dec 6, 2023

View reviewed changes

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

address comments

48afd51

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2023

CabinfeverB reviewed Dec 7, 2023

View reviewed changes

lhy1024 added 2 commits December 7, 2023 10:47

Merge branch 'master' into fix-rule-sync

e2a79c6

fix test

ac72124

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'master' of github.com:tikv/pd into fix-rule-sync

9f72fe8

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2023

Merge branch 'fix-rule-sync' of github.com:lhy1024/pd into fix-rule-sync

8b020e9

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 8, 2023

make test stable

0c70123

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 8, 2023

lhy1024 added 5 commits December 8, 2023 21:36

Merge branch 'master' into fix-rule-sync

89ff1fe

fix data race in test

ff23490

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'fix-rule-sync' of github.com:lhy1024/pd into fix-rule-sync

4702d2a

Merge branch 'master' into fix-rule-sync

13e49f8

Merge branch 'master' into fix-rule-sync

520fbcb

rleungx approved these changes Dec 12, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 12, 2023

rleungx reviewed Dec 12, 2023

View reviewed changes

Merge branch 'master' into fix-rule-sync

4087916

lhy1024 closed this Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: fix rule sync when meet "no rule left" and concurrency #7481

mcs: fix rule sync when meet "no rule left" and concurrency #7481

lhy1024 commented Nov 29, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

rleungx Nov 30, 2023

lhy1024 Nov 30, 2023

JmPotato Nov 30, 2023

lhy1024 Nov 30, 2023

rleungx Nov 30, 2023 •

edited

Loading

lhy1024 Nov 30, 2023

CabinfeverB left a comment

ti-chi-bot bot commented Dec 8, 2023

rleungx Dec 12, 2023

rleungx Dec 12, 2023

lhy1024 commented Dec 14, 2023

		ruleWatcher *etcdutil.LoopWatcher
		groupWatcher *etcdutil.LoopWatcher

mcs: fix rule sync when meet "no rule left" and concurrency #7481

mcs: fix rule sync when meet "no rule left" and concurrency #7481

Conversation

lhy1024 commented Nov 29, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 29, 2023 • edited Loading

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

rleungx Nov 30, 2023

Choose a reason for hiding this comment

lhy1024 Nov 30, 2023

Choose a reason for hiding this comment

JmPotato Nov 30, 2023

Choose a reason for hiding this comment

lhy1024 Nov 30, 2023

Choose a reason for hiding this comment

rleungx Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

lhy1024 Nov 30, 2023

Choose a reason for hiding this comment

CabinfeverB left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 8, 2023

rleungx Dec 12, 2023

Choose a reason for hiding this comment

rleungx Dec 12, 2023

Choose a reason for hiding this comment

lhy1024 commented Dec 14, 2023

lhy1024 commented Nov 29, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

rleungx Nov 30, 2023 •

edited

Loading