udp: support forwarding packets between workers by ggreenway · Pull Request #13086 · envoyproxy/envoy

ggreenway · 2020-09-14T17:53:29Z

Signed-off-by: Greg Greenway ggreenway@apple.com

Commit Message: Also, a few fixes in active_quic_listener that were discovered while fixing
tests to work with this change:

Buffered CHLOs wouldn't be processed unless another readable event occurred on the socket. Fixed it so that some CHLOs are processed every event loop until the queue is empty.
Disabling a quic listener wouldn't immediately stop accepting new connections; it only stopped accepting connections that were buffered. The implementation would allow up to 16 connections to be accepted after disabling the listener. This made it tricky to make tests work consistently. Fixed by having also checking the runtime disable in onData().

Additional Description:
Risk Level: Medium
Testing: Fixed all existing tests; still need to add some new
Docs Changes:
Release Notes: This is mostly invisible to the user; will be used to build user-facing features later
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Also, a few fixes in active_quic_listener that were discovered while fixing tests to work with this change. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-09-14T17:54:54Z

I took inspiration on this from the ConnectionBalancer code, since it's doing a similar operation.

ggreenway · 2020-09-14T19:18:45Z

@lambdai @mattklein123 would the crash fixed in #12748 logically apply to this change as well? Eg, should I use a shared_ptr in the same way #12748 did?

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010

Nice packet re-route interface! I have some thought regarding to the QUIC fix. Overall looks good!

include/envoy/event/dispatcher.h

source/extensions/quic_listeners/quiche/active_quic_listener.cc

Signed-off-by: Greg Greenway <ggreenway@apple.com>

source/extensions/quic_listeners/quiche/active_quic_listener.cc

mattklein123

Thanks for working on this. Flushing out a few high level comments before diving into the detailed review.

/wait

include/envoy/network/listener.h

source/common/network/udp_listener_impl.cc

source/common/runtime/runtime_features.cc

include/envoy/network/listener.h

include/envoy/server/worker.h

source/extensions/quic_listeners/quiche/active_quic_listener.cc

the dispatcher for processing buffered chlos Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123

LGTM at a high level. Flushing out a few more comments. Thank you!

/wait

include/envoy/network/connection_handler.h

source/common/runtime/runtime_features.cc

source/extensions/quic_listeners/quiche/active_quic_listener.cc

source/extensions/quic_listeners/quiche/active_quic_listener.h

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-09-21T22:38:09Z

ci failure due to disk full

/retest

repokitteh-read-only · 2020-09-21T22:38:13Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13086 (comment) was created by @ggreenway.

see: more, trace.

ggreenway · 2020-09-21T23:06:54Z

coverage error wasn't caused by this PR:

Code coverage 96.5 is good and higher than limit of 96.5
Checking per-extension coverage
Per-extension coverage failed:
Code coverage for source/server/admin is lower than limit of 95.5 (95.3)

danzh2010

LGTM, alone aside the renaming request. Also can you speak of #13086 (comment)?

danzh2010 · 2020-09-22T16:45:45Z

test/extensions/quic_listeners/quiche/active_quic_listener_test.cc

-              Network::UdpPacketWriterPtr udp_packet_writer =
-                  std::make_unique<Quic::UdpGsoBatchWriter>(io_handle, scope);
-              return udp_packet_writer;
+#if UDP_GSO_BATCH_WRITER_PLATFORM_SUPPORT


Right now GSO support is determined at runtime. But GSO code can also compile at the linux kernels that doesn't support it. I think what we want here is PLATFORM_SUPPORT_POSIX or something like that.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-09-22T17:34:55Z

Also can you speak of #13086 (comment)?

Sorry, I missed that earlier. Replied.

ggreenway · 2020-09-22T17:50:06Z

@mattklein123 can you give this a final pass/approval?

mattklein123

Thanks this is really neat. Just a few small things from another pass through. Thank you!

/wait

mattklein123 · 2020-09-22T18:10:09Z

include/envoy/network/connection_handler.h

  virtual ConnectionHandler::ActiveListenerPtr
-  createActiveUdpListener(ConnectionHandler& parent, Event::Dispatcher& disptacher,
-                          Network::ListenerConfig& config) PURE;
+  createActiveUdpListener(uint32_t worker_id, ConnectionHandler& parent,


nit: worker_index, here and anywhere else we still have id. Also needs doc comment update.

mattklein123 · 2020-09-22T18:15:53Z

source/server/connection_handler_impl.cc

+    Network::UdpListenerCallbacks* udp_listener =
+        dynamic_cast<ActiveUdpListenerBase*>(details.listener_.get());
+    ASSERT(udp_listener != nullptr);
+    details.typed_listener_ = *static_cast<Network::UdpListenerCallbacks*>(udp_listener);


Is this static cast needed? It's the type of the variable above?

That must have been added during some previous variation. Removed.

mattklein123 · 2020-09-22T18:16:33Z

source/server/connection_handler_impl.cc

+    Network::UdpListenerCallbacks* udp_listener =
+        dynamic_cast<ActiveUdpListenerBase*>(details.listener_.get());


Why is the dynamic_cast needed here? This seems bad? Can you add a TODO of some type to figure out how to clean this up?

IIRC there is some mess here that we have dealt with in the past around extensions actually implementing active listener base, etc.? Is that right? It would be good to have a comment if that is the case. I know I have commented on similar things in the past. I think possible if we make QUIC/QUICHE not extension based this gets better.

Good point.

I fixed this by promoting part of ActiveUdpListenerBase to /include, so that the factory can return that type. No more casts needed. Take a look at the commit; I had to switch inheritance of ActiveListener to virtual. If we don't like that, the same can be accomplished by making ActiveListenerImplBase into a mixin template, or templatizing whether it inherits from ActiveListener or ActiveUdpListener. Either is fine with me.

mattklein123 · 2020-09-22T18:24:07Z

source/server/connection_handler_impl.h

+    absl::variant<absl::monostate, std::reference_wrapper<ActiveTcpListener>,
+                  std::reference_wrapper<Network::UdpListenerCallbacks>>
+        typed_listener_;


Just for my understanding, why do we need the variant? Is this just because it truly is a variant and it avoids multiple variables that are XOR? Maybe just add a small comment?

It's just a safe union

Signed-off-by: Greg Greenway <ggreenway@apple.com>

needing unsafe dynamic_cast Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123

Epic, thanks for the cleanups!

ggreenway · 2020-09-22T23:08:16Z

coverage error wasn't caused by this PR:

Code coverage 96.5 is good and higher than limit of 96.5
Checking per-extension coverage
Per-extension coverage failed:
Code coverage for source/server/admin is lower than limit of 95.5 (95.3)

Same on latest run

ggreenway · 2020-09-22T23:12:58Z

coverage error wasn't caused by this PR:

Code coverage 96.5 is good and higher than limit of 96.5
Checking per-extension coverage
Per-extension coverage failed:
Code coverage for source/server/admin is lower than limit of 95.5 (95.3)

Same on latest run

Actually, it looks like the stub I added is counting against coverage:

    Network::UdpListenerWorkerRouterOptRef udpListenerWorkerRouter() override {
      NOT_REACHED_GCOVR_EXCL_LINE;
    }

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-09-23T05:01:48Z

asan failure says (killed) on.a compilation step, so I think it's some resource exhaustion, not a real failure caused by this PR. @mattklein123 can you approve one more time?

/retest

repokitteh-read-only · 2020-09-23T05:01:52Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13086 (comment) was created by @ggreenway.

see: more, trace.

udp: support forwarding packets between workers

7fe3b1d

Also, a few fixes in active_quic_listener that were discovered while fixing tests to work with this change. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway requested review from alyssawilk and mattklein123 as code owners September 14, 2020 17:53

ggreenway assigned mattklein123 and danzh2010 Sep 14, 2020

ggreenway added 2 commits September 14, 2020 12:18

Merge remote-tracking branch 'upstream/master' into udp-worker-routing

f7f0899

clang-tidy

da80953

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010 reviewed Sep 14, 2020

View reviewed changes

rename

4c85ce4

Signed-off-by: Greg Greenway <ggreenway@apple.com>

antoniovicente reviewed Sep 14, 2020

View reviewed changes

source/extensions/quic_listeners/quiche/active_quic_listener.cc Show resolved Hide resolved

source/extensions/quic_listeners/quiche/active_quic_listener.cc Outdated Show resolved Hide resolved

source/extensions/quic_listeners/quiche/active_quic_listener.cc Outdated Show resolved Hide resolved

mattklein123 requested changes Sep 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 14, 2020

ggreenway added 4 commits September 14, 2020 16:23

Re-use the file event on the socket instead of creating a new event on

e4a1d6b

the dispatcher for processing buffered chlos Signed-off-by: Greg Greenway <ggreenway@apple.com>

add comment about performance

decfdd0

Signed-off-by: Greg Greenway <ggreenway@apple.com>

opt ref

5ef79ae

Signed-off-by: Greg Greenway <ggreenway@apple.com>

non-release assert

3c1485a

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Sep 15, 2020

ggreenway added 2 commits September 14, 2020 17:42

fix build

b1f030e

Signed-off-by: Greg Greenway <ggreenway@apple.com>

spelling

b3588c3

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123 requested changes Sep 15, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 15, 2020

ggreenway added 5 commits September 15, 2020 12:23

opt-ref all the things

3641fd2

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into udp-worker-routing

8ad53b6

id -> index

21a86f0

Signed-off-by: Greg Greenway <ggreenway@apple.com>

only schedule if there is a buffered chlo

d863caf

Signed-off-by: Greg Greenway <ggreenway@apple.com>

*ForTests

2380204

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Sep 15, 2020

ggreenway mentioned this pull request Sep 15, 2020

QUIC/HTTP3: docs #12923

Closed

danzh2010 reviewed Sep 22, 2020

View reviewed changes

rename to clarify this is only a compile-time check

c012882

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into udp-worker-routing

1d57d02

mattklein123 requested changes Sep 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2020

ggreenway added 4 commits September 22, 2020 11:33

/s/worker_id/worker_index/ everywhere

e2437b4

Signed-off-by: Greg Greenway <ggreenway@apple.com>

doc comment on function

0faba2e

Signed-off-by: Greg Greenway <ggreenway@apple.com>

remove no-op static_cast

f8d27af

Signed-off-by: Greg Greenway <ggreenway@apple.com>

promote an interface so that the factory can return it and avoid

efc73e0

needing unsafe dynamic_cast Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Sep 22, 2020

danzh2010 previously approved these changes Sep 22, 2020

View reviewed changes

ggreenway added 2 commits September 22, 2020 14:25

clang_tidy

155459d

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into udp-worker-routing

fc4ba9d

ggreenway dismissed danzh2010’s stale review via fc4ba9d September 22, 2020 21:28

mattklein123 previously approved these changes Sep 22, 2020

View reviewed changes

lower coverage limit

5bf2394

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed mattklein123’s stale review via 5bf2394 September 23, 2020 00:13

mattklein123 approved these changes Sep 23, 2020

View reviewed changes

ggreenway merged commit f8c3acc into envoyproxy:master Sep 23, 2020

alyssawilk mentioned this pull request Apr 15, 2021

envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing deprecation #16011

Closed

		Network::UdpListenerCallbacks* udp_listener =
		dynamic_cast<ActiveUdpListenerBase*>(details.listener_.get());

Conversation

ggreenway commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Sep 14, 2020

Uh oh!

ggreenway commented Sep 14, 2020

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway commented Sep 21, 2020

Uh oh!

repokitteh-read-only bot commented Sep 21, 2020

Uh oh!

ggreenway commented Sep 21, 2020

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Sep 22, 2020

Uh oh!

ggreenway commented Sep 22, 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!

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!

ggreenway commented Sep 22, 2020

Uh oh!

ggreenway commented Sep 22, 2020

Uh oh!

ggreenway commented Sep 23, 2020

Uh oh!

repokitteh-read-only bot commented Sep 23, 2020

Uh oh!

Reviewers

ggreenway commented Sep 14, 2020 •

edited

Loading