Fix service update which would changed the nodeport (#2284)

pkg/manager/member/tidb_member_manager.go

@@ -300,6 +300,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

@@ -1259,6 +1259,100 @@ var _ = ginkgo.Describe("[tidb-operator] TiDBCluster", func() {
 			framework.ExpectNoError(err)
 		})
 	})
+
+	ginkgo.It("Ensure Service NodePort Not Change", func() {
+		// 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(nodeTc, 30*time.Minute, 15*time.Second)
+		framework.ExpectNoError(err, "Expected TiDB cluster ready")
+
+		// expect tidb service type is Nodeport
+		var s *corev1.Service
+		err = wait.Poll(5*time.Second, 1*time.Minute, func() (done bool, err error) {
+			s, err = c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+			if err != nil {
+				framework.Logf(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
+
+		// f is the function to check whether service nodeport have changed for 1 min
+		f := func() error {
+			return wait.Poll(5*time.Second, 1*time.Minute, func() (done bool, err error) {
+				s, err := c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				if s.Spec.Type != corev1.ServiceTypeNodePort {
+					return false, err
+				}
+				for _, dport := range s.Spec.Ports {
+					for _, eport := range ports {
+						if dport.Port == eport.Port && dport.NodePort != eport.NodePort {
+							return false, fmt.Errorf("nodePort tidbcluster tidb service NodePort changed")
+						}
+					}
+				}
+				return false, nil
+			})
+		}
+		// check whether nodeport have changed for 1 min
+		err = f()
+		framework.ExpectEqual(err, wait.ErrWaitTimeout)
+		framework.Logf("tidbcluster tidb service NodePort haven't changed")
+
+		nodeTc, err = cli.PingcapV1alpha1().TidbClusters(ns).Get("nodeport", metav1.GetOptions{})
+		framework.ExpectNoError(err)
+		err = controller.GuaranteedUpdate(genericCli, nodeTc, func() error {
+			nodeTc.Spec.TiDB.Service.Annotations = map[string]string{
+				"foo": "bar",
+			}
+			return nil
+		})
+		framework.ExpectNoError(err)
+
+		// check whether the tidb svc have updated
+		err = wait.Poll(5*time.Second, 1*time.Minute, func() (done bool, err error) {
+			s, err := c.CoreV1().Services(ns).Get("nodeport-tidb", metav1.GetOptions{})
+			if err != nil {
+				return false, err
+			}
+			if s.Annotations == nil {
+				return false, nil
+			}
+			v, ok := s.Annotations["foo"]
+			if !ok {
+				return false, nil
+			}
+			if v != "bar" {
+				return false, fmt.Errorf("tidb svc annotation foo not equal bar")
+			}
+			return true, nil
+		})
+		framework.ExpectNoError(err)
+		framework.Logf("tidb nodeport svc updated")
+
+		// check whether nodeport have changed for 1 min
+		err = f()
+		framework.ExpectEqual(err, wait.ErrWaitTimeout)
+		framework.Logf("nodePort tidbcluster tidb service NodePort haven't changed after update")
+	})
 })
 
 func newTidbClusterConfig(cfg *tests.Config, ns, clusterName, password, tidbVersion string) tests.TidbClusterConfig {