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

*: make "initial-corrupt-check", "corrupt-check-time" stable #10934

Closed
wants to merge 6 commits into from

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 25, 2019

@xiang90
Copy link
Contributor

xiang90 commented Jul 25, 2019

lgtm

@jpbetz
Copy link
Contributor

jpbetz commented Jul 25, 2019

lgtm

Thanks for enabling for the functional tester as well.

@codecov-io
Copy link

codecov-io commented Jul 25, 2019

Codecov Report

Merging #10934 into master will increase coverage by 0.83%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10934      +/-   ##
==========================================
+ Coverage   63.34%   64.17%   +0.83%     
==========================================
  Files         400      400              
  Lines       37689    37690       +1     
==========================================
+ Hits        23873    24189     +316     
+ Misses      12217    11884     -333     
- Partials     1599     1617      +18
Impacted Files Coverage Δ
embed/config.go 50.84% <100%> (+0.13%) ⬆️
etcdmain/config.go 82.88% <100%> (ø) ⬆️
embed/etcd.go 67.71% <100%> (ø) ⬆️
pkg/transport/timeout_conn.go 60% <0%> (-20%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-13.89%) ⬇️
auth/store.go 57.47% <0%> (-11.12%) ⬇️
pkg/transport/listener.go 49.54% <0%> (-3.64%) ⬇️
etcdserver/api/rafthttp/peer.go 77.65% <0%> (-1.12%) ⬇️
embed/serve.go 36.48% <0%> (-0.91%) ⬇️
etcdserver/server.go 67.95% <0%> (-0.34%) ⬇️
... and 28 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 84a3804...7b90a7a. Read the comment docs.

@jingyih
Copy link
Contributor

jingyih commented Jul 25, 2019

Does this feature work if TLS is enabled among peers [1]? If not, we probably want to fix it before promoting the flag?

[1]

func (s *EtcdServer) getPeerHashKVs(rev int64) (resps []*peerHashKVResp) {

@gyuho
Copy link
Contributor Author

gyuho commented Jul 25, 2019

@jingyih Good point. We need fix that.

@gyuho gyuho added the WIP label Jul 25, 2019
@jingyih
Copy link
Contributor

jingyih commented Jul 25, 2019

@jingyih Good point. We need fix that.

I am looking at how to fix it. Fundamentally, the server does not have the cert files of the client side, which means we cannot create a clientv3 on server side and reach out to another server. Instead, we need to get peer hash from the peer http [1]?

@gyuho, could you comment?

[1] https://github.com/etcd-io/etcd/blob/master/etcdserver/api/etcdhttp/peer.go

@jingyih
Copy link
Contributor

jingyih commented Jul 25, 2019

@gyuho Never mind. I think I misunderstood some concepts.

@jpbetz
Copy link
Contributor

jpbetz commented Jul 25, 2019

At some point, maybe as a future task, we should clean up the peer communication infrastructure. It should be possible to introduce a new request type without having to sort out TLS each time..

@gyuho gyuho mentioned this pull request Jul 26, 2019
19 tasks
@gyuho
Copy link
Contributor Author

gyuho commented Jul 29, 2019

As @jpbetz mentioned, there's no easy way to configure client cert from a remote peer, when peer-to-peer is TLS-enabled. Please let me know if anyone has a better idea. Let's move this to etcd v4.

@jingyih
Copy link
Contributor

jingyih commented Feb 18, 2020

@gyuho
I just merged #11621. Let's reopen this PR and make the flags stable.

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