Fast evict leader #4091

mianhk · 2021-07-23T06:29:05Z

What problem does this PR solve?

Close #3902
Ref: #3918

What is changed and how does it work?

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.

Fast upgrade TiKV when tikv replicas number are less than 2

ti-chi-bot · 2021-07-23T06:29:06Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

DanielZhangQD
csuzhangxc

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-07-23T06:29:09Z

All committers have signed the CLA.

codecov-commenter · 2021-07-23T06:41:46Z

Codecov Report

Merging #4091 (8e775bd) into master (f8224ad) will increase coverage by 1.87%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #4091      +/-   ##
==========================================
+ Coverage   61.19%   63.06%   +1.87%     
==========================================
  Files         178      182       +4     
  Lines       18913    21219    +2306     
==========================================
+ Hits        11573    13381    +1808     
- Misses       6198     6593     +395     
- Partials     1142     1245     +103

Flag	Coverage Δ
e2e	`31.30% <62.50%> (?)`
unittest	`61.18% <62.50%> (-0.01%)`	⬇️

DanielZhangQD · 2021-07-26T02:48:18Z

/run-all-tests

DanielZhangQD · 2021-07-26T03:32:45Z

pkg/manager/member/tikv_upgrader.go

@@ -149,6 +149,12 @@ func (u *tikvUpgrader) Upgrade(meta metav1.Object, oldSet *apps.StatefulSet, new
 			return nil
 		}

+		if len(podOrdinals) < 2 {


What about moving the check to L79 and just check the *oldSet.Spec.Replicas?

Ok, not clear for meam of the partition of statefulset before.

DanielZhangQD · 2021-07-26T03:33:02Z

pkg/manager/member/tikv_upgrader.go

@@ -203,6 +209,7 @@ func (u *tikvUpgrader) readyToUpgrade(upgradePod *corev1.Pod, tc *v1alpha1.TidbC
 	}

 	tlsEnabled := tc.IsTLSClusterEnabled()
+


No need to update this.

DanielZhangQD · 2021-07-26T03:45:35Z

pkg/webhook/pod/tikv_deleter.go

 	name := payload.pod.Name
 	namespace := payload.pod.Namespace
 	tc, ok := payload.controller.(*v1alpha1.TidbCluster)
 	if !ok {
 		err := fmt.Errorf("tikv pod[%s/%s]'s controller is not a tidbcluster", namespace, name)
 		return util.ARFail(err)
 	}
+
+	if tc.TiKVStsActualReplicas() < 2 {


Should use TiKVStsDesiredReplicas?

DanielZhangQD · 2021-07-26T03:45:53Z

pkg/webhook/pod/tikv_deleter.go

@@ -205,7 +211,7 @@ func (pc *PodAdmissionControl) admitDeleteUpTiKVPodDuringUpgrading(payload *admi
 		}
 	}

-	if !isTiKVReadyToUpgrade(payload.pod, store, tc.TiKVEvictLeaderTimeout()) {
+	if !isTiKVReadyToUpgrade(payload.pod, store, stores, tc.TiKVEvictLeaderTimeout()) {


No need to update this, right?

…nto fast_evict_leader

DanielZhangQD · 2021-08-04T10:08:39Z

pkg/manager/member/tikv_upgrader.go

+		klog.Infof("TiKV statefulset replicas are less than 2, skip waiting to evict region leader for tc %s/%s", ns, tcName)
+		setUpgradePartition(newSet, *oldSet.Spec.UpdateStrategy.RollingUpdate.Partition)


Suggested change

klog.Infof("TiKV statefulset replicas are less than 2, skip waiting to evict region leader for tc %s/%s", ns, tcName)

setUpgradePartition(newSet, *oldSet.Spec.UpdateStrategy.RollingUpdate.Partition)

klog.Infof("TiKV statefulset replicas are less than 2, skip evicting region leader for tc %s/%s", ns, tcName)

setUpgradePartition(newSet, 0)

DanielZhangQD · 2021-08-04T10:10:46Z

pkg/manager/member/tikv_upgrader_test.go

+				oldSet.Status.CurrentReplicas = 1
+				oldSet.Status.UpdatedReplicas = 1
+				oldSet.Spec.Replicas = pointer.Int32Ptr(1)
+				oldSet.Spec.UpdateStrategy.RollingUpdate.Partition = pointer.Int32Ptr(0)


Suggested change

oldSet.Spec.UpdateStrategy.RollingUpdate.Partition = pointer.Int32Ptr(0)

oldSet.Spec.UpdateStrategy.RollingUpdate.Partition = pointer.Int32Ptr(2)

DanielZhangQD · 2021-08-04T10:12:02Z

pkg/webhook/pod/tikv_deleter.go

@@ -190,6 +190,12 @@ func (pc *PodAdmissionControl) admitDeleteUpTiKVPodDuringUpgrading(payload *admi
 		err := fmt.Errorf("tikv pod[%s/%s]'s controller is not a tidbcluster", namespace, name)
 		return util.ARFail(err)
 	}
+
+	if tc.TiKVStsDesiredReplicas() < 2 {
+		klog.Infof("TiKV statefulset replicas are less than 2, skip waiting to evict region leader for Pod %s/%s", namespace, name)


Suggested change

klog.Infof("TiKV statefulset replicas are less than 2, skip waiting to evict region leader for Pod %s/%s", namespace, name)

klog.Infof("TiKV statefulset replicas are less than 2, skip evicting region leader for Pod %s/%s", namespace, name)

DanielZhangQD · 2021-08-04T10:12:19Z

pkg/webhook/pod/tikv_util.go

@@ -111,6 +111,7 @@ func isTiKVReadyToUpgrade(upgradePod *core.Pod, store *pdapi.StoreInfo, evictLea
 			klog.Errorf("parse annotation:[%s] to time failed.", EvictLeaderBeginTime)
 			return false
 		}
+


No need to update this.

DanielZhangQD · 2021-08-04T10:13:07Z

@mianhk Please add the test result in the PR description, thanks!

mianhk · 2021-08-11T04:02:31Z

@mianhk Please add the test result in the PR description, thanks!

I add e2e test and fix promblems above. @DanielZhangQD PTAL.

tests/e2e/tidbcluster/tidbcluster.go

DanielZhangQD · 2021-08-24T08:38:01Z

/run-all-tests

tests/e2e/tidbcluster/tidbcluster.go

DanielZhangQD · 2021-08-24T08:44:28Z

/run-all-tests

DanielZhangQD

LGTM

DanielZhangQD · 2021-08-24T08:45:01Z

/merge

ti-chi-bot · 2021-08-24T08:45:03Z

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

Commit hash: 27cfb19

DanielZhangQD · 2021-08-24T09:04:04Z

/run-all-tests

DanielZhangQD · 2021-08-24T09:33:22Z

/run-all-tests

DanielZhangQD · 2021-08-25T01:27:52Z

/run-all-tests

shonge and others added 3 commits July 23, 2021 14:22

Fast evict leaders

fc73196

optimize code

41bf5b6

Fast evict leaders when TiKV replica is less than 2

d6d49f2

ti-chi-bot requested review from csuzhangxc and july2993 July 23, 2021 06:29

Merge branch 'master' into fast_evict_leader

8b51952

DanielZhangQD reviewed Jul 26, 2021

View reviewed changes

DanielZhangQD mentioned this pull request Jul 26, 2021

[WIP] Fast upgrade tikv pod #3918

Closed

10 tasks

mianhk added 3 commits July 28, 2021 11:26

Optimize code

2ff0663

Merge remote-tracking branch 'upstream/master' into fast_evict_leader

1d4bc5e

Merge branch 'fast_evict_leader' of github.com:mianhk/tidb-operator i…

11d8a91

…nto fast_evict_leader

mianhk requested a review from DanielZhangQD July 29, 2021 02:48

tisonkun mentioned this pull request Jul 30, 2021

No need to wait tikv transfer leader timeout while upgrading tidb cluster with single tikv node #3902

Closed

DanielZhangQD reviewed Aug 4, 2021

View reviewed changes

fix test error and add e2e test

137a19a

mianhk added 2 commits August 11, 2021 12:18

optimize code

cc8a2f4

add: tikv upgrade status

23dce57

csuzhangxc reviewed Aug 12, 2021

View reviewed changes

tests/e2e/tidbcluster/tidbcluster.go Outdated Show resolved Hide resolved

fix: e2e test add evict check

a2ba8f5

csuzhangxc approved these changes Aug 13, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 label Aug 13, 2021

Merge branch 'master' into fast_evict_leader

a437963

DanielZhangQD reviewed Aug 24, 2021

View reviewed changes

tests/e2e/tidbcluster/tidbcluster.go Show resolved Hide resolved

tests/e2e/tidbcluster/tidbcluster.go Outdated Show resolved Hide resolved

Apply suggestions from code review

27cfb19

DanielZhangQD approved these changes Aug 24, 2021

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Aug 24, 2021

ti-chi-bot added the status/can-merge label Aug 24, 2021

Merge branch 'master' into fast_evict_leader

02af2bc

Merge branch 'master' into fast_evict_leader

eebff35

Merge branch 'master' into fast_evict_leader

8e775bd

ti-chi-bot merged commit 3515c02 into pingcap:master Aug 25, 2021

DanielZhangQD mentioned this pull request Dec 31, 2021

No need to evict region leader during upgrade if only 1 TiKV store in the cluster #4335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast evict leader #4091

Fast evict leader #4091

mianhk commented Jul 23, 2021

ti-chi-bot commented Jul 23, 2021 •

edited

CLAassistant commented Jul 23, 2021 •

edited

codecov-commenter commented Jul 23, 2021 •

edited

DanielZhangQD commented Jul 26, 2021

DanielZhangQD Jul 26, 2021

mianhk Jul 28, 2021

DanielZhangQD Jul 26, 2021

DanielZhangQD Jul 26, 2021

DanielZhangQD Jul 26, 2021

DanielZhangQD Aug 4, 2021

DanielZhangQD Aug 4, 2021

DanielZhangQD Aug 4, 2021

DanielZhangQD Aug 4, 2021

DanielZhangQD commented Aug 4, 2021

mianhk commented Aug 11, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD left a comment

DanielZhangQD commented Aug 24, 2021

ti-chi-bot commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 25, 2021

		@@ -203,6 +209,7 @@ func (u tikvUpgrader) readyToUpgrade(upgradePod corev1.Pod, tc *v1alpha1.TidbC
		}

		tlsEnabled := tc.IsTLSClusterEnabled()

		klog.Infof("TiKV statefulset replicas are less than 2, skip waiting to evict region leader for tc %s/%s", ns, tcName)
		setUpgradePartition(newSet, *oldSet.Spec.UpdateStrategy.RollingUpdate.Partition)

	oldSet.Spec.UpdateStrategy.RollingUpdate.Partition = pointer.Int32Ptr(0)
	oldSet.Spec.UpdateStrategy.RollingUpdate.Partition = pointer.Int32Ptr(2)

Fast evict leader #4091

Fast evict leader #4091

Conversation

mianhk commented Jul 23, 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 Jul 23, 2021 • edited

CLAassistant commented Jul 23, 2021 • edited

codecov-commenter commented Jul 23, 2021 • edited

Codecov Report

DanielZhangQD commented Jul 26, 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

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 Aug 4, 2021

mianhk commented Aug 11, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD left a comment

Choose a reason for hiding this comment

DanielZhangQD commented Aug 24, 2021

ti-chi-bot commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 24, 2021

DanielZhangQD commented Aug 25, 2021

ti-chi-bot commented Jul 23, 2021 •

edited

CLAassistant commented Jul 23, 2021 •

edited

codecov-commenter commented Jul 23, 2021 •

edited