quiche: make flow control configurable by danzh2010 · Pull Request #15865 · envoyproxy/envoy

danzh2010 · 2021-04-07T04:52:00Z

Commit Message: Add initial stream and connection level flow control windows to envoy.config.core.v3.QuicProtocolOptions which is be used in QUIC listener config and Http3 upstream cluster config.

Risk Level: low
Testing: re-enable more Http3 downstream protocol test.

Part of #2557 #12930 #14829

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

repokitteh-read-only · 2021-04-07T04:52:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15865 was opened by danzh2010.

see: more, trace.

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

danzh2010 · 2021-04-08T04:14:49Z

/retest

repokitteh-read-only · 2021-04-08T04:14:52Z

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

🐱

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

see: more, trace.

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

api/envoy/config/core/v3/protocol.proto

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

danzh2010 · 2021-04-12T03:12:49Z

/retest

repokitteh-read-only · 2021-04-12T03:12:52Z

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

🐱

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

see: more, trace.

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

danzh2010 · 2021-04-13T14:45:30Z

/assign @antoniovicente @alyssawilk

repokitteh-read-only · 2021-04-23T04:21:23Z

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

🐱

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

see: more, trace.

danzh2010 · 2021-04-23T14:36:33Z

/retest

repokitteh-read-only · 2021-04-23T14:36:36Z

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

🐱

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

see: more, trace.

alyssawilk · 2021-04-26T15:47:54Z

@antoniovicente you still doing passes here or should I take this back now I've returned?

antoniovicente

@alyssawilk

I think this looks good except for a few nits. Off to you.

test/common/quic/active_quic_listener_test.cc

antoniovicente · 2021-04-27T00:21:57Z

test/integration/quic_http_integration_test.cc

        *dispatcher_,
-        /*send_buffer_limit=*/2 *
-            Http3::Utility::OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE);
+        // Use smaller window then the configured one to have test coverage of client codec buffer


change "configured" to "default"

source/common/quic/client_connection_factory_impl.cc

alyssawilk

Looks great! Thanks for first pass Antonio - I literally had to delete 2 comments when I saw you'd asked the same questions already :-P

alyssawilk · 2021-04-27T13:17:04Z

source/common/quic/envoy_quic_client_stream.cc

 void EnvoyQuicClientStream::encodeMetadata(const Http::MetadataMapVector& /*metadata_map_vector*/) {
  // Metadata Frame is not supported in QUIC.
  // TODO(danzh): add stats for metadata not supported error.
+  ENVOY_BUG(false, "METADATA is not supported in Http3.");


I think HTTP/1.1 just swallows it with a stat increase. I'd mildly prefer that to crashing in debug mode.

okay, I downgrade it to debug log and incremented the stats.

alyssawilk · 2021-04-27T13:21:07Z

test/common/quic/active_quic_listener_test.cc

+                ->config()
+                ->GetInitialSessionFlowControlWindowToSend());
+  // IETF Quic supports low flow control limit. But Google Quic only supports flow control window no
+  // smaller than 16kB.


do we have a test for setting below 16k making sure that gQUIC ends up 16k? Basically if "upstream" ever changes the default I'd like to make sure we catch it.

alyssawilk · 2021-04-27T13:22:59Z

test/config/utility.h


+  // Adjust the upstream route with larger timeout if running tsan. This is the duration between
+  // whole request being processed and whole response received.
+  static void adjustUpstreamTimeoutForTsan(


Can you file a tracking bug for this one? I'm concerned about QUIC being slower, both on tsan and not and I'd like us to look into it at some point.

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

alyssawilk

LGTM, modulo any remaining comments from Antonio :-)

alyssawilk · 2021-04-28T15:17:17Z

(oh, and making sure CI is happy)

danzh2010 · 2021-04-28T17:07:39Z

Windows CI fails with irrelevant test, filed #16214

danzh2010 · 2021-04-28T17:07:45Z

/retest

repokitteh-read-only · 2021-04-28T17:07:50Z

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

🐱

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

see: more, trace.

danzh2010 · 2021-04-28T22:28:47Z

/retest

repokitteh-read-only · 2021-04-28T22:28:51Z

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

🐱

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

see: more, trace.

antoniovicente

Looks good. Thanks Dan.

alyssawilk · 2021-04-29T13:22:57Z

Argh, looks like this conflicts with the headers PR. Can you do a main merge, and we'll need another stamp from @envoyproxy/api-shepherds but otherwise this is good to go!

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

htuch

/lgtm api

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

Commit Message: Add initial stream and connection level flow control windows to envoy.config.core.v3.QuicProtocolOptions which is be used in QUIC listener config and Http3 upstream cluster config. Risk Level: low Testing: re-enable more Http3 downstream protocol test. Part of envoyproxy#2557 envoyproxy#12930 envoyproxy#14829 Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

danzh1989 added 10 commits April 1, 2021 12:03

stash conflict

d563ede

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

resolve conflict

b383bcd

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

Merge branch 'master' into flowcontrol

1edab39

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

add flow control window to config

6541d12

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

Merge branch 'master' into flowcontrol

6f9bcc3

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

re-enabled tests

3dc4f63

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

Merge branch 'master' into flowcontrol

203578b

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

fix build

29f5db8

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

add protodoc_manifest

68a5dcf

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

add upstream support

745dcbb

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

repokitteh-read-only bot added the api label Apr 7, 2021

repokitteh-read-only bot assigned markdroth Apr 7, 2021

danzh1989 added 3 commits April 7, 2021 13:14

fix doc

7dddea3

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

fix protodoc_manifest

dc4287d

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

remove proto deps

ce8ab12

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

danzh1989 added 2 commits April 8, 2021 11:14

rename quic stream flow control

e5b8366

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

revert quic_config proto change

3338ebe

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

antoniovicente reviewed Apr 8, 2021

View reviewed changes

api/envoy/config/core/v3/protocol.proto Show resolved Hide resolved

danzh1989 added 2 commits April 11, 2021 18:30

fix compiler option

c59d793

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

proto fix

4d9eef5

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

danzh2010 mentioned this pull request Apr 12, 2021

http3: turning up more tests #15853

Merged

fix batch writer test build

a838c48

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

repokitteh-read-only bot assigned alyssawilk and antoniovicente Apr 13, 2021

antoniovicente previously approved these changes Apr 27, 2021

View reviewed changes

alyssawilk reviewed Apr 27, 2021

View reviewed changes

stats for metadata not supported

c8f74a0

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

danzh2010 dismissed antoniovicente’s stale review via c8f74a0 April 27, 2021 19:12

alyssawilk previously approved these changes Apr 28, 2021

View reviewed changes

antoniovicente previously approved these changes Apr 28, 2021

View reviewed changes

Merge branch 'master' into flowcontrol

ef1f953

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

danzh2010 dismissed stale reviews from antoniovicente and alyssawilk via ef1f953 April 29, 2021 18:51

enable more test

b8340d9

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

htuch reviewed Apr 30, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Apr 30, 2021

disable the test again

ccb8bbd

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

alyssawilk approved these changes May 3, 2021

View reviewed changes

alyssawilk merged commit fe62d97 into envoyproxy:main May 3, 2021

Conversation

danzh2010 commented Apr 7, 2021

Uh oh!

repokitteh-read-only bot commented Apr 7, 2021

Uh oh!

danzh2010 commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

Uh oh!

danzh2010 commented Apr 12, 2021

Uh oh!

repokitteh-read-only bot commented Apr 12, 2021

Uh oh!

danzh2010 commented Apr 13, 2021

Uh oh!

repokitteh-read-only bot commented Apr 23, 2021

Uh oh!

danzh2010 commented Apr 23, 2021

Uh oh!

repokitteh-read-only bot commented Apr 23, 2021

Uh oh!

alyssawilk commented Apr 26, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

danzh2010 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

danzh2010 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Apr 28, 2021

Uh oh!

danzh2010 commented Apr 28, 2021

Uh oh!

danzh2010 commented Apr 28, 2021

Uh oh!

repokitteh-read-only bot commented Apr 28, 2021

Uh oh!

danzh2010 commented Apr 28, 2021

Uh oh!

repokitteh-read-only bot commented Apr 28, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Apr 29, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!