mcs: support scheduler config forward and enable some tests #7256

lhy1024 · 2023-10-25T16:46:10Z

What problem does this PR solve?

Issue Number: Ref #5839

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

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

ti-chi-bot · 2023-10-25T16:46:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
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.

lhy1024 · 2023-10-25T16:46:53Z

tests/pdctl/scheduler/scheduler_test.go

@@ -160,8 +169,11 @@ func (suite *schedulerTestSuite) checkScheduler(cluster *tests.TestCluster) {
 		checkSchedulerCommand(args, expected)

 		// check update success
-		expectedConfig["store-id-ranges"] = map[string]interface{}{"2": []interface{}{map[string]interface{}{"end-key": "", "start-key": ""}}, "3": []interface{}{map[string]interface{}{"end-key": "", "start-key": ""}}}
-		checkSchedulerConfigCommand(expectedConfig, schedulers[idx])
+		// FIXME: remove this check after scheduler config is updated


But scheduler handler in scheduling server cannot update config, such as grant-leader-scheduler, grant-hot-region-scheduler, balance-hot-region-scheduler, evict-slow-store-scheduler

lhy1024 · 2023-10-25T16:47:07Z

pkg/schedule/handler/handler.go

-		return schedulers, nil
+		// The default scheduler could not be deleted, it could only be disabled.
+		// TODO: Should we distinguish between disabled and removed schedulers?
+		var enabledSchedulers []string


Should we distinguish between disabled and removed schedulers?

After discussion, we will not distinguish them.

…region-scheduler, evict-slow-store-scheduler Signed-off-by: lhy1024 <[email protected]>

codecov · 2023-10-25T18:43:52Z

Codecov Report

Merging #7256 (5d2cc9c) into master (ded917b) will decrease coverage by 0.04%.
The diff coverage is 59.77%.

@@            Coverage Diff             @@
##           master    #7256      +/-   ##
==========================================
- Coverage   74.42%   74.38%   -0.04%     
==========================================
  Files         446      446              
  Lines       48288    48339      +51     
==========================================
+ Hits        35939    35958      +19     
- Misses       9187     9192       +5     
- Partials     3162     3189      +27

Flag	Coverage Δ
unittests	`74.38% <59.77%> (-0.04%)`	⬇️

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

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

rleungx · 2023-10-30T04:56:31Z

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

 	router.GET("/diagnostic/:name", getDiagnosticResult)
+	router.GET("/config/:name/:suffix", getSchedulerConfigByName)
 	// TODO: in the future, we should split pauseOrResumeScheduler to two different APIs.
 	// And we need to do one-to-two forwarding in the API middleware.
 	router.POST("/:name", pauseOrResumeScheduler)


How about unifying these formats?

How about

router.GET("", getSchedulers) router.GET("/:name", getDiagnosticResult) router.POST("/:name", pauseOrResumeScheduler) router.GET("/config/:name/:suffix", getSchedulerConfigByName)

How about using
router.GET("", getSchedulers)
router.GET("/:name/diagnose", getDiagnosticResult)
router.POST("/:name", pauseOrResumeScheduler)
router.GET("/:name/config", getSchedulerConfigByName)

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

rleungx · 2023-10-31T09:09:23Z

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

+	router.GET("/config/:name/list", getSchedulerConfigByName)
+	router.GET("/config/:name/roles", getSchedulerConfigByName) // compatibility for shuffle-region-scheduler


Do we need these two?

Yes, otherwise we'll modify shuffle region scheduler

unify to "/list" and use new api in pd-ctl

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

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

lhy1024 · 2023-11-01T02:28:16Z

@rleungx @JmPotato ptal

JmPotato · 2023-11-01T05:41:49Z

/merge

ti-chi-bot · 2023-11-01T05:41:51Z

@JmPotato: 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-01T05:41:53Z

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

Commit hash: 71da057

ti-chi-bot · 2023-11-01T05:42:06Z

@lhy1024: 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: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

mcs: support scheduler config and enable some tests

14e2985

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

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 25, 2023

ti-chi-bot bot requested review from nolouch and rleungx October 25, 2023 16:46

lhy1024 changed the title ~~mcs: support scheduler config and enable some tests~~ mcs: support scheduler config forward and enable some tests Oct 25, 2023

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 25, 2023

lhy1024 commented Oct 25, 2023

View reviewed changes

skip grant-leader-scheduler, grant-hot-region-scheduler, balance-hot-…

2ff4473

…region-scheduler, evict-slow-store-scheduler Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2023

lhy1024 mentioned this pull request Oct 25, 2023

mcs: some schedulers cannot sync config #7257

Closed

lhy1024 requested a review from JmPotato October 27, 2023 02:39

lhy1024 mentioned this pull request Oct 27, 2023

mcs: forward request to scheduling server #7178

Closed

25 tasks

lhy1024 added 2 commits October 30, 2023 11:34

Merge branch 'master' of github.com:tikv/pd into sche-redirect7

2f5ca61

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

make test stable

77c19d4

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

lhy1024 force-pushed the sche-redirect7 branch from a231728 to 77c19d4 Compare October 30, 2023 03:43

rleungx reviewed Oct 30, 2023

View reviewed changes

address comments

d7adde1

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

lhy1024 force-pushed the sche-redirect7 branch from 83d8c48 to d7adde1 Compare October 30, 2023 07:04

lhy1024 requested a review from rleungx October 30, 2023 08:33

add all scheduler config support

fe028b6

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

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 Oct 31, 2023

rleungx reviewed Oct 31, 2023

View reviewed changes

address comments and make test stable

71da057

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

rleungx approved these changes Nov 1, 2023

View reviewed changes

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

JmPotato approved these changes Nov 1, 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 1, 2023

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

Merge branch 'master' into sche-redirect7

5d2cc9c

ti-chi-bot bot merged commit 4e45e95 into tikv:master Nov 1, 2023
21 of 23 checks passed

lhy1024 deleted the sche-redirect7 branch November 1, 2023 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: support scheduler config forward and enable some tests #7256

mcs: support scheduler config forward and enable some tests #7256

lhy1024 commented Oct 25, 2023

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

edited

Loading

lhy1024 Oct 25, 2023 •

edited

Loading

lhy1024 Oct 27, 2023

lhy1024 Oct 25, 2023

lhy1024 Oct 27, 2023

codecov bot commented Oct 25, 2023 •

edited

Loading

rleungx Oct 30, 2023

lhy1024 Oct 30, 2023

rleungx Oct 30, 2023

rleungx Oct 31, 2023

lhy1024 Oct 31, 2023

lhy1024 Oct 31, 2023

lhy1024 commented Nov 1, 2023

JmPotato commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

		router.GET("/config/:name/list", getSchedulerConfigByName)
		router.GET("/config/:name/roles", getSchedulerConfigByName) // compatibility for shuffle-region-scheduler

mcs: support scheduler config forward and enable some tests #7256

mcs: support scheduler config forward and enable some tests #7256

Conversation

lhy1024 commented Oct 25, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Oct 25, 2023 • edited Loading

lhy1024 Oct 25, 2023 • edited Loading

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 Oct 25, 2023 • edited Loading

Codecov Report

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

lhy1024 commented Nov 1, 2023

JmPotato commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

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

edited

Loading

lhy1024 Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading