grid: Track HTTP/3 broken-ness in ConnectivityGrid. #15933

RyanTheOptimist · 2021-04-12T23:14:03Z

Mark HTTP/3 as broken when the HTTP/3 connect attempt fails but the
fallback attempt succeeds.
When HTTP/3 is broken, do not attempt to initiate new HTTP/3 connections.
Add an assert that the Grid's host and the callback's host are the same.

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

RyanTheOptimist · 2021-04-12T23:15:04Z

/assign @alyssawilk

This doesn't yet handle marking QUIC as un-broken and I'm not entire sure I'm in love with this approach for tracking state, so perhaps we should chat about that.

alyssawilk

Sweet! Excited to see progress on brokenness. Here's some preliminary thoughts, and happy to sync up when you're in :-)

source/common/http/conn_pool_grid.cc

alyssawilk · 2021-04-13T15:07:21Z

source/common/http/conn_pool_grid.cc

  ENVOY_LOG(trace, "{} pool successfully connected to host '{}'.", describePool(attempt->pool()),
            grid_.host_->hostname());
+  if (http3_attempt_failed_) {
+    // If the HTTP/3 attempt failed but the other attempt succeeded, mark HTTP/3 as broken.
+    grid_.set_is_http3_broken(true);


Don't we need to handle the case where H2 succeeds before H3 fails as well?

Right, that's the TODO here:

// TODO: Ensure that if HTTP/2 succeeds, we can allow the HTTP/3 connection to run to completion.

How 'bout I address this in a followup since I suspect that might result in a bit of refactoring?

Turned out to be less work than I expected, so I've resolved this TODO and handle this case now. (Added a test for it)

alyssawilk · 2021-04-13T15:09:02Z

source/common/http/conn_pool_grid.h

@@ -91,6 +91,8 @@ class ConnectivityGrid : public ConnectionPool::Instance,
                                  const StreamInfo::StreamInfo& info,
                                  absl::optional<Http::Protocol> protocol);

+    bool http3_attempt_failed() const { return http3_attempt_failed_; }


You and your google habits. no snake case for you. :-P

http3AttemptFailed()
ditto for brokenness below

Done! I renamed is_http3_broken() and set_is_http3_broken(). I think that's all?

source/common/http/conn_pool_grid.cc

Mark HTTP/3 as broken when the HTTP/3 connect attempt fails but the fallback attempt succeeds. When HTTP/3 is broken, do not attempt to initiate new HTTP/3 connections. Add an assert that the Grid's host and the callback's host are the same. Signed-off-by: Ryan Hamilton <[email protected]>

Signed-off-by: Ryan Hamilton <[email protected]>

…nection fails. Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2021-04-13T22:29:16Z

Blergh, somehow two of my commits did not have signoff on them, do I had to follow the instructions to fix which required a force push. I'd love to understand how those commits did not get signoff.

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk

Yeah! This is looking solid :-)
I think you need a main merge, probably to deal with the newStream returns. Also adding David since it'd be good to have another person familiar with the code, though he's welcome to only do a cursory pass.

alyssawilk · 2021-04-14T13:04:04Z

source/common/http/conn_pool_grid.cc

@@ -70,11 +78,14 @@ void ConnectivityGrid::WrapperCallbacks::onConnectionAttemptFailed(

  // If this point is reached, all pools have been tried. Pass the pool failure up to the
  // original caller.


update comment, as pool failure may not be passed up?

Good point. Done.

alyssawilk · 2021-04-14T13:05:09Z

source/common/http/conn_pool_grid.cc

+  auto delete_this_on_return = attempt->removeFromList(connection_attempts_);
+  ConnectionPool::Callbacks* callbacks = inner_callbacks_;
+  inner_callbacks_ = nullptr;
+  // If there are other connection attempts in progress, let them complete.


optimization: If H3 connected but H2 was created due to timeout, do we want H2 to complete? Should we only continue this path if if it's an H3 attempt outstanding?

Sure works for me. I have some feeling in the back of my mind that there might be some corner case where we might wish TCP had run to completion ('cause HTTP/3 is going to end up getting marked as broken) but that doesn't actually seem plausible. Done.

source/common/http/conn_pool_grid.cc

alyssawilk · 2021-04-14T13:08:27Z

source/common/http/conn_pool_grid.cc

+              host_->hostname());
+    createNextPool();
+    pool = ++pool;
+  }
  // TODO(#15649) track pools with successful connections: don't always start at


are you doing this TODO?

Not as part of this PR, but I'm very happy to do it.

I think you are - by skipping pool ahead above :-P Functioanlly if QUIC is broken we start at H2

LOL! I totally misread what the TODO was for. You're right. Removed.

source/common/http/conn_pool_grid.cc

alyssawilk · 2021-04-14T13:09:41Z

source/common/http/conn_pool_grid.h

+    // Removes this from the owning list, deleting it.
+    void deleteThis();
+
+    // Marks HTTP/3 broken If the HTTP/3 attempt failed but the other attempt succeeded.


nit: the other -> a TCP?

alyssawilk · 2021-04-14T13:10:39Z

source/common/http/conn_pool_grid.h

    // The timer which tracks when new connections should be attempted.
    Event::TimerPtr next_attempt_timer_;
    // The iterator to the last pool which had a connection attempt.
    PoolIterator current_;
+    // True if the HTTP/3 attempt failed.
+    bool http3_attempt_failed_{};
+    // True if the ALPN attempt succeeded.


Should we call this TCP? I thought H3 has ALPN for all we hard code it.

Oh, based on previous conversations I thought that was the name of the other pool but looking at the code, I see it's called "Mixed". Sure, we TCP works for me. Done.

alyssawilk · 2021-04-14T13:11:06Z

source/common/http/conn_pool_grid.h

@@ -131,6 +139,12 @@ class ConnectivityGrid : public ConnectionPool::Instance,
  // Returns the next pool in the ordered priority list.
  absl::optional<PoolIterator> nextPool(PoolIterator pool_it);

+  // Returns true if |pool| is the Grid's HTTP/3 connection pool.


nit: Envoy rarely does the || style
Also optional Grid's -> grid's?

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk

Looks good!
Just a few more nits and I think we can land this :-)

source/common/http/conn_pool_grid.cc

alyssawilk · 2021-04-14T17:20:36Z

source/common/http/conn_pool_grid.cc

+              host_->hostname());
+    createNextPool();
+    pool = ++pool;
+  }
  // TODO(#15649) track pools with successful connections: don't always start at


I think you are - by skipping pool ahead above :-P Functioanlly if QUIC is broken we start at H2

test/common/http/conn_pool_grid_test.cc

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist

Thanks for the review!

source/common/http/conn_pool_grid.cc

RyanTheOptimist · 2021-04-14T18:31:33Z

source/common/http/conn_pool_grid.cc

+              host_->hostname());
+    createNextPool();
+    pool = ++pool;
+  }
  // TODO(#15649) track pools with successful connections: don't always start at


LOL! I totally misread what the TODO was for. You're right. Removed.

test/common/http/conn_pool_grid_test.cc

RyanTheOptimist · 2021-04-15T01:20:20Z

/retest

repokitteh-read-only · 2021-04-15T01:20:24Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15933 (comment) was created by @RyanTheOptimist.

see: more, trace.

alyssawilk

LG! Merge pending master being unlocked :-)

alyssawilk · 2021-04-16T01:54:44Z

Argh, I was hoping to merge this when the branch opened but we're still slogging through release work and it's not going to happen.
@asraa or @antoniovicente would you mind merging it for me tomorrow since I'll be out? it's code not used in prod and I've cleared not needing cross-company sign off, just need to merge :-)

antoniovicente · 2021-04-16T01:57:53Z

Argh, I was hoping to merge this when the branch opened but we're still slogging through release work and it's not going to happen.
@asraa or @antoniovicente would you mind merging it for me tomorrow since I'll be out? it's code not used in prod and I've cleared not needing cross-company sign off, just need to merge :-)

Sure, I'll look into it tomorrow

RyanTheOptimist · 2021-04-16T02:08:36Z

Argh, I was hoping to merge this when the branch opened but we're still slogging through release work and it's not going to happen.
@asraa or @antoniovicente would you mind merging it for me tomorrow since I'll be out? it's code not used in prod and I've cleared not needing cross-company sign off, just need to merge :-)

Sure, I'll look into it tomorrow

Thanks, @antoniovicente !

DavidSchinazi

Nice!

source/common/http/conn_pool_grid.h

source/common/http/conn_pool_grid.cc

test/common/http/conn_pool_grid_test.cc

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi

Thanks!

source/common/http/conn_pool_grid.cc

source/common/http/conn_pool_grid.h

Signed-off-by: Ryan Hamilton <[email protected]>

source/common/http/conn_pool_grid.h

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2021-04-18T02:18:43Z

/retest

repokitteh-read-only · 2021-04-18T02:18:46Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15933 (comment) was created by @RyanTheOptimist.

see: more, trace.

…ancels the request to the underlying pool, if it is still registered with the pool when it is destroyed. (It would be really nice if the Cancellable object itself could take care of this cleanup, but one step at a time). Signed-off-by: Ryan Hamilton <[email protected]>

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi

Nice fix on the lifetimes!

asraa

Thanks! Merging with @alyssawilk out

* grid: Track HTTP/3 broken-ness in ConnectivityGrid. Mark HTTP/3 as broken when the HTTP/3 connect attempt fails but the fallback attempt succeeds. When HTTP/3 is broken, do not attempt to initiate new HTTP/3 connections. Add an assert that the Grid's host and the callback's host are the same. Signed-off-by: Ryan Hamilton <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

repokitteh-read-only bot assigned alyssawilk Apr 12, 2021

alyssawilk reviewed Apr 13, 2021

View reviewed changes

RyanTheOptimist added 4 commits April 13, 2021 22:27

Adress comments from Alyssa

257196e

Signed-off-by: Ryan Hamilton <[email protected]>

Format and cleanup

869fe58

Signed-off-by: Ryan Hamilton <[email protected]>

Handle the ALPN pool connection succeeding before the HTTP/3 pool con…

d9859ce

…nection fails. Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist force-pushed the h3broken branch from 08c7571 to d9859ce Compare April 13, 2021 22:28

spelling

bc696a8

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk assigned DavidSchinazi Apr 14, 2021

alyssawilk reviewed Apr 14, 2021

View reviewed changes

Merge branch 'main' into h3broken

ace25d5

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk added the waiting label Apr 14, 2021

Address comments from Alyssa.

7ee4c93

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist force-pushed the h3broken branch from 1f8b70d to 7ee4c93 Compare April 14, 2021 16:26

repokitteh-read-only bot removed the waiting label Apr 14, 2021

alyssawilk reviewed Apr 14, 2021

View reviewed changes

Hopefully final nits

aa1451b

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist commented Apr 14, 2021

View reviewed changes

alyssawilk previously approved these changes Apr 15, 2021

View reviewed changes

antoniovicente self-assigned this Apr 16, 2021

DavidSchinazi reviewed Apr 16, 2021

View reviewed changes

Address comments from David

c6e3468

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist dismissed alyssawilk’s stale review via c6e3468 April 16, 2021 19:46

DavidSchinazi reviewed Apr 16, 2021

View reviewed changes

source/common/http/conn_pool_grid.cc Show resolved Hide resolved

source/common/http/conn_pool_grid.h Show resolved Hide resolved

source/common/http/conn_pool_grid.h Show resolved Hide resolved

Merge

29fa7ac

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi previously approved these changes Apr 16, 2021

View reviewed changes

source/common/http/conn_pool_grid.h Show resolved Hide resolved

asraa previously approved these changes Apr 16, 2021

View reviewed changes

Format

66695ab

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist dismissed stale reviews from asraa and DavidSchinazi via 66695ab April 17, 2021 15:15

RyanTheOptimist added 3 commits April 19, 2021 01:59

format

fa79855

Signed-off-by: Ryan Hamilton <[email protected]>

Tidy

0e0e26c

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi approved these changes Apr 19, 2021

View reviewed changes

asraa approved these changes Apr 19, 2021

View reviewed changes

asraa merged commit f5665d0 into envoyproxy:main Apr 19, 2021

		@@ -70,11 +78,14 @@ void ConnectivityGrid::WrapperCallbacks::onConnectionAttemptFailed(

		// If this point is reached, all pools have been tried. Pass the pool failure up to the
		// original caller.

grid: Track HTTP/3 broken-ness in ConnectivityGrid. #15933

grid: Track HTTP/3 broken-ness in ConnectivityGrid. #15933

Conversation

RyanTheOptimist commented Apr 12, 2021

RyanTheOptimist commented Apr 12, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Apr 13, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Apr 15, 2021

repokitteh-read-only bot commented Apr 15, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Apr 16, 2021

antoniovicente commented Apr 16, 2021

RyanTheOptimist commented Apr 16, 2021

DavidSchinazi left a comment

Choose a reason for hiding this comment

DavidSchinazi left a comment

Choose a reason for hiding this comment

RyanTheOptimist commented Apr 18, 2021

repokitteh-read-only bot commented Apr 18, 2021

DavidSchinazi left a comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment