Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit Restart rate for pd and tikv in admission webhook #1532

Merged
merged 8 commits into from
Jan 15, 2020
Merged

Limit Restart rate for pd and tikv in admission webhook #1532

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a37a6f4
Add restarter limit in webhook
Jan 9, 2020
b292e32
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 9, 2020
2b12988
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 10, 2020
42545dc
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 10, 2020
86bae56
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 14, 2020
94287fc
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 14, 2020
7a9efc1
Merge branch 'master' into revise_pod_webhook_for_restart
Yisaer Jan 15, 2020
b02ed57
Update pkg/webhook/pod/util.go
Yisaer Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/manager/member/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ func imagePullFailed(pod *corev1.Pod) bool {
return false
}

func MemberPodName(tcName string, ordinal int32, memberType v1alpha1.MemberType) string {
return fmt.Sprintf("%s-%s-%d", tcName, memberType.String(), ordinal)
}

func TikvPodName(tcName string, ordinal int32) string {
return fmt.Sprintf("%s-%d", controller.TiKVMemberName(tcName), ordinal)
}
Expand Down
19 changes: 17 additions & 2 deletions pkg/webhook/pod/pd_deleter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package pod

import (
"github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1"
"github.com/pingcap/tidb-operator/pkg/label"
pdutil "github.com/pingcap/tidb-operator/pkg/manager/member"
operatorUtils "github.com/pingcap/tidb-operator/pkg/util"
"github.com/pingcap/tidb-operator/pkg/webhook/util"
Expand All @@ -27,11 +28,25 @@ func (pc *PodAdmissionControl) admitDeletePdPods(payload *admitPayload) *admissi

name := payload.pod.Name
namespace := payload.pod.Namespace
isInOrdinal, err := operatorUtils.IsPodOrdinalNotExceedReplicas(payload.pod, payload.ownerStatefulSet)
ordinal, err := operatorUtils.GetOrdinalFromPodName(name)
if err != nil {
return util.ARFail(err)
}
ordinal, err := operatorUtils.GetOrdinalFromPodName(name)

// If the pd pod is deleted by restarter, it is necessary to check former pd restart status
if _, exist := payload.pod.Annotations[label.AnnPodDeferDeleting]; exist {
existed, err := checkFormerPodRestartStatus(pc.kubeCli, v1alpha1.PDMemberType, payload.tc, namespace, ordinal, *payload.ownerStatefulSet.Spec.Replicas)
if err != nil {
return util.ARFail(err)
}
if existed {
return &admission.AdmissionResponse{
Allowed: false,
}
}
}

isInOrdinal, err := operatorUtils.IsPodOrdinalNotExceedReplicas(payload.pod, payload.ownerStatefulSet)
if err != nil {
return util.ARFail(err)
}
Expand Down
17 changes: 17 additions & 0 deletions pkg/webhook/pod/tikv_deleter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,23 @@ func (pc *PodAdmissionControl) admitDeleteTiKVPods(payload *admitPayload) *admis
name := pod.Name
namespace := pod.Namespace
tcName := tc.Name
ordinal, err := operatorUtils.GetOrdinalFromPodName(name)
if err != nil {
return util.ARFail(err)
}

// If the tikv pod is deleted by restarter, it is necessary to check former tikv restart status
if _, exist := payload.pod.Annotations[label.AnnPodDeferDeleting]; exist {
existed, err := checkFormerPodRestartStatus(pc.kubeCli, v1alpha1.TiKVMemberType, payload.tc, namespace, ordinal, *payload.ownerStatefulSet.Spec.Replicas)
if err != nil {
return util.ARFail(err)
}
if existed {
return &admission.AdmissionResponse{
Allowed: false,
}
}
}

storesInfo, err := pdClient.GetStores()
if err != nil {
Expand Down
16 changes: 16 additions & 0 deletions pkg/webhook/pod/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,19 @@ func getOwnerStatefulSetForTiDBComponent(pod *core.Pod, kubeCli kubernetes.Inter
}
return kubeCli.AppsV1().StatefulSets(namespace).Get(ownerStatefulSetName, meta.GetOptions{})
}

// checkFormerPodRestartStatus whether there are any form pod is going to be restarted
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
// return true if existed
func checkFormerPodRestartStatus(kubeCli kubernetes.Interface, memberType v1alpha1.MemberType, tc *v1alpha1.TidbCluster, namespace string, ordinal int32, replicas int32) (bool, error) {
for i := replicas - 1; i > ordinal; i-- {
Copy link
Contributor

@cofyc cofyc Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use helper.GetPodOrdinals to get desired ordinals of pods instead

tidb-operator/pkg/manager/member/tidb_member_manager.go

Line 789 in 0bf6259

for id := range helper.GetPodOrdinals(tc.Status.TiDB.StatefulSet.Replicas, set) {

because the ordinals may not be consecutive with AdvancedStatefulset

Copy link
Contributor

@cofyc cofyc Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can list all pods of the statefulset? In this case, I think it's simpler because we need to get pod objects from the API server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I'm wrong because we need to check the desired pods and return an error if the pod is not created yet, otherwise, pods can be deleted before the previous replica is recreated.

Copy link
Contributor Author

@Yisaer Yisaer Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that the admission pod webhook only considered the pod controlled by apps.StatefulSet for now. Though it would received the deleting request of advancedStatefulset, there are some places still not support AdvancedStatefulset yet.

tidb-operator/pkg/webhook/pod/util.go

Line 126 in 0bf6259

func getOwnerStatefulSetForTiDBComponent(pod *core.Pod, kubeCli kubernetes.Interface) (*apps.StatefulSet, error) {

I think it should be supported in another issue to take it as one whole problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kind of AdvancedStatefulset is StatefulSet too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only change here is to use helper.GetPodOrdinals to get the list of ordinals instead of for loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the returned set has a method List() to get a sorted slice in asc order, all ordinals <= ordinal can be ignored in the loop

Copy link
Contributor Author

@Yisaer Yisaer Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if advancedStatefulSet is enabled, would the following method in pkg/webhook/pod/util.go get the owner advancedstatefulset ?

func getOwnerStatefulSetForTiDBComponent(pod *core.Pod, kubeCli kubernetes.Interface) (*apps.StatefulSet, error) {
	name := pod.Name
	namespace := pod.Namespace
	var ownerStatefulSetName string
	for _, ownerReference := range pod.OwnerReferences {
		if ownerReference.Kind == "StatefulSet" {
			ownerStatefulSetName = ownerReference.Name
			break
		}
	}
	if len(ownerStatefulSetName) == 0 {
		return nil, fmt.Errorf(failToFindTidbComponentOwnerStatefulset, namespace, name)
	}
	return kubeCli.AppsV1().StatefulSets(namespace).Get(ownerStatefulSetName, meta.GetOptions{})
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will because it does not check the APIGroup (the only different between advanced statefulset with k8s statefulset is the APIGroup)
I'm fine to update later because the admission controller does not support --features flag yet.

podName := memberUtil.MemberPodName(tc.Name, i, memberType)
pod, err := kubeCli.CoreV1().Pods(namespace).Get(podName, meta.GetOptions{})
if err != nil {
return false, err
}
if _, existed := pod.Annotations[label.AnnPodDeferDeleting]; existed {
return true, nil
}
}
return false, nil
}