Pause rolling-upgrade process of tidb statefulset #470

shuijing198799 · 2019-05-09T04:15:17Z

What problem does this PR solve?

Achieved the need to pause TiDB cluster upgrades

What is changed and how it works?

Add a webhoo to hijack the request of update statefulset from apiserver, add annotation to crd tidb-cluster and pause the rolling-update process. And also provider a tkcli tool to detect the update information of tidb statefulset.

use tkcli to fetch the tidb upgrade information:

$ ./tkctl upinfo
Name: e2e-cluster1
Namespace: e2e-cluster1
CreationTimestamp: 2019-05-06 20:28:28 +0800 CST
Statu: Normal
Name State
---- -----
e2e-cluster1-tidb-0 updated
e2e-cluster1-tidb-1 updated

use kubectl to change annotate to CRD tidb-cluster:

kubectl annotate tc e2e-cluster1 -n e2e-cluster1 tidb.pingcap.com/tidb-partition=0 --overwrite
tidbcluster.pingcap.com/e2e-cluster1 annotated

Tests

E2E test
Stability test
Manual test (add detailed scripts or steps below)

Code changes

Add yaml files
Has Go code change

@xiaojingchen @tennix @aylei @weekface PTAL

Does this PR introduce a user-facing change?:

NONE

manifests/webhook.yaml

tennix · 2019-05-09T05:27:29Z

manifests/webhook.yaml

+kind: Service
+metadata:
+  name: admission-webhook-example-svc
+  namespace: pingcap


Leave the namespace field to allow user specify it from the command line.

Oh, seems namespace is required by cluster role binding. So you can keep it here, but please change it to tidb-admin the default namespace in our documentations.

tennix · 2019-05-09T05:27:53Z

manifests/webhook.yaml

+  name: admission-webhook-example-svc
+  namespace: pingcap
+  labels:
+    app: admission-webhook-example


Why name it example?

tennix · 2019-05-09T05:29:48Z

pkg/webhook/route/route.go

+			body = data
+		} else {
+			responseAdmissionReview.Response = util.ARFail(err)
+			goto returnData


Abstract this as a function to avoid using goto.

tennix · 2019-05-09T05:59:51Z

manifests/webhook.yaml

+          command: 
+          - /usr/local/bin/tidb-webhook
+          env:
+            - name: NAMESPACE


Please use downward API to get namespace dynamically.

tennix · 2019-05-09T06:00:10Z

manifests/webhook.yaml

+          env:
+            - name: NAMESPACE
+              value: pingcap
+            - name: SERVICENAME


s/SERVICENAME/SERVICE_NAME/

xiaojingchen · 2019-05-09T06:20:29Z

cmd/webhook/main.go

@@ -0,0 +1,84 @@
+// Copyright 2018 PingCAP, Inc.


Suggested change

// Copyright 2018 PingCAP, Inc.

// Copyright 2019 PingCAP, Inc.

tennix · 2019-05-09T06:23:05Z

pkg/webhook/util/certs.go

+
+// Setup the server cert. For example, user apiservers and admission webhooks
+// can use the cert to prove their identify to the kube-apiserver
+func SetupServerCert(namespaceName, serviceName string) (*CertContext, error) {


Why create certificates temporarily? Does this need to be persistent?

tennix · 2019-05-09T06:25:23Z

pkg/webhook/webhook.go

+		KubeCli:    kubecli,
+		Cli:        cli,
+		Context:    context,
+		ConfigName: "validating-webhook-configuration",


This should be configurable.

tennix · 2019-05-09T06:29:18Z

pkg/webhook/webhook.go

+
+func strPtr(s string) *string { return &s }
+
+func (ws *WebhookServer) RegisterWebhook(namespace string, svcName string) error {


Should this be applied to the cluster with a yaml file?

xiaojingchen · 2019-05-09T06:29:41Z

@shuijing198799 webhook is a new component of tidb-operator, so need support helm to manage it.
you can add it to charts/tidb-operator just like tidb-controller and tidb-scheduler

tennix · 2019-05-09T06:33:10Z

pkg/webhook/statefulset/statefulset.go

+		return util.ARFail(err)
+	}
+
+	cli, _, err := util.GetNewClient()


Create a new client every time a stateful set is updated?

pkg/webhook/statefulset/statefulset.go

weekface · 2019-05-09T05:40:48Z

cmd/webhook/main.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main


We may use admission-controller instead of webhook ?

https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#what-does-each-admission-controller-do

It seems that k8s does not provide such a admission controller。

webhook is not a nice component name.

seems right

pkg/tkctl/cmd/upinfo/upinfo.go

weekface · 2019-05-09T09:08:05Z

cmd/webhook/main.go

+
+func main() {
+
+	cli, kubeCli, err := util.GetNewClient()


tidb-operator/cmd/controller-manager/main.go

Lines 89 to 101 in 617546b

cfg, err := rest.InClusterConfig()

if err != nil {

glog.Fatalf("failed to get config: %v", err)

}

cli, err := versioned.NewForConfig(cfg)

if err != nil {

glog.Fatalf("failed to create Clientset: %v", err)

}

kubeCli, err := kubernetes.NewForConfig(cfg)

if err != nil {

glog.Fatalf("failed to get kubernetes Clientset: %v", err)

}

weekface · 2019-05-09T09:09:11Z

manifests/webhook-rbac.yaml

+subjects:
+- kind: ServiceAccount
+  namespace: pingcap
+  name: admission-webhook-example-sa


example ?

weekface · 2019-05-09T09:12:05Z

pkg/webhook/statefulset/statefulset.go

+)
+
+func AdmitStatefulSets(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+	glog.Infof("admit statefulsets")


log more pieces of information, for example, the namespace and name of the statefulset

weekface · 2019-05-09T09:13:02Z

pkg/webhook/statefulset/statefulset.go

+		return util.ARFail(err)
+	}
+
+	if set.Labels[label.ComponentLabelKey] == "tidb" {


Suggested change

if set.Labels[label.ComponentLabelKey] == "tidb" {

if set.Labels[label.ComponentLabelKey] == label.TiDBLabelVal {

xiaojingchen · 2019-05-09T12:28:48Z

cmd/webhook/main.go

+	}
+
+	ns := os.Getenv("NAMESPACE")
+	if ns == "" {


use strings.TrimSpace(ns)== ""

xiaojingchen · 2019-05-09T12:28:49Z

cmd/webhook/main.go

+	}
+
+	ns := os.Getenv("NAMESPACE")
+	if ns == "" {


use strings.TrimSpace(ns)== ""

xiaojingchen · 2019-05-09T12:31:11Z

pkg/tkctl/cmd/upinfo/upinfo.go

+		You can omit --tidbcluster=<name> option by running 'tkc use <clusterName>',
+`
+	upinfoExample = `
+		# get current tidb cluster info (set by tkc use)


Suggested change

# get current tidb cluster info (set by tkc use)

# get current tidb cluster info (set by tkc user)

pkg/webhook/webhook.go

cofyc · 2019-05-10T05:32:57Z

How about moving the design proposal in google docs to the repo?

shuijing198799 · 2019-05-10T12:23:37Z

/run-e2e-tests

shuijing198799 · 2019-05-10T13:21:25Z

/run-e2e-tests

…te useless function

shuijing198799 · 2019-05-17T04:02:23Z

/run-e2e-tests

weekface · 2019-05-21T08:09:42Z

ref:
kubernetes/kubernetes#72944
https://github.com/kubedb/project/issues/309#issuecomment-428320597

Allow Mutating|ValidatingWebhookConfiguration to use secret for CABundle

pkg/webhook/route/route.go

weekface · 2019-05-21T09:51:24Z

tests/actions.go

+
+	if upgradePaused() {
+
+		time.Sleep(5 * time.Minute)


The upgrade process take more than 5 minutes, so you must sleep more than it, for exmaple 10 minutes.

upgradePause make sure that the last TiDB pod is upgraded and healthy, So I think 5 minute is enough.

weekface · 2019-05-21T09:56:47Z

add a release note like: #477

aylei · 2019-05-21T11:57:05Z

charts/tidb-operator/templates/admission-controller-deployment.yaml

+            - -tlsKeyFile=/etc/webhook/certs/key.pem
+            - -alsologtostderr
+            - -v={{ .Values.admissionController.logLevel }}
+            - 2>&1


what's the purpose of this redirection?

aylei · 2019-05-21T12:00:19Z

charts/tidb-operator/templates/admission-controller-deployment.yaml

+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: tidb-admission-controller-deployment


I don't want to argue about naming style, bug add -deployment suffix to a deployment object is wired, so does the tidb-admission-controller-pod and admission-controller-svc and admission-controller-cfg

aylei · 2019-05-21T12:13:28Z

pkg/webhook/statefulset/statefulset.go

+		return util.ARFail(err)
+	}
+
+	if versionCli == nil {


Could we put the initialization of versionCli to init() in favor of fail-fast

aylei · 2019-05-21T12:18:49Z

pkg/webhook/util/util.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// toAdmissionResponse is a helper function to create an AdmissionResponse


Suggested change

// toAdmissionResponse is a helper function to create an AdmissionResponse

// ARFail is a helper function to create an AdmissionResponse

pkg/webhook/util/util.go

aylei · 2019-05-21T12:26:17Z

tests/actions.go

+	for _, container := range pod.Spec.Containers {
+		if container.Name == label.TiDBLabelVal ||
+			container.Name == label.TiKVLabelVal ||
+			container.Name == label.PDLabelVal {


Suggested change

container.Name == label.PDLabelVal {

if container.Name == v1alpha1.PDMemberType.String() ||

Though the literal value is same coincidentally, we should use v1alpha1.XXMemberType here (So do TiDB and TiKV).

tests/actions.go

pkg/tkctl/cmd/upinfo/upinfo.go

…tor into pause-upgrade2

shuijing198799 · 2019-05-22T06:44:41Z

/run-e2e-tests

shuijing198799 · 2019-05-22T07:17:18Z

/run-e2e-tests

shuijing198799 · 2019-05-22T07:24:38Z

/run-e2e-tests

shuijing198799 · 2019-05-22T07:41:33Z

/run-e2e-tests

shuijing198799 · 2019-05-22T08:41:10Z

/run-e2e-tests

shuijing198799 · 2019-05-22T10:18:48Z

/run-e2e-tests

shuijing198799 · 2019-05-22T10:29:09Z

/run-e2e-tests

shuijing198799 · 2019-05-22T11:30:05Z

/run-e2e-tests

shuijing198799 · 2019-05-22T11:56:23Z

/run-e2e-tests

shuijing198799 · 2019-05-22T12:08:33Z

/run-e2e-tests

weekface · 2019-05-23T02:42:00Z

/run-e2e-tests

weekface

LGTM

xiaojingchen

LGTM

* en: split troubleshoot to several docs * Apply suggestions from code review Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> * add a missing step * fix lint * Apply suggestions from code review Co-authored-by: Lilian Lee <lilin@pingcap.com> * add aliases for zh troubleshoot * Update zh/tips.md * align with the Chinese doc Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> Co-authored-by: lilin90 <lilin@pingcap.com>

add webhook and clietn

d61af18

tennix reviewed May 9, 2019

View reviewed changes

manifests/webhook.yaml Outdated Show resolved Hide resolved

tennix reviewed May 9, 2019

View reviewed changes

manifests/webhook.yaml Outdated

env:

- name: NAMESPACE

value: pingcap

- name: SERVICENAME

Copy link

Member

tennix May 9, 2019

Choose a reason for hiding this comment

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

s/SERVICENAME/SERVICE_NAME/

xiaojingchen reviewed May 9, 2019

View reviewed changes

tennix reviewed May 9, 2019

View reviewed changes

pkg/webhook/statefulset/statefulset.go Show resolved Hide resolved

xiaojingchen reviewed May 9, 2019

View reviewed changes

pkg/webhook/statefulset/statefulset.go Show resolved Hide resolved

weekface reviewed May 9, 2019

View reviewed changes

xiaojingchen reviewed May 9, 2019

View reviewed changes

cmd/webhook/main.go Outdated

}

ns := os.Getenv("NAMESPACE")

if ns == "" {

Copy link

Contributor

xiaojingchen May 9, 2019

Choose a reason for hiding this comment

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

use strings.TrimSpace(ns)== ""

xiaojingchen reviewed May 9, 2019

View reviewed changes

cmd/webhook/main.go Outdated

}

ns := os.Getenv("NAMESPACE")

if ns == "" {

Copy link

Contributor

xiaojingchen May 9, 2019

Choose a reason for hiding this comment

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

use strings.TrimSpace(ns)== ""

xiaojingchen reviewed May 9, 2019

View reviewed changes

pkg/webhook/webhook.go Show resolved Hide resolved

cofyc mentioned this pull request May 10, 2019

Design proposal of stable scheduling in TiDB #466

Merged

make change via comments

9a3b917

fix the binary name

841cf41

shuijing198799 added 3 commits May 11, 2019 14:25

init deserialzer instead of create it everytime

26acae3

Merge remote-tracking branch 'upstream/master' into pause-upgrade2

847c00e

1. add pause test 2. use helm to manager 3. fix test case bug 4. dele…

b94ef93

…te useless function

weekface reviewed May 21, 2019

View reviewed changes

weekface added 2 commits May 21, 2019 17:56

Merge branch 'master' into pause-upgrade2

3ebdec4

Merge branch 'master' into pause-upgrade2

01dbd25

aylei reviewed May 21, 2019

View reviewed changes

shuijing198799 added 3 commits May 22, 2019 14:18

use yaml to manage the admission-controller and modify via comment

35f3dd4

Merge branch 'pause-upgrade2' of github.com:shuijing198799/tidb-opera…

3100024

…tor into pause-upgrade2

go fmt

2f7de79

make check

d9f7a4c

no sidecar-injector

97c220e

fix manifest dir for e2e test

97aed78

use common image

d43eb93

Merge branch 'master' into pause-upgrade2

d63b0d2

Merge branch 'master' into pause-upgrade2

c16cc2a

weekface approved these changes May 23, 2019

View reviewed changes

xiaojingchen approved these changes May 23, 2019

View reviewed changes

weekface merged commit cb9badc into pingcap:master May 23, 2019

	// Copyright 2018 PingCAP, Inc.
	// Copyright 2019 PingCAP, Inc.


		func strPtr(s string) *string { return &s }

		func (ws *WebhookServer) RegisterWebhook(namespace string, svcName string) error {

	cfg, err := rest.InClusterConfig()
	if err != nil {
	glog.Fatalf("failed to get config: %v", err)
	}

	cli, err := versioned.NewForConfig(cfg)
	if err != nil {
	glog.Fatalf("failed to create Clientset: %v", err)
	}
	kubeCli, err := kubernetes.NewForConfig(cfg)
	if err != nil {
	glog.Fatalf("failed to get kubernetes Clientset: %v", err)
	}

	if set.Labels[label.ComponentLabelKey] == "tidb" {
	if set.Labels[label.ComponentLabelKey] == label.TiDBLabelVal {

	# get current tidb cluster info (set by tkc use)
	# get current tidb cluster info (set by tkc user)

	// toAdmissionResponse is a helper function to create an AdmissionResponse
	// ARFail is a helper function to create an AdmissionResponse

	container.Name == label.PDLabelVal {
	if container.Name == v1alpha1.PDMemberType.String() \|\|

Pause rolling-upgrade process of tidb statefulset #470

Pause rolling-upgrade process of tidb statefulset #470

Conversation

shuijing198799 commented May 9, 2019 • edited by weekface Loading

What problem does this PR solve?

What is changed and how it works?

Does this PR introduce a user-facing change?:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaojingchen commented May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weekface May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented May 10, 2019

shuijing198799 commented May 10, 2019

shuijing198799 commented May 10, 2019

shuijing198799 commented May 17, 2019

weekface commented May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weekface commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

shuijing198799 commented May 22, 2019

weekface commented May 23, 2019

weekface left a comment

Choose a reason for hiding this comment

xiaojingchen left a comment

Choose a reason for hiding this comment

shuijing198799 commented May 9, 2019 •

edited by weekface

Loading

xiaojingchen commented May 9, 2019 •

edited

Loading

weekface May 9, 2019 •

edited

Loading

weekface commented May 21, 2019 •

edited

Loading