-
Notifications
You must be signed in to change notification settings - Fork 500
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.
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
Conversation
please add a release note for this enhancement |
Does this need to be cherry-picked to 1.1? |
This is a non user-facing change. Is this necessary to add it in release note? @aylei |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please add a descriptive release note
// checkFormerPodRestartStatus whether there are any form pod is going to be restarted | ||
// 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-- { |
There was a problem hiding this comment.
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
for id := range helper.GetPodOrdinals(tc.Status.TiDB.StatefulSet.Replicas, set) { |
because the ordinals may not be consecutive with AdvancedStatefulset
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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{})
}
There was a problem hiding this comment.
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.
Co-Authored-By: Yecheng Fu <cofyc.jackson@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please do not include ordered list mark at the begining of release note. Refer to https://github.com/pingcap/tidb-operator/blob/master/docs/release-note-guide.md for release note language guide.
cherry pick to release-1.1 in PR #1555 |
What problem does this PR solve?
Currently, the restarter would restart the pod as soon as possible which would have risks for
tikv
andpd
pod.In this request, the admission webhook would check if there are any former
tikv
orpd
pod which is annotated withtidb.pingcap.com/pod-defer-deleting
when being received the pod delete request withtidb.pingcap.com/pod-defer-deleting
annotation too. The request would be rejected if existed.Does this PR introduce a user-facing change?: