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

etcdserver/api/etcdhttp: add reason field for /health response #11983

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

tangcong
Copy link
Contributor

@tangcong tangcong commented Jun 6, 2020

Fix issue #11599 . @gyuho

{"health":"false","reason":"NOSPACE"}

@gyuho
Copy link
Contributor

gyuho commented Jun 9, 2020

@wenjiaswe @jingyih Can we make sure kube-apiserver /healthz be compatible with this change?

@tangcong
Copy link
Contributor Author

tangcong commented Jun 9, 2020

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #11983 into master will increase coverage by 0.39%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11983      +/-   ##
==========================================
+ Coverage   65.50%   65.90%   +0.39%     
==========================================
  Files         403      403              
  Lines       37320    37323       +3     
==========================================
+ Hits        24447    24598     +151     
+ Misses      11337    11176     -161     
- Partials     1536     1549      +13     
Impacted Files Coverage Δ
etcdserver/api/etcdhttp/metrics.go 41.37% <0.00%> (-7.61%) ⬇️
client/keys.go 68.34% <0.00%> (-23.12%) ⬇️
auth/simple_token.go 74.78% <0.00%> (-14.29%) ⬇️
auth/jwt.go 64.04% <0.00%> (-13.49%) ⬇️
clientv3/naming/grpc.go 63.15% <0.00%> (-12.29%) ⬇️
etcdserver/api/v3rpc/lease.go 72.15% <0.00%> (-7.60%) ⬇️
proxy/grpcproxy/watcher.go 85.71% <0.00%> (-4.09%) ⬇️
clientv3/leasing/cache.go 87.22% <0.00%> (-3.89%) ⬇️
proxy/grpcproxy/watch.go 89.94% <0.00%> (-2.96%) ⬇️
lease/leasehttp/http.go 61.31% <0.00%> (-2.92%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3082a7d...3304241. Read the comment docs.

@tangcong tangcong force-pushed the add_reason_field_for_health branch from 711a228 to 3304241 Compare June 10, 2020 03:17
@wenjiaswe
Copy link
Contributor

Wenjia Jingyi Hu Can we make sure kube-apiserver /healthz be compatible with this change?

k8s liveness probe and storage backend health check should be fine, it's not decoding the body but use the http code to verify.

@tangcong
Copy link
Contributor Author

Wenjia Jingyi Hu Can we make sure kube-apiserver /healthz be compatible with this change?

k8s liveness probe and storage backend health check should be fine, it's not decoding the body but use the http code to verify.

thanks. it is better if we can print a clear error message in apiserver health check. @wenjiaswe @xiang90 @gyuho

@tangcong
Copy link
Contributor Author

issue #11599 describes /health still returns {"health": "true"} when etcd has exceeded DB and detected DB corruption.It looks like the return is correct {"health": "false"} in master and release-3.4 branch. do you have any suggestions for this pr ? thanks. @gyuho

@gyuho
Copy link
Contributor

gyuho commented Jun 16, 2020

lgtm thanks @tangcong @wenjiaswe

@gyuho gyuho merged commit d8c8f90 into etcd-io:master Jun 16, 2020
@tangcong tangcong deleted the add_reason_field_for_health branch February 26, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants