From 0873cff0163e7cf1dc8563cd452e96c92b20bb4b Mon Sep 17 00:00:00 2001 From: xiaojingchen Date: Fri, 21 Dec 2018 17:17:44 +0800 Subject: [PATCH 1/3] fix pd control bugs --- pkg/controller/pd_control.go | 42 ++++++ pkg/controller/pd_control_test.go | 221 +++++++++++++++++++++++++----- pkg/manager/member/pd_failover.go | 4 +- 3 files changed, 227 insertions(+), 40 deletions(-) diff --git a/pkg/controller/pd_control.go b/pkg/controller/pd_control.go index b43a30f81e8..7e0d84f6fe0 100644 --- a/pkg/controller/pd_control.go +++ b/pkg/controller/pd_control.go @@ -304,6 +304,20 @@ func (pc *pdClient) GetStore(storeID uint64) (*StoreInfo, error) { } func (pc *pdClient) DeleteStore(storeID uint64) error { + var exist bool + stores, err := pc.GetStores() + if err != nil { + return err + } + for _, store := range stores.Stores { + if store.Store.GetId() == storeID { + exist = true + break + } + } + if !exist { + return nil + } apiURL := fmt.Sprintf("%s/%s/%d", pc.url, storePrefix, storeID) req, err := http.NewRequest("DELETE", apiURL, nil) if err != nil { @@ -328,6 +342,20 @@ func (pc *pdClient) DeleteStore(storeID uint64) error { } func (pc *pdClient) DeleteMemberByID(memberID uint64) error { + var exist bool + members, err := pc.GetMembers() + if err != nil { + return err + } + for _, member := range members.Members { + if member.MemberId == memberID { + exist = true + break + } + } + if !exist { + return nil + } apiURL := fmt.Sprintf("%s/%s/id/%d", pc.url, membersPrefix, memberID) req, err := http.NewRequest("DELETE", apiURL, nil) if err != nil { @@ -346,6 +374,20 @@ func (pc *pdClient) DeleteMemberByID(memberID uint64) error { } func (pc *pdClient) DeleteMember(name string) error { + var exist bool + members, err := pc.GetMembers() + if err != nil { + return err + } + for _, member := range members.Members { + if member.Name == name { + exist = true + break + } + } + if !exist { + return nil + } apiURL := fmt.Sprintf("%s/%s/name/%s", pc.url, membersPrefix, name) req, err := http.NewRequest("DELETE", apiURL, nil) if err != nil { diff --git a/pkg/controller/pd_control_test.go b/pkg/controller/pd_control_test.go index bc81f4f2ac8..0d7a614ee9a 100644 --- a/pkg/controller/pd_control_test.go +++ b/pkg/controller/pd_control_test.go @@ -359,29 +359,78 @@ func TestSetStoreLabels(t *testing.T) { func TestDeleteMember(t *testing.T) { g := NewGomegaWithT(t) name := "testMember" + member := &pdpb.Member{Name: name, MemberId: uint64(1)} + membersExist := &MembersInfo{ + Members: []*pdpb.Member{ + member, + }, + Leader: member, + EtcdLeader: member, + } + membersExistBytes, err := json.Marshal(membersExist) + g.Expect(err).NotTo(HaveOccurred()) + + membersNotExist := &MembersInfo{ + Members: []*pdpb.Member{}, + } + membersNotExistBytes, err := json.Marshal(membersNotExist) + g.Expect(err).NotTo(HaveOccurred()) + tcs := []struct { - caseName string - path string - method string - want bool + caseName string + prePath string + preMethod string + preResp []byte + exist bool + path string + method string + want bool }{{ - caseName: "success_DeleteMember", - path: fmt.Sprintf("/%s/name/%s", membersPrefix, name), - method: "DELETE", - want: true, + caseName: "success_DeleteMember", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersExistBytes, + exist: true, + path: fmt.Sprintf("/%s/name/%s", membersPrefix, name), + method: "DELETE", + want: true, }, { - caseName: "failed_DeleteMember", - path: fmt.Sprintf("/%s/name/%s", membersPrefix, name), - method: "DELETE", - want: false, + caseName: "failed_DeleteMember", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersExistBytes, + exist: true, + path: fmt.Sprintf("/%s/name/%s", membersPrefix, name), + method: "DELETE", + want: false, + }, { + caseName: "delete_not_exist_member", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersNotExistBytes, + exist: false, + path: fmt.Sprintf("/%s/name/%s", membersPrefix, name), + method: "DELETE", + want: true, }, } for _, tc := range tcs { + count := 1 svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + if count == 1 { + g.Expect(request.Method).To(Equal(tc.preMethod), "check method") + g.Expect(request.URL.Path).To(Equal(tc.prePath), "check url") + w.Header().Set("Content-Type", ContentTypeJSON) + w.WriteHeader(http.StatusOK) + w.Write(tc.preResp) + count++ + return + } + + g.Expect(tc.exist).To(BeTrue()) g.Expect(request.Method).To(Equal(tc.method), "check method") g.Expect(request.URL.Path).To(Equal(tc.path), "check url") - w.Header().Set("Content-Type", ContentTypeJSON) if tc.want { w.WriteHeader(http.StatusOK) @@ -404,29 +453,78 @@ func TestDeleteMember(t *testing.T) { func TestDeleteMemberByID(t *testing.T) { g := NewGomegaWithT(t) id := uint64(1) + member := &pdpb.Member{Name: "test", MemberId: id} + membersExist := &MembersInfo{ + Members: []*pdpb.Member{ + member, + }, + Leader: member, + EtcdLeader: member, + } + membersExistBytes, err := json.Marshal(membersExist) + g.Expect(err).NotTo(HaveOccurred()) + + membersNotExist := &MembersInfo{ + Members: []*pdpb.Member{}, + } + membersNotExistBytes, err := json.Marshal(membersNotExist) + g.Expect(err).NotTo(HaveOccurred()) + tcs := []struct { - caseName string - path string - method string - want bool + caseName string + prePath string + preMethod string + preResp []byte + exist bool + path string + method string + want bool }{{ - caseName: "success_DeleteMemberByID", - path: fmt.Sprintf("/%s/id/%d", membersPrefix, id), - method: "DELETE", - want: true, + caseName: "success_DeleteMemberByID", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersExistBytes, + exist: true, + path: fmt.Sprintf("/%s/id/%d", membersPrefix, id), + method: "DELETE", + want: true, }, { - caseName: "failed_DeleteMemberByID", - path: fmt.Sprintf("/%s/id/%d", membersPrefix, id), - method: "DELETE", - want: false, + caseName: "failed_DeleteMemberByID", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersExistBytes, + exist: true, + path: fmt.Sprintf("/%s/id/%d", membersPrefix, id), + method: "DELETE", + want: false, + }, { + caseName: "delete_not_exit_member", + prePath: fmt.Sprintf("/%s", membersPrefix), + preMethod: "GET", + preResp: membersNotExistBytes, + exist: false, + path: fmt.Sprintf("/%s/id/%d", membersPrefix, id), + method: "DELETE", + want: true, }, } for _, tc := range tcs { + count := 1 svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + if count == 1 { + g.Expect(request.Method).To(Equal(tc.preMethod), "check method") + g.Expect(request.URL.Path).To(Equal(tc.prePath), "check url") + w.Header().Set("Content-Type", ContentTypeJSON) + w.WriteHeader(http.StatusOK) + w.Write(tc.preResp) + count++ + return + } + + g.Expect(tc.exist).To(BeTrue()) g.Expect(request.Method).To(Equal(tc.method), "check method") g.Expect(request.URL.Path).To(Equal(tc.path), "check url") - w.Header().Set("Content-Type", ContentTypeJSON) if tc.want { w.WriteHeader(http.StatusOK) @@ -449,26 +547,73 @@ func TestDeleteMemberByID(t *testing.T) { func TestDeleteStore(t *testing.T) { g := NewGomegaWithT(t) storeID := uint64(1) + store := &StoreInfo{ + Store: &MetaStore{Store: &metapb.Store{Id: storeID, State: metapb.StoreState_Up}}, + Status: &StoreStatus{}, + } + stores := &StoresInfo{ + Count: 1, + Stores: []*StoreInfo{ + store, + }, + } + + storesBytes, err := json.Marshal(stores) + g.Expect(err).NotTo(HaveOccurred()) + tcs := []struct { - caseName string - path string - method string - want bool + caseName string + prePath string + preMethod string + preResp []byte + exist bool + path string + method string + want bool }{{ - caseName: "success_DeleteStore", - path: fmt.Sprintf("/%s/%d", storePrefix, storeID), - method: "DELETE", - want: true, + caseName: "success_DeleteStore", + prePath: fmt.Sprintf("/%s", storesPrefix), + preMethod: "GET", + preResp: storesBytes, + exist: true, + path: fmt.Sprintf("/%s/%d", storePrefix, storeID), + method: "DELETE", + want: true, }, { - caseName: "failed_DeleteStore", - path: fmt.Sprintf("/%s/%d", storePrefix, storeID), - method: "DELETE", - want: false, + caseName: "failed_DeleteStore", + prePath: fmt.Sprintf("/%s", storesPrefix), + preMethod: "GET", + preResp: storesBytes, + exist: true, + path: fmt.Sprintf("/%s/%d", storePrefix, storeID), + method: "DELETE", + want: false, + }, { + caseName: "delete_not_exist_store", + prePath: fmt.Sprintf("/%s", storesPrefix), + preMethod: "GET", + preResp: storesBytes, + exist: true, + path: fmt.Sprintf("/%s/%d", storePrefix, storeID), + method: "DELETE", + want: true, }, } for _, tc := range tcs { + count := 1 svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + if count == 1 { + g.Expect(request.Method).To(Equal(tc.preMethod), "check method") + g.Expect(request.URL.Path).To(Equal(tc.prePath), "check url") + w.Header().Set("Content-Type", ContentTypeJSON) + w.WriteHeader(http.StatusOK) + w.Write(tc.preResp) + count++ + return + } + + g.Expect(tc.exist).To(BeTrue()) g.Expect(request.Method).To(Equal(tc.method), "check method") g.Expect(request.URL.Path).To(Equal(tc.path), "check url") diff --git a/pkg/manager/member/pd_failover.go b/pkg/manager/member/pd_failover.go index e6c0e74f532..9ccfd8969fc 100644 --- a/pkg/manager/member/pd_failover.go +++ b/pkg/manager/member/pd_failover.go @@ -157,12 +157,12 @@ func (pf *pdFailover) tryToDeleteAFailureMember(tc *v1alpha1.TidbCluster) error return nil } - memberID, err := strconv.Atoi(failureMember.MemberID) + memberID, err := strconv.ParseUint(failureMember.MemberID, 10, 64) if err != nil { return err } // invoke deleteMember api to delete a member from the pd cluster - err = pf.pdControl.GetPDClient(tc).DeleteMemberByID(uint64(memberID)) + err = pf.pdControl.GetPDClient(tc).DeleteMemberByID(memberID) if err != nil { return err } From 01dc579ac1e8b860665a27d8900e8e19a47d35c1 Mon Sep 17 00:00:00 2001 From: xiaojingchen Date: Fri, 21 Dec 2018 17:34:48 +0800 Subject: [PATCH 2/3] fix bugs --- pkg/controller/pd_control.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/pd_control.go b/pkg/controller/pd_control.go index 7e0d84f6fe0..a630f9816a5 100644 --- a/pkg/controller/pd_control.go +++ b/pkg/controller/pd_control.go @@ -330,7 +330,7 @@ func (pc *pdClient) DeleteStore(storeID uint64) error { defer DeferClose(res.Body, &err) // Remove an offline store should returns http.StatusOK - if res.StatusCode == http.StatusOK { + if res.StatusCode == http.StatusOK || res.StatusCode == http.StatusNotFound { return nil } body, err := ioutil.ReadAll(res.Body) @@ -366,7 +366,7 @@ func (pc *pdClient) DeleteMemberByID(memberID uint64) error { return err } defer DeferClose(res.Body, &err) - if res.StatusCode == http.StatusOK { + if res.StatusCode == http.StatusOK || res.StatusCode == http.StatusNotFound { return nil } err2 := readErrorBody(res.Body) From 2f2df9a97ccef89f97f014f227b2a3933c8acc0d Mon Sep 17 00:00:00 2001 From: xiaojingchen Date: Fri, 21 Dec 2018 19:44:21 +0800 Subject: [PATCH 3/3] address comment --- pkg/manager/member/pd_failover_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/manager/member/pd_failover_test.go b/pkg/manager/member/pd_failover_test.go index 1e3e2bb2106..278e06cbb8e 100644 --- a/pkg/manager/member/pd_failover_test.go +++ b/pkg/manager/member/pd_failover_test.go @@ -243,7 +243,7 @@ func TestPDFailoverFailover(t *testing.T) { g.Expect(int(tc.Spec.PD.Replicas)).To(Equal(3)) g.Expect(len(tc.Status.PD.FailureMembers)).To(Equal(1)) g.Expect(tc.Status.PD.FailureMembers).To(Equal(map[string]v1alpha1.PDFailureMember{ - "test-pd-1": {PodName: "test-pd-1", MemberID: "1", PVCUID: "pvc-1-uid", MemberDeleted: false}, + "test-pd-1": {PodName: "test-pd-1", MemberID: "12891273174085095651", PVCUID: "pvc-1-uid", MemberDeleted: false}, })) }, }, @@ -554,7 +554,7 @@ func oneFailureMember(tc *v1alpha1.TidbCluster) { pd2: {Name: pd2, ID: "2", Health: true}, } tc.Status.PD.FailureMembers = map[string]v1alpha1.PDFailureMember{ - pd1: {PodName: pd1, PVCUID: "pvc-1-uid", MemberID: "1"}, + pd1: {PodName: pd1, PVCUID: "pvc-1-uid", MemberID: "12891273174085095651"}, } } @@ -577,7 +577,7 @@ func oneNotReadyMember(tc *v1alpha1.TidbCluster) { pd2 := ordinalPodName(v1alpha1.PDMemberType, tc.GetName(), 2) tc.Status.PD.Members = map[string]v1alpha1.PDMember{ pd0: {Name: pd0, ID: "0", Health: true}, - pd1: {Name: pd1, ID: "1", Health: false, LastTransitionTime: metav1.Time{Time: time.Now().Add(-10 * time.Minute)}}, + pd1: {Name: pd1, ID: "12891273174085095651", Health: false, LastTransitionTime: metav1.Time{Time: time.Now().Add(-10 * time.Minute)}}, pd2: {Name: pd2, ID: "2", Health: true}, } } @@ -588,11 +588,11 @@ func oneNotReadyMemberAndAFailureMember(tc *v1alpha1.TidbCluster) { pd2 := ordinalPodName(v1alpha1.PDMemberType, tc.GetName(), 2) tc.Status.PD.Members = map[string]v1alpha1.PDMember{ pd0: {Name: pd0, ID: "0", Health: true}, - pd1: {Name: pd1, ID: "1", Health: false, LastTransitionTime: metav1.Time{Time: time.Now().Add(-10 * time.Minute)}}, + pd1: {Name: pd1, ID: "12891273174085095651", Health: false, LastTransitionTime: metav1.Time{Time: time.Now().Add(-10 * time.Minute)}}, pd2: {Name: pd2, ID: "2", Health: true}, } tc.Status.PD.FailureMembers = map[string]v1alpha1.PDFailureMember{ - pd1: {PodName: pd1, PVCUID: "pvc-1-uid", MemberID: "1"}, + pd1: {PodName: pd1, PVCUID: "pvc-1-uid", MemberID: "12891273174085095651"}, } } @@ -602,7 +602,7 @@ func allMembersReady(tc *v1alpha1.TidbCluster) { pd2 := ordinalPodName(v1alpha1.PDMemberType, tc.GetName(), 2) tc.Status.PD.Members = map[string]v1alpha1.PDMember{ pd0: {Name: pd0, ID: "0", Health: true}, - pd1: {Name: pd1, ID: "1", Health: true}, + pd1: {Name: pd1, ID: "12891273174085095651", Health: true}, pd2: {Name: pd2, ID: "2", Health: true}, } } @@ -613,7 +613,7 @@ func twoMembersNotReady(tc *v1alpha1.TidbCluster) { pd2 := ordinalPodName(v1alpha1.PDMemberType, tc.GetName(), 2) tc.Status.PD.Members = map[string]v1alpha1.PDMember{ pd0: {Name: pd0, ID: "0", Health: false}, - pd1: {Name: pd1, ID: "1", Health: false}, + pd1: {Name: pd1, ID: "12891273174085095651", Health: false}, pd2: {Name: pd2, ID: "2", Health: true}, } }