fix: can't recover after a upgrade failed #120

xiaojingchen · 2018-10-13T05:13:38Z

this pr fixes #119 issue.
In order to fix the issue and make codes more reasonable, this pr rewrited upgrade feature and changed some unit test case, so the code has changed a lot, but in principle it should only affect the upgrade.

btw the pr should be merge after pr #112
This PR also fixes #134
@weekface @tennix @onlymellb @gregwebs PTAL

…en/tidb-operator into tidb-graceful-upgrade

…rator into fix-ugrade

xiaojingchen · 2018-10-14T05:20:22Z

/run-e2e-tests

weekface

I think there is no need to forceUpgrade:

https://github.com/pingcap/tidb-operator/pull/120/files#diff-1f146ae94e5d78830633639d134919cdR44

The gracefulUpgrade can cover this all the situations.

weekface · 2018-10-15T03:12:17Z

pkg/manager/member/pd_upgrader.go

+				return controller.RequeueErrorf("tidbcluster: [%s/%s]'s pd upgraded pod: [%s] are not ready", ns, tcName, podName)
+			}
+			continue
+		} else {


Remove else

weekface · 2018-10-15T03:12:43Z

pkg/manager/member/pd_upgrader.go

-	if tc.Status.PD.StatefulSet.CurrentReplicas == 0 {
-		return controller.RequeueErrorf("tidbcluster: [%s/%s]'s pd doesn't have old version pod to upgrade", ns, tcName)
+	if !templateEqual(newSet.Spec.Template, oldSet.Spec.Template) {
+		return nil


Set Partition to the max

And add a comment to indicate the reason why return nil.

the Partition default is max

weekface · 2018-10-15T03:15:08Z

pkg/manager/member/pd_upgrader.go

-	if !tc.PDAllPodsStarted() {
-		return controller.RequeueErrorf("tidbcluster: [%s/%s]'s pd pods are not all created", ns, tcName)
-	}
+	setUpgradePartition(newSet, *oldSet.Spec.UpdateStrategy.RollingUpdate.Partition)


This line should be added to the else condition of:
https://github.com/pingcap/tidb-operator/pull/120/files#diff-1f146ae94e5d78830633639d134919cdR69

weekface · 2018-10-15T03:16:19Z

pkg/manager/member/pd_upgrader.go

+			return err
+		}
+		return controller.RequeueErrorf("tidbcluster: [%s/%s]'s pd member: [%s] is transferring leader to pd member: [%s]", ns, tcName, upgradePodName, targetName)
+	} else {


Remove else

weekface · 2018-10-15T03:17:41Z

pkg/manager/member/pd_upgrader.go

+	upgradePodName := pdPodName(tcName, ordinal)
+	if tc.Status.PD.Leader.Name == upgradePodName {
+		var targetName string
+		if ordinal == *newSet.Spec.Replicas-1 {


define *newSet.Spec.Replicas-1 as a var:

lastOrdinal = *newSet.Spec.Replicas-1

xiaojingchen · 2018-10-15T11:03:17Z

/run-e2e-tests

…rator into fix-ugrade

xiaojingchen · 2018-10-16T11:59:07Z

/run-e2e-tests

weekface

LGTM

weekface · 2018-10-17T03:20:46Z

pkg/manager/member/tikv_upgrader.go

-		podName := tikvPodName(tcName, i-1)
+	setUpgradePartition(newSet, *oldSet.Spec.UpdateStrategy.RollingUpdate.Partition)
+	for i := tc.Status.TiKV.StatefulSet.Replicas - 1; i >= 0; i-- {
+		store := tku.getStoreByOrdinal(tc, i)


The getStoreByOrdinal method should not range over tc.Status.TiKV.TombstoneStores . The reason is: #124

xiaojingchen · 2018-10-17T07:09:22Z

/run-e2e-tests

weekface · 2018-10-18T03:19:36Z

pkg/manager/member/tikv_upgrader.go

@@ -84,11 +84,16 @@ func (tku *tikvUpgrader) Upgrade(tc *v1alpha1.TidbCluster, oldSet *apps.Stateful
 			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s tikv pod: [%s] have not label: %s", ns, tcName, podName, apps.ControllerRevisionHashLabelKey)
 		}

+		if store == nil {
+			setUpgradePartition(newSet, i)


Why set partition to i?

It should return an error if store == nil

…rator into fix-ugrade

weekface

LGTM

xiaojingchen · 2018-10-18T08:14:21Z

/run-e2e-tests

tennix · 2018-10-18T08:27:37Z

pkg/manager/member/pd_upgrader.go

-			targetName = pdPodName(tcName, *newSet.Spec.Replicas-1)
+		revision, exist := pod.Labels[apps.ControllerRevisionHashLabelKey]
+		if !exist {
+			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s pd pod: [%s] have not label: %s", ns, tcName, podName, apps.ControllerRevisionHashLabelKey)


s/have not/has no/

tennix · 2018-10-18T08:36:38Z

pkg/manager/member/pd_upgrader.go

+	return nil
+}
+
+func (pu *pdUpgrader) pdStatefulSetIsUpgrading(set *apps.StatefulSet, tc *v1alpha1.TidbCluster) (bool, error) {


This method is unused and duplicated with the method in pd_member_manager.go

tennix · 2018-10-18T08:38:08Z

pkg/manager/member/pd_upgrader_test.go

@@ -106,7 +129,7 @@ func TestPDUpgraderUpgrade(t *testing.T) {
 			changePods:        nil,
 			transferLeaderErr: false,
 			errExpectFn: func(g *GomegaWithT, err error) {
-				g.Expect(err.Error()).To(Equal("tidbcluster: [default/upgrader]'s pd upgraded pods are not all ready"))
+				g.Expect(err.Error()).To(Equal(fmt.Sprintf("tidbcluster: [default/upgrader]'s pd upgraded pod: [%s] are not ready", pdPodName(upgradeTcName, 2))))


tennix · 2018-10-18T08:42:29Z

pkg/manager/member/tidb_upgrader.go

+		}
+		revision, exist := pod.Labels[apps.ControllerRevisionHashLabelKey]
+		if !exist {
+			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s tidb pod: [%s] have not label: %s", ns, tcName, podName, apps.ControllerRevisionHashLabelKey)


s/have not/has no/

tennix · 2018-10-18T08:47:02Z

pkg/manager/member/tikv_upgrader.go

-			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s upgraded tikv pods are not all running", ns, tcName)
+		revision, exist := pod.Labels[apps.ControllerRevisionHashLabelKey]
+		if !exist {
+			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s tikv pod: [%s] have not label: %s", ns, tcName, podName, apps.ControllerRevisionHashLabelKey)


s/have not/has no/

tennix · 2018-10-18T08:50:16Z

pkg/manager/member/tikv_upgrader.go

+			return controller.RequeueErrorf("tidbcluster: [%s/%s]'s tikv pod: [%s] have not label: %s", ns, tcName, podName, apps.ControllerRevisionHashLabelKey)
+		}
+
+		if store == nil {


Move this to after line 76 to avoid retrieving pod when store is nil.

tennix · 2018-10-18T08:52:40Z

pkg/manager/member/tikv_upgrader.go

 		}
 	}

-	return nil
+	return controller.RequeueErrorf("tidbcluster: [%s/%s] have not find store status of tikv pod: [%s]", ns, tcName, upgradePodName)


"no store status found for tikv pod"

xiaojingchen · 2018-10-18T11:20:13Z

/run-e2e-tests

xiaojingchen · 2018-10-18T12:12:26Z

/run-e2e-tests

xiaojingchen · 2018-10-18T15:58:46Z

/run-e2e-tests

xiaojingchen · 2018-10-19T03:05:16Z

/run-e2e-tests

tennix

LGTM

tennix · 2018-10-19T05:15:16Z

/run-e2e-tests

xiaojingchen · 2018-10-19T07:55:26Z

/run-e2e-tests

* tidb graceful upgrade * fix upgrade state sync bug

Co-authored-by: Lilian Lee <[email protected]> Co-authored-by: pingcap-github-bot <[email protected]>

xiaojingchen and others added 11 commits October 10, 2018 14:46

tidb graceful upgrade

a7de64c

Merge branch 'master' into tidb-graceful-upgrade

2cfbde5

Merge branch 'master' into tidb-graceful-upgrade

2b78c7b

Merge branch 'tidb-graceful-upgrade' of https://github.com/xiaojingch…

fc67d9a

…en/tidb-operator into tidb-graceful-upgrade

address comment

19ac0fa

address comment

16b03a3

rewrite upgrade to fix some bugs

99f3a37

Merge branch 'master' into fix-ugrade

bf8b5a4

Merge branch 'master' into fix-ugrade

5781db6

Merge branch 'master' into fix-ugrade

756691a

Merge branch 'fix-ugrade' of https://github.com/xiaojingchen/tidb-ope…

645f067

…rator into fix-ugrade

weekface reviewed Oct 15, 2018

View reviewed changes

fix bugs

4cf1d11

address comment

24524f2

xiaojingchen force-pushed the fix-ugrade branch from c60c0d1 to 24524f2 Compare October 15, 2018 11:29

tennix and others added 5 commits October 15, 2018 20:40

Merge branch 'master' into fix-ugrade

8d149b7

fix upgrade state sync bug

19eddb5

Merge branch 'fix-ugrade' of https://github.com/xiaojingchen/tidb-ope…

3afa97a

…rator into fix-ugrade

Merge branch 'fix-ugrade' of https://github.com/xiaojingchen/tidb-ope…

3caece1

…rator into fix-ugrade

Merge branch 'fix-ugrade' of https://github.com/xiaojingchen/tidb-ope…

bd16aaf

…rator into fix-ugrade

weekface reviewed Oct 17, 2018

View reviewed changes

weekface suggested changes Oct 17, 2018

View reviewed changes

fix tikv upgrade bug

2de52da

weekface reviewed Oct 18, 2018

View reviewed changes

weekface and others added 2 commits October 18, 2018 11:19

Merge branch 'master' into fix-ugrade

f666f20

fix empty point bug

5938988

xiaojingchen added 2 commits October 18, 2018 12:25

Merge branch 'fix-ugrade' of https://github.com/xiaojingchen/tidb-ope…

7cb3f53

…rator into fix-ugrade

address comment

5429058

weekface previously approved these changes Oct 18, 2018

View reviewed changes

tennix reviewed Oct 18, 2018

View reviewed changes

xiaojingchen dismissed weekface’s stale review via c0c361a October 18, 2018 11:17

address comment

8ea29e7

xiaojingchen force-pushed the fix-ugrade branch from c0c361a to 8ea29e7 Compare October 18, 2018 11:19

xiaojingchen added 2 commits October 18, 2018 23:47

address comment

7239470

Merge branch 'master' into fix-ugrade

e0b1163

weekface mentioned this pull request Oct 19, 2018

panic during startup #134

Closed

Merge branch 'master' into fix-ugrade

63d333f

tennix approved these changes Oct 19, 2018

View reviewed changes

Merge branch 'master' into fix-ugrade

8b29919

weekface approved these changes Oct 19, 2018

View reviewed changes

weekface merged commit 71b0ecd into pingcap:master Oct 19, 2018

queenliuxx pushed a commit to queenliuxx/tidb-operator that referenced this pull request Dec 19, 2018

fix: can't recover after a upgrade failed (pingcap#120)

34c4b71

* tidb graceful upgrade * fix upgrade state sync bug

yahonda pushed a commit that referenced this pull request Dec 27, 2021

zh: fix link and indent issues #120

7e761cd

Co-authored-by: Lilian Lee <[email protected]> Co-authored-by: pingcap-github-bot <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: can't recover after a upgrade failed #120

fix: can't recover after a upgrade failed #120

xiaojingchen commented Oct 13, 2018 •

edited by weekface

Loading

xiaojingchen commented Oct 14, 2018

weekface left a comment

weekface Oct 15, 2018

weekface Oct 15, 2018

weekface Oct 15, 2018

xiaojingchen Oct 18, 2018

weekface Oct 15, 2018

weekface Oct 15, 2018

weekface Oct 15, 2018

xiaojingchen commented Oct 15, 2018

xiaojingchen commented Oct 16, 2018

weekface left a comment

weekface Oct 17, 2018

xiaojingchen commented Oct 17, 2018

weekface Oct 18, 2018

weekface Oct 18, 2018

weekface left a comment

xiaojingchen commented Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

tennix Oct 18, 2018

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 19, 2018

tennix left a comment

tennix commented Oct 19, 2018

xiaojingchen commented Oct 19, 2018

fix: can't recover after a upgrade failed #120

fix: can't recover after a upgrade failed #120

Conversation

xiaojingchen commented Oct 13, 2018 • edited by weekface Loading

xiaojingchen commented Oct 14, 2018

weekface 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

Choose a reason for hiding this comment

xiaojingchen commented Oct 15, 2018

xiaojingchen commented Oct 16, 2018

weekface left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaojingchen commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weekface left a comment

Choose a reason for hiding this comment

xiaojingchen commented Oct 18, 2018

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

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 18, 2018

xiaojingchen commented Oct 19, 2018

tennix left a comment

Choose a reason for hiding this comment

tennix commented Oct 19, 2018

xiaojingchen commented Oct 19, 2018

xiaojingchen commented Oct 13, 2018 •

edited by weekface

Loading