don't failover if TiDB pod is not scheduled and perform recovery no matter what state failover pods are in

pkg/controller/tidbcluster/tidb_cluster_controller.go

@@ -108,7 +108,7 @@ func NewController(
 	tiflashScaler := mm.NewTiFlashScaler(pdControl, pvcInformer.Lister(), pvcControl, podInformer.Lister())
 	pdFailover := mm.NewPDFailover(cli, pdControl, pdFailoverPeriod, podInformer.Lister(), podControl, pvcInformer.Lister(), pvcControl, pvInformer.Lister(), recorder)
 	tikvFailover := mm.NewTiKVFailover(tikvFailoverPeriod, recorder)
-	tidbFailover := mm.NewTiDBFailover(tidbFailoverPeriod, recorder)
+	tidbFailover := mm.NewTiDBFailover(tidbFailoverPeriod, recorder, podInformer.Lister())
 	tiflashFailover := mm.NewTiFlashFailover(tiflashFailoverPeriod, recorder)
 	pdUpgrader := mm.NewPDUpgrader(pdControl, podControl, podInformer.Lister())
 	tikvUpgrader := mm.NewTiKVUpgrader(pdControl, podControl, podInformer.Lister())

pkg/manager/member/tidb_failover.go

@@ -20,20 +20,24 @@ import (
 	"github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog"
+	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 )
 
 type tidbFailover struct {
 	tidbFailoverPeriod time.Duration
 	recorder           record.EventRecorder
+	podLister          corelisters.PodLister
 }
 
 // NewTiDBFailover returns a tidbFailover instance
-func NewTiDBFailover(failoverPeriod time.Duration, recorder record.EventRecorder) Failover {
+func NewTiDBFailover(failoverPeriod time.Duration, recorder record.EventRecorder, podLister corelisters.PodLister) Failover {
 	return &tidbFailover{
 		tidbFailoverPeriod: failoverPeriod,
 		recorder:           recorder,
+		podLister:          podLister,
 	}
 }
 
@@ -50,24 +54,38 @@ func (tf *tidbFailover) Failover(tc *v1alpha1.TidbCluster) error {
 		}
 	}
 
-	if tc.Spec.TiDB.MaxFailoverCount != nil && *tc.Spec.TiDB.MaxFailoverCount > 0 {
-		maxFailoverCount := *tc.Spec.TiDB.MaxFailoverCount
-		if len(tc.Status.TiDB.FailureMembers) >= int(maxFailoverCount) {
-			klog.Warningf("the failure members count reached the limit:%d", tc.Spec.TiDB.MaxFailoverCount)
-			return nil
-		}
-		for _, tidbMember := range tc.Status.TiDB.Members {
-			_, exist := tc.Status.TiDB.FailureMembers[tidbMember.Name]
-			deadline := tidbMember.LastTransitionTime.Add(tf.tidbFailoverPeriod)
-			if !tidbMember.Health && time.Now().After(deadline) && !exist {
-				tc.Status.TiDB.FailureMembers[tidbMember.Name] = v1alpha1.TiDBFailureMember{
-					PodName:   tidbMember.Name,
-					CreatedAt: metav1.Now(),
-				}
-				msg := fmt.Sprintf("tidb[%s] is unhealthy", tidbMember.Name)
-				tf.recorder.Event(tc, corev1.EventTypeWarning, unHealthEventReason, fmt.Sprintf(unHealthEventMsgPattern, "tidb", tidbMember.Name, msg))
+	if tc.Spec.TiDB.MaxFailoverCount == nil || *tc.Spec.TiDB.MaxFailoverCount <= 0 {
+		klog.Infof("tidb failover is disabled for %s/%s, skipped", tc.Namespace, tc.Name)
+		return nil
+	}
+
+	maxFailoverCount := *tc.Spec.TiDB.MaxFailoverCount
+	for _, tidbMember := range tc.Status.TiDB.Members {
+		_, exist := tc.Status.TiDB.FailureMembers[tidbMember.Name]
+		deadline := tidbMember.LastTransitionTime.Add(tf.tidbFailoverPeriod)
+		if !tidbMember.Health && time.Now().After(deadline) && !exist {
+			if len(tc.Status.TiDB.FailureMembers) >= int(maxFailoverCount) {
+				klog.Warningf("the failover count reachs the limit (%d), no more failover pods will be created", maxFailoverCount)
 				break
 			}
+			pod, err := tf.podLister.Pods(tc.Namespace).Get(tidbMember.Name)
+			if err != nil {
+				return err
+			}
+			_, condition := podutil.GetPodCondition(&pod.Status, corev1.PodScheduled)
+			if condition == nil || condition.Status != corev1.ConditionTrue {
+				// if a member is unheathy because it's not scheduled yet, we
+				// should not create failover pod for it
+				klog.Warningf("pod %s/%s is not scheduled yet, skipping failover", pod.Namespace, pod.Name)
+				continue
+			}
+			tc.Status.TiDB.FailureMembers[tidbMember.Name] = v1alpha1.TiDBFailureMember{
+				PodName:   tidbMember.Name,
+				CreatedAt: metav1.Now(),
+			}
+			msg := fmt.Sprintf("tidb[%s] is unhealthy", tidbMember.Name)
+			tf.recorder.Event(tc, corev1.EventTypeWarning, unHealthEventReason, fmt.Sprintf(unHealthEventMsgPattern, "tidb", tidbMember.Name, msg))
+			break
 		}
 	}
 

pkg/manager/member/tidb_failover_test.go

@@ -14,40 +14,31 @@
 package member
 
 import (
+	"context"
 	"testing"
 	"time"
 
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/types"
+	kubeinformers "k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
 
 	"github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
 )
 
-func TestFakeTiDBFailoverFailover(t *testing.T) {
-	type testcase struct {
+func TestTiDBFailoverFailover(t *testing.T) {
+	tests := []struct {
 		name        string
+		pods        []*corev1.Pod
 		update      func(*v1alpha1.TidbCluster)
 		errExpectFn func(*GomegaWithT, error)
 		expectFn    func(*GomegaWithT, *v1alpha1.TidbCluster)
-	}
-
-	testFn := func(test *testcase, t *testing.T) {
-		t.Logf(test.name)
-		g := NewGomegaWithT(t)
-		tidbFailover := newTiDBFailover()
-		tc := newTidbClusterForTiDBFailover()
-		test.update(tc)
-
-		err := tidbFailover.Failover(tc)
-		test.errExpectFn(g, err)
-		test.expectFn(g, tc)
-	}
-
-	tests := []testcase{
+	}{
 		{
 			name: "all tidb members are ready",
 			update: func(tc *v1alpha1.TidbCluster) {
@@ -72,6 +63,36 @@ func TestFakeTiDBFailoverFailover(t *testing.T) {
 		},
 		{
 			name: "one tidb member failed",
+			pods: []*corev1.Pod{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-0",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-1",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
 			update: func(tc *v1alpha1.TidbCluster) {
 				tc.Status.TiDB.Members = map[string]v1alpha1.TiDBMember{
 					"failover-tidb-0": {
@@ -92,8 +113,90 @@ func TestFakeTiDBFailoverFailover(t *testing.T) {
 				t.Expect(int(tc.Spec.TiDB.Replicas)).To(Equal(2))
 			},
 		},
+		{
+			name: "one tidb member failed but not scheduled yet",
+			pods: []*corev1.Pod{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-0",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionUnknown,
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-1",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
+			update: func(tc *v1alpha1.TidbCluster) {
+				tc.Status.TiDB.Members = map[string]v1alpha1.TiDBMember{
+					"failover-tidb-0": {
+						Name:   "failover-tidb-0",
+						Health: false,
+					},
+					"failover-tidb-1": {
+						Name:   "failover-tidb-1",
+						Health: true,
+					},
+				}
+			},
+			errExpectFn: func(t *GomegaWithT, err error) {
+				t.Expect(err).NotTo(HaveOccurred())
+			},
+			expectFn: func(t *GomegaWithT, tc *v1alpha1.TidbCluster) {
+				t.Expect(len(tc.Status.TiDB.FailureMembers)).To(Equal(0))
+				t.Expect(int(tc.Spec.TiDB.Replicas)).To(Equal(2))
+			},
+		},
 		{
 			name: "two tidb members failed",
+			pods: []*corev1.Pod{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-0",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: corev1.NamespaceDefault,
+						Name:      "failover-tidb-1",
+					},
+					Status: corev1.PodStatus{
+						Conditions: []corev1.PodCondition{
+							{
+								Type:   corev1.PodScheduled,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
 			update: func(tc *v1alpha1.TidbCluster) {
 				tc.Status.TiDB.Members = map[string]v1alpha1.TiDBMember{
 					"failover-tidb-0": {
@@ -207,30 +310,31 @@ func TestFakeTiDBFailoverFailover(t *testing.T) {
 		},
 	}
 
-	for i := range tests {
-		testFn(&tests[i], t)
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			fakeClient := fake.NewSimpleClientset()
+			for _, pod := range test.pods {
+				fakeClient.CoreV1().Pods(pod.Namespace).Create(pod)
+			}
+			tidbFailover := newTiDBFailover(ctx, fakeClient)
+			tc := newTidbClusterForTiDBFailover()
+			test.update(tc)
+			err := tidbFailover.Failover(tc)
+			test.errExpectFn(g, err)
+			test.expectFn(g, tc)
+		})
 	}
 }
 
-func TestFakeTiDBFailoverRecover(t *testing.T) {
-	type testcase struct {
+func TestTiDBFailoverRecover(t *testing.T) {
+	tests := []struct {
 		name     string
 		update   func(*v1alpha1.TidbCluster)
 		expectFn func(*GomegaWithT, *v1alpha1.TidbCluster)
-	}
-
-	testFn := func(test *testcase, t *testing.T) {
-		t.Log(test.name)
-		g := NewGomegaWithT(t)
-		tidbFailover := newTiDBFailover()
-		tc := newTidbClusterForTiDBFailover()
-		test.update(tc)
-
-		tidbFailover.Recover(tc)
-		test.expectFn(g, tc)
-	}
-
-	tests := []testcase{
+	}{
 		{
 			name: "have not failure tidb member to recover",
 			update: func(tc *v1alpha1.TidbCluster) {
@@ -377,14 +481,28 @@ func TestFakeTiDBFailoverRecover(t *testing.T) {
 		},
 	}
 
-	for i := range tests {
-		testFn(&tests[i], t)
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			fakeClient := fake.NewSimpleClientset()
+			tidbFailover := newTiDBFailover(ctx, fakeClient)
+			tc := newTidbClusterForTiDBFailover()
+			test.update(tc)
+			tidbFailover.Recover(tc)
+			test.expectFn(g, tc)
+		})
 	}
 }
 
-func newTiDBFailover() Failover {
+func newTiDBFailover(ctx context.Context, client kubernetes.Interface) Failover {
+	kubeInformerFactory := kubeinformers.NewSharedInformerFactory(client, 0)
+	podLister := kubeInformerFactory.Core().V1().Pods().Lister()
+	kubeInformerFactory.Start(ctx.Done())
+	kubeInformerFactory.WaitForCacheSync(ctx.Done())
 	recorder := record.NewFakeRecorder(100)
-	return &tidbFailover{tidbFailoverPeriod: time.Duration(5 * time.Minute), recorder: recorder}
+	return NewTiDBFailover(time.Duration(5*time.Minute), recorder, podLister)
 }
 
 func newTidbClusterForTiDBFailover() *v1alpha1.TidbCluster {