conn_pool_grid: try TCP immediately with h3 state FailedRecently by danzh2010 · Pull Request #20722 · envoyproxy/envoy

danzh2010 · 2022-04-07T21:54:58Z

Signed-off-by: Dan Zhang danzh@google.com

Commit Message: Add a new state FailedRecently to the status tracker. And make the connectivity grid check this state in newStream(). If the h3 pool is in this state, do not delay TCP racing.

Risk Level: low, grid only
Testing: added unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Part of #18715

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2022-04-08T15:00:16Z

failed thrift_proxy:integration_test on Windows seems unrelated.

danzh2010 · 2022-04-08T15:00:28Z

/assign @RyanTheOptimist

danzh2010 · 2022-04-08T15:16:31Z

/retest

repokitteh-read-only · 2022-04-08T15:16:34Z

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

🐱

Caused by: a #20722 (comment) was created by @danzh2010.

see: more, trace.

RyanTheOptimist

Looks great!

RyanTheOptimist · 2022-04-09T14:20:32Z

source/common/http/conn_pool_grid.cc

  auto attempt = std::make_unique<ConnectionAttemptCallbacks>(*this, current_);
  LinkedList::moveIntoList(std::move(attempt), connection_attempts_);
-  if (!next_attempt_timer_->enabled()) {
+  if (next_attempt_timer_ != nullptr && !next_attempt_timer_->enabled()) {


I thought that next_attempt_timer_ was initialized in the constructor and could never be null. Is this check needed?

You're right. It's not needed

RyanTheOptimist · 2022-04-09T14:27:25Z

test/common/http/http3_status_tracker_impl_test.cc

+  EXPECT_FALSE(tracker_.isHttp3Confirmed());
+
+  EXPECT_CALL(*timer_, enabled()).WillOnce(Return(false));
+  EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(5 * 60 * 1000), nullptr));


Should we also test what happens when the timer fires? Or, maybe we tested that earlier?

We tested in MarkBrokenThenExpires that when timer fires, isHttp3Broken() returns false and hasHttp3FailedRecently() returns true

RyanTheOptimist · 2022-04-09T15:03:29Z

source/common/http/conn_pool_grid.cc

  PoolIterator pool = pools_.begin();
-  if (!shouldAttemptHttp3() || !options.can_use_http3_) {
+  Instance::StreamOptions overriding_options(options);
+  bool delay_tcp_attempt{true};


nit: we have two initializations, one uses {} and the other (). Could they both use {} (or even = to be more idiomatic)?

RyanTheOptimist · 2022-04-09T15:05:57Z

test/common/http/conn_pool_grid_test.cc

+  EXPECT_NE(grid_->first(), nullptr);
+  // The 2nd pool should be TCP pool and it should have been created together with h3 pool.
+  EXPECT_NE(grid_->second(), nullptr);
+  EXPECT_EQ(2u, grid_->callbacks_.size());


I'm not quite sure I understand how this test that the TCP attempt is not delayed?

By checking EXPECT_NE(grid_->second(), nullptr). The 2nd pool (TCP) is created immediately with the quic pool without waiting for the alarm to fire.

Ah, thanks. I see.

RyanTheOptimist · 2022-04-09T15:06:34Z

source/common/http/conn_pool_grid.cc

  }
+  if (!delay_tcp_attempt) {
+    // Immediately start TCP attempt if HTTP/3 failed recently.
+    wrapped_callbacks_.front()->tryAnotherConnection();


Should we stop the failover timer at this point since there is nothing else to try?

next_attempt_timer_ is not accessible from the grid, so I left as is. The alarm should trigger another call to tryAnotherConnection() and early returns if there is no next pool. We definitely can cancel it explicitly by adding a getter interface, but I'm wondering if it's worth it.

Yeah, fair enough!

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist · 2022-04-11T17:21:11Z

test/common/http/conn_pool_grid_test.cc

+  EXPECT_NE(grid_->first(), nullptr);
+  // The 2nd pool should be TCP pool and it should have been created together with h3 pool.
+  EXPECT_NE(grid_->second(), nullptr);
+  EXPECT_EQ(2u, grid_->callbacks_.size());


Ah, thanks. I see.

RyanTheOptimist · 2022-04-11T17:21:51Z

source/common/http/conn_pool_grid.cc

  }
+  if (!delay_tcp_attempt) {
+    // Immediately start TCP attempt if HTTP/3 failed recently.
+    wrapped_callbacks_.front()->tryAnotherConnection();


Yeah, fair enough!

RyanTheOptimist · 2022-04-11T17:23:10Z

FYI: I've enabled auto-merge so once CI passes it should land automatically.

danzh2010 · 2022-04-11T19:58:13Z

/retest

repokitteh-read-only · 2022-04-11T19:58:17Z

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

🐱

Caused by: a #20722 (comment) was created by @danzh2010.

see: more, trace.

…oyproxy#20722) Commit Message: Add a new state FailedRecently to the status tracker. And make the connectivity grid check this state in newStream(). If the h3 pool is in this state, do not delay TCP racing. Risk Level: low, grid only Testing: added unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Part of envoyproxy#18715 Signed-off-by: Dan Zhang danzh@google.com Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>

…oyproxy#20722) Commit Message: Add a new state FailedRecently to the status tracker. And make the connectivity grid check this state in newStream(). If the h3 pool is in this state, do not delay TCP racing. Risk Level: low, grid only Testing: added unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Part of envoyproxy#18715 Signed-off-by: Dan Zhang danzh@google.com

danzh1989 added 3 commits April 4, 2022 18:17

add recent failed h3 status

eb39bdd

Signed-off-by: Dan Zhang <danzh@google.com>

add more tests

455f4cc

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into h3failrecently

283aa5a

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned RyanTheOptimist Apr 8, 2022

RyanTheOptimist reviewed Apr 9, 2022

View reviewed changes

comments

b31136a

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist approved these changes Apr 11, 2022

View reviewed changes

RyanTheOptimist enabled auto-merge (squash) April 11, 2022 17:22

RyanTheOptimist merged commit 833ed92 into envoyproxy:main Apr 11, 2022

Conversation

danzh2010 commented Apr 7, 2022

Uh oh!

danzh2010 commented Apr 8, 2022

Uh oh!

danzh2010 commented Apr 8, 2022

Uh oh!

danzh2010 commented Apr 8, 2022

Uh oh!

repokitteh-read-only bot commented Apr 8, 2022

Uh oh!

RyanTheOptimist 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!

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!

RyanTheOptimist commented Apr 11, 2022

Uh oh!

danzh2010 commented Apr 11, 2022

Uh oh!

repokitteh-read-only bot commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants