sync pump cluster in new pump_member_manager #1269

aylei · 2019-12-02T12:20:36Z

Signed-off-by: Aylei rayingecho@gmail.com

close #1191

Note:

The strategy to manage start script and configmap will also applied to TiKV/TiDB/PD if this review passed
Documentation will be tracked in separate issues:
- Document how to manage pump in TidbCluster CR docs-cn#2092
- https://github.com/pingcap/docs/issues/1684
Verify in stability test is non goal for now.

Check List

Tests

Unit test
E2E test

Code changes

Has Helm charts change
Has Go code change

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Does this PR introduce a user-facing change?:

Custom resource `TidbCluster` is extended to support managing Pump cluster.

@weekface @tennix @cofyc @Yisaer @AstroProfundis PTAL

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-12-02T12:25:25Z

/run-e2e-test

aylei · 2019-12-02T12:26:09Z

charts/tidb-operator/templates/controller-manager-rbac.yaml

@@ -28,7 +28,7 @@ rules:
  - events
  verbs: ["*"]
 - apiGroups: [""]
-  resources: ["endpoints"]
+  resources: ["endpoints","configmaps"]


To operate configmaps of pump

aylei · 2019-12-02T12:27:05Z

manifests/webhook.yaml

@@ -95,7 +95,7 @@ spec:
            - -v=4
            # TODO webhook is not bundled with tidb-operator yet and tested in
            # e2e only, make it configurable in helm chart
-            - -features=AdvancedStatefulSet=true
+            # - -features=AdvancedStatefulSet=true


Disable asts temporarily, a new [Serial] tag will address this #1257

aylei · 2019-12-02T12:30:43Z

pkg/manager/member/pump_member_manager.go

+}
+
+// syncStatefulSet syncs the pump statefulset
+// TODO: sync statefulset status of pump to tidbcluster


aylei · 2019-12-02T12:33:34Z

pkg/manager/member/pump_member_manager.go

+
+	oldCmTemp, err := pmm.cmLister.ConfigMaps(newCm.Namespace).Get(newCm.Name)
+	if errors.IsNotFound(err) {
+		// TODO: garbage collection for pump configmaps


aylei · 2019-12-02T12:35:48Z

pkg/manager/member/pump_member_manager.go

+	}
+	if spec.HostNetwork() {
+		// For backward compatibility, set HOSTNAME to POD_NAME in hostNetwork mode
+		envs = append(envs, corev1.EnvVar{


This can avoid rolling-update when user upgrade from helm chart, but I'm not sure if there is any side-effect, @cofyc PTAL

you don't do this because this is a new sts to deploy (no need to keep back-compatibility like pd/tikv sts)

you can always use POD_NAME for both pod network and host network mode because sts member address is always <pod-name>.

See the comment below, we have to consider the helm chart.

Actually I prefer to use POD_NAME, which is more accurate and has no hidden tricks, but we have to document that adopt pump via TidbCluster CR will trigger an rolling-update, this a trade-off, both looks not good but acceptable to me.

/cc @weekface @tennix how do you think?

What about adding a feature gate for adopting helm managed resources?

No gate is needed, if user leave the .spec.pump as nil, the helm resource will not be adopted. The real concern is that whether user has the ability to adopt pump cluster and do not trigger rolling-update. If not, abandon helm for existing cluster will require extra effort.

adapt old pump clusters and do not trigger rolling-update when update tidb-operator is reasonable for the user.

So users can allow the adoption when they need to update pump clusters? If so, then it's reasonable and we can document about this.

OK, if both approaches make sense, I pretend to keep it as is to avoid additional code change (mainly in e2e, there is a case to verify the sts do not get rolling-update after adoption, which need to be re-written if we choose to change the podTemplate in this PR), further improvement on the PodTemplate of pump could be made through other enhancement issues.

aylei · 2019-12-02T12:36:49Z

pkg/manager/member/pump_member_manager.go

+func getPumpStartScript(tc *v1alpha1.TidbCluster) (string, error) {
+	buff := new(bytes.Buffer)
+	// Keep the logic same as helm chart, but pump has not supported tls yet (no cert mounted)
+	// TODO: support tls


TLS of pump is not actually supported in tidb-cluster helm chart, nor in controller. We may document this limitation first @AstroProfundis PTAL

aylei · 2019-12-02T15:15:16Z

/run-e2e-in-kind

aylei · 2019-12-02T16:02:27Z

/run-e2e-test

cofyc · 2019-12-03T02:29:01Z

pkg/manager/member/pump_member_manager.go

+/pump \
+-pd-urls={{ .Scheme }}://{{ .ClusterName }}-pd:2379 \
+-L={{ .LogLevel }} \
+-advertise-addr=` + "`" + `echo ${HOSTNAME}` + "`" + `.{{ .ClusterName }}-pump:8250 \


this is strange, does this work? -advertise-addr={{.PodName}}.{{ .ClusterName }}-pump:8250
we don't need to pass HOSTNAME environment for sts pod, because the pod address is predictable

Use the HOSTNAME environment deliberately to keep backward compatibility with the helm chart, otherwise the pump statefulset will get rolling-update if we set the new env var POD_NAME.

tests/actions.go

pkg/manager/member/utils.go

manifests/webhook.yaml

pkg/apis/pingcap/v1alpha1/types.go

Co-Authored-By: Tennix <tennix@users.noreply.github.com>

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-12-03T04:04:01Z

/run-e2e-test

pkg/manager/member/pump_member_manager.go

aylei · 2019-12-03T04:20:25Z

In order to not blocking the development of #1192 #1264 #1263 , I will separate the configmapControlInterface to another PR.

tennix

LGTM

weekface

lgtm

aylei · 2019-12-03T10:09:50Z

/run-e2e-test

sync pump cluster in new pump_member_manager

626e033

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei requested review from weekface, cofyc, tennix and Yisaer December 2, 2019 12:20

aylei assigned weekface and tennix Dec 2, 2019

aylei mentioned this pull request Dec 2, 2019

Document how to manage pump in TidbCluster CR pingcap/docs-cn#2092

Closed

Merge branch 'master' into fix-1191

ee6272b

This was referenced Dec 2, 2019

sync pump status to TidbCluster CR #1271

Closed

Garbage collection of configmaps created by rolling-update configUpdateStrategy #1272

Closed

aylei commented Dec 2, 2019

View reviewed changes

aylei requested a review from AstroProfundis December 2, 2019 12:37

cofyc reviewed Dec 3, 2019

View reviewed changes

tests/actions.go Show resolved Hide resolved

Merge branch 'master' into fix-1191

cafd42a

tennix reviewed Dec 3, 2019

View reviewed changes

pkg/manager/member/utils.go Outdated Show resolved Hide resolved

manifests/webhook.yaml Show resolved Hide resolved

pkg/apis/pingcap/v1alpha1/types.go Outdated Show resolved Hide resolved

aylei and others added 2 commits December 3, 2019 11:31

Update pkg/manager/member/utils.go

4300f51

Co-Authored-By: Tennix <tennix@users.noreply.github.com>

address review comments, extract log-level from pump config

51060c5

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei requested review from cofyc and tennix December 3, 2019 04:05

tennix reviewed Dec 3, 2019

View reviewed changes

pkg/manager/member/pump_member_manager.go Show resolved Hide resolved

aylei mentioned this pull request Dec 3, 2019

Add ConfigMapControlInterface and grant configmap privilege #1275

Merged

Merge branch 'master' into fix-1191

700f06a

tennix approved these changes Dec 3, 2019

View reviewed changes

weekface approved these changes Dec 3, 2019

View reviewed changes

aylei merged commit 00c2fd8 into pingcap:master Dec 3, 2019

aylei mentioned this pull request Aug 5, 2020

Document how to manage pump in tidbcluster CR pingcap/docs-tidb-operator#625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync pump cluster in new pump_member_manager #1269

sync pump cluster in new pump_member_manager #1269

aylei commented Dec 2, 2019 •

edited

Loading

aylei commented Dec 2, 2019

aylei Dec 2, 2019

aylei Dec 2, 2019

aylei Dec 2, 2019

aylei Dec 2, 2019

aylei Dec 2, 2019

cofyc Dec 3, 2019 •

edited

Loading

aylei Dec 3, 2019

aylei Dec 3, 2019 •

edited

Loading

tennix Dec 3, 2019

aylei Dec 3, 2019

weekface Dec 3, 2019

weekface Dec 3, 2019

tennix Dec 3, 2019

aylei Dec 3, 2019

aylei Dec 2, 2019

aylei commented Dec 2, 2019

aylei commented Dec 2, 2019

cofyc Dec 3, 2019

aylei Dec 3, 2019

aylei commented Dec 3, 2019

aylei commented Dec 3, 2019

tennix left a comment

weekface left a comment

aylei commented Dec 3, 2019

sync pump cluster in new pump_member_manager #1269

sync pump cluster in new pump_member_manager #1269

Conversation

aylei commented Dec 2, 2019 • edited Loading

Check List

Does this PR introduce a user-facing change?:

aylei commented Dec 2, 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

cofyc Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei Dec 3, 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

aylei commented Dec 2, 2019

aylei commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Dec 3, 2019

aylei commented Dec 3, 2019

tennix left a comment

Choose a reason for hiding this comment

weekface left a comment

Choose a reason for hiding this comment

aylei commented Dec 3, 2019

aylei commented Dec 2, 2019 •

edited

Loading

cofyc Dec 3, 2019 •

edited

Loading

aylei Dec 3, 2019 •

edited

Loading