From 8c45779174a78a2f6e569faf9b9ca82f8bece480 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Mon, 16 Mar 2020 13:16:21 +0800 Subject: [PATCH 1/2] Revert "current delete slot annotations check in Advanced Statefulset upgrader is not right (#1851)" This reverts commit 596d10dc259c621f601721b9ffe31ff22278f506. --- pkg/upgrader/upgrader.go | 28 ++++---- pkg/upgrader/upgrader_test.go | 128 +++++++++++++++++----------------- 2 files changed, 76 insertions(+), 80 deletions(-) diff --git a/pkg/upgrader/upgrader.go b/pkg/upgrader/upgrader.go index 3163b2344e..6ad8729126 100644 --- a/pkg/upgrader/upgrader.go +++ b/pkg/upgrader/upgrader.go @@ -73,22 +73,18 @@ 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") - // 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) - // } - // } + 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 7f86a59b31..0861328c44 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, From f86067c31da5f66eaa231de8fb48efcff1934eea Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Mon, 16 Mar 2020 15:50:16 +0800 Subject: [PATCH 2/2] only check relevant tidb clusters --- pkg/upgrader/upgrader.go | 46 ++++++++++++--------- pkg/upgrader/upgrader_test.go | 78 ++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 21 deletions(-) diff --git a/pkg/upgrader/upgrader.go b/pkg/upgrader/upgrader.go index 6ad8729126..b59bec2ddb 100644 --- a/pkg/upgrader/upgrader.go +++ b/pkg/upgrader/upgrader.go @@ -25,7 +25,7 @@ import ( "github.com/pingcap/tidb-operator/pkg/label" utildiscovery "github.com/pingcap/tidb-operator/pkg/util/discovery" appsv1 "k8s.io/api/apps/v1" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes" @@ -58,47 +58,53 @@ var _ Interface = &upgrader{} // isOwnedByTidbCluster checks if the given object is owned by TidbCluster. // Schema Kind and Group are checked, Version is ignored. -func isOwnedByTidbCluster(obj metav1.Object) bool { +func isOwnedByTidbCluster(obj metav1.Object) (bool, *metav1.OwnerReference) { ref := metav1.GetControllerOf(obj) if ref == nil { - return false + return false, nil } gv, err := schema.ParseGroupVersion(ref.APIVersion) if err != nil { - return false + return false, nil } - return ref.Kind == v1alpha1.TiDBClusterKind && gv.Group == v1alpha1.SchemeGroupVersion.Group + return ref.Kind == v1alpha1.TiDBClusterKind && gv.Group == v1alpha1.SchemeGroupVersion.Group, ref } 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) - } - } stsList, err := u.kubeCli.AppsV1().StatefulSets(u.ns).List(metav1.ListOptions{}) if err != nil { return err } stsToMigrate := make([]appsv1.StatefulSet, 0) + tidbClusters := make([]*v1alpha1.TidbCluster, 0) for _, sts := range stsList.Items { - if isOwnedByTidbCluster(&sts) { + if ok, tcRef := isOwnedByTidbCluster(&sts); ok { stsToMigrate = append(stsToMigrate, sts) + tc, err := u.cli.PingcapV1alpha1().TidbClusters(sts.Namespace).Get(tcRef.Name, metav1.GetOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + if tc != nil { + tidbClusters = append(tidbClusters, tc) + } } } if len(stsToMigrate) <= 0 { klog.Infof("Upgrader: found 0 Kubernetes StatefulSets owned by TidbCluster, nothing need to do") return nil } + klog.Infof("Upgrader: %d Kubernetes Statfulsets owned by TidbCluster should be migrated to Advanced Statefulsets", len(stsToMigrate)) + // Check if relavant TidbClusters have delete slots annotations set. + for _, tc := range tidbClusters { + // 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) + } + } klog.Infof("Upgrader: found %d Kubernetes StatefulSets owned by TidbCluster, trying to migrate one by one", len(stsToMigrate)) for _, sts := range stsToMigrate { _, err := helper.Upgrade(u.kubeCli, u.asCli, &sts) @@ -116,7 +122,7 @@ func (u *upgrader) Upgrade() error { } stsList, err := u.asCli.AppsV1().StatefulSets(u.ns).List(metav1.ListOptions{}) if err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { klog.Infof("Upgrader: Kubernetes server does't have Advanced StatefulSets resources, skip to revert") return nil } @@ -124,7 +130,7 @@ func (u *upgrader) Upgrade() error { } stsToMigrate := make([]asappsv1.StatefulSet, 0) for _, sts := range stsList.Items { - if isOwnedByTidbCluster(&sts) { + if ok, _ := isOwnedByTidbCluster(&sts); ok { stsToMigrate = append(stsToMigrate, sts) } } diff --git a/pkg/upgrader/upgrader_test.go b/pkg/upgrader/upgrader_test.go index 0861328c44..91b3625cb0 100644 --- a/pkg/upgrader/upgrader_test.go +++ b/pkg/upgrader/upgrader_test.go @@ -86,7 +86,7 @@ func TestIsOwnedByTidbCluster(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ok := isOwnedByTidbCluster(&tt.sts) + ok, _ := isOwnedByTidbCluster(&tt.sts) if tt.wantOK != ok { t.Errorf("got %v, want %v", ok, tt.wantOK) } @@ -160,10 +160,12 @@ func TestDeleteSlotAnns(t *testing.T) { } var ( + ownerTCName = "foo" validOwnerRefs = []metav1.OwnerReference{ { APIVersion: "pingcap.com/v1alpha1", Kind: "TidbCluster", + Name: ownerTCName, Controller: pointer.BoolPtr(true), }, } @@ -319,6 +321,8 @@ func TestUpgrade(t *testing.T) { tidbClusters: []v1alpha1.TidbCluster{ { ObjectMeta: metav1.ObjectMeta{ + Name: ownerTCName, + Namespace: "sts", Annotations: map[string]string{ label.AnnTiDBDeleteSlots: "[1,2]", }, @@ -378,6 +382,78 @@ func TestUpgrade(t *testing.T) { }, }, }, + { + name: "should upgrade if tc has delete slot annotations but does not own Kubernetes StatefulSets", + tidbClusters: []v1alpha1.TidbCluster{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: ownerTCName, + Namespace: "sts", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "sts", + 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: false, + wantAdvancedStatefulsets: []asappsv1.StatefulSet{ + { + TypeMeta: metav1.TypeMeta{ + Kind: "StatefulSet", + APIVersion: "apps.pingcap.com/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sts1", + Namespace: "sts", + OwnerReferences: validOwnerRefs, + }, + }, + { + TypeMeta: metav1.TypeMeta{ + Kind: "StatefulSet", + APIVersion: "apps.pingcap.com/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sts2", + Namespace: "sts", + OwnerReferences: validOwnerRefs, + }, + }, + }, + wantStatefulsets: nil, + }, { name: "should ignore if sts is not owned by TidbCluster", tidbClusters: nil,