Fix service update which would changed the nodeport #2284

Yisaer · 2020-04-24T07:46:12Z

What problem does this PR solve?

currently if we make tidb service type as nodeport, it would change the nodeport in every reconcilation. This pr fix this bug.

What is changed and how does it work?

Check List

Tests

E2E test

Related changes

Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

Fix the bug when `TidbCluster` service type is `NodePort`, the nodeport would change frequently.

cofyc · 2020-04-24T08:22:09Z

pkg/controller/service_control.go

+						}
+					}
+				}
+			}


why the node port may change? because we empty the NodePort field each time? what's your solution to fix this?

UpdateService is a generic function to update service and resolve conflicts if necessary. Trying to implement this logic in this function is strange. I think we should generate the correct desired service spec before calling the update API. And generating the correct desired service can help avoid unnecessary API updates (skip if the desired spec matches existing spec).

This sounds make sense, updated.

…b-operator into fix_tidb_nodeport_svc

DanielZhangQD · 2020-04-24T11:26:05Z

tests/e2e/tidbcluster/tidbcluster.go

+			framework.Failf("nodePort tidbcluster tidb service type isn't NodePort")
+		}
+		ports := s.Spec.Ports
+		// check node port unchanged for 5mins


Update some attributes of the service and then check no change for nodeport?

DanielZhangQD

LGTM

DanielZhangQD · 2020-04-26T03:42:11Z

@Yisaer please also check why the nodePort changed even if there is no update to the service configuration, maybe some checks are not correct which causes the service to be updated during each sync.

Yisaer · 2020-04-26T03:45:33Z

o check why the nodePort changed even if there is no update to the service configuration, maybe some checks are not correct which causes the service to b

I think the previous disscusion have already answered the question. The nodeport changed because we left nodeport empty during each syncing.

cofyc · 2020-04-26T04:57:39Z

tests/e2e/tidbcluster/tidbcluster.go

+			}
+			return false, nil
+		})
+		framework.ExpectError(err)


framework.ExpectEqual(err, wait.ErrWaitTimeout)

actually, you can return these errors in this wait function. this will make this function shorter and errors will be printed if the test fails because what you expect is Timeout error.

cofyc · 2020-04-26T04:58:07Z

tests/e2e/tidbcluster/tidbcluster.go

+		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())


actually, you can return these errors in this wait function. this will make this function shorter and errors will be printed if the test fails because what you expect is Timeout error.

cofyc · 2020-04-26T05:03:04Z

tests/e2e/tidbcluster/tidbcluster.go

+		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())


better to use framework.Logf which writes to ginkgo writer and the logs can be printed into ginkgo test report if failed.

cofyc · 2020-04-26T05:03:43Z

tests/e2e/tidbcluster/tidbcluster.go

+		ports := s.Spec.Ports
+
+		// check node port unchanged for 5mins
+		err = wait.Poll(5*time.Second, 5*time.Minute, func() (done bool, err error) {


1 minute IMO is enough because our reconciliation period is 30 seconds.

cofyc · 2020-04-26T05:04:21Z

tests/e2e/tidbcluster/tidbcluster.go

+			"foo": "bar",
+		}
+		_, err = cli.PingcapV1alpha1().TidbClusters(ns).Update(tc)
+		framework.ExpectNoError(err)


use previous nodeTC and controller.GuaranteedUpdate, e.g.

err = controller.GuaranteedUpdate(genericCli, nodeTc, func() error { nodeTc.Spec.TiDB.Service.Annotations = map[string]string{ "foo": "bar", } return nil })

controller.GuaranteedUpdate( can help resolve conflicts (status of TidbCluster objects are updated by controller frequently) and make our test stable.

cofyc · 2020-04-26T05:09:08Z

tests/e2e/tidbcluster/tidbcluster.go

+			}
+			return true, nil
+		})
+		framework.ExpectNoError(err)


you should wait for Timeout error to happen (NodePort not changed after the update in a few minutes). IMO you can reuse previous wait function. Testing the update of service succeeds or not is out of the scope of this test.

DanielZhangQD · 2020-04-26T05:37:17Z

o check why the nodePort changed even if there is no update to the service configuration, maybe some checks are not correct which causes the service to b

I think the previous disscusion have already answered the question. The nodeport changed because we left nodeport empty during each syncing.

I mean what why the service is updated even if no change to the service config in the tc, I have checked this and found that ClusterIP is included in the ServiceLastAppliedConfigAnnotation, but there is no ClusterIP in the new service spec, so the quality check always returns false here, please fix this issue as it's the cause of it would change the nodeport in every reconcilation in the issue description.

Yisaer · 2020-04-26T06:05:35Z

o check why the nodePort changed even if there is no update to the service configuration, maybe some checks are not correct which causes the service to b

I think the previous disscusion have already answered the question. The nodeport changed because we left nodeport empty during each syncing.

I mean what why the service is updated even if no change to the service config in the tc, I have checked this and found that ClusterIP is included in the ServiceLastAppliedConfigAnnotation, but there is no ClusterIP in the new service spec, so the quality check always returns false here, please fix this issue as it's the cause of it would change the nodeport in every reconcilation in the issue description.

I suggest to create a new issue to solve this problem.

Yisaer · 2020-04-26T06:52:52Z

/run-e2e-in-kind

DanielZhangQD

LGTM

sre-bot · 2020-05-07T12:28:29Z

cherry pick to release-1.1 in PR #2390

Co-authored-by: Song Gao <disxiaofei@163.com>

fix nodeport change bug

2ceba12

Yisaer added type/bug Something isn't working needs-cherry-pick-1.1 labels Apr 24, 2020

Yisaer requested review from weekface, cofyc and DanielZhangQD April 24, 2020 07:54

cofyc reviewed Apr 24, 2020

View reviewed changes

Song Gao and others added 4 commits April 24, 2020 17:00

add method

2c4f472

Merge branch 'master' into fix_tidb_nodeport_svc

bdcb729

remove useless

5244fd6

Merge branch 'fix_tidb_nodeport_svc' of https://github.com/Yisaer/tid…

eb46821

…b-operator into fix_tidb_nodeport_svc

DanielZhangQD reviewed Apr 24, 2020

View reviewed changes

Song Gao and others added 3 commits April 24, 2020 20:32

add check

09c0b4a

fix e2e test

425006c

Merge branch 'master' into fix_tidb_nodeport_svc

9b3b621

DanielZhangQD previously approved these changes Apr 26, 2020

View reviewed changes

DanielZhangQD self-requested a review April 26, 2020 03:30

Yisaer requested a review from cofyc April 26, 2020 04:18

Merge branch 'master' into fix_tidb_nodeport_svc

b627042

cofyc reviewed Apr 26, 2020

View reviewed changes

update e2e

79ff420

Yisaer dismissed DanielZhangQD’s stale review via 79ff420 April 26, 2020 06:39

DanielZhangQD mentioned this pull request Apr 26, 2020

services are updated during every each sync #2299

Merged

Merge branch 'master' into fix_tidb_nodeport_svc

9064254

DanielZhangQD approved these changes Apr 26, 2020

View reviewed changes

Yisaer requested a review from cofyc April 27, 2020 02:16

sre-bot closed this in #2299 Apr 27, 2020

sre-bot mentioned this pull request Apr 27, 2020

services are updated during every each sync (#2299) #2310

Merged

Yisaer reopened this May 2, 2020

Yisaer added 2 commits May 2, 2020 13:36

Merge branch 'master' into fix_tidb_nodeport_svc

0ba603f

Merge branch 'master' into fix_tidb_nodeport_svc

4130cfa

cofyc approved these changes May 6, 2020

View reviewed changes

Merge branch 'master' into fix_tidb_nodeport_svc

a933c40

Yisaer merged commit ac2b137 into pingcap:master May 7, 2020

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

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

4f2bba8

sre-bot mentioned this pull request May 7, 2020

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

Merged

Yisaer added a commit that referenced this pull request May 8, 2020

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

2b870b7

Co-authored-by: Song Gao <disxiaofei@163.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix service update which would changed the nodeport #2284

Fix service update which would changed the nodeport #2284

Yisaer commented Apr 24, 2020 •

edited

Loading

cofyc Apr 24, 2020 •

edited

Loading

Yisaer Apr 24, 2020

DanielZhangQD Apr 24, 2020

Yisaer Apr 25, 2020

Yisaer Apr 25, 2020

DanielZhangQD left a comment

DanielZhangQD commented Apr 26, 2020

Yisaer commented Apr 26, 2020

cofyc Apr 26, 2020

cofyc Apr 26, 2020

cofyc Apr 26, 2020

cofyc Apr 26, 2020

cofyc Apr 26, 2020

Yisaer Apr 27, 2020

cofyc Apr 26, 2020

Yisaer Apr 27, 2020

cofyc Apr 26, 2020

Yisaer Apr 27, 2020

cofyc Apr 26, 2020 •

edited

Loading

Yisaer Apr 27, 2020

DanielZhangQD commented Apr 26, 2020

Yisaer commented Apr 26, 2020

Yisaer commented Apr 26, 2020

DanielZhangQD left a comment

sre-bot commented May 7, 2020

Fix service update which would changed the nodeport #2284

Fix service update which would changed the nodeport #2284

Conversation

Yisaer commented Apr 24, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

cofyc Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

DanielZhangQD commented Apr 26, 2020

Yisaer commented Apr 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD commented Apr 26, 2020

Yisaer commented Apr 26, 2020

Yisaer commented Apr 26, 2020

DanielZhangQD left a comment

Choose a reason for hiding this comment

sre-bot commented May 7, 2020

Yisaer commented Apr 24, 2020 •

edited

Loading

cofyc Apr 24, 2020 •

edited

Loading

cofyc Apr 26, 2020 •

edited

Loading