cluster: Scheduler Dry Run mechanism #5283

CabinfeverB · 2022-07-07T06:29:21Z

What problem does this PR solve?

Issue Number: ref #5257

What is changed and how does it work?

add a new struct named diagnosisManager and coordinator have it. diagnosisManagerwill share scheduler with coordinator.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2022-07-07T06:29:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2022-07-07T06:48:45Z

Codecov Report

Merging #5283 (6b5cd2b) into master (3a51a52) will increase coverage by 0.05%.
The diff coverage is 82.14%.

❗ Current head 6b5cd2b differs from pull request most recent head a30b24a. Consider uploading reports for the commit a30b24a to get more accurate results

@@            Coverage Diff             @@
##           master    #5283      +/-   ##
==========================================
+ Coverage   75.77%   75.82%   +0.05%     
==========================================
  Files         311      310       -1     
  Lines       30781    30805      +24     
==========================================
+ Hits        23324    23358      +34     
+ Misses       5463     5455       -8     
+ Partials     1994     1992       -2

Flag	Coverage Δ
unittests	`75.82% <82.14%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
server/cluster/coordinator.go	`72.49% <82.14%> (+1.64%)`	⬆️
server/tso/local_allocator.go	`71.62% <0.00%> (-6.76%)`	⬇️
pkg/etcdutil/etcdutil.go	`84.88% <0.00%> (-3.49%)`	⬇️
server/schedule/labeler/rules.go	`87.50% <0.00%> (-2.28%)`	⬇️
server/config/persist_options.go	`93.03% <0.00%> (-1.05%)`	⬇️
server/election/leadership.go	`77.31% <0.00%> (-1.04%)`	⬇️
server/cluster/cluster.go	`83.54% <0.00%> (-0.92%)`	⬇️
server/server.go	`74.13% <0.00%> (ø)`
server/schedule/region_scatterer.go	`79.83% <0.00%> (ø)`
server/schedule/plan/status.go
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a51a52...a30b24a. Read the comment docs.

rleungx · 2022-07-07T10:05:42Z

server/cluster/coordinator.go

+type diagnosisManager struct {
+	cluster        *RaftCluster
+	schedulers     map[string]*scheduleController
+	enableDiganose map[string]bool


What is it used for?

rleungx · 2022-07-07T10:06:46Z

server/cluster/coordinator.go

+	cluster        *RaftCluster
+	schedulers     map[string]*scheduleController
+	enableDiganose map[string]bool
+	dryRunResult   map[string]*diagnosisResult


I think the results can be a slice. The diagnosisResult should have a timestamp.

rleungx · 2022-07-07T10:07:27Z

server/cluster/coordinator.go

+
+type diagnosisResult struct {
+	result []plan.Plan
+	ops    []*operator.Operator


Do we need ops?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-11T02:56:22Z

server/cluster/coordinator.go

+	plans           []plan.Plan
+	schedulablePlan []plan.Plan


What is the difference between schedulablePlan and plans?

I change the name

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-12T05:42:15Z

server/cluster/coordinator.go

+
+func (d *diagnosisManager) diagnosisDryRun(name string) error {
+	if _, ok := d.schedulers[name]; !ok {
+		return errors.Errorf("no scheduler named %s", name)


How about:

Suggested change

return errors.Errorf("no scheduler named %s", name)

return errs.ErrSchedulerNotFound.FastGenByArgs()

rleungx

The rest LGTM.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-13T06:56:58Z

/merge

ti-chi-bot · 2022-07-13T06:56:59Z

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

/run-all-tests

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 · 2022-07-13T06:57:01Z

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

Commit hash: 6b5cd2b

ti-chi-bot · 2022-07-13T06:57:14Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

ti-chi-bot added do-not-merge/needs-linked-issue release-note The PR should write the release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 7, 2022

ti-chi-bot requested review from disksing and nolouch July 7, 2022 06:29

diagnosis dry run

45dafdb

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB force-pushed the diagnose_dryrun branch from 4a6a6c5 to 45dafdb Compare July 7, 2022 06:40

CabinfeverB requested a review from rleungx July 7, 2022 10:06

rleungx reviewed Jul 7, 2022

View reviewed changes

CabinfeverB changed the title ~~Diagnose dryrun~~ cluster: Scheduler Dry Run mechanism Jul 8, 2022

diagnosis dry run

9e47b10

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jul 11, 2022

View reviewed changes

diagnosis dry run

9879d1b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added release-note-none and removed do-not-merge/needs-linked-issue release-note The PR should write the release note. labels Jul 11, 2022

CabinfeverB marked this pull request as ready for review July 11, 2022 03:32

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

CabinfeverB requested review from AndreMouche and removed request for disksing July 11, 2022 05:46

rleungx reviewed Jul 12, 2022

View reviewed changes

rleungx approved these changes Jul 12, 2022

View reviewed changes

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

nolouch approved these changes Jul 12, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 12, 2022

address the comment

6b5cd2b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 13, 2022

Merge branch 'master' into diagnose_dryrun

a30b24a

ti-chi-bot merged commit 1f5dcb3 into tikv:master Jul 13, 2022

rleungx mentioned this pull request Jul 14, 2022

Tracking issue for the scheduling diagnosis #5257

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: Scheduler Dry Run mechanism #5283

cluster: Scheduler Dry Run mechanism #5283

CabinfeverB commented Jul 7, 2022 •

edited

ti-chi-bot commented Jul 7, 2022 •

edited

codecov bot commented Jul 7, 2022 •

edited

rleungx Jul 7, 2022

rleungx Jul 7, 2022

rleungx Jul 7, 2022

rleungx Jul 11, 2022

CabinfeverB Jul 11, 2022

rleungx Jul 12, 2022

rleungx left a comment

rleungx commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

	return errors.Errorf("no scheduler named %s", name)
	return errs.ErrSchedulerNotFound.FastGenByArgs()

cluster: Scheduler Dry Run mechanism #5283

cluster: Scheduler Dry Run mechanism #5283

Conversation

CabinfeverB commented Jul 7, 2022 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Jul 7, 2022 • edited

codecov bot commented Jul 7, 2022 • edited

Codecov Report

rleungx Jul 7, 2022

Choose a reason for hiding this comment

rleungx Jul 7, 2022

Choose a reason for hiding this comment

rleungx Jul 7, 2022

Choose a reason for hiding this comment

rleungx Jul 11, 2022

Choose a reason for hiding this comment

CabinfeverB Jul 11, 2022

Choose a reason for hiding this comment

rleungx Jul 12, 2022

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

rleungx commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

CabinfeverB commented Jul 7, 2022 •

edited

ti-chi-bot commented Jul 7, 2022 •

edited

codecov bot commented Jul 7, 2022 •

edited