UCP: support monitor mount external configuration #2294

mikechengwei · 2020-04-26T02:36:30Z

What problem does this PR solve?

What is changed and how does it work?

support monitor mount external config map.
user can create an external configMap contains prometheus-config and dashboard-config which replace default monitor config.

Check List

Tests

Unit test
E2E test

Code changes

Has Go code change

Side effects

None

…alConfigMaps

sre-bot · 2020-04-26T02:39:41Z

Thanks for your contribution. If your PR get merged, you will be rewarded 100 points.

Yisaer

The whole structures seems LGTM. What about prometheus start args?

mikechengwei · 2020-04-26T03:50:37Z

@Yisaer I think we can put the command configuration in the external configMap too.
for example

apiVersion: v1
kind: ConfigMap
metadata:
  name: test-monitor
  labels:
    app.kubernetes.io/name: tidb-cluster
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/instance: test
    app.kubernetes.io/component: monitor
    helm.sh/chart: tidb-cluster-dev
data:
  prometheus-config: |-
  command-config: |-
  dashboard-config: |-

What do you think？

Yisaer · 2020-04-26T04:50:04Z

@Yisaer I think we can put the command configuration in the external configMap too.
for example

apiVersion: v1
kind: ConfigMap
metadata:
  name: test-monitor
  labels:
    app.kubernetes.io/name: tidb-cluster
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/instance: test
    app.kubernetes.io/component: monitor
    helm.sh/chart: tidb-cluster-dev
data:
  prometheus-config: |-
  command-config: |-
  dashboard-config: |-

What do you think？

This LGTM. I think we could use a more easy understanding name for user instead of command-config.

…alConfigMaps � Conflicts: � docs/api-references/docs.md

Yisaer · 2020-04-26T06:50:07Z

pkg/apis/pingcap/v1alpha1/tidbmonitor_types.go

+
+// Config  is the the desired state of Prometheus Configuration
+type Configuration struct {
+	ExternalConfigMap string `json:"externalConfigMap,omitempty"`


What about use ConfigMapRef here?

Yisaer · 2020-04-26T06:51:07Z

When this pr is merged, please also update the monitor document to show how to use this feature for community.

Yisaer · 2020-04-26T07:08:45Z

@mikechengwei It seems the document check failed in github action test. Please remove the ./output/bin/gen-crd-api-references-docs and re-execute the ./hack/update-crd-groups.sh.

Recently we have update the document tool.

pkg/apis/pingcap/v1alpha1/tidbmonitor_types.go

yeya24 · 2020-04-26T15:03:35Z

pkg/apis/pingcap/v1alpha1/tidbmonitor_types.go

@@ -79,6 +79,13 @@ type PrometheusSpec struct {
 	Service  ServiceSpec `json:"service,omitempty"`
 	// +optional
 	ReserveDays int `json:"reserveDays,omitempty"`


@Yisaer Not related to this pr, but I am curious about this field. So we only support retention with unit d? Not support other time like h?

I think you are right. Maybe we would create a new proper property for this in the future.

I have support custom command ,so I think reserveDays or other time field is not necessary.

yeya24 · 2020-04-26T15:08:10Z

manifests/monitor/monitor-configMap.yaml

+                "folder": "",
+                "name": "0",
+                "options": {
+                    "path": "/grafana-dashboard-definitions/tidb"


How to support additional dashboards for tidb monitor in this pr? We should support this as well

It seems we could also export grafana config in the configmap.

If we support custom dashboards by configMap , we will need to append extra configMap to volumes and VolumeMounts . reference

tidb-operator/pkg/monitor/monitor/util.go

Line 312 in 7ccb528

VolumeMounts: []core.VolumeMount{

.
And I think users can dynamically add dashboard in Grafana.So, this feature is not important.

yeya24 · 2020-04-27T18:00:27Z

manifests/monitor/monitor-configMap.yaml

+            }
+        ]
+    }
+  start-command: |-


IMO the start-command is neither flexible nor scalable. A possible way is to do something like Prometheus-operator https://github.com/coreos/prometheus-operator/blob/87a7bea9f028c62396bef665f2887df326fa97f7/pkg/apis/monitoring/v1/types.go#L244. We can have a containers field in the tidb monitor spec. Then in the real generating phase, we do a patch to containers like here.

This approach is more recommended since it can help specify something other than the start commands for Prometheus, like env vars, volumes, etc

I have only support command field patch in container, other field patch I think is not important now.

yeya24 · 2020-05-11T18:37:15Z

Any updates @mikechengwei

mikechengwei · 2020-05-12T08:04:23Z

Sorry,I am busy recently. Now I will continue .

mikechengwei · 2020-05-28T08:07:49Z

/merge @Yisaer can you help me merge this pr?

Yisaer · 2020-05-28T08:12:52Z

@mikechengwei Currently we are releasing the new version. so please update the base branch tonight or tomorrow. or you can enable the maintainer edit so that I can update the base branch myself when newly-releases finished.

mikechengwei · 2020-05-28T12:18:27Z

I don't find button which enable the maintainer edit . I think be able to enable it only when creating pr . So you can tell me I will update branch myself. @Yisaer

…alConfigMap

…perator into externalConfigMap

Yisaer · 2020-05-29T05:11:02Z

/merge

sre-bot · 2020-05-29T05:11:34Z

/run-all-tests

sre-bot · 2020-05-29T05:57:35Z

@mikechengwei merge failed.

mikechengwei · 2020-05-29T06:05:01Z

This e2e test error may not be caused by my code.

Yisaer · 2020-05-29T06:06:58Z

/merge

sre-bot · 2020-05-29T06:07:00Z

Your auto merge job has been accepted, waiting for:

2445

Yisaer · 2020-05-29T07:40:44Z

@mikechengwei could you update the base branch? or you can enable maintainers edit like following.

mikechengwei · 2020-05-29T08:11:14Z

@Yisaer

mikechengwei · 2020-05-29T08:12:18Z

@Yisaer I have update base branch

Yisaer · 2020-05-29T12:52:31Z

/merge

sre-bot · 2020-05-29T12:54:10Z

/run-all-tests

cofyc · 2020-05-29T13:12:53Z

I guess it's the limitation of Github. This feature requires the PR is from a user-owned repo, but your PR is from a team org.
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

cofyc · 2020-05-29T13:17:25Z

Maybe we can merge the PR onto the base branch in CI and don't require the PR branch to be up to date before merging. (ref: #396)

Because all PRs are merged by sre-bot sequentially, no broken state can happen on the master.

mikechengwei · 2020-05-29T13:44:02Z

I agree, frequently up to date is a trouble

sre-bot · 2020-05-29T13:57:09Z

Congratulation! You have awarded a badge for usability challenge program! Please fill the form to get your reward! http://tidbcommunity.mikecrm.com/QMCv4QL

sre-bot · 2020-05-29T13:57:10Z

Team bigger complete task #2180 and get 300 score, current score 100

sre-bot · 2020-05-29T13:57:46Z

cherry pick to release-1.1 in PR #2594

Co-authored-by: Mike <842725815@qq.com>

mikechengwei added 6 commits January 26, 2020 00:57

e2e case for clear TiDB failureMembers when scale TiDB to zero

cdc9561

increase timeout time

1b4149b

optimize comment

b659c07

optimize comment

08b16cf

Merge branch 'master' of github.com:pingcap/tidb-operator

d9415cc

support monitor external config map

7c9cc90

sre-bot added the contribution label Apr 26, 2020

Merge branch 'master' of github.com:pingcap/tidb-operator into extern…

8b302f9

…alConfigMaps

mikechengwei added 2 commits April 26, 2020 10:46

fix fake configmap

090e010

fix fake configmap

5beecd3

Yisaer reviewed Apr 26, 2020

View reviewed changes

update docs

b17dbda

mikechengwei added 3 commits April 26, 2020 13:48

Merge branch 'master' of github.com:pingcap/tidb-operator into extern…

4b0be1c

…alConfigMaps � Conflicts: � docs/api-references/docs.md

add start-command config

679c19a

fix configMap demo

f66642c

Yisaer reviewed Apr 26, 2020

View reviewed changes

use ConfigMapRef field

4fc8ec1

update docs

4ca7926

Yisaer reviewed Apr 26, 2020

View reviewed changes

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

Yisaer requested a review from yeya24 April 26, 2020 07:55

Merge branch 'master' into externalConfigMap

d630a33

yeya24 reviewed Apr 26, 2020

View reviewed changes

yeya24 reviewed Apr 27, 2020

View reviewed changes

mikechengwei added 2 commits May 27, 2020 21:11

Merge branch 'master' into externalConfigMap

4959d64

Merge branch 'master' into externalConfigMap

813acae

mikechengwei added 3 commits May 28, 2020 20:18

Merge branch 'master' into externalConfigMap

41f5823

Merge branch 'master' of github.com:pingcap/tidb-operator into extern…

364498a

…alConfigMap

Merge branch 'externalConfigMap' of github.com:tongcheng-elong/tidb-o…

c30ef23

…perator into externalConfigMap

Merge branch 'master' into externalConfigMap

1793824

sre-bot merged commit 8ef0a99 into pingcap:master May 29, 2020

sre-bot pushed a commit to sre-bot/tidb-operator that referenced this pull request May 29, 2020

UCP: support monitor mount external configuration (pingcap#2294)

0765aa7

sre-bot mentioned this pull request May 29, 2020

UCP: support monitor mount external configuration (#2294) #2594

Merged

Yisaer pushed a commit that referenced this pull request May 29, 2020

UCP: support monitor mount external configuration (#2294) (#2594)

a0f5224

Co-authored-by: Mike <842725815@qq.com>

UCP: support monitor mount external configuration #2294

UCP: support monitor mount external configuration #2294

Conversation

mikechengwei commented Apr 26, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

sre-bot commented Apr 26, 2020

Yisaer left a comment

Choose a reason for hiding this comment

mikechengwei commented Apr 26, 2020

Yisaer commented Apr 26, 2020

Yisaer Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

mikechengwei Apr 26, 2020

Choose a reason for hiding this comment

Yisaer commented Apr 26, 2020 • edited Loading

Yisaer commented Apr 26, 2020 • edited Loading

yeya24 Apr 26, 2020

Choose a reason for hiding this comment

Yisaer Apr 27, 2020

Choose a reason for hiding this comment

mikechengwei May 14, 2020

Choose a reason for hiding this comment

yeya24 Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

Yisaer Apr 27, 2020

Choose a reason for hiding this comment

mikechengwei May 14, 2020

Choose a reason for hiding this comment

yeya24 Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

mikechengwei May 14, 2020

Choose a reason for hiding this comment

yeya24 commented May 11, 2020

mikechengwei commented May 12, 2020

mikechengwei commented May 28, 2020

Yisaer commented May 28, 2020 • edited Loading

mikechengwei commented May 28, 2020

Yisaer commented May 29, 2020

sre-bot commented May 29, 2020

sre-bot commented May 29, 2020

mikechengwei commented May 29, 2020

Yisaer commented May 29, 2020

sre-bot commented May 29, 2020

Yisaer commented May 29, 2020

mikechengwei commented May 29, 2020

mikechengwei commented May 29, 2020

Yisaer commented May 29, 2020

sre-bot commented May 29, 2020

cofyc commented May 29, 2020

cofyc commented May 29, 2020 • edited Loading

mikechengwei commented May 29, 2020

sre-bot commented May 29, 2020

sre-bot commented May 29, 2020

sre-bot commented May 29, 2020

mikechengwei commented Apr 26, 2020 •

edited

Loading

Yisaer Apr 26, 2020 •

edited

Loading

Yisaer commented Apr 26, 2020 •

edited

Loading

Yisaer commented Apr 26, 2020 •

edited

Loading

yeya24 Apr 26, 2020 •

edited

Loading

yeya24 Apr 27, 2020 •

edited

Loading

Yisaer commented May 28, 2020 •

edited

Loading

cofyc commented May 29, 2020 •

edited

Loading