conn_pool: track streams across the pool by alyssawilk · Pull Request #13684 · envoyproxy/envoy

alyssawilk · 2020-10-21T19:01:45Z

Tracking active, pending, and available capacity for each thread local cluster, for eventual use in cluster-wide prefetch

Risk Level: low
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a

alyssawilk · 2020-10-21T19:03:09Z

I had originally planned to land this with the use in source/common/upstream/cluster_manager_impl.cc but it was a large and fragile enough change I think I'd prefer them to be separate (unless you prefer otherwise)

I really don't like how fragile the stream tracking is, but there's a weak link between the codec's streams and the connection pool's streams with different lifetimes, which I'm not sure how to address better than I have here.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente

I had originally planned to land this with the use in source/common/upstream/cluster_manager_impl.cc but it was a large and fragile enough change I think I'd prefer them to be separate (unless you prefer otherwise)

I really don't like how fragile the stream tracking is, but there's a weak link between the codec's streams and the connection pool's streams with different lifetimes, which I'm not sure how to address better than I have here.

I did a review pass. The main thing that jumped at me that concurrent_stream_limit can be set to uint64_t max which may cause some of the tracking counters to behave erratically.

antoniovicente · 2020-10-22T20:24:51Z

include/envoy/upstream/cluster_manager.h

+    ASSERT(connecting_capacity_ == 0);
+  }
+  void checkAndDecrement(uint32_t& value, uint32_t& delta) {
+    ASSERT(value - delta <= value);


Would this be equivalent?
ASSERT(delta <= value);

antoniovicente · 2020-10-22T20:26:09Z

include/envoy/upstream/cluster_manager.h

+    ASSERT(active_streams_ == 0);
+    ASSERT(connecting_capacity_ == 0);
+  }
+  void checkAndDecrement(uint32_t& value, uint32_t& delta) {


nit: delta does not need to be a non-const reference. Change to "uint32_t delta"

antoniovicente · 2020-10-22T21:06:52Z

include/envoy/upstream/cluster_manager.h

+    value -= delta;
+  }
+
+  void incrPendingStreams(uint32_t delta) { pending_streams_ += delta; }


I wonder if a mismatch in inc/dec could result in some of these counters overflowing because of missing decrements. I think that the protection you have against this is the ASSERTs that require counters to be 0 at destruction time.

and the decrement asserts, which caught a lot of issues earlier on.

Can we ASSERT that we don't overflow on increment also?

antoniovicente · 2020-10-22T21:10:47Z

source/common/conn_pool/conn_pool_base.cc

           client->effectiveConcurrentStreamLimit());
    ASSERT(client->real_host_description_);
+    // Increase the connecting capacity to reflect the streams this connection can serve.
+    state_.incrConnectingCapacity(client->effectiveConcurrentStreamLimit());


Is there a limit on the configured concurrency per H2 connection? These uint32 counters could overflow if the configured concurrency limit is close to uint32 max and we end up creating multiple connections to backends for the cluster. Note that effectiveConcurrentStreamLimit can return uint64_t max when concurrency limit is set to 0. See

envoy/source/common/conn_pool/conn_pool_base.cc

Line 458 in cb7691c

concurrent_stream_limit_(translateZeroToUnlimited(concurrent_stream_limit)),

Yeah, in practice these are set by config, and

google.protobuf.UInt32Value max_concurrent_streams = 2
[(validate.rules).uint32 = {lte: 2147483647 gte: 1}];

so they're functionally capped at uint32. Would you prefer I make all the base class types uint32_t, and the new types uint64 for overflow safety?

Sorry, I didn't realize that you had replied to some comments until now. I should have tagged the PR with waiting:any instead of waiting.

What if someone does not set config and ends up with the default concurrent_stream_limit of 0?
I did some tracing through the H2 implementation and found that initializeAndValidateOptions does translate a config value of 0 to 2147483647 as documented. It seems to me that concurrent_stream_limit should never be 0 when it reaches this constructor.

Still, connecting_capacity_ is an uint32_t. If there are more than 2 endpoints in the cluster, I think this will overflow when using the H2 default of 2**31-1 since I think these stats are per cluster, not per host/endpoint.

antoniovicente · 2020-10-22T21:52:25Z

source/common/conn_pool/conn_pool_base.h

  }

+  uint64_t currentUnusedCapacity() const {
+    return std::min(remaining_streams_, concurrent_stream_limit_ - numActiveStreams());


Consider adding: ASSERT(numActiveStreams() <= concurrent_stream_limit_);

antoniovicente · 2020-10-22T21:59:52Z

source/common/http/http1/conn_pool.cc

          parent, parent.host_->cluster().maxRequestsPerConnection(),
          1 // HTTP1 always has a concurrent-request-limit of 1 per connection.
      ) {
+  codec_client_->setCodecClientCallbacks(*this);


Interesting, these callbacks were not set for H1 until now.

antoniovicente · 2020-10-22T22:04:20Z

source/common/upstream/load_balancer_impl.cc

                    std::accumulate(per_priority_load.degraded_priority_load_.get().begin(),
                                    per_priority_load.degraded_priority_load_.get().end(), 0));
+
+  total_healthy_hosts = 0;


This function contains an early return on line 195; total_healthy_hosts is not initialized on early return.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-11-12T15:48:47Z

I think I've addressed the overflow concerns. PTAL?

alyssawilk · 2020-11-16T17:59:43Z

ping :-)

antoniovicente · 2020-11-14T06:30:07Z

include/envoy/upstream/cluster_manager.h

+    value += delta;
+  }
+
+  void incrPendingStreams(uint32_t delta) { checkAndIncrement<uint32_t>(pending_streams_, delta); }


optional: you can omit the template type on these template function calls and rely on the compiler doing type deduction.

mattklein123

At a high level this LGTM.

I do have one question though: are we duplicating information that we already have in metrics? There are two parts to this question:

On one hand, we are trying to not drive functionality through stats because it leads to bugs when stats get excluded.
On the other hand we might be duplicating accounting logic and then not have visibility into certain metrics.

To account for (2) we have discussed potentially allowing stats to be marked as not excludable if we know they are used in critical logic. So my question is did you consider using stats for this and decide not to because of potential exclusion bugs? cc @jmarantz

/wait-any

jmarantz · 2020-11-17T00:03:53Z

Having some stats be force-included independent of the disallow patterns makes sense to me. Is there an open bug for that?

mattklein123 · 2020-11-17T00:05:34Z

Having some stats be force-included independent of the disallow patterns makes sense to me. Is there an open bug for that?

I don't think it's tracked yet, but it does keep coming up and it seems like we might want that capability.

alyssawilk · 2020-11-17T18:00:40Z

There may be overlap with stats, but I don't think we can use stats because prefetch data needs to be per-worker since the connections are only available locally (until/if we have cross-thread pools)

mattklein123 · 2020-11-17T18:11:32Z

There may be overlap with stats, but I don't think we can use stats because prefetch data needs to be per-worker since the connections are only available locally (until/if we have cross-thread pools)

OK that makes sense. I will take another quick pass today.

mattklein123

At a high level looks good with some naming nits. This code is all tricky so IMO it would be better to split the accounting changes out from the other changes which seem unrelated, then we can review the integration separately? Up to you.

/wait

mattklein123 · 2020-11-18T01:23:19Z

include/envoy/upstream/cluster_manager.h

+  uint32_t active_streams_{};
+  // Tracks the stream capacity if all connecting connections were connected but
+  // excluding streams which are in use.
+  uint64_t connecting_capacity_{};


nit: can you name this connecting_stream_capacity_ or connection_stream_capacity_ and similar for the accessor methods? I was confused when I first started to look at the code below.

mattklein123 · 2020-11-18T01:24:27Z

include/envoy/upstream/cluster_manager.h

+  // Tracks the stream capacity if all connecting connections were connected but
+  // excluding streams which are in use.


nit: this is a bit hard to parse. Can you clarify a bit? This is basically: total theoretical capacity - pending - active?

mattklein123 · 2020-11-18T01:27:28Z

source/common/upstream/cluster_manager_impl.cc


 void ClusterManagerImpl::maybePrefetch(
-    ThreadLocalClusterManagerImpl::ClusterEntryPtr& cluster_entry,
+    ThreadLocalClusterManagerImpl::ClusterEntryPtr& cluster_entry, const ClusterConnectivityState&,


It might be better to split this part of the change out from the accounting work but up to you.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks LGTM with one other potential question/comment.

/wait-any

mattklein123 · 2020-11-18T18:36:25Z

source/common/upstream/load_balancer_impl.h

+  // The total count of healthy hosts across all priority levels.
+  uint32_t total_healthy_hosts_;


I think it would be better to revert all of this also as unrelated. If not reverted, does this need to be zero initialized somewhere?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Nice, thanks.

Tracking active, pending, and available capacity for each thread local cluster, for eventual use in cluster-wide prefetch Risk Level: low Testing: new unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Tracking active, pending, and available capacity for each thread local cluster, for eventual use in cluster-wide prefetch Risk Level: low Testing: new unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Qin Qin <qqin@google.com>

alyssawilk requested a review from snowp as a code owner October 21, 2020 19:01

alyssawilk assigned antoniovicente Oct 21, 2020

alyssawilk force-pushed the next_peekahead branch from 0677c9e to a4e2405 Compare October 21, 2020 19:07

conn_pool: track streams across the pool

a5a418c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the next_peekahead branch from a4e2405 to a5a418c Compare October 21, 2020 19:07

fix

74d8ff4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente reviewed Oct 22, 2020

View reviewed changes

antoniovicente added the waiting label Oct 23, 2020

Merge branch 'master' into next_peekahead

c9e9214

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente added waiting:any and removed waiting labels Nov 4, 2020

alyssawilk added 3 commits November 9, 2020 14:55

Merge branch 'master' into next_peekahead

152ac52

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

rolling back h1 lifetime changes

83a687b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

address overflow

947dc2a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting:any label Nov 10, 2020

alyssawilk mentioned this pull request Nov 10, 2020

ordering between codec client callbacks StreamDecoderWrapper is confusing #13537

Closed

Merge branch 'master' into next_peekahead

88005f9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente previously approved these changes Nov 16, 2020

View reviewed changes

alyssawilk assigned mattklein123 Nov 16, 2020

mattklein123 reviewed Nov 16, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Nov 16, 2020

repokitteh-read-only bot removed the waiting:any label Nov 17, 2020

jmarantz mentioned this pull request Nov 17, 2020

stats: prevent disabling some stats #14051

Open

mattklein123 requested changes Nov 18, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 18, 2020

reviewer comments

da4a406

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed antoniovicente’s stale review via da4a406 November 18, 2020 17:28

repokitteh-read-only bot removed the waiting label Nov 18, 2020

mattklein123 reviewed Nov 18, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Nov 18, 2020

comment

742a5bf

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting:any label Nov 18, 2020

mattklein123 approved these changes Nov 18, 2020

View reviewed changes

alyssawilk merged commit 8d62990 into envoyproxy:master Nov 18, 2020

alyssawilk deleted the next_peekahead branch June 10, 2021 13:43

		// Tracks the stream capacity if all connecting connections were connected but
		// excluding streams which are in use.

		// The total count of healthy hosts across all priority levels.
		uint32_t total_healthy_hosts_;

Conversation

alyssawilk commented Oct 21, 2020

Uh oh!

alyssawilk commented Oct 21, 2020

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 12, 2020

Uh oh!

alyssawilk commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Nov 17, 2020

Uh oh!

mattklein123 commented Nov 17, 2020

Uh oh!

alyssawilk commented Nov 17, 2020

Uh oh!

mattklein123 commented Nov 17, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antoniovicente Nov 4, 2020 •

edited

Loading