From 596d10dc259c621f601721b9ffe31ff22278f506 Mon Sep 17 00:00:00 2001
From: Yecheng Fu <fuyecheng@pingcap.com>
Date: Tue, 3 Mar 2020 16:28:48 +0800
Subject: [PATCH] current delete slot annotations check in Advanced Statefulset
 upgrader is not right (#1851)

---
 pkg/upgrader/upgrader.go      |  28 ++++----
 pkg/upgrader/upgrader_test.go | 128 +++++++++++++++++-----------------
 2 files changed, 80 insertions(+), 76 deletions(-)

diff --git a/pkg/upgrader/upgrader.go b/pkg/upgrader/upgrader.go
index 6ad8729126..3163b2344e 100644
--- a/pkg/upgrader/upgrader.go
+++ b/pkg/upgrader/upgrader.go
@@ -73,18 +73,22 @@ func isOwnedByTidbCluster(obj metav1.Object) bool {
 func (u *upgrader) Upgrade() error {
 	if features.DefaultFeatureGate.Enabled(features.AdvancedStatefulSet) {
 		klog.Infof("Upgrader: migrating Kubernetes StatefulSets to Advanced StatefulSets")
-		tcList, err := u.cli.PingcapV1alpha1().TidbClusters(u.ns).List(metav1.ListOptions{})
-		if err != nil {
-			return err
-		}
-		for _, tc := range tcList.Items {
-			// Existing delete slots annotations must be removed first. This is
-			// a safety check to ensure no pods are affected in upgrading
-			// process.
-			if anns := deleteSlotAnns(&tc); len(anns) > 0 {
-				return fmt.Errorf("Upgrader: TidbCluster %s/%s has delete slot annotations %v, please remove them before enabling AdvancedStatefulSet feature", tc.Namespace, tc.Name, anns)
-			}
-		}
+		// We should not check this, otherwise the controller-manager with
+		// advanced statefulset cannot be restarted when some clusters have
+		// delete slot annotations set.
+		// TODO find a better way
+		// tcList, err := u.cli.PingcapV1alpha1().TidbClusters(u.ns).List(metav1.ListOptions{})
+		// if err != nil {
+		// return err
+		// }
+		// for _, tc := range tcList.Items {
+		// // Existing delete slots annotations must be removed first. This is
+		// // a safety check to ensure no pods are affected in upgrading
+		// // process.
+		// if anns := deleteSlotAnns(&tc); len(anns) > 0 {
+		// return fmt.Errorf("Upgrader: TidbCluster %s/%s has delete slot annotations %v, please remove them before enabling AdvancedStatefulSet feature", tc.Namespace, tc.Name, anns)
+		// }
+		// }
 		stsList, err := u.kubeCli.AppsV1().StatefulSets(u.ns).List(metav1.ListOptions{})
 		if err != nil {
 			return err
diff --git a/pkg/upgrader/upgrader_test.go b/pkg/upgrader/upgrader_test.go
index 0861328c44..7f86a59b31 100644
--- a/pkg/upgrader/upgrader_test.go
+++ b/pkg/upgrader/upgrader_test.go
@@ -314,70 +314,70 @@ func TestUpgrade(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "should not upgrade if tc has delete slot annotations",
-			tidbClusters: []v1alpha1.TidbCluster{
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Annotations: map[string]string{
-							label.AnnTiDBDeleteSlots: "[1,2]",
-						},
-					},
-				},
-			},
-			statefulsets: []appsv1.StatefulSet{
-				{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:            "sts1",
-						Namespace:       "sts",
-						OwnerReferences: validOwnerRefs,
-					},
-				},
-				{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:            "sts2",
-						Namespace:       "sts",
-						OwnerReferences: validOwnerRefs,
-					},
-				},
-			},
-			feature:                  "AdvancedStatefulSet=true",
-			ns:                       metav1.NamespaceAll,
-			wantErr:                  true,
-			wantAdvancedStatefulsets: nil,
-			wantStatefulsets: []appsv1.StatefulSet{
-				{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:            "sts1",
-						Namespace:       "sts",
-						OwnerReferences: validOwnerRefs,
-					},
-				},
-				{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:            "sts2",
-						Namespace:       "sts",
-						OwnerReferences: validOwnerRefs,
-					},
-				},
-			},
-		},
+		// {
+		// name: "should not upgrade if tc has delete slot annotations",
+		// tidbClusters: []v1alpha1.TidbCluster{
+		// {
+		// ObjectMeta: metav1.ObjectMeta{
+		// Annotations: map[string]string{
+		// label.AnnTiDBDeleteSlots: "[1,2]",
+		// },
+		// },
+		// },
+		// },
+		// statefulsets: []appsv1.StatefulSet{
+		// {
+		// TypeMeta: metav1.TypeMeta{
+		// Kind:       "StatefulSet",
+		// APIVersion: "apps/v1",
+		// },
+		// ObjectMeta: metav1.ObjectMeta{
+		// Name:            "sts1",
+		// Namespace:       "sts",
+		// OwnerReferences: validOwnerRefs,
+		// },
+		// },
+		// {
+		// TypeMeta: metav1.TypeMeta{
+		// Kind:       "StatefulSet",
+		// APIVersion: "apps/v1",
+		// },
+		// ObjectMeta: metav1.ObjectMeta{
+		// Name:            "sts2",
+		// Namespace:       "sts",
+		// OwnerReferences: validOwnerRefs,
+		// },
+		// },
+		// },
+		// feature:                  "AdvancedStatefulSet=true",
+		// ns:                       metav1.NamespaceAll,
+		// wantErr:                  true,
+		// wantAdvancedStatefulsets: nil,
+		// wantStatefulsets: []appsv1.StatefulSet{
+		// {
+		// TypeMeta: metav1.TypeMeta{
+		// Kind:       "StatefulSet",
+		// APIVersion: "apps/v1",
+		// },
+		// ObjectMeta: metav1.ObjectMeta{
+		// Name:            "sts1",
+		// Namespace:       "sts",
+		// OwnerReferences: validOwnerRefs,
+		// },
+		// },
+		// {
+		// TypeMeta: metav1.TypeMeta{
+		// Kind:       "StatefulSet",
+		// APIVersion: "apps/v1",
+		// },
+		// ObjectMeta: metav1.ObjectMeta{
+		// Name:            "sts2",
+		// Namespace:       "sts",
+		// OwnerReferences: validOwnerRefs,
+		// },
+		// },
+		// },
+		// },
 		{
 			name:         "should ignore if sts is not owned by TidbCluster",
 			tidbClusters: nil,