support scale in tikv simultaneously #3881

lizhemingi · 2021-03-29T06:23:56Z

Signed-off-by: Zheming Li nkdudu@126.com

What problem does this PR solve?

Closes #3849

What is changed and how does it work?

Scale in multi tikv store in one ScaleIn call.

Find as many as possible but no more than spec.tikv.maxScaleInReplicas in scaleMulti.

Try every possible ordinal found and aggregate all error to return.

Code changes

Has Go code change
Has CI related scripts change

Tests

Unit test
E2E test
Manual test
No code

Side effects

Breaking backward compatibility
Other side effects:

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

None

ti-chi-bot · 2021-03-29T06:23:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

csuzhangxc
july2993

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.

CLAassistant · 2021-03-29T06:24:04Z

All committers have signed the CLA.

DanielZhangQD · 2021-03-30T03:15:04Z

@duduainankai Thanks for the contribution!
Did you verify this change? Could you please update the test result in the PR description?
Here are the test cases in my mind:

Default case, scale in one by one.
spec.tikv.maxScaleInReplicas=2, spec.tikv.replicas=5, scale in from 5 to 3, two Pods are scaled in at the same time
spec.tikv.maxScaleInReplicas=3, spec.tikv.replicas=5, scale in from 5 to 2, two Pods are scaled in at the same time, tikv-2 cannot be scaled in
spec.tikv.maxScaleInReplicas=2, spec.tikv.replicas=6, scale in from 6 to 3, two Pods are scaled in at the same time
spec.tikv.maxScaleInReplicas=4, spec.tikv.replicas=5, scale in from 5 to 3, two Pods are scaled in at the same time

Please feel free to add more cases.

It would be better to add e2e cases for this change.

pkg/apis/pingcap/v1alpha1/types.go

lizhemingi · 2021-03-30T03:33:51Z

@duduainankai Thanks for the contribution!
Did you verify this change? Could you please update the test result in the PR description?
Here are the test cases in my mind:

Default case, scale in one by one.

spec.tikv.maxScaleInReplicas=2, spec.tikv.replicas=5, scale in from 5 to 3, two Pods are scaled in at the same time

spec.tikv.maxScaleInReplicas=3, spec.tikv.replicas=5, scale in from 5 to 2, two Pods are scaled in at the same time, tikv-2 cannot be scaled in

spec.tikv.maxScaleInReplicas=2, spec.tikv.replicas=6, scale in from 6 to 3, two Pods are scaled in at the same time

spec.tikv.maxScaleInReplicas=4, spec.tikv.replicas=5, scale in from 5 to 3, two Pods are scaled in at the same time

Please feel free to add more cases.

It would be better to add e2e cases for this change.

I already done some of these tests in unit test, and I will fill up reset.

After that I will try to add e2e cases.

codecov-io · 2021-04-01T03:16:50Z

Codecov Report

Merging #3881 (ee9aab0) into master (3761156) will decrease coverage by 5.43%.
The diff coverage is 79.00%.

@@            Coverage Diff             @@
##           master    #3881      +/-   ##
==========================================
- Coverage   67.91%   62.47%   -5.44%     
==========================================
  Files         175      171       -4     
  Lines       18612    18155     -457     
==========================================
- Hits        12641    11343    -1298     
- Misses       4877     5714     +837     
- Partials     1094     1098       +4

Flag	Coverage Δ
e2e	`?`
unittest	`62.47% <79.00%> (+0.06%)`	⬆️

DanielZhangQD · 2021-04-21T02:46:08Z

@duduainankai
Thanks for the contribution!
Could you please also help update the code to cover the following cases:

support scale in multiple TiFlash Pods at the same time
support scale out multiple TiFlash/TiKV Pods at the same time

DanielZhangQD · 2021-04-22T06:15:40Z

@duduainankai Just a friendly ping, are you available to continue working on this PR?

lizhemingi · 2021-04-22T06:24:08Z

@DanielZhangQD oh sorry for the slow response.

I would like to continue on this PR but it's a little busy these days, so maybe I need more time.

Is there any ETA for this feature to be merged ? If it's out of my time I will let you know first.

DanielZhangQD · 2021-04-22T07:09:48Z

@DanielZhangQD oh sorry for the slow response.

I would like to continue on this PR but it's a little busy these days, so maybe I need more time.

Is there any ETA for this feature to be merged ? If it's out of my time I will let you know first.

@duduainankai Thanks a lot!
It's not so urgent now, what about finishing it by 05/14 or 05/21?

lizhemingi · 2021-04-22T08:10:47Z

@DanielZhangQD oh sorry for the slow response.
I would like to continue on this PR but it's a little busy these days, so maybe I need more time.
Is there any ETA for this feature to be merged ? If it's out of my time I will let you know first.

@duduainankai Thanks a lot!
It's not so urgent now, what about finishing it by 05/14 or 05/21?

@DanielZhangQD
Got. I will finish it ASAP.

lizhemingi · 2021-04-22T08:12:24Z

And maybe you can review the current PR first, I already fill up the case in unit test. @DanielZhangQD

DanielZhangQD · 2021-04-22T08:13:48Z

And maybe you can review the current PR first, I already fill up the case in unit test. @DanielZhangQD

OK. /cc @csuzhangxc

pkg/apis/pingcap/v1alpha1/types.go

lizhemingi · 2021-05-07T06:33:20Z

@DanielZhangQD

Hi, I already finish "support scale in multiple TiFlash Pods at the same time" and add test for it, mainly copy from tikv_scaler_test.

But I found it a little different when do scale in between TiKV and TiFlash (like no preCheck in TiFlash and etc) so I remove and modify some test case. And I want to confirm that is by design or not, because it seems quite reasonable to do preCheck when scale in.

DanielZhangQD · 2021-05-07T06:55:00Z

@DanielZhangQD

Hi, I already finish "support scale in multiple TiFlash Pods at the same time" and add test for it, mainly copy from tikv_scaler_test.

But I found it a little different when do scale in between TiKV and TiFlash (like no preCheck in TiFlash and etc) so I remove and modify some test case. And I want to confirm that is by design or not, because it seems quite reasonable to do preCheck when scale in.

@duduainankai We need to keep the preCheck for the TiKV, as for the TiFlash, it's not done yet, please refer to #3170.

DanielZhangQD · 2021-05-07T06:57:02Z

BTW, there are a lot of changes in this PR, please add e2e tests to cover the changes.
You can do manual tests first to make sure that the changes work as expected, update the result in the PR description, and then add e2e test cases.
Thanks a lot!

lizhemingi · 2021-05-07T07:42:36Z

Got.

pkg/apis/pingcap/v1alpha1/types.go

pkg/manager/member/scaler.go

pkg/manager/member/tiflash_member_manager_test.go

DanielZhangQD · 2021-05-07T14:11:09Z

pkg/manager/member/tiflash_scaler.go

 	tc, ok := meta.(*v1alpha1.TidbCluster)
 	if !ok {
-		return nil
+		return fmt.Errorf("tiflashScaler.ScaleIn: failed to convert cluster %s/%s", meta.GetNamespace(), meta.GetName())


Return error here will cause the TC to be resynced, however, what's the point of resync here if it's not a TC?

Sorry I didn't notice that TiFlash ignore this error since it's determined an error in TiKV.

So the difference here is by design ?

Actually, I think TiKV should also ignore this error.

Got. I will correct it.

DanielZhangQD · 2021-05-07T14:29:25Z

pkg/manager/member/tiflash_scaler.go

+		} else {
+			doSet = true
+			if deleteSlots.Has(ordinal) {
+				actualDeleteSlots.Insert(ordinal)


Could you please help verify the cases in https://github.com/pingcap/tidb-operator/blob/master/pkg/manager/member/scaler_test.go#L343-L611?
And you have to enable the advanced statefulset to enable the deleteslots functionality, refer to https://docs.pingcap.com/zh/tidb-in-kubernetes/dev/advanced-statefulset.
Please also help verify the Kubernetes native statefulset cases.
Thanks!

pkg/manager/member/tiflash_scaler.go

pkg/manager/member/tikv_scaler_test.go

DanielZhangQD · 2021-05-07T14:41:07Z

BTW, there are a lot of changes in this PR, please add e2e tests to cover the changes.
You can do manual tests first to make sure that the changes work as expected, update the result in the PR description, and then add e2e test cases.
Thanks a lot!

@duduainankai Please help cover the following cases during the test:

Kubernetes native StatefulSet
Advanced StatefulSet with delete slots https://docs.pingcap.com/tidb-in-kubernetes/dev/advanced-statefulset

DanielZhangQD · 2021-05-08T02:25:09Z

@duduainankai Could you please join the slack channel of sig-k8s https://tidbcommunity.slack.com/archives/CHD0HA3LZ?
BTW, you can continue the testing now and @csuzhangxc will help prepare a test plan of this PR, we will share it with you when it's ready.
Thanks!

pkg/manager/member/scaler.go

lizhemingi · 2021-05-08T03:11:28Z

@duduainankai Could you please join the slack channel of sig-k8s https://tidbcommunity.slack.com/archives/CHD0HA3LZ?
BTW, you can continue the testing now and @csuzhangxc will help prepare a test plan of this PR, we will share it with you when it's ready.
Thanks!

I already joined the channel and you can find me by 'lizheming'.

lizhemingi · 2022-08-05T02:30:51Z

/run-all-tests

manifests/crd.yaml

ti-chi-bot · 2022-08-09T02:03:45Z

@liubog2008: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

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.

DanielZhangQD · 2022-08-09T05:46:32Z

/run-all-tests

KanShiori · 2022-08-09T07:20:56Z

pkg/apis/pingcap/v1alpha1/types.go

+	// ScaleInParallelism configures max scale in replicas for TiKV stores.
+	// +kubebuilder:default=1
+	// +optional
+	ScaleInParallelism *int32 `json:"scaleInParallelism,omitempty"`


does it support negative number, how about adding a validation?

Sounds reasonable.

@DanielZhangQD Should we treat 0 as an illegal argument too ?

DanielZhangQD · 2022-08-10T03:07:10Z

/run-all-tests

DanielZhangQD · 2022-08-10T08:13:19Z

/test pull-e2e-kind pull-e2e-kind-serial

DanielZhangQD · 2022-08-10T11:48:55Z

@lizhemingi could you please fix the CI?

DanielZhangQD · 2022-08-10T11:57:08Z

/test pull-e2e-kind pull-e2e-kind-serial

Signed-off-by: Zheming Li <nkdudu@126.com>

lizhemingi · 2022-08-10T13:18:16Z

@lizhemingi could you please fix the CI?

Done.

DanielZhangQD · 2022-08-12T02:07:45Z

/run-all-test

DanielZhangQD · 2022-08-15T01:24:55Z

/run-all-tests

DanielZhangQD · 2022-08-10T11:52:04Z

pkg/apis/pingcap/v1alpha1/types.go

+	// ScaleInParallelism configures max scale in replicas for TiKV stores.
+	// +kubebuilder:default=1
+	// +optional
+	ScaleInParallelism *int32 `json:"scaleInParallelism,omitempty"`


pkg/apis/pingcap/v1alpha1/validation/validation.go

DanielZhangQD · 2022-08-15T01:29:24Z

/merge

ti-chi-bot · 2022-08-15T01:29:26Z

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

Commit hash: 3941104

DanielZhangQD · 2022-08-15T01:38:56Z

@lizhemingi

I am very sorry that it took so long to finish the review of this PR due to our reasons.
But you didn't give up, you kept pushing forward the completion of this PR, and did a lot of testing to ensure the PR quality.
Thank you very much for your great effort in this PR.

DanielZhangQD · 2022-08-15T05:52:39Z

/test pull-e2e-kind pull-e2e-kind-br

DanielZhangQD · 2022-08-15T07:42:09Z

/test pull-e2e-kind

ti-chi-bot requested review from lichunzhu and lonng March 29, 2021 06:24

lizhemingi force-pushed the support-scale-in-simultaneously branch 2 times, most recently from 6e1bda7 to eb862dd Compare March 29, 2021 12:20

DanielZhangQD reviewed Mar 30, 2021

View reviewed changes

pkg/apis/pingcap/v1alpha1/types.go Outdated Show resolved Hide resolved

DanielZhangQD requested review from csuzhangxc and july2993 April 21, 2021 02:40

july2993 reviewed Apr 23, 2021

View reviewed changes

pkg/apis/pingcap/v1alpha1/types.go Outdated Show resolved Hide resolved

lizhemingi force-pushed the support-scale-in-simultaneously branch from 0d568f3 to 3b96df3 Compare May 7, 2021 11:40

DanielZhangQD reviewed May 7, 2021

View reviewed changes

csuzhangxc reviewed May 8, 2021

View reviewed changes

pkg/manager/member/scaler.go Show resolved Hide resolved

lizhemingi commented Aug 5, 2022

View reviewed changes

manifests/crd.yaml Show resolved Hide resolved

lizhemingi force-pushed the support-scale-in-simultaneously branch from 0d756d0 to 966dfaa Compare August 8, 2022 03:39

liubog2008 approved these changes Aug 9, 2022

View reviewed changes

KanShiori reviewed Aug 9, 2022

View reviewed changes

support scale tikv/tiflash simultaneously

a8d771c

Signed-off-by: Zheming Li <nkdudu@126.com>

lizhemingi force-pushed the support-scale-in-simultaneously branch from e83e96f to a8d771c Compare August 10, 2022 13:02

Merge branch 'master' into support-scale-in-simultaneously

11da33a

Merge branch 'master' into support-scale-in-simultaneously

3941104

KanShiori approved these changes Aug 15, 2022

View reviewed changes

DanielZhangQD reviewed Aug 15, 2022

View reviewed changes

ti-chi-bot added the status/can-merge label Aug 15, 2022

ti-chi-bot merged commit 45c6120 into pingcap:master Aug 15, 2022

KanShiori pushed a commit to KanShiori/tidb-operator that referenced this pull request Aug 15, 2022

support scale in tikv simultaneously (pingcap#3881)

571aed6

xhebox pushed a commit to KanShiori/tidb-operator that referenced this pull request Sep 16, 2022

support scale in tikv simultaneously (pingcap#3881)

60c4bd0

csuzhangxc mentioned this pull request Mar 12, 2024

support scale TiDB simultaneously #5570

Merged

10 tasks

support scale in tikv simultaneously #3881

support scale in tikv simultaneously #3881

Conversation

lizhemingi commented Mar 29, 2021

What problem does this PR solve?

What is changed and how does it work?

Code changes

Tests

Side effects

Related changes

Release Notes

ti-chi-bot commented Mar 29, 2021 • edited Loading

CLAassistant commented Mar 29, 2021 • edited Loading

DanielZhangQD commented Mar 30, 2021

lizhemingi commented Mar 30, 2021

codecov-io commented Apr 1, 2021

Codecov Report

DanielZhangQD commented Apr 21, 2021

DanielZhangQD commented Apr 22, 2021

lizhemingi commented Apr 22, 2021

DanielZhangQD commented Apr 22, 2021

lizhemingi commented Apr 22, 2021

lizhemingi commented Apr 22, 2021

DanielZhangQD commented Apr 22, 2021

lizhemingi commented May 7, 2021

DanielZhangQD commented May 7, 2021

DanielZhangQD commented May 7, 2021

lizhemingi commented May 7, 2021

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

DanielZhangQD commented May 7, 2021

DanielZhangQD commented May 8, 2021

lizhemingi commented May 8, 2021

lizhemingi commented Aug 5, 2022

ti-chi-bot commented Aug 9, 2022

DanielZhangQD commented Aug 9, 2022

KanShiori Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizhemingi Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD commented Aug 10, 2022

DanielZhangQD commented Aug 10, 2022

DanielZhangQD commented Aug 10, 2022

DanielZhangQD commented Aug 10, 2022

lizhemingi commented Aug 10, 2022

DanielZhangQD commented Aug 12, 2022

DanielZhangQD commented Aug 15, 2022

Choose a reason for hiding this comment

DanielZhangQD commented Aug 15, 2022

ti-chi-bot commented Aug 15, 2022

DanielZhangQD commented Aug 15, 2022

DanielZhangQD commented Aug 15, 2022

DanielZhangQD commented Aug 15, 2022

ti-chi-bot commented Mar 29, 2021 •

edited

Loading

CLAassistant commented Mar 29, 2021 •

edited

Loading

KanShiori Aug 9, 2022 •

edited

Loading

lizhemingi Aug 9, 2022 •

edited

Loading