Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support mysqlNodePort and statusNodePort #2941

Merged
merged 13 commits into from
Jul 17, 2020

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

resolve #2915. support user specied mysqlNodePort and statusNodePort

What is changed and how does it work?

  1. Add MySQLNodePort and StatusNodePort field to support specified mysqlNodePort and `statusNodePort.
  2. Don't retain old svc config if MySQLNodePort or StatusNodePort is set.

Check List

Tests

  • Unit test
  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)
    Test manually set mySQLNodePort and statusNodePort. They can work in svc. Besides, if we don't set them, the svc port won't change every time.

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:

Support specifying mysqlNodePort and statusNodePort for TiDB Service Spec.

@@ -33,5 +33,8 @@ spec:
baseImage: pingcap/tidb
replicas: 1
service:
type: ClusterIP
type: NodePort
exposeStatus: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is updated by mistake.. I think we don't need to update this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 2a9b38e

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not necessary for the basic example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted in da93fcb

pkg/util/util.go Outdated
@@ -240,6 +240,13 @@ func RetainManagedFields(desiredSvc, existedSvc *corev1.Service) {
// Retain NodePorts
for id, dport := range desiredSvc.Spec.Ports {
for _, eport := range existedSvc.Spec.Ports {
if tidbSvcSpec != nil {
if dport.Name == "mysql-client" && tidbSvcSpec.GetMySQLNodePort() != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The port name for tidb is configurable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this should be a common function that may be called by other service sync functions and may also add other fields (even if not now), is it better to handle this out of this function? WDYT @lichunzhu @cofyc

Copy link
Contributor

@DanielZhangQD DanielZhangQD Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we may check dport.NodePort, if !=0, skip the logic to get port from existedSvc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice advice, updated in 2a9b38e

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #2941 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2941   +/-   ##
=======================================
  Coverage   40.91%   40.91%           
=======================================
  Files         153      153           
  Lines       16748    16752    +4     
=======================================
+ Hits         6852     6854    +2     
- Misses       9354     9355    +1     
- Partials      542      543    +1     
Flag Coverage Δ
#unittest 40.91% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@DanielZhangQD DanielZhangQD merged commit 29bc884 into pingcap:master Jul 17, 2020
ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 17, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-1.1 in PR #2986

DanielZhangQD pushed a commit that referenced this pull request Jul 17, 2020
* cherry pick #2941 to release-1.1

Signed-off-by: ti-srebot <[email protected]>

* regenerate code

Co-authored-by: Chunzhu Li <[email protected]>
@lichunzhu lichunzhu deleted the supportNodePort branch September 2, 2020 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support mysqlNodePort and statusNodePort
5 participants