Skip to content

conn_pool: support max stream duration for upstream connections #15068

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.

Sign up for GitHub

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

Closed
esmet wants to merge 4 commits into from
Closed

conn_pool: support max stream duration for upstream connections #15068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f143d0c
conn_pool: support max stream duration for upstream connections
esmet Feb 17, 2021
4e8e73d
Formatting
esmet Feb 17, 2021
9d7d810
Only drain non-closed connections
esmet Feb 17, 2021
0964e31
Pipe down maxConnectionDuration from CommonHttpProtocolOptions
esmet Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/envoy/upstream/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ class PrioritySet {
COUNTER(upstream_cx_http2_total) \
COUNTER(upstream_cx_idle_timeout) \
COUNTER(upstream_cx_max_requests) \
COUNTER(upstream_cx_max_duration) \
COUNTER(upstream_cx_none_healthy) \
COUNTER(upstream_cx_overflow) \
COUNTER(upstream_cx_pool_overflow) \
Expand Down Expand Up @@ -728,6 +729,11 @@ class ClusterInfo {
*/
virtual const absl::optional<std::chrono::milliseconds> idleTimeout() const PURE;

/**
* @return the max duration for upstream connection pool connections.
Copy link
Contributor

@alyssawilk alyssawilk Feb 22, 2021

Choose a reason for hiding this comment

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

I assume all the API plumbing to actually set this is TODO?
I'd rephrase the comments to make it clear that this is the max time before drain, not a hard max cutoff. There's a huge difference between the two and I'm not actually clear on which one is desired (or if both are)

Copy link
Contributor Author

@esmet esmet Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The missing API plumbing pieces might be some sloppiness on my part from managing two branches of this code - let me check that out.

I will rephrase the comment to mention that it is the max time before drain. I want the behavior to enforce a maximum time after which new requests will no longer use this connection. It's ok for existing requests to drain and finish, as long as new ones no longer attempt to use this connection. The idea is to provide a guarantee that after the configured time, all new traffic is flowing through newly created connections, which in practice will be (well, could be) newer upstream backends that all happen to share an IP (think Kubernetes Cluster IPs backed by pods via label selectors). Let me know if that makes sense or if my thinking is misguided.

Copy link
Contributor

@snowp snowp Feb 22, 2021

Choose a reason for hiding this comment

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

I think having this cause draining is the most flexible, since it can be coupled with max_stream_duration to ensure that even long lived streams eventually move to the new connection.

Re your example I would imagine that TCP connections wouldn't survive the IP moving from one pod to another since the new upstream would probably respond with a RST if Envoy tried to send it data on a connection established with a previous pod, causing Envoy to establish a new connection? I still think this feature is useful (e.g. when going through a L4 proxy), but not clear to me why this would be required for the IP reuse case.

Copy link
Contributor Author

@esmet esmet Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I should have clarified that the use case is a single IP whose upstream backends change without any of them going unhealthy. Consider a case where there is a Service IP with a label selector that targets healthy pods with version=old. Then, the Service manifest is updated to select on labels version=new instead. Now, any new connection through this IP will reach pods with version=new, but the existing connections to healthy pods with version=old still remain. For the sake of this example, pods with version=old and version=new may coexist for an arbitrary amount of time. The max connection duration feature would help ensure that after a configured amount of time, those old connections would be drained so that all new requests eventually go through new connections.

Copy link
Contributor

@alyssawilk alyssawilk Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

curious, any reason the upstream can't do its own drain (version=old doing GOAWAY + HTTP/1.1 connection close, nack health)? It'd be a useful feature either way for things like internet based load balancing having limited TTL when DNS can expire, so no objection just curious :-)

Copy link
Contributor Author

@esmet esmet Feb 23, 2021

Choose a reason for hiding this comment

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

The upstream could do its own drain, but I think it's extra useful for the solution to be upstream-agnostic and implemented purely in Envoy.

The exact use case is for canary deployments using two Service IPs, one that represents the canary and one that represents the stable service. We use Envoy's traffic splitting capabilities to slowly move traffic over to the canary IP, and, finally, update the stable IP to point to the canary pods. I think the real solution is to just discover Endpoint objects and push those into Envoy instead of using the abstract Service IP. However, and to your point, it's useful to be able to solve this use case with Service IPs alone.

*/
virtual const absl::optional<std::chrono::milliseconds> maxConnectionDuration() const PURE;

/**
* @return how many streams should be anticipated per each current stream.
*/
Expand Down
14 changes: 14 additions & 0 deletions source/common/conn_pool/conn_pool_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ ActiveClient::ActiveClient(ConnPoolImplBase& parent, uint32_t lifetime_stream_li
conn_length_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
parent_.host()->cluster().stats().upstream_cx_length_ms_, parent_.dispatcher().timeSource());
connect_timer_->enableTimer(parent_.host()->cluster().connectTimeout());
const auto max_connection_duration = parent_.host()->cluster().maxConnectionDuration();
if (max_connection_duration) {
lifetime_timer_ = parent_.dispatcher().createTimer([this]() -> void { onLifetimeTimeout(); });
lifetime_timer_->enableTimer(max_connection_duration.value());
}
parent_.host()->stats().cx_total_.inc();
parent_.host()->stats().cx_active_.inc();
parent_.host()->cluster().stats().upstream_cx_total_.inc();
Expand Down Expand Up @@ -559,5 +564,14 @@ void ActiveClient::onConnectTimeout() {
close();
}

void ActiveClient::onLifetimeTimeout() {
if (state_ != ActiveClient::State::CLOSED) {
Copy link
Contributor

@alyssawilk alyssawilk Feb 22, 2021

Choose a reason for hiding this comment

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

Please make sure capacity stats are handed correctly. This should decrease the local capacity and possibly trigger prefetching.

Copy link
Contributor Author

@esmet esmet Feb 22, 2021

Choose a reason for hiding this comment

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

Good call, I'll look into this.

ENVOY_CONN_LOG(debug, "lifetime timeout, DRAINING", *this);
parent_.host()->cluster().stats().upstream_cx_max_duration_.inc();
parent_.transitionActiveClientState(*this,
Envoy::ConnectionPool::ActiveClient::State::DRAINING);
}
}

} // namespace ConnectionPool
} // namespace Envoy
5 changes: 5 additions & 0 deletions source/common/conn_pool/conn_pool_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ class ActiveClient : public LinkedObject<ActiveClient>,
// Called if the connection does not complete within the cluster's connectTimeout()
void onConnectTimeout();

// Called if the maximum connection duration is reached. If set, this puts an upper
// bound on the lifetime of any connection.
void onLifetimeTimeout();

// Returns the concurrent stream limit, accounting for if the total stream limit
// is less than the concurrent stream limit.
uint32_t effectiveConcurrentStreamLimit() const {
Expand Down Expand Up @@ -82,6 +86,7 @@ class ActiveClient : public LinkedObject<ActiveClient>,
Stats::TimespanPtr conn_connect_ms_;
Stats::TimespanPtr conn_length_;
Event::TimerPtr connect_timer_;
Event::TimerPtr lifetime_timer_;
bool resources_released_{false};
bool timed_out_{false};
};
Expand Down
4 changes: 4 additions & 0 deletions source/common/upstream/upstream_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,9 @@ class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable<Logger::I
const absl::optional<std::chrono::milliseconds> idleTimeout() const override {
return idle_timeout_;
}
const absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
return max_connection_duration_;
}
float perUpstreamPreconnectRatio() const override { return per_upstream_preconnect_ratio_; }
float peekaheadRatio() const override { return peekahead_ratio_; }
uint32_t perConnectionBufferLimitBytes() const override {
Expand Down Expand Up @@ -675,6 +678,7 @@ class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable<Logger::I
const uint32_t max_response_headers_count_;
const std::chrono::milliseconds connect_timeout_;
absl::optional<std::chrono::milliseconds> idle_timeout_;
absl::optional<std::chrono::milliseconds> max_connection_duration_;
const float per_upstream_preconnect_ratio_;
const float peekahead_ratio_;
const uint32_t per_connection_buffer_limit_bytes_;
Expand Down
7 changes: 7 additions & 0 deletions test/mocks/upstream/cluster_info.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ MockIdleTimeEnabledClusterInfo::MockIdleTimeEnabledClusterInfo() {

MockIdleTimeEnabledClusterInfo::~MockIdleTimeEnabledClusterInfo() = default;

MockMaxConnectionDurationEnabledClusterInfo::MockMaxConnectionDurationEnabledClusterInfo() {
ON_CALL(*this, maxConnectionDuration()).WillByDefault(Return(std::chrono::milliseconds(1000)));
}

MockMaxConnectionDurationEnabledClusterInfo::~MockMaxConnectionDurationEnabledClusterInfo() =
default;

MockClusterInfo::MockClusterInfo()
: http2_options_(::Envoy::Http2::Utility::initializeAndValidateOptions(
envoy::config::core::v3::Http2ProtocolOptions())),
Expand Down
7 changes: 7 additions & 0 deletions test/mocks/upstream/cluster_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class MockClusterInfo : public ClusterInfo {
MOCK_METHOD(bool, addedViaApi, (), (const));
MOCK_METHOD(std::chrono::milliseconds, connectTimeout, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, idleTimeout, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, maxConnectionDuration, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, maxStreamDuration, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, grpcTimeoutHeaderMax, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, grpcTimeoutHeaderOffset, (),
Expand Down Expand Up @@ -200,5 +201,11 @@ class MockIdleTimeEnabledClusterInfo : public MockClusterInfo {
~MockIdleTimeEnabledClusterInfo() override;
};

class MockMaxConnectionDurationEnabledClusterInfo : public MockClusterInfo {
public:
MockMaxConnectionDurationEnabledClusterInfo();
~MockMaxConnectionDurationEnabledClusterInfo() override;
};

} // namespace Upstream
} // namespace Envoy