Listener: implement internal listener#16763
Conversation
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
|
Perhaps we need two reviewers: @antoniovicente can you review the dispatcher part? Last time we chatted that there is per worker component to create the connection, which doesn't have to be the dispatcher. However, I feel the dispatcher is the best fit. |
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
| socklen_t error_size = sizeof(error); | ||
| RELEASE_ASSERT(socket_->getSocketOption(SOL_SOCKET, SO_ERROR, &error, &error_size).rc_ == 0, | ||
| ""); | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_address") && |
There was a problem hiding this comment.
It would be nicer if we could have internal connections act more like standard connections. Having to special case internal connections in Network::ConnectionImpl and other places makes thinking about these connection types much harder. Could we try to make internal connections a more well-behaved and self-contained extension?
There was a problem hiding this comment.
There is no fundamental known problem to make internal connection acts as connection.
Can I list it as TODO? This PR is not trying to restrict the changes at InternalListener add/update/delete, this place is proof of concept that a connection could be instantiated.
Most importantly, when the runtime feature is disabled it's clear that the normal connection doesn't have behavior change
There was a problem hiding this comment.
Take line 876 below for example. We have an ASSERT that seems to be only relevant if connection type is not internal. Would it be possible to remove the conditionals in this file that depend on connection type == internal?
| config_ = &config; | ||
| } | ||
|
|
||
| void ActiveTcpSocket::onTimeout() { |
There was a problem hiding this comment.
This PR is very large and difficult to review. Should you try to do a pure refactor of this class in a separate PR before extending its functionality?
There was a problem hiding this comment.
Sorry! This part doesn't bring logic change and the existing test cases cover each line of the function.
I am open to create a dedicated PR but I also want some opinions from @htuch and @mattklein123 to see if that PR is greatly reduce the load of review.
There was a problem hiding this comment.
I see references to internal connection type in some of the modified files, so at least some parts of this PR is not a pure refactor.
There was a problem hiding this comment.
Yeah, I think it would make review much more easier and faster if you can split this into two PRs:
- The pure refactor, which hopefully we can process without too much bike shed and eliminate a large amount of the churn.
- The new internal connection aspects. This is where the meat of the PR is, and where we should be able to spend the bulk of the review time.
There was a problem hiding this comment.
@antoniovicente @htuch Is #16947 a smooth refactor to you?
There was a problem hiding this comment.
I'm looking through it right now. It is not a trivial refactor. I see several class renames and new uses of the curiously recurring template pattern for not immediately clear reasons.
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
|
/wait |
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
| } | ||
| } | ||
|
|
||
| void ActiveInternalListener::newConnection(Network::ConnectionSocketPtr&& socket, |
There was a problem hiding this comment.
feel like this whole method can be moved to base class since it looks same with the ActiveTcpListener one.
|
Creating new for refactor |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Client connection can be created with least changes using underlying user space io socket per comment #16763 Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Commit Message:
Allow adding internal address listener to the envoy.
This PR covers the listener CRUD but the dispatch is not yet providing the functionality
to create a connection to the internal listener.
The end2end connection establishment can be found in #16095
The internal connection saves ~3% cpu usage vs a tcp listener on loopback device at 1Gbps transmit rate.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Runtime guard: envoy.reloadable_features.internal_address
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]