scheduler: skip evict-leader-scheduler when setting schedule deny label #8303

okJiang · 2024-06-17T09:50:52Z

What problem does this PR solve?

Issue Number: Close #7853 ref #7300

What is changed and how does it work?

- add a real cluster test to test `skip evict-leader-scheduler when setting schedule deny label`
- add `DeleteStoreLabel` API and `DeleteScheduler` API

Check List

Tests

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

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: okJiang <819421878@qq.com>

codecov · 2024-06-17T10:01:15Z

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.34%. Comparing base (debb5fe) to head (fa47700).

❗ Current head fa47700 differs from pull request most recent head f981b69

Please upload reports for the commit f981b69 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8303      +/-   ##
==========================================
+ Coverage   77.30%   77.34%   +0.03%     
==========================================
  Files         470      470              
  Lines       61500    61377     -123     
==========================================
- Hits        47545    47469      -76     
+ Misses      10393    10354      -39     
+ Partials     3562     3554       -8

Flag	Coverage Δ
unittests	`77.34% <72.22%> (+0.03%)`	⬆️

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

pkg/schedule/schedulers/scheduler_controller.go

Signed-off-by: okJiang <819421878@qq.com>

pkg/schedule/schedulers/scheduler_controller.go

HuSharp · 2024-06-18T07:27:38Z

/test ?

ti-chi-bot · 2024-06-18T07:27:40Z

@HuSharp: The following commands are available to trigger required jobs:

/test build

The following commands are available to trigger optional jobs:

/test pull-integration-copr-test
/test pull-integration-realcluster-test

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

tikv/pd/ghpr_build

In response to this:

/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/test-infra repository.

HuSharp · 2024-06-18T07:27:48Z

/test pull-integration-realcluster-test

Signed-off-by: okJiang <819421878@qq.com>

okJiang · 2024-06-18T08:31:01Z

/test pull-integration-realcluster-test

Signed-off-by: okJiang <819421878@qq.com>

tests/integrations/realcluster/scheduler_test.go

Signed-off-by: okJiang <819421878@qq.com>

tests/integrations/realcluster/deploy.sh

Signed-off-by: okJiang <819421878@qq.com>

rleungx · 2024-06-20T07:57:31Z

tests/integrations/realcluster/reboot_pd_test.go

-	"context"
-	"os/exec"
-	"testing"
+// func restartTiUP() {


How about using the build flag enable_flaky_tests?

I'm not quite sure how enable_flaky_tests works. I reverted the changes to this file first because the test is just not working, not because it's unstable. ad19424

It's not a flaky test, can be fixed by #8303 (comment)

I fixed it. fa47700

Signed-off-by: okJiang <819421878@qq.com>

tests/integrations/realcluster/Makefile

Signed-off-by: okJiang <819421878@qq.com>

HuSharp · 2024-06-24T03:41:51Z

ci failed, Can you test this PR with lightning/br and paste a picture to indicate the change?

okJiang · 2024-06-24T07:05:06Z

ci failed, Can you test this PR with lightning/br and paste a picture to indicate the change?

I think realcluster test is enough for this pr😂 It's not really worth spending a lot of time making specific changes for this requirement to do e2e testing. It's completely sufficient to know that evict leader still works when calling the API.

Signed-off-by: okJiang <819421878@qq.com>

HuSharp

Left some comments, rest LGTM!

HuSharp · 2024-06-24T08:08:21Z

tests/integrations/realcluster/reboot_pd_test.go

@@ -4,14 +4,13 @@
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
-//     http://www.apache.org/licenses/LICENSE-2.0
+//	http://www.apache.org/licenses/LICENSE-2.0


why change it?

After adding comments and uncommenting it, the IDE automatically formatted it. If it will have any impact, I can revert it

2024-06-24.16.22.42.mov

Please let me keep it😭

prefer to revert it

HuSharp · 2024-06-24T08:08:34Z

tests/integrations/realcluster/scheduler_test.go

+		return true
+	}, testutil.WithWaitFor(time.Minute))
+
+	re.NoError(pdHTTPCli.DeleteScheduler(ctx, schedulers.EvictLeaderName))


I just mean L143~L152

If the test exits in L143~L152 which means this test is failed, maybe all tests will be regarded as failed and then no need to delete the scheduler by defer function?

You can check the log in Jenkins, maybe it has some redundant delete?
https://do.pingcap.net/jenkins/blue/organizations/jenkins/tikv%2Fpd%2Fpull_integration_realcluster_test/detail/pull_integration_realcluster_test/98/pipeline/#step-77-log-200

HuSharp · 2024-06-24T08:10:41Z

tests/integrations/realcluster/deploy.sh

 		> $CUR_PATH/playground.log 2>&1 &
 else
  # CI will download the binaries in the prepare phase.
  # ref https://github.com/PingCAP-QE/ci/blob/387e9e533b365174962ccb1959442a7070f9cd66/pipelines/tikv/pd/latest/pull_integration_realcluster_test.groovy#L55-L68
 	color-green "using existing binaries..."
+	make pd-server WITH_RACE=1


Please remove this line, as the comment above explains that there is no need to make pd :) may reduce test time

Signed-off-by: okJiang <819421878@qq.com>

rleungx · 2024-06-24T08:31:56Z

server/cluster/cluster.go

+	if len(newStore.GetLabels()) == 0 {
+		return errors.Errorf("the label key %s does not exist", labelKey)
+	}


Should we check it before cloning?

~~Clone store before L1223(newStore.Labels = labels)?~~

rleungx · 2024-06-24T08:32:26Z

tests/integrations/realcluster/reboot_pd_test.go

@@ -4,14 +4,13 @@
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
-//     http://www.apache.org/licenses/LICENSE-2.0
+//	http://www.apache.org/licenses/LICENSE-2.0


prefer to revert it

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot · 2024-06-24T08:44:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, rleungx

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [HuSharp,rleungx]

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 · 2024-06-24T08:44:44Z

[LGTM Timeline notifier]

Timeline:

2024-06-24 08:26:10.346559716 +0000 UTC m=+621696.832048546: ☑️ agreed by HuSharp.
2024-06-24 08:44:43.289495517 +0000 UTC m=+622809.774984347: ☑️ agreed by rleungx.

River2000i · 2024-09-09T02:42:28Z

@rleungx @lance6716 Can you help me cp to 7.5 for testing? We can hold off on merging until we have a conclusion on whether to backport to v7.5.x and then merge.

ti-chi-bot · 2024-09-09T02:57:03Z

In response to a cherrypick label: new pull request created to branch release-7.5: #8602.

ref tikv#7300, close tikv#7853 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

…el (tikv#8303) ref tikv#7300, close tikv#7853 - add a real cluster test to test `skip evict-leader-scheduler when setting schedule deny label` - add `DeleteStoreLabel` API and `DeleteScheduler` API Signed-off-by: okJiang <819421878@qq.com>

okJiang added 2 commits June 17, 2024 17:43

skip evict-leader-scheduler

844aa84

Signed-off-by: okJiang <819421878@qq.com>

refine comment

0f95d6e

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 17, 2024

okJiang changed the title ~~scheduler: skil evict-leader-scheduler when setting schedule deny lable~~ scheduler: skil evict-leader-scheduler when setting schedule deny label Jun 17, 2024

okJiang changed the title ~~scheduler: skil evict-leader-scheduler when setting schedule deny label~~ scheduler: skip evict-leader-scheduler when setting schedule deny label Jun 17, 2024

rleungx reviewed Jun 18, 2024

View reviewed changes

pkg/schedule/schedulers/scheduler_controller.go Outdated Show resolved Hide resolved

fix comment

78200a4

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 18, 2024

rleungx reviewed Jun 18, 2024

View reviewed changes

pkg/schedule/schedulers/scheduler_controller.go Show resolved Hide resolved

add real cluster test

de0c440

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 18, 2024

fix lint

c3e122d

Signed-off-by: okJiang <819421878@qq.com>

rleungx reviewed Jun 18, 2024

View reviewed changes

tests/integrations/realcluster/scheduler_test.go Outdated Show resolved Hide resolved

tests/integrations/realcluster/scheduler_test.go Outdated Show resolved Hide resolved

okJiang added 2 commits June 18, 2024 17:19

add log

d37ce02

Signed-off-by: okJiang <819421878@qq.com>

fix

15c9eee

Signed-off-by: okJiang <819421878@qq.com>

HuSharp reviewed Jun 18, 2024

View reviewed changes

tests/integrations/realcluster/deploy.sh Show resolved Hide resolved

okJiang added 5 commits June 18, 2024 17:25

fix comment

fbca577

Signed-off-by: okJiang <819421878@qq.com>

add log

61865f3

Signed-off-by: okJiang <819421878@qq.com>

revert pd count

3430f19

Signed-off-by: okJiang <819421878@qq.com>

fix ci

2efe6ec

Signed-off-by: okJiang <819421878@qq.com>

clean data after test

52e4c96

Signed-off-by: okJiang <819421878@qq.com>

rleungx reviewed Jun 20, 2024

View reviewed changes

revert comment

ad19424

Signed-off-by: okJiang <819421878@qq.com>

HuSharp reviewed Jun 20, 2024

View reviewed changes

tests/integrations/realcluster/Makefile Outdated Show resolved Hide resolved

okJiang added 2 commits June 20, 2024 16:54

fix delete not found

fa47700

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'master' of github.com:tikv/pd into fix-scheduler-deny

f400afa

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'master' into fix-scheduler-deny

077d864

fix ci

18d1a97

Signed-off-by: okJiang <819421878@qq.com>

HuSharp reviewed Jun 24, 2024

View reviewed changes

fix comment

1a31857

Signed-off-by: okJiang <819421878@qq.com>

HuSharp approved these changes Jun 24, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 24, 2024

rleungx reviewed Jun 24, 2024

View reviewed changes

fix comment

f981b69

Signed-off-by: okJiang <819421878@qq.com>

rleungx approved these changes Jun 24, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2024

ti-chi-bot bot merged commit 26e90e9 into tikv:master Jun 24, 2024
17 checks passed

okJiang deleted the fix-scheduler-deny branch July 30, 2024 02:05

rleungx added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Sep 9, 2024

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Sep 9, 2024

This is an automated cherry-pick of tikv#8303

d1f6f9a

ref tikv#7300, close tikv#7853 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Sep 9, 2024

scheduler: skip evict-leader-scheduler when setting schedule deny label (#8303) #8602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: skip evict-leader-scheduler when setting schedule deny label #8303

scheduler: skip evict-leader-scheduler when setting schedule deny label #8303

okJiang commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

HuSharp commented Jun 18, 2024

ti-chi-bot bot commented Jun 18, 2024

HuSharp commented Jun 18, 2024

okJiang commented Jun 18, 2024

rleungx Jun 20, 2024

okJiang Jun 20, 2024

HuSharp Jun 20, 2024

okJiang Jun 20, 2024

rleungx Jun 20, 2024

HuSharp commented Jun 24, 2024

okJiang commented Jun 24, 2024

HuSharp left a comment

HuSharp Jun 24, 2024

okJiang Jun 24, 2024

HuSharp Jun 24, 2024

okJiang Jun 24, 2024

rleungx Jun 24, 2024

HuSharp Jun 24, 2024

HuSharp Jun 24, 2024 •

edited

Loading

rleungx Jun 24, 2024

okJiang Jun 24, 2024 •

edited

Loading

rleungx Jun 24, 2024

ti-chi-bot bot commented Jun 24, 2024

ti-chi-bot bot commented Jun 24, 2024

River2000i commented Sep 9, 2024

ti-chi-bot commented Sep 9, 2024

scheduler: skip evict-leader-scheduler when setting schedule deny label #8303

scheduler: skip evict-leader-scheduler when setting schedule deny label #8303

Conversation

okJiang commented Jun 17, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

HuSharp commented Jun 18, 2024

ti-chi-bot bot commented Jun 18, 2024

HuSharp commented Jun 18, 2024

okJiang commented Jun 18, 2024

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

HuSharp commented Jun 24, 2024

okJiang commented Jun 24, 2024

HuSharp left a 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

HuSharp Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 24, 2024

ti-chi-bot bot commented Jun 24, 2024

[LGTM Timeline notifier]

River2000i commented Sep 9, 2024

ti-chi-bot commented Sep 9, 2024

okJiang commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

HuSharp Jun 24, 2024 •

edited

Loading

okJiang Jun 24, 2024 •

edited

Loading