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

*: let TestEvictLeaderScheduler run in two modes #8663

Merged
merged 3 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions pkg/schedule/schedulers/evict_leader.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,6 @@
return false, errs.ErrScheduleConfigNotExist.FastGenByArgs()
}

func (conf *evictLeaderSchedulerConfig) removeStore(id uint64) {
conf.Lock()
defer conf.Unlock()
// if the store is not existed, no need to resume leader transfer
_, _ = conf.removeStoreLocked(id)
}

func (conf *evictLeaderSchedulerConfig) resetStoreLocked(id uint64, keyRange []core.KeyRange) {
if err := conf.cluster.PauseLeaderTransfer(id); err != nil {
log.Error("pause leader transfer failed", zap.Uint64("store-id", id), errs.ZapError(err))
Expand Down Expand Up @@ -182,6 +175,12 @@
return true, nil
}

func (conf *evictLeaderSchedulerConfig) resumeLeaderTransferIfExist(id uint64) {
Copy link
Contributor

@lhy1024 lhy1024 Sep 24, 2024

Choose a reason for hiding this comment

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

Do we need to cherry pick it to pd-cse?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, only master has the issue.

conf.RLock()
defer conf.RUnlock()
conf.cluster.ResumeLeaderTransfer(id)

Check warning on line 181 in pkg/schedule/schedulers/evict_leader.go

View check run for this annotation

Codecov / codecov/patch

pkg/schedule/schedulers/evict_leader.go#L178-L181

Added lines #L178 - L181 were not covered by tests
}

func (conf *evictLeaderSchedulerConfig) update(id uint64, newRanges []core.KeyRange, batch int) error {
conf.Lock()
defer conf.Unlock()
Expand Down Expand Up @@ -415,7 +414,7 @@
batchFloat, ok := input["batch"].(float64)
if ok {
if batchFloat < 1 || batchFloat > 10 {
handler.config.removeStore(id)
handler.config.resumeLeaderTransferIfExist(id)

Check warning on line 417 in pkg/schedule/schedulers/evict_leader.go

View check run for this annotation

Codecov / codecov/patch

pkg/schedule/schedulers/evict_leader.go#L417

Added line #L417 was not covered by tests
handler.rd.JSON(w, http.StatusBadRequest, "batch is invalid, it should be in [1, 10]")
return
}
Expand All @@ -425,7 +424,7 @@
ranges, ok := (input["ranges"]).([]string)
if ok {
if !inputHasStoreID {
handler.config.removeStore(id)
handler.config.resumeLeaderTransferIfExist(id)

Check warning on line 427 in pkg/schedule/schedulers/evict_leader.go

View check run for this annotation

Codecov / codecov/patch

pkg/schedule/schedulers/evict_leader.go#L427

Added line #L427 was not covered by tests
handler.rd.JSON(w, http.StatusInternalServerError, errs.ErrSchedulerConfig.FastGenByArgs("id"))
return
}
Expand All @@ -435,11 +434,12 @@

newRanges, err = getKeyRanges(ranges)
if err != nil {
handler.config.removeStore(id)
handler.config.resumeLeaderTransferIfExist(id)

Check warning on line 437 in pkg/schedule/schedulers/evict_leader.go

View check run for this annotation

Codecov / codecov/patch

pkg/schedule/schedulers/evict_leader.go#L437

Added line #L437 was not covered by tests
handler.rd.JSON(w, http.StatusInternalServerError, err.Error())
return
}

// StoreIDWithRanges is only changed in update function.
err = handler.config.update(id, newRanges, batch)
if err != nil {
handler.rd.JSON(w, http.StatusInternalServerError, err.Error())
Expand Down
4 changes: 3 additions & 1 deletion pkg/schedule/schedulers/grant_leader.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@

err := handler.config.buildWithArgs(args)
if err != nil {
_, _ = handler.config.removeStore(id)
handler.config.Lock()
handler.config.cluster.ResumeLeaderTransfer(id)
handler.config.Unlock()

Check warning on line 276 in pkg/schedule/schedulers/grant_leader.go

View check run for this annotation

Codecov / codecov/patch

pkg/schedule/schedulers/grant_leader.go#L274-L276

Added lines #L274 - L276 were not covered by tests
handler.rd.JSON(w, http.StatusBadRequest, err.Error())
return
}
Expand Down
2 changes: 1 addition & 1 deletion tests/server/api/region_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (suite *regionTestSuite) TearDownTest() {
return true
})
}
suite.env.RunFuncInTwoModes(cleanFunc)
suite.env.RunTestBasedOnMode(cleanFunc)
}

func (suite *regionTestSuite) TestSplitRegions() {
Expand Down
2 changes: 1 addition & 1 deletion tests/server/api/rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (suite *ruleTestSuite) TearDownTest() {
err = tu.CheckPostJSON(tests.TestDialClient, urlPrefix+"/pd/api/v1/config/placement-rule", data, tu.StatusOK(re))
re.NoError(err)
}
suite.env.RunFuncInTwoModes(cleanFunc)
suite.env.RunTestBasedOnMode(cleanFunc)
}

func (suite *ruleTestSuite) TestSet() {
Expand Down
10 changes: 0 additions & 10 deletions tests/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,16 +371,6 @@ func (s *SchedulingTestEnvironment) RunTestInAPIMode(test func(*TestCluster)) {
test(s.clusters[APIMode])
}

// RunFuncInTwoModes is to run func in two modes.
func (s *SchedulingTestEnvironment) RunFuncInTwoModes(f func(*TestCluster)) {
if c, ok := s.clusters[PDMode]; ok {
f(c)
}
if c, ok := s.clusters[APIMode]; ok {
f(c)
}
}

// Cleanup is to cleanup the environment.
func (s *SchedulingTestEnvironment) Cleanup() {
for _, cluster := range s.clusters {
Expand Down
2 changes: 1 addition & 1 deletion tools/pd-ctl/tests/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (suite *configTestSuite) TearDownTest() {
err = testutil.CheckPostJSON(testDialClient, urlPrefix+"/pd/api/v1/config/placement-rule", data, testutil.StatusOK(re))
re.NoError(err)
}
suite.env.RunFuncInTwoModes(cleanFunc)
suite.env.RunTestBasedOnMode(cleanFunc)
suite.env.Cleanup()
}

Expand Down
2 changes: 1 addition & 1 deletion tools/pd-ctl/tests/hot/hot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (suite *hotTestSuite) TearDownTest() {
}
hotStat.HotCache.CleanCache()
}
suite.env.RunFuncInTwoModes(cleanFunc)
suite.env.RunTestBasedOnMode(cleanFunc)
}

func (suite *hotTestSuite) TestHot() {
Expand Down
16 changes: 10 additions & 6 deletions tools/pd-ctl/tests/scheduler/scheduler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (suite *schedulerTestSuite) TearDownTest() {
}
}
}
suite.env.RunFuncInTwoModes(cleanFunc)
suite.env.RunTestBasedOnMode(cleanFunc)
suite.env.Cleanup()
}

Expand Down Expand Up @@ -816,8 +816,7 @@ func (suite *schedulerTestSuite) checkSchedulerDiagnostic(cluster *pdTests.TestC
}

func (suite *schedulerTestSuite) TestEvictLeaderScheduler() {
// FIXME: API mode may have the problem
suite.env.RunTestInPDMode(suite.checkEvictLeaderScheduler)
suite.env.RunTestBasedOnMode(suite.checkEvictLeaderScheduler)
}

func (suite *schedulerTestSuite) checkEvictLeaderScheduler(cluster *pdTests.TestCluster) {
Expand Down Expand Up @@ -864,9 +863,14 @@ func (suite *schedulerTestSuite) checkEvictLeaderScheduler(cluster *pdTests.Test
output, err = tests.ExecuteCommand(cmd, []string{"-u", pdAddr, "scheduler", "add", "evict-leader-scheduler", "1"}...)
re.NoError(err)
re.Contains(string(output), "Success!")
output, err = tests.ExecuteCommand(cmd, []string{"-u", pdAddr, "scheduler", "remove", "evict-leader-scheduler-1"}...)
re.NoError(err)
re.Contains(string(output), "Success!")
testutil.Eventually(re, func() bool {
output, err = tests.ExecuteCommand(cmd, []string{"-u", pdAddr, "scheduler", "remove", "evict-leader-scheduler-1"}...)
return err == nil && strings.Contains(string(output), "Success!")
})
testutil.Eventually(re, func() bool {
output, err = tests.ExecuteCommand(cmd, []string{"-u", pdAddr, "scheduler", "show"}...)
return err == nil && !strings.Contains(string(output), "evict-leader-scheduler")
})
}

func mustExec(re *require.Assertions, cmd *cobra.Command, args []string, v any) string {
Expand Down