mcs: use a controller to manage scheduling jobs #7270

rleungx · 2023-10-26T10:20:35Z

What problem does this PR solve?

Issue Number: Ref #5839.

What is changed and how does it work?

This PR uses a controller to start/stop the scheduling jobs so that we can dynamically control the scheduling jobs according to if there is a scheduling service.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-10-26T10:20:38Z

[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-10-26T10:20:38Z

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

Signed-off-by: Ryan Leung <[email protected]>

JmPotato · 2023-11-07T07:21:55Z

server/cluster/cluster.go

+			c.enabledServices.Store(mcsutils.SchedulingServiceName, true)
+		} else if !c.schedulingController.running.Load() {
+			c.startSchedulingJobs()
+			c.enabledServices.Delete(mcsutils.SchedulingServiceName)


This enabledServices naming is confusing to me, it seems more like servicesToEnable since we will delete the service name from it after starting.

If the scheduling server is started, the enabledServices will store it, otherwise, it will be deleted in enabledServices.

It is also confusing to me

Any better idea?

How about independentServices? And do we need maintain this in PD mode?

CabinfeverB · 2023-11-07T08:50:14Z

server/server.go

@@ -489,7 +489,7 @@ func (s *Server) startServer(ctx context.Context) error {
 	s.safePointV2Manager = gc.NewSafePointManagerV2(s.ctx, s.storage, s.storage, s.storage)
 	s.hbStreams = hbstream.NewHeartbeatStreams(ctx, s.clusterID, "", s.cluster)
 	// initial hot_region_storage in here.
-	if !s.IsAPIServiceMode() {
+	if !s.IsServiceEnabled(mcs.SchedulingServiceName) {


When running startServer, is the RaftCluter running?

CabinfeverB · 2023-11-07T09:24:30Z

server/cluster/scheduling_controller.go

+	sc.coordinator.Stop()
+	sc.cancel()
+	sc.wg.Wait()
+	sc.running.Store(false)


It's better to use CompareAndSwap to avoid stop more one time.

CabinfeverB · 2023-11-07T09:48:14Z

server/cluster/cluster.go

+	checkFn := func() {
+		if c.isAPIServiceMode {
+			once.Do(c.initSchedulers)
+			c.enabledServices.Store(mcsutils.SchedulingServiceName, true)


As described in this code, scheduling will not start again in api mode. I think we can delete runServiceCheckJob first and put it in the next PR. Because once the PR merged, the scheduling service must be deployed

CabinfeverB · 2023-11-08T07:09:39Z

server/cluster/cluster.go

+			c.enabledServices.Store(mcsutils.SchedulingServiceName, true)
+		} else if !c.schedulingController.running.Load() {
+			c.startSchedulingJobs()
+			c.enabledServices.Delete(mcsutils.SchedulingServiceName)


How about independentServices? And do we need maintain this in PD mode?

server/server.go

Signed-off-by: Ryan Leung <[email protected]>

CabinfeverB

Rest LGTM

CabinfeverB · 2023-11-09T05:34:20Z

server/cluster/scheduling_controller.go

+	sc.mu.Lock()
+	defer sc.mu.Unlock()
+	if sc.running {
+		log.Warn("scheduling service is already running")


It will print frequently

Signed-off-by: Ryan Leung <[email protected]>

rleungx · 2023-11-09T08:19:45Z

PTAL @JmPotato @lhy1024 @nolouch

lhy1024 · 2023-11-09T08:28:15Z

server/cluster/cluster.go

-func (c *RaftCluster) GetPausedSchedulerDelayUntil(name string) (int64, error) {
-	return c.coordinator.GetSchedulersController().GetPausedSchedulerDelayUntil(name)
+// IsServiceIndependent returns whether the service is independent.
+func (c *RaftCluster) IsServiceIndependent(name string) bool {


Do we need to add a comment to indicate what service to be supported? only scheduling?

Maybe later, right now, we just have a scheduling service.

server/cluster/scheduling_controller.go

nolouch

lgtm

nolouch · 2023-11-10T09:53:43Z

/merge

ti-chi-bot · 2023-11-10T09:53:45Z

@nolouch: 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-10T09:53:47Z

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

Commit hash: e807c69

ti-chi-bot · 2023-11-10T09:59:25Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

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.

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.

ref tikv#5839 Signed-off-by: Ryan Leung <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 26, 2023

ti-chi-bot bot requested review from disksing and HunDunDM October 26, 2023 10:20

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 26, 2023

rleungx force-pushed the dynamic-enable-scheduling branch 3 times, most recently from 212f0df to 1684a14 Compare November 2, 2023 05:07

rleungx changed the title ~~mcs: dynamically enable scheduling service~~ mcs: use a controller to manage scheduling jobs Nov 2, 2023

rleungx marked this pull request as ready for review November 2, 2023 05:08

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Nov 2, 2023

rleungx requested review from JmPotato, nolouch, lhy1024 and CabinfeverB and removed request for disksing and HunDunDM November 3, 2023 03:24

rleungx force-pushed the dynamic-enable-scheduling branch from 6cdff51 to 85b941c Compare November 7, 2023 06:43

rleungx added 3 commits November 7, 2023 14:47

dynamically enable scheduling service

f4db0b4

Signed-off-by: Ryan Leung <[email protected]>

extract scheduling control flow from cluster

737536a

Signed-off-by: Ryan Leung <[email protected]>

fix tests

36bc79d

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the dynamic-enable-scheduling branch from 85b941c to 36bc79d Compare November 7, 2023 06:47

clean up

be68446

Signed-off-by: Ryan Leung <[email protected]>

JmPotato reviewed Nov 7, 2023

View reviewed changes

rleungx force-pushed the dynamic-enable-scheduling branch from 31eee0e to 2d6d331 Compare November 7, 2023 08:35

CabinfeverB reviewed Nov 7, 2023

View reviewed changes

CabinfeverB reviewed Nov 8, 2023

View reviewed changes

rleungx force-pushed the dynamic-enable-scheduling branch from 1f37ab8 to e2aee75 Compare November 9, 2023 04:05

address comments

2ce4ff0

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the dynamic-enable-scheduling branch from e2aee75 to 2ce4ff0 Compare November 9, 2023 04:12

use internal lock

81bdba6

Signed-off-by: Ryan Leung <[email protected]>

CabinfeverB reviewed Nov 9, 2023

View reviewed changes

rleungx added 2 commits November 9, 2023 14:13

remove log

705f429

Signed-off-by: Ryan Leung <[email protected]>

Merge branch 'master' into dynamic-enable-scheduling

e807c69

CabinfeverB approved these changes Nov 9, 2023

View reviewed changes

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

lhy1024 approved these changes Nov 9, 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 9, 2023

nolouch approved these changes Nov 10, 2023

View reviewed changes

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

ti-chi-bot bot added 2 commits November 10, 2023 09:53

Merge branch 'master' into dynamic-enable-scheduling

69a46dc

Merge branch 'master' into dynamic-enable-scheduling

f539c11

ti-chi-bot bot merged commit 1a0233b into tikv:master Nov 10, 2023

rleungx deleted the dynamic-enable-scheduling branch November 13, 2023 02:08

rleungx mentioned this pull request Nov 30, 2023

Separate scheduling into an independent service #5839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: use a controller to manage scheduling jobs #7270

mcs: use a controller to manage scheduling jobs #7270

rleungx commented Oct 26, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 26, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 26, 2023

JmPotato Nov 7, 2023

rleungx Nov 7, 2023

CabinfeverB Nov 7, 2023

rleungx Nov 7, 2023

CabinfeverB Nov 8, 2023

CabinfeverB Nov 7, 2023

rleungx Nov 7, 2023

CabinfeverB Nov 7, 2023

CabinfeverB Nov 7, 2023

rleungx Nov 7, 2023

CabinfeverB Nov 8, 2023

CabinfeverB left a comment

CabinfeverB Nov 9, 2023

rleungx commented Nov 9, 2023

lhy1024 Nov 9, 2023

rleungx Nov 9, 2023

nolouch left a comment

nolouch commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

mcs: use a controller to manage scheduling jobs #7270

mcs: use a controller to manage scheduling jobs #7270

Conversation

rleungx commented Oct 26, 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 Oct 26, 2023 • edited Loading

ti-chi-bot bot commented Oct 26, 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

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

CabinfeverB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

rleungx commented Oct 26, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 26, 2023 •

edited

Loading