Make service port-name configurable in monitor #1521

Yisaer · 2020-01-09T06:37:00Z

What problem does this PR solve?

#1512
Make service port-name configurable in the monitor (Both TidbMonitor and monitor config in values.yaml).

Check List

Tests

Manual test

Yisaer · 2020-01-09T06:50:22Z

/run-e2e-in-kind

aylei

what about the PD and TiDB services?

charts/tidb-cluster/values.yaml

pkg/monitor/monitor/util.go

Yisaer · 2020-01-10T05:21:53Z

what about the PD and TiDB services?

I think we could create a new issue as good first issue for community contributors to finish other service port name config work. @aylei

ref #1538

aylei · 2020-01-15T12:33:00Z

pkg/monitor/monitor/util.go

+					break
+				}
+			}
+		}


This is redundant, why not use the desired port name at the first place?

grafanaPortName := "http-grafana" if monitor.Spec.Grafana.Service.PortName != nil { grafanaPortName := *monitor.Spec.Grafana.Service.PortName } ... Name: grafanaPortName, ...

cofyc · 2020-01-16T07:56:07Z

pkg/monitor/monitor/util.go

+	prometheusPortName := "http-prometheus"
+	grafanaPortName := "http-grafana"
+
+	if monitor.Spec.Reloader.Service.PortName != nil {


what if PortName is non-nil but empty, should we use the default name in that case?
cc @aylei do you recommend to use *string or string?

what if PortName is non-nil but empty, should we use the default name in that case?
cc @aylei do you recommend to use *string or string?

I think operator don't need to consider this case, if monitor service synced failed because of setting wrong by users, users should be responsible for it.

It is about our purpose. For kubernetes, port name is optional and could be empty, so the current code is okay. If we think empty is not a valid value, then we could consider empty as unset, pick default for it and use string as the type, otherwise, just keep *string

cofyc

LGTM

aylei · 2020-01-16T08:19:22Z

pkg/monitor/monitor/util.go

+	reloaderPortName := "tcp-reloader"
+	prometheusPortName := "http-prometheus"
+	grafanaPortName := "http-grafana"
+
+	if monitor.Spec.Reloader.Service.PortName != nil {


I recommend add an access method for each Spec to get the port name to encapsulate the details of port name calculation.

cofyc · 2020-01-17T05:16:58Z

pkg/apis/pingcap/v1alpha1/tidbmonitor.go

+
+func (tm *TidbMonitor) ReloaderPortName() *string {
+	return tm.Spec.Reloader.Service.PortName
+}


move all default values here, and in controller logic we can simply use TidbMonitor.(PrometheusPortName|GrafanaPortName|ReloaderPortName)? like

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

Lines 373 to 379 in d314b25

Affinity: spec.Affinity(),

Tolerations: spec.Tolerations(),

NodeSelector: spec.NodeSelector(),

SchedulerName: spec.SchedulerName(),

SecurityContext: spec.PodSecurityContext(),

HostNetwork: spec.HostNetwork(),

DNSPolicy: spec.DnsPolicy(),

cofyc

LGTM

aylei

LGTM

sre-bot · 2020-01-17T07:17:57Z

cherry pick to release-1.1 in PR #1578

* add port name * Update zz_generated.deepcopy.go * fix * Update values.yaml * Update util.go * add Monitor Portname access * exponse public access for tibdmonitor Co-authored-by: Song Gao <disxiaofei@163.com>

add port name

eefe7fb

aylei reviewed Jan 9, 2020

View reviewed changes

charts/tidb-cluster/values.yaml Outdated Show resolved Hide resolved

pkg/monitor/monitor/util.go Outdated Show resolved Hide resolved

pkg/monitor/monitor/util.go Outdated Show resolved Hide resolved

pkg/monitor/monitor/util.go Outdated Show resolved Hide resolved

Song Gao added 2 commits January 9, 2020 15:19

Update zz_generated.deepcopy.go

c7ce070

fix

54a8e84

aylei added the needs-cherry-pick-1.1 label Jan 9, 2020

Song Gao and others added 2 commits January 9, 2020 16:58

Update values.yaml

7cbd58f

Merge branch 'master' into config_service_port_name

11ed589

Yisaer requested a review from aylei January 9, 2020 10:18

Merge branch 'master' into config_service_port_name

f0a4d5f

Yisaer mentioned this pull request Jan 10, 2020

Make Service port name configurable for tidb and pd service #1538

Closed

Yisaer added 5 commits January 10, 2020 13:54

Merge branch 'master' into config_service_port_name

390b629

Merge branch 'master' into config_service_port_name

c813b99

Merge branch 'master' into config_service_port_name

bfbbad5

Merge branch 'master' into config_service_port_name

98fe796

Merge branch 'master' into config_service_port_name

279908b

aylei reviewed Jan 15, 2020

View reviewed changes

Yisaer and others added 3 commits January 15, 2020 22:55

Merge branch 'master' into config_service_port_name

58136fb

Merge branch 'master' into config_service_port_name

5960e1b

Update util.go

756c567

Yisaer requested review from aylei and cofyc January 16, 2020 07:30

Merge branch 'master' into config_service_port_name

a6ed1e0

cofyc reviewed Jan 16, 2020

View reviewed changes

cofyc previously approved these changes Jan 16, 2020

View reviewed changes

aylei reviewed Jan 16, 2020

View reviewed changes

Yisaer and others added 2 commits January 16, 2020 18:33

Merge branch 'master' into config_service_port_name

bacda91

add Monitor Portname access

73f8ebb

Yisaer dismissed cofyc’s stale review via 73f8ebb January 17, 2020 05:04

Yisaer requested review from aylei and cofyc January 17, 2020 05:06

cofyc reviewed Jan 17, 2020

View reviewed changes

exponse public access for tibdmonitor

22f936c

cofyc approved these changes Jan 17, 2020

View reviewed changes

Merge branch 'master' into config_service_port_name

6fe6733

Yisaer added the area/monitor monitoring label Jan 17, 2020

aylei approved these changes Jan 17, 2020

View reviewed changes

aylei merged commit 97d07a1 into pingcap:master Jan 17, 2020

sre-bot mentioned this pull request Jan 17, 2020

Make service port-name configurable in monitor (#1521) #1578

Merged

Yisaer mentioned this pull request Jan 17, 2020

Follow istio's service port requirements #1512

Closed

Yisaer deleted the config_service_port_name branch February 26, 2020 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make service port-name configurable in monitor #1521

Make service port-name configurable in monitor #1521

Yisaer commented Jan 9, 2020

Yisaer commented Jan 9, 2020

aylei left a comment

Yisaer commented Jan 10, 2020 •

edited

Loading

aylei Jan 15, 2020

cofyc Jan 16, 2020

Yisaer Jan 16, 2020

aylei Jan 16, 2020 •

edited

Loading

cofyc left a comment

aylei Jan 16, 2020

Yisaer Jan 17, 2020

cofyc Jan 17, 2020 •

edited

Loading

cofyc left a comment

aylei left a comment

sre-bot commented Jan 17, 2020

	Affinity: spec.Affinity(),
	Tolerations: spec.Tolerations(),
	NodeSelector: spec.NodeSelector(),
	SchedulerName: spec.SchedulerName(),
	SecurityContext: spec.PodSecurityContext(),
	HostNetwork: spec.HostNetwork(),
	DNSPolicy: spec.DnsPolicy(),

Make service port-name configurable in monitor #1521

Make service port-name configurable in monitor #1521

Conversation

Yisaer commented Jan 9, 2020

What problem does this PR solve?

Check List

Yisaer commented Jan 9, 2020

aylei left a comment

Choose a reason for hiding this comment

Yisaer commented Jan 10, 2020 • edited Loading

aylei Jan 15, 2020

Choose a reason for hiding this comment

cofyc Jan 16, 2020

Choose a reason for hiding this comment

Yisaer Jan 16, 2020

Choose a reason for hiding this comment

aylei Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

cofyc left a comment

Choose a reason for hiding this comment

aylei Jan 16, 2020

Choose a reason for hiding this comment

Yisaer Jan 17, 2020

Choose a reason for hiding this comment

cofyc Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

cofyc left a comment

Choose a reason for hiding this comment

aylei left a comment

Choose a reason for hiding this comment

sre-bot commented Jan 17, 2020

Yisaer commented Jan 10, 2020 •

edited

Loading

aylei Jan 16, 2020 •

edited

Loading

cofyc Jan 17, 2020 •

edited

Loading