Fix service update which would changed the nodeport

pkg/manager/member/tidb_member_manager.go

@@ -276,6 +276,7 @@ func (tmm *tidbMemberManager) syncTiDBService(tc *v1alpha1.TidbCluster) error {
 		return err
 	}
 	oldSvc := oldSvcTmp.DeepCopy()
+	util.RemainNodeport(newSvc, oldSvc)
 
 	equal, err := controller.ServiceEqual(newSvc, oldSvc)
 	if err != nil {

pkg/util/util.go

@@ -226,3 +226,19 @@ func IsOwnedByTidbCluster(obj metav1.Object) (bool, *metav1.OwnerReference) {
 	}
 	return ref.Kind == v1alpha1.TiDBClusterKind && gv.Group == v1alpha1.SchemeGroupVersion.Group, ref
 }
+
+// RemainNodeport is to make service nodeport unchanged during each reconciliation
+func RemainNodeport(desiredSvc, existedSvc *corev1.Service) {
+	if desiredSvc.Spec.Type != corev1.ServiceTypeNodePort && desiredSvc.Spec.Type != corev1.ServiceTypeLoadBalancer {
+		return
+	}
+	for id, dport := range desiredSvc.Spec.Ports {
+		for _, eport := range existedSvc.Spec.Ports {
+			if dport.Port == eport.Port && dport.Protocol == eport.Protocol {
+				dport.NodePort = eport.NodePort
+				desiredSvc.Spec.Ports[id] = dport
+				break
+			}
+		}
+	}
+}

tests/e2e/tidbcluster/tidbcluster.go

@@ -528,6 +528,92 @@ var _ = ginkgo.Describe("[tidb-operator] TiDBCluster", func() {
 		})
 
 		framework.ExpectNoError(err)
+
+		// Create TidbCluster with NodePort to check whether node port would change
+		nodeTc := fixture.GetTidbCluster(ns, "nodeport", utilimage.TiDBV3Version)
+		nodeTc.Spec.PD.Replicas = 1
+		nodeTc.Spec.TiKV.Replicas = 1
+		nodeTc.Spec.TiDB.Replicas = 1
+		nodeTc.Spec.TiDB.Service = &v1alpha1.TiDBServiceSpec{
+			ServiceSpec: v1alpha1.ServiceSpec{
+				Type: corev1.ServiceTypeNodePort,
+			},
+		}
+		err = genericCli.Create(context.TODO(), nodeTc)
+		framework.ExpectNoError(err, "Expected TiDB cluster created")
+		err = oa.WaitForTidbClusterReady(tc, 30*time.Minute, 15*time.Second)
+		framework.ExpectNoError(err, "Expected TiDB cluster ready")
+
+		var s *corev1.Service
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {
+			s, err = c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+			if err != nil {
+				klog.Errorf(err.Error())
+				return false, nil
+			}
+			if s.Spec.Type != corev1.ServiceTypeNodePort {
+				return false, fmt.Errorf("nodePort tidbcluster tidb service type isn't NodePort")
+			}
+			return true, nil
+		})
+		framework.ExpectNoError(err)
+		ports := s.Spec.Ports
+
+		// check node port unchanged for 5mins
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {
+			s, err := c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+			if err != nil {
+				klog.Errorf(err.Error())
+				return true, nil
+			}
+			if s.Spec.Type != corev1.ServiceTypeNodePort {
+				klog.Error("nodePort tidbcluster tidb service type isn't NodePort")
+				return true, nil
+			}
+			for _, dport := range s.Spec.Ports {
+				for _, eport := range ports {
+					if dport.Port == eport.Port && dport.NodePort != eport.NodePort {
+						klog.Error("nodePort tidbcluster tidb service NodePort changed")
+						return true, nil
+					}
+				}
+			}
+			return false, nil
+		})
+		framework.ExpectError(err)
+		klog.Info("nodePort tidbcluster tidb service NodePort haven't changed")
+
+		tc, err = cli.PingcapV1alpha1().TidbClusters(ns).Get("nodeport", metav1.GetOptions{})
+		framework.ExpectNoError(err)
+		tc.Spec.TiDB.Service.Annotations = map[string]string{
+			"foo": "bar",
+		}
+		_, err = cli.PingcapV1alpha1().TidbClusters(ns).Update(tc)
+		framework.ExpectNoError(err)
+
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {
+			s, err = c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+			if err != nil {
+				return false, nil
+			}
+			if s.Annotations == nil {
+				return false, nil
+			}
+			v, ok := s.Annotations["foo"]
+			if !ok || v != "bar" {
+				return false, nil
+			}
+			for _, oport := range ports {
+				for _, eport := range s.Spec.Ports {
+					if oport.Port == eport.Port && oport.Protocol == eport.Protocol && oport.NodePort != eport.NodePort {
+						return false, fmt.Errorf("node port has been changed")
+					}
+				}
+			}
+			return true, nil
+		})
+		framework.ExpectNoError(err)
+		klog.Info("nodePort tidbcluster tidb service NodePort haven't changed after update")
 	})
 
 	updateStrategy := v1alpha1.ConfigUpdateStrategyInPlace