listener: enable connection balancer to support multiple addresses listener#21774
listener: enable connection balancer to support multiple addresses listener#21774mattklein123 merged 17 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
…pports pipe Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
|
/assign @adisuissa @mattklein123 |
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks. Generally LGTM. Flushing out some comments.
/wait
source/server/active_tcp_listener.h
Outdated
| std::atomic<uint64_t> num_listener_connections_{}; | ||
|
|
||
| Network::ConnectionBalancer& connection_balancer_; | ||
| Network::Address::InstanceConstSharedPtr address_; |
There was a problem hiding this comment.
This listener must know it's address because it has a socket, right? It seems like this shouldn't be necessary?
There was a problem hiding this comment.
I have think about another two ways:
- We can get the address from the accepted socket, but it won't work with the case of the connection is redirected by the iptable
- The socket object is held by the socket listener, then we can add an interface to the listener to return the socket address.
envoy/envoy/network/listener.h
Line 398 in 17d7d79
The second way works also. But let me know if the second way you think better.
There was a problem hiding this comment.
I add a comment here https://github.com/envoyproxy/envoy/pull/21774/files#diff-55b19f848d75e4c86be99ac5c87457e24a960e71913de4b60153ea5e47c4d32cR90
But let me know if you prefer another way.
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks a few more questions and comments.
/wait
source/server/active_tcp_listener.h
Outdated
| // This is the address of this listener is listening on. And used for get the correct listener | ||
| // when rebalancing. The accepted socket can't be used to get the listening address, since | ||
| // the accepted socket's remote address can be another address than the listening address. | ||
| Network::Address::InstanceConstSharedPtr address_; |
There was a problem hiding this comment.
| Network::Address::InstanceConstSharedPtr address_; | |
| Network::Address::InstanceConstSharedPtr listen_address_; |
Can't you get this from the config though? Is't that stored in ActiveListenerImplBase in config_ or somewhere else?
There was a problem hiding this comment.
After add multiple addresses support for ListenerImpl, then the ListenerImpl's address() will changes to addresses() (also need to move address() interface to base ListenerConfig), there still no way for ActiveListener knows which address is listening. I have tried to pass an index of addresses() to the ActiveListener, but I thought the index isn't readable in the end, hold a Network::Address::InstanceConstSharedPtr is more readable.
There was a problem hiding this comment.
OK this makes sense, thanks.
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
…stener (envoyproxy#21774) Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
3 participants
Commit Message: listener: enable connection balancer to support multiple addresses listener
Additional Description:
This PR enables the
ListenerImplto create connection balancer for each listening address. And pass the specific connection balancer toActiveTcpListener. Also enable theConnectionHandlerImpl::getBalancedHandlerByTagto distinguish different address.Please reference the full implementation of multiple addresses in listener at #19367
Risk Level: high
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Part of #11184