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.

Sign up for GitHub

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

Jump to bottom

Add readinessProbe config for tidb #3438

Merged
merged 6 commits into from
Oct 30, 2020
Merged

Add readinessProbe config for tidb #3438

merged 6 commits into from
Oct 30, 2020

Conversation

july2993
Copy link
Contributor

@july2993 july2993 commented Oct 28, 2020 •
edited by DanielZhangQD
Loading

What problem does this PR solve?

Add readinessProbe config for tidb to support probe tidb's readiness by requesting 127.0.0.0:10080/status

fix #2132

What is changed and how does it work?

ref pingcap/tidb#20649
Add this config support like this:

tidb:
  readinessProbe:
    type: command

This will make tidb pod use Exec handle to run curl probe the url 127.0.0.1:10080/status
Default still use TCPPPort(4000) for compatibility

Check List

Tests

Side effects

Related changes

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

Does this PR introduce a user-facing change?:

Add `spec.tidb.readinessProbe` config to support requesting `127.0.0.0:10080/status` for TiDB's readiness probe, TiDB version >= v4.0.9 required

@july2993
Add readinessProbe config for tidb
b4ac7cb 
ref pingcap/tidb#20649
Add this config support like this:
```
tidb:
  readinessProbe:
  statusAPI: {}
```
This will make tidb pod use Exec handle to run curl probe the url 127.0.0.1:10080/status
Default still use TCPPPort(4000) for compatibility
@july2993 july2993 added the status/PTAL PR needs to be reviewed label Oct 28, 2020
@july2993 july2993 mentioned this pull request Oct 28, 2020
Merged
lichunzhu
lichunzhu previously approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM
BTW, should UT test setting Host? I think it's also okay without that.

@july2993
Change config format
222f708 
as:
```
config:
  readinessProbe:
    type: "command"
```
@tennix
Copy link
Member

tennix commented Oct 28, 2020

For the manual test, did you use the tidb image with curl installed?

@july2993
Copy link
Contributor Author

july2993 commented Oct 28, 2020 •
edited
Loading

For the manual test, did you use the tidb image with curl installed?

yes, you can set baseImage as july2993/tidb instead of pingcap/tidb in the tc.yaml

build and push using pingcap/tidb#20694

only version v4.0.7, see https://hub.docker.com/repository/docker/july2993/tidb

@codecov-io
Copy link

codecov-io commented Oct 29, 2020 •
edited
Loading

Codecov Report

Merging #3438 into master will increase coverage by 3.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3438      +/-   ##
==========================================
+ Coverage   52.13%   55.33%   +3.20%     
==========================================
  Files         162      162              
  Lines       16587    16609      +22     
==========================================
+ Hits         8647     9191     +544     
+ Misses       7103     6538     -565     
- Partials      837      880      +43
Flag Coverage Δ
#unittest 55.33% <100.00%> (?)

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

DanielZhangQD
DanielZhangQD approved these changes Oct 29, 2020
View reviewed changes
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

@july2993
Copy link
Contributor Author

/test pull-e2e-kind

1 similar comment
@july2993
Copy link
Contributor Author

/test pull-e2e-kind

@DanielZhangQD
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 3429

@DanielZhangQD DanielZhangQD merged commit e861816 into pingcap:master Oct 30, 2020
ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Oct 30, 2020
@july2993 @ti-srebot
cherry pick pingcap#3438 to release-1.1
6b4eac5 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Oct 30, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-1.1 in PR #3445

@july2993 july2993 deleted the tidb_pro branch October 30, 2020 07:29
DanielZhangQD pushed a commit that referenced this pull request Nov 2, 2020
@ti-srebot @july2993
Add readinessProbe config for tidb (#3438) (#3445)
78aa518 
* cherry pick #3438 to release-1.1

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* ./hack/update-all.sh

Co-authored-by: july2993 <july2993@gmail.com>
@ti-srebot ti-srebot mentioned this pull request Nov 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ti-srebot ti-srebot ti-srebot approved these changes

@DanielZhangQD DanielZhangQD DanielZhangQD approved these changes

@lichunzhu lichunzhu lichunzhu left review comments

@lonng lonng Awaiting requested review from lonng

Assignees
No one assigned
Labels
CanMerge needs-cherry-pick-1.1 status/LGT2 status/PTAL PR needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TiDB Pod Probe doesn't work with TLS
6 participants
@july2993 @tennix @codecov-io @DanielZhangQD @ti-srebot @lichunzhu