Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

schedule: impl balance range scheduler #9005

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

bufferflies
Copy link
Contributor

@bufferflies bufferflies commented Jan 17, 2025

What problem does this PR solve?

Issue Number: Close #9006

What is changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

Release note

None.

Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Copy link
Contributor

ti-chi-bot bot commented Jan 17, 2025

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

@ti-chi-bot 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. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jan 17, 2025
Copy link
Contributor

ti-chi-bot bot commented Jan 17, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from bufferflies, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Jan 17, 2025
@ti-chi-bot 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 Jan 17, 2025
@bufferflies bufferflies force-pushed the balance_key_range_scheduler branch from b5cb26c to 50d905f Compare February 13, 2025 08:31
@bufferflies bufferflies force-pushed the balance_key_range_scheduler branch 7 times, most recently from 0125dd5 to 120a2bf Compare February 13, 2025 11:46
@bufferflies
Copy link
Contributor Author

/retest

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 73.71795% with 82 lines in your changes missing coverage. Please review.

Project coverage is 76.25%. Comparing base (4eb7235) to head (246aaa6).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9005      +/-   ##
==========================================
- Coverage   76.28%   76.25%   -0.03%     
==========================================
  Files         468      468              
  Lines       71422    71844     +422     
==========================================
+ Hits        54484    54785     +301     
- Misses      13538    13630      +92     
- Partials     3400     3429      +29     
Flag Coverage Δ
unittests 76.25% <73.71%> (-0.03%) ⬇️

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

@bufferflies bufferflies marked this pull request as ready for review February 14, 2025 02:05
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2025
@bufferflies
Copy link
Contributor Author

/test pull-integration-realcluster-test

Copy link
Contributor

ti-chi-bot bot commented Feb 17, 2025

@bufferflies: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test pull-integration-realcluster-test

The following commands are available to trigger optional jobs:

/debug pull-unit-test
/test pull-integration-copr-test

Use /test all to run the following jobs that were automatically triggered:

tikv/pd/ghpr_build
tikv/pd/pull_integration_realcluster_test

In response to this:

/test pull-integration-realcluster-test

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-sigs/prow repository.

Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies force-pushed the balance_key_range_scheduler branch from 120a2bf to 2854acd Compare February 17, 2025 08:54
pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved

func (s *balanceRangeScheduler) prepare(cluster sche.SchedulerCluster, opInfluence operator.OpInfluence, job *balanceRangeSchedulerJob) (*balanceRangeSchedulerPlan, error) {
krs := core.NewKeyRanges(job.Ranges)
scanRegions, err := cluster.BatchScanRegions(krs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to separate krs into multiple batches if it is a big range?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to separate the ranges by many times because the duration of the lock is the same

Copy link
Member

@rleungx rleungx Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we acquire the lock multiple times and for every time we hold the lock for a short time, the other places can also get the lock. But if we hold the lock for a long time, it may cause starvation in other places.

pkg/schedule/schedulers/balance_range.go Show resolved Hide resolved
}

opInfluence := s.OpController.GetOpInfluence(cluster.GetBasicCluster(), operator.WithRangeOption(job.Ranges))
plan, err := s.prepare(cluster, opInfluence, job)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For every time we schedule, it will scan the range from the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the info on the regions and stores in the given ranges may be updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it affect the other requests like get region or heartbeat if the range is large?

Copy link
Contributor Author

@bufferflies bufferflies Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the operator of scanning regions acquires Rlock which shares with the region heartbeat. We could reuse the previous distribution by interval duration, not every scheduler.

conf.RLock()
defer conf.RLock()
for _, job := range conf.jobs {
if job.Status == finished {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we change the status to finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

Copy link
Member

@rleungx rleungx Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all ranges are balanced, do we still need to wait fa or timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes in this pr. I will add other conditions to check whether the distribution is balanced in next PR.

pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved
pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved
pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved
pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved
pkg/schedule/operator/operator_controller.go Outdated Show resolved Hide resolved
pkg/schedule/schedulers/balance_range.go Outdated Show resolved Hide resolved
func (s *balanceRangeScheduler) transferPeer(plan *balanceRangeSchedulerPlan, dstStores []*core.StoreInfo) *operator.Operator {
excludeTargets := plan.region.GetStoreIDs()
if plan.job.Role == leader {
excludeTargets = make(map[uint64]struct{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to exclude the current leader store id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

@bufferflies bufferflies force-pushed the balance_key_range_scheduler branch from 1628d72 to 40f0f56 Compare February 18, 2025 09:44
Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies force-pushed the balance_key_range_scheduler branch from 40f0f56 to 246aaa6 Compare February 19, 2025 02:20
Copy link
Contributor

ti-chi-bot bot commented Feb 19, 2025

@bufferflies: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-integration-realcluster-test 246aaa6 link true /test pull-integration-realcluster-test

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@bufferflies bufferflies requested a review from nolouch February 19, 2025 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has signed the dco. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheduler: implement balance range scheduler algorithm
2 participants