mcs: dynamic enable scheduling jobs #7325

rleungx · 2023-11-06T10:13:10Z

What problem does this PR solve?

Issue Number: Ref #5839. Also close #7375.

What is changed and how does it work?

This PR supports dynamic enable/disable scheduling service.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-11-06T10:13:12Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
lhy1024

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.

ti-chi-bot · 2023-11-06T10:13:12Z

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

pkg/schedule/schedulers/scheduler_controller.go

server/api/middleware.go

CabinfeverB · 2023-11-15T05:16:36Z

pkg/schedule/schedulers/scheduler_controller.go

 	schedulerStatusGauge.Reset()
 	ruleStatusGauge.Reset()
 	// create in map again
-	rulesCntStatusGauge = ruleStatusGauge.WithLabelValues("rule_count")
-	groupsCntStatusGauge = ruleStatusGauge.WithLabelValues("group_count")
+	ruleStatusGauge.WithLabelValues("rule_count").Set(0)


Why Set(0) after Reset

server/cluster/cluster.go

CabinfeverB · 2023-11-15T07:21:17Z

tests/server/cluster/cluster_test.go

@@ -1562,6 +1562,7 @@ func TestTransferLeaderBack(t *testing.T) {
 	svr := leaderServer.GetServer()
 	rc := cluster.NewRaftCluster(ctx, svr.ClusterID(), syncer.NewRegionSyncer(svr), svr.GetClient(), svr.GetHTTPClient())
 	rc.InitCluster(svr.GetAllocator(), svr.GetPersistOptions(), svr.GetStorage(), svr.GetBasicCluster(), svr.GetHBStreams(), svr.GetKeyspaceGroupManager())
+	rc.SchedulingController = cluster.NewSchedulingController(ctx, rc.GetBasicCluster(), rc.GetOpts(), rc.GetRuleManager())


How about putting the initialization of SchedulingController into InitCluster? This way we can avoid one less function call and avoid forgetting

It doesn't init RuleManager.

Also put the initialization of RuleManager into InitCluster?

We can remove it now

Signed-off-by: Ryan Leung <rleungx@gmail.com>

codecov · 2023-11-16T06:04:50Z

Codecov Report

Merging #7325 (013577b) into master (dda748a) will decrease coverage by 0.09%.
The diff coverage is 88.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7325      +/-   ##
==========================================
- Coverage   74.27%   74.19%   -0.09%     
==========================================
  Files         451      451              
  Lines       48967    49044      +77     
==========================================
+ Hits        36372    36387      +15     
- Misses       9375     9441      +66     
+ Partials     3220     3216       -4

Flag	Coverage Δ
unittests	`74.19% <88.97%> (-0.09%)`	⬇️

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

CabinfeverB

rest LGTM!

CabinfeverB · 2023-11-16T06:48:48Z

tests/server/cluster/cluster_test.go

@@ -1562,6 +1562,7 @@ func TestTransferLeaderBack(t *testing.T) {
 	svr := leaderServer.GetServer()
 	rc := cluster.NewRaftCluster(ctx, svr.ClusterID(), syncer.NewRegionSyncer(svr), svr.GetClient(), svr.GetHTTPClient())
 	rc.InitCluster(svr.GetAllocator(), svr.GetPersistOptions(), svr.GetStorage(), svr.GetBasicCluster(), svr.GetHBStreams(), svr.GetKeyspaceGroupManager())
+	rc.SchedulingController = cluster.NewSchedulingController(ctx, rc.GetBasicCluster(), rc.GetOpts(), rc.GetRuleManager())


We can remove it now

CabinfeverB · 2023-11-16T06:56:31Z

server/cluster/scheduling_controller.go

 )

-type schedulingController struct {
+// SchedulingController is used to manage all schedulers and checkers.
+type SchedulingController struct {


And maybe we can keep unexported :)

CabinfeverB · 2023-11-16T06:56:42Z

server/cluster/scheduling_controller.go

@@ -52,26 +56,23 @@ type schedulingController struct {
 	running     bool
 }

-func newSchedulingController(parentCtx context.Context) *schedulingController {
+// NewSchedulingController creates a new scheduling controller.
+func NewSchedulingController(parentCtx context.Context, basicCluster *core.BasicCluster, opt sc.ConfProvider, ruleManager *placement.RuleManager) *SchedulingController {


Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024

LGTM. Do we need a manual test for it?

rleungx · 2023-11-16T09:43:26Z

LGTM. Do we need a manual test for it?

Yes, will do more tests later.

rleungx · 2023-11-17T08:14:46Z

@nolouch PTAL

rleungx · 2023-11-20T03:09:32Z

/merge

ti-chi-bot · 2023-11-20T03:09:34Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-11-20T03:09:36Z

This pull request has been accepted and is ready to merge.

Commit hash: 013577b

ref tikv#5839, close tikv#7375 Signed-off-by: Ryan Leung <rleungx@gmail.com>

ti-chi-bot bot requested review from disksing and Yisaer November 6, 2023 10:13

rleungx force-pushed the dynamic-scheduling branch 2 times, most recently from 65f4a45 to 9ca0d80 Compare November 14, 2023 04:15

rleungx marked this pull request as ready for review November 14, 2023 04:15

rleungx force-pushed the dynamic-scheduling branch 2 times, most recently from 7c6b012 to 1271686 Compare November 15, 2023 02:54

rleungx requested review from nolouch, lhy1024 and CabinfeverB and removed request for disksing and Yisaer November 15, 2023 02:54

lhy1024 reviewed Nov 15, 2023

View reviewed changes

pkg/schedule/schedulers/scheduler_controller.go Outdated Show resolved Hide resolved

CabinfeverB reviewed Nov 15, 2023

View reviewed changes

rleungx added 3 commits November 16, 2023 13:30

dynamically enable scheduling service

af02296

Signed-off-by: Ryan Leung <rleungx@gmail.com>

extract scheduling control flow from cluster

95ffe81

Signed-off-by: Ryan Leung <rleungx@gmail.com>

fix tests

214b898

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx added 7 commits November 16, 2023 13:30

add test

25fe502

Signed-off-by: Ryan Leung <rleungx@gmail.com>

resolve conflicts

55357d8

Signed-off-by: Ryan Leung <rleungx@gmail.com>

improve the tests

8c09994

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address the comment

41477cf

Signed-off-by: Ryan Leung <rleungx@gmail.com>

fix test

69fbc80

Signed-off-by: Ryan Leung <rleungx@gmail.com>

fix test

2572a3a

Signed-off-by: Ryan Leung <rleungx@gmail.com>

clean up

fce7646

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the dynamic-scheduling branch from 54af163 to fce7646 Compare November 16, 2023 05:42

CabinfeverB approved these changes Nov 16, 2023

View reviewed changes

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

address comments

e643d7d

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the dynamic-scheduling branch from 677c6a3 to e643d7d Compare November 16, 2023 08:56

lhy1024 approved these changes Nov 16, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 16, 2023

rleungx added 3 commits November 16, 2023 17:43

Merge branch 'master' into dynamic-scheduling

cf1d140

Merge branch 'master' into dynamic-scheduling

dcd34a0

Merge branch 'master' into dynamic-scheduling

013577b

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 20, 2023

ti-chi-bot bot merged commit 9845c12 into tikv:master Nov 20, 2023
26 checks passed

rleungx deleted the dynamic-scheduling branch November 20, 2023 03:14

rleungx mentioned this pull request Nov 30, 2023

Separate scheduling into an independent service #5839

Closed

rleungx added a commit to rleungx/pd that referenced this pull request Dec 1, 2023

mcs: dynamic enable scheduling jobs (tikv#7325)

61adf27

ref tikv#5839, close tikv#7375 Signed-off-by: Ryan Leung <rleungx@gmail.com>

CabinfeverB mentioned this pull request Feb 9, 2024

PD collects region stats in API mode #7816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: dynamic enable scheduling jobs #7325

mcs: dynamic enable scheduling jobs #7325

rleungx commented Nov 6, 2023 •

edited

Loading

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

edited

Loading

ti-chi-bot bot commented Nov 6, 2023

CabinfeverB Nov 15, 2023

CabinfeverB Nov 15, 2023

rleungx Nov 15, 2023

CabinfeverB Nov 15, 2023

rleungx Nov 16, 2023

CabinfeverB Nov 16, 2023

codecov bot commented Nov 16, 2023 •

edited

Loading

CabinfeverB left a comment

CabinfeverB Nov 16, 2023

CabinfeverB Nov 16, 2023

CabinfeverB Nov 16, 2023

lhy1024 left a comment

rleungx commented Nov 16, 2023

rleungx commented Nov 17, 2023

rleungx commented Nov 20, 2023

ti-chi-bot bot commented Nov 20, 2023

ti-chi-bot bot commented Nov 20, 2023

mcs: dynamic enable scheduling jobs #7325

mcs: dynamic enable scheduling jobs #7325

Conversation

rleungx commented Nov 6, 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 6, 2023 • edited Loading

ti-chi-bot bot commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

CabinfeverB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

rleungx commented Nov 16, 2023

rleungx commented Nov 17, 2023

rleungx commented Nov 20, 2023

ti-chi-bot bot commented Nov 20, 2023

ti-chi-bot bot commented Nov 20, 2023

rleungx commented Nov 6, 2023 •

edited

Loading

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

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading