Fix TidbMonitor several error (#1962) (#1985)

* remain nodeport and fix env


* address the comment

Co-authored-by: Song Gao <2695690803@qq.com>

pkg/controller/generic_control.go

@@ -271,8 +271,27 @@ func (w *typedWrapper) CreateOrUpdateService(controller runtime.Object, svc *cor
 			}
 			existingSvc.Annotations[LastAppliedConfigAnnotation] = string(b)
 			clusterIp := existingSvc.Spec.ClusterIP
+			ports := existingSvc.Spec.Ports
+			serviceType := existingSvc.Spec.Type
+
 			existingSvc.Spec = desiredSvc.Spec
 			existingSvc.Spec.ClusterIP = clusterIp
+
+			// If the existed service and the desired service is NodePort or LoadBalancerType, we should keep the nodePort unchanged.
+			if (serviceType == corev1.ServiceTypeNodePort || serviceType == corev1.ServiceTypeLoadBalancer) &&
+				(desiredSvc.Spec.Type == corev1.ServiceTypeNodePort || desiredSvc.Spec.Type == corev1.ServiceTypeLoadBalancer) {
+				for i, dport := range existingSvc.Spec.Ports {
+					for _, eport := range ports {
+						// Because the portName could be edited,
+						// we use Port number to link the desired Service Port and the existed Service Port in the nested loop
+						if dport.Port == eport.Port && dport.Protocol == eport.Protocol {
+							dport.NodePort = eport.NodePort
+							existingSvc.Spec.Ports[i] = dport
+							break
+						}
+					}
+				}
+			}
 		}
 		return nil
 	})

pkg/monitor/monitor/monitor_manager.go

@@ -105,8 +105,8 @@ func (mm *MonitorManager) Sync(monitor *v1alpha1.TidbMonitor) error {
 }
 
 func (mm *MonitorManager) syncTidbMonitorService(monitor *v1alpha1.TidbMonitor) error {
-	service := getMonitorService(monitor)
-	for _, svc := range service {
+	services := getMonitorService(monitor)
+	for _, svc := range services {
 		_, err := mm.typedControl.CreateOrUpdateService(monitor, svc)
 		if err != nil {
 			klog.Errorf("tm[%s/%s]'s service[%s] failed to sync,err: %v", monitor.Namespace, monitor.Name, svc.Name, err)

pkg/monitor/monitor/util.go

@@ -17,6 +17,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"github.com/pingcap/tidb-operator/pkg/util"
+	"sort"
 	"strconv"
 
 	"github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1"
@@ -513,6 +514,7 @@ func getMonitorGrafanaContainer(secret *core.Secret, monitor *v1alpha1.TidbMonit
 	if monitor.Spec.Grafana.ImagePullPolicy != nil {
 		c.ImagePullPolicy = *monitor.Spec.Grafana.ImagePullPolicy
 	}
+	c.Env = sortEnvByName(c.Env)
 	return c
 }
 
@@ -668,9 +670,10 @@ func getMonitorService(monitor *v1alpha1.TidbMonitor) []*core.Service {
 		grafanaPortName = *monitor.BaseGrafanaSpec().PortName()
 	}
 
+	promethuesName := fmt.Sprintf("%s-prometheus", monitor.Name)
 	prometheusService := &core.Service{
 		ObjectMeta: meta.ObjectMeta{
-			Name:            fmt.Sprintf("%s-prometheus", monitor.Name),
+			Name:            promethuesName,
 			Namespace:       monitor.Namespace,
 			Labels:          monitorLabel,
 			OwnerReferences: []meta.OwnerReference{controller.GetTiDBMonitorOwnerRef(monitor)},
@@ -689,10 +692,15 @@ func getMonitorService(monitor *v1alpha1.TidbMonitor) []*core.Service {
 			Selector: labels,
 		},
 	}
+	if monitor.BasePrometheusSpec().ServiceType() == core.ServiceTypeLoadBalancer {
+		if monitor.Spec.Prometheus.Service.LoadBalancerIP != nil {
+			prometheusService.Spec.LoadBalancerIP = *monitor.Spec.Prometheus.Service.LoadBalancerIP
+		}
+	}
 
 	reloaderService := &core.Service{
 		ObjectMeta: meta.ObjectMeta{
-			Name:            fmt.Sprintf("%s-reloader", monitor.Name),
+			Name:            fmt.Sprintf("%s-monitor-reloader", monitor.Name),
 			Namespace:       monitor.Namespace,
 			Labels:          monitorLabel,
 			OwnerReferences: []meta.OwnerReference{controller.GetTiDBMonitorOwnerRef(monitor)},
@@ -715,6 +723,12 @@ func getMonitorService(monitor *v1alpha1.TidbMonitor) []*core.Service {
 		},
 	}
 
+	if monitor.BaseReloaderSpec().ServiceType() == core.ServiceTypeLoadBalancer {
+		if monitor.Spec.Reloader.Service.LoadBalancerIP != nil {
+			reloaderService.Spec.LoadBalancerIP = *monitor.Spec.Reloader.Service.LoadBalancerIP
+		}
+	}
+
 	services = append(services, prometheusService, reloaderService)
 	if monitor.Spec.Grafana != nil {
 		grafanaService := &core.Service{
@@ -741,6 +755,13 @@ func getMonitorService(monitor *v1alpha1.TidbMonitor) []*core.Service {
 				},
 			},
 		}
+
+		if monitor.BaseGrafanaSpec().ServiceType() == core.ServiceTypeLoadBalancer {
+			if monitor.Spec.Grafana.Service.LoadBalancerIP != nil {
+				grafanaService.Spec.LoadBalancerIP = *monitor.Spec.Grafana.Service.LoadBalancerIP
+			}
+		}
+
 		services = append(services, grafanaService)
 	}
 	return services
@@ -770,3 +791,27 @@ func getMonitorPVC(monitor *v1alpha1.TidbMonitor) *core.PersistentVolumeClaim {
 		},
 	}
 }
+
+// sortEnvByName in order to avoid syncing same template into different results
+func sortEnvByName(envlist []core.EnvVar) []core.EnvVar {
+	if envlist == nil || len(envlist) < 1 {
+		return envlist
+	}
+	var wrappers EnvListWrapper
+	wrappers = envlist
+	sort.Sort(wrappers)
+	return wrappers
+}
+
+type EnvListWrapper []core.EnvVar
+
+func (e EnvListWrapper) Len() int {
+	return len(e)
+}
+func (e EnvListWrapper) Swap(i, j int) {
+	e[i], e[j] = e[j], e[i]
+}
+
+func (e EnvListWrapper) Less(i, j int) bool {
+	return e[i].Name < e[j].Name
+}

tests/e2e/tidbcluster/tidbcluster.go

@@ -765,6 +765,58 @@ var _ = ginkgo.Describe("[tidb-operator] TiDBCluster", func() {
 		value, existed = pv.Labels[label.ManagedByLabelKey]
 		framework.ExpectEqual(existed, true)
 		framework.ExpectEqual(value, label.TiDBOperator)
+
+		// update TidbMonitor and check whether portName is updated and the nodePort is unchanged
+		tm, err = cli.PingcapV1alpha1().TidbMonitors(ns).Get(tm.Name, metav1.GetOptions{})
+		framework.ExpectNoError(err, "fetch latest tidbmonitor error")
+		tm.Spec.Prometheus.Service.Type = corev1.ServiceTypeNodePort
+		tm, err = cli.PingcapV1alpha1().TidbMonitors(ns).Update(tm)
+		framework.ExpectNoError(err, "update tidbmonitor service type error")
+
+		var targetPort int32
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {
+			prometheusSvc, err := c.CoreV1().Services(ns).Get(fmt.Sprintf("%s-prometheus", tm.Name), metav1.GetOptions{})
+			if err != nil {
+				return false, nil
+			}
+			if len(prometheusSvc.Spec.Ports) != 1 {
+				return false, nil
+			}
+			if prometheusSvc.Spec.Type != corev1.ServiceTypeNodePort {
+				return false, nil
+			}
+			targetPort = prometheusSvc.Spec.Ports[0].NodePort
+			return true, nil
+		})
+		framework.ExpectNoError(err, "first update tidbmonitor service error")
+
+		tm, err = cli.PingcapV1alpha1().TidbMonitors(ns).Get(tm.Name, metav1.GetOptions{})
+		framework.ExpectNoError(err, "fetch latest tidbmonitor again error")
+		newPortName := "any-other-word"
+		tm.Spec.Prometheus.Service.PortName = &newPortName
+		tm, err = cli.PingcapV1alpha1().TidbMonitors(ns).Update(tm)
+		framework.ExpectNoError(err, "update tidbmonitor service portName error")
+
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {
+			prometheusSvc, err := c.CoreV1().Services(ns).Get(fmt.Sprintf("%s-prometheus", tm.Name), metav1.GetOptions{})
+			if err != nil {
+				return false, nil
+			}
+			if len(prometheusSvc.Spec.Ports) != 1 {
+				return false, nil
+			}
+			if prometheusSvc.Spec.Type != corev1.ServiceTypeNodePort {
+				return false, nil
+			}
+			if prometheusSvc.Spec.Ports[0].Name != "any-other-word" {
+				return false, nil
+			}
+			if prometheusSvc.Spec.Ports[0].NodePort != targetPort {
+				return false, nil
+			}
+			return true, nil
+		})
+		framework.ExpectNoError(err, "second update tidbmonitor service error")
 	})
 
 	ginkgo.It("[Feature: AdvancedStatefulSet] Upgrading tidb cluster while pods are not consecutive", func() {

tests/pkg/fixture/fixture.go

@@ -182,6 +182,12 @@ func NewTidbMonitor(name, namespace string, tc *v1alpha1.TidbCluster, grafanaEna
 				Type:        corev1.ServiceTypeClusterIP,
 				Annotations: map[string]string{},
 			},
+			Envs: map[string]string{
+				"A":    "B",
+				"foo":  "hello",
+				"bar":  "query",
+				"some": "any",
+			},
 		}
 	}
 	if persist {