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

Updated servePeers to remove the grpc server #13565

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Dec 28, 2021

@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from f5c71ad to 46c5ee7 Compare January 4, 2022 06:16
@ahrtr
Copy link
Member Author

ahrtr commented Jan 4, 2022

Just rebased the PR.

@ptabor
Copy link
Contributor

ptabor commented Jan 14, 2022

That's pretty big change.

I agree that wide access to this port is misleading and promotes bad usage patterns.

If we want to proceed I think we should mention it as an explicit breaking change in the Changelog as some user's might depend on this behavior.

@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from 46c5ee7 to fa14e8f Compare January 14, 2022 19:19
@ahrtr
Copy link
Member Author

ahrtr commented Jan 14, 2022

Thanks @ptabor for the feedback. Just added a breaking change item.

@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from fa14e8f to 2a060d5 Compare January 16, 2022 22:27
@ahrtr ahrtr changed the title Updated servePeers to remvoe the grpc server Updated servePeers to remove the grpc server Jan 16, 2022
@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from 2a060d5 to d059e79 Compare January 18, 2022 19:07
@ahrtr
Copy link
Member Author

ahrtr commented Jan 18, 2022

@ptabor I removed the gRPC server from peer listener in the integration test as well. All tests passed. Usually users do not know that the 2380 port can also serve client requests because there is no any documentation on this. So I think it might be safe to remove the misleading functionality.

cc @serathius @spzala

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

LGTM

@ptabor
Copy link
Contributor

ptabor commented Jan 20, 2022

It seems it could break --experimental-corrupt-check-time. Maybe this mode is not properly tested.

@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from d059e79 to 2b8d1c3 Compare January 26, 2022 07:25
@ahrtr
Copy link
Member Author

ahrtr commented Jan 26, 2022

It seems it could break --experimental-corrupt-check-time. Maybe this mode is not properly tested.

@ptabor I tested manually in my local environment, and confirmed that it doesn't break the corruption check functionality.

Firstly I build the etcd binary using my branch ahrtr:remove_peer_serve_client_requests.

Secondly, start a etcd cluster with only 2 members (note that I already got the etcd/bin directory included in the $PATH),

etcd --name infra1 --listen-client-urls http://127.0.0.1:2379 --advertise-client-urls http://127.0.0.1:2379 --listen-peer-urls http://127.0.0.1:12380 --initial-advertise-peer-urls http://127.0.0.1:12380 --initial-cluster-token etcd-cluster-1 --initial-cluster 'infra1=http://127.0.0.1:12380,infra2=http://127.0.0.1:22380' --initial-cluster-state new --enable-pprof --logger=zap --log-outputs=stderr --experimental-corrupt-check-time 5s

etcd --name infra2 --listen-client-urls http://127.0.0.1:22379 --advertise-client-urls http://127.0.0.1:22379 --listen-peer-urls http://127.0.0.1:22380 --initial-advertise-peer-urls http://127.0.0.1:22380 --initial-cluster-token etcd-cluster-1 --initial-cluster 'infra1=http://127.0.0.1:12380,infra2=http://127.0.0.1:22380' --initial-cluster-state new --enable-pprof --logger=zap --log-outputs=stderr --experimental-corrupt-check-time 5s

Thirdly, confirmed that infra1 is the leader,

$ etcdctl  --endpoints http://127.0.0.1:2379,http://127.0.0.1:22379 endpoint status -w table
+------------------------+------------------+-----------+---------+-----------+------------+-----------+------------+--------------------+--------+
|        ENDPOINT        |        ID        |  VERSION  | DB SIZE | IS LEADER | IS LEARNER | RAFT TERM | RAFT INDEX | RAFT APPLIED INDEX | ERRORS |
+------------------------+------------------+-----------+---------+-----------+------------+-----------+------------+--------------------+--------+
|  http://127.0.0.1:2379 | 8211f1d0f64f3269 | 3.6.0-pre |   25 kB |      true |      false |         2 |          7 |                  7 |        |
| http://127.0.0.1:22379 | 91bc3c398fb3c146 | 3.6.0-pre |   25 kB |     false |      false |         2 |          7 |                  7 |        |
+------------------------+------------------+-----------+---------+-----------+------------+-----------+------------+--------------------+--------+

Lastly, the leader prints the following log entries periodically (every 5 seconds),

{"level":"info","ts":"2022-01-26T15:24:11.112+0800","caller":"etcdserver/corrupt.go:244","msg":"finished peer corruption check","number-of-peers-checked":1}
{"level":"info","ts":"2022-01-26T15:24:16.115+0800","caller":"etcdserver/corrupt.go:244","msg":"finished peer corruption check","number-of-peers-checked":1}
{"level":"info","ts":"2022-01-26T15:24:21.119+0800","caller":"etcdserver/corrupt.go:244","msg":"finished peer corruption check","number-of-peers-checked":1}
{"level":"info","ts":"2022-01-26T15:24:26.124+0800","caller":"etcdserver/corrupt.go:244","msg":"finished peer corruption check","number-of-peers-checked":1}
{"level":"info","ts":"2022-01-26T15:24:31.126+0800","caller":"etcdserver/corrupt.go:244","msg":"finished peer corruption check","number-of-peers-checked":1}
......

BTW, I rebased the PR. Please let me know if you have other concerns or comments.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 26, 2022

cc @spzala

@ahrtr ahrtr force-pushed the remove_peer_serve_client_requests branch from 2b8d1c3 to a879ccf Compare January 27, 2022 08:22
@ahrtr
Copy link
Member Author

ahrtr commented Jan 27, 2022

Just rebased this PR.

@ahrtr
Copy link
Member Author

ahrtr commented Feb 15, 2022

FYI, the command etcdctl endpoint hashkv --cluster is also working well on this PR, @ptabor

$ etcdctl endpoint hashkv --cluster -w table
+------------------------+------------+
|        ENDPOINT        |    HASH    |
+------------------------+------------+
|  http://127.0.0.1:2379 | 1084519789 |
| http://127.0.0.1:22379 | 1084519789 |
| http://127.0.0.1:32379 | 1084519789 |
+------------------------+------------+

@ahrtr
Copy link
Member Author

ahrtr commented Feb 25, 2022

Hi @ptabor , Do you still have any concern? cc @serathius @spzala

@serathius
Copy link
Member

@ptabor any other concerns?

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you for checking.

@ptabor ptabor merged commit 088807c into etcd-io:main Mar 1, 2022
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.

peer listen address can serve client requests
3 participants