quiche: add multi-worker support for QUIC via BPF #9424

danzh2010 · 2019-12-19T18:10:07Z

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

Add BPF filter to dispatch QUIC traffic by connection id cross all workers.
Fix integration test to start after all workers starts to listen.

Risk Level: low, not in use yet
Testing: added new integration test with concurrency_ > 1 and with QUIC connection migration.
Part of #2557, #1193

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

danzh2010 · 2019-12-19T20:06:13Z

/assign @alyssawilk

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

moderation · 2019-12-20T19:00:19Z

I'm intrigued to know if this will work on RHEL 7 / Centos. Red Hat have back ported some BPF to their ancient kernels but advanced networking stuff like Cilium doesn't work. Apparently more advanced BPF capabilities have been included in RHEL 8.

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

mattklein123

This is SO cool. :)

Flushing out a few comments from a first pass.

/wait

source/extensions/quic_listeners/quiche/active_quic_listener.cc

mattklein123 · 2019-12-20T20:42:52Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+      {0x80, 0, 0, 0000000000}, //                   ld len
+      {0x35, 0, 9, 0x00000009}, //                   jlt #0x9, packet_too_short
+      {0x30, 0, 0, 0000000000}, //                   ldb [0]
+      {0x54, 0, 0, 0x00000080}, //                   and #0x80
+      {0x15, 0, 2, 0000000000}, //                   jne #0, ietf_long_header
+      {0x20, 0, 0, 0x00000001}, //                   ld [1]
+      {0x05, 0, 0, 0x00000005}, //                   ja return
+      {0x80, 0, 0, 0000000000}, //                   ietf_long_header: ld len
+      {0x35, 0, 2, 0x0000000e}, //                   jlt #0xe, packet_too_short
+      {0x20, 0, 0, 0x00000006}, //                   ld [6]
+      {0x05, 0, 0, 0x00000001}, //                   ja return
+      {0x20, 0, 0,              // packet_too_short: ld rxhash
+       static_cast<uint32_t>(SKF_AD_OFF + SKF_AD_RXHASH)},
+      {0x94, 0, 0, concurrency_}, // return:         mod #socket_count
+      {0x16, 0, 0, 0000000000},   //                 ret a


Presumably you copied this from somewhere? Do we have to have attribution? Either way, can you have a block comment up top that explains what this code does?

2nd question (sorry I don't now much about BPF internals): Is this a generic language that runs on all actual architecture? I assume this is the BPF "machine" language and the kernel will compile this to x86, aarch, etc?

This is indirectly translated from Google's internal BPF utils. I added comment about QUIC's connection ID on top.

I don't know how the kernel compiles it, but it is architecture-independent unlike assembly.

source/extensions/quic_listeners/quiche/active_quic_listener.cc

mattklein123 · 2019-12-20T20:49:44Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

+  // Change to a new port, and connection should still continue.
+  Network::Address::InstanceConstSharedPtr local_addr =
+      Network::Test::getCanonicalLoopbackAddress(version_);
+  std::cerr << "Switch socket and send the rest data\n";


del or switch to ENVOY_LOG

Changed it into comment.

tools/spelling_dictionary.txt

mattklein123 · 2019-12-20T20:50:53Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

@@ -218,5 +225,103 @@ TEST_P(QuicHttpIntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) {
            1);
 }

+TEST_P(QuicHttpIntegrationTest, MultipleQuicListeners) {


Please do a --runs_per_test=1000 loop for both of these tests both with and without BPF support.

The 2nd test is not supposed to pass without BPF. And I tested the first one with/without BPF support with 1000 runs.

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

mattklein123 · 2019-12-26T22:38:06Z

@danzh2010 the OSX test timed out on quic_http_integration_test which is probably legit.

Per our previous discussion, IMO getting this working on OSX is not a good use of time for you, but it's up to you. Feel free to just disable all of this on OSX if you want. (Though if we do that we should test no BPF on linux as well.)

/wait

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

danzh2010 · 2019-12-26T23:41:19Z

@danzh2010 the OSX test timed out on quic_http_integration_test which is probably legit.

Per our previous discussion, IMO getting this working on OSX is not a good use of time for you, but it's up to you. Feel free to just disable all of this on OSX if you want. (Though if we do that we should test no BPF on linux as well.)

/wait

Added NO BPF support test.

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

mattklein123 · 2019-12-29T21:58:37Z

@danzh2010 looks like OSX is still failing... :( (Again I would just disable it but up to you)

/wait

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

danzh2010 · 2019-12-30T22:34:53Z

@danzh2010 looks like OSX is still failing... :( (Again I would just disable it but up to you)

I would try to debug a bit more before I disable the test in MacOS.

mattklein123 · 2020-01-03T16:38:19Z

Sorry needs a master merge and then will take a look. Also looks like OSX is still failing.

/wait

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

mattklein123

Amazing stuff. Just a few nits and then LGTM.

/wait

mattklein123 · 2020-01-05T17:32:29Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+                     "to socket in Mac OS. QUIC connection is not stable with concurrency > 1");
+#else
+    ENVOY_LOG(warn, "BPF filter is not supported on this platform. QUIC won't support connection "
+                    "migration and NET rebinding.");


mattklein123 · 2020-01-05T17:33:34Z

source/extensions/quic_listeners/quiche/envoy_quic_client_connection.cc

@@ -92,6 +92,14 @@ void EnvoyQuicClientConnection::setUpConnectionSocket() {
  }
 }

+void EnvoyQuicClientConnection::switchConnectionSocket(
+    Network::ConnectionSocketPtr&& connection_socket) {
+  auto writer = new EnvoyQuicPacketWriter(*connection_socket);


nit: prefer storing in unique_ptr and then releasing into the QUICHE code, in case intermediate code throws an exception.

tools/spelling_dictionary.txt

alyssawilk

So great to see this going public for people to play around with! Mostly nits on my end and I owe the tests a pass tomorrow.

alyssawilk · 2020-01-06T21:50:47Z

include/envoy/server/active_udp_listener_config.h

@@ -21,7 +21,7 @@ class ActiveUdpListenerConfigFactory {
   * Create an ActiveUdpListenerFactory object according to given message.
   */
  virtual Network::ActiveUdpListenerFactoryPtr
-  createActiveUdpListenerFactory(const Protobuf::Message& message) PURE;
+  createActiveUdpListenerFactory(const Protobuf::Message& message, uint32_t concurrency) PURE;


Mind documenting arguments?

alyssawilk · 2020-01-06T21:51:30Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

@@ -1,5 +1,12 @@
 #include "extensions/quic_listeners/quiche/active_quic_listener.h"

+#if defined(__linux__)
+#include <linux/filter.h>
+


super nitty but mind removing the trailing line here?

alyssawilk · 2020-01-06T21:52:14Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+    if (!ok) {
+      ENVOY_LOG(warn, "Fail to apply socket options to socket {} on listener {} after binding",
+                listen_socket_.ioHandle().fd(), listener_config.name());
+      throw EnvoyException("Fail to apply socket options.");


Fail -> Failed

alyssawilk · 2020-01-06T21:53:17Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+                                                   Network::ListenerConfig& config) {
+  std::unique_ptr<Network::Socket::Options> options = std::make_unique<Network::Socket::Options>();
+#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)
+  // This BPF filter reads the 1st word of QUIC connection id in the UDP payload and mod it by the


mod -> mods

alyssawilk · 2020-01-06T21:53:53Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)
+  // This BPF filter reads the 1st word of QUIC connection id in the UDP payload and mod it by the
+  // number of workers to get the socket index in the SO_REUSEPORT socket groups. QUIC packets
+  // should be at least 9 bytes, with the 1st byte indicates one of the below QUIC packet headers:


indicates -> indicating

alyssawilk · 2020-01-06T21:54:24Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+  // 1) IETF QUIC long header: most significant bit is 1. The connection id starts from the 7th
+  // byte.
+  // 2) IETF QUIC short header: most significant bit is 0. The connection id starts from 2nd
+  // bytes.


bytes -> byte here and below

alyssawilk · 2020-01-06T21:56:44Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+  // 3) Google QUIC header: most significant bit is 0. The connection id starts from 2nd
+  // bytes.
+  // Any packet that doesn't belong to any of the three packet header types are dispatched
+  // based on 5-tuple source/destination addresses.


Because they will not have this filter applied, or because we do payload inspection and best effort guess?
(I really should know this one but if even I don't we should comment =P)

this is according to the BPF code: if the payload doesn't seems to be a valid QUIC packet, dispatch it using the network 5 tuple

alyssawilk · 2020-01-06T21:57:51Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+                     "to socket in Mac OS. QUIC connection is not stable with concurrency > 1");
+#else
+    ENVOY_LOG(warn, "BPF filter is not supported on this platform. QUIC won't support connection "
+                    "migration and NET rebinding.");


NAT port rebinding?

alyssawilk · 2020-01-06T21:58:35Z

source/extensions/quic_listeners/quiche/active_quic_listener.cc

+#else
+  if (concurrency_ > 1) {
+#ifdef __APPLE__
+    // TODO(#8794) Figure out how SO_REUSEPORT behave in Mac OS.


your call but I'd say we just list it somewhere as not supported until/if someone cares, rather than TODOing

alyssawilk · 2020-01-06T21:59:11Z

source/extensions/quic_listeners/quiche/envoy_quic_client_connection.h

@@ -40,6 +40,8 @@ class EnvoyQuicClientConnection : public EnvoyQuicConnection, public Network::Ud
  // Register file event and apply socket options.
  void setUpConnectionSocket();

+  void switchConnectionSocket(Network::ConnectionSocketPtr&& connection_socket);


alyssawilk · 2020-01-08T16:38:27Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

+}
+
+TEST_P(QuicHttpIntegrationTest, MultipleQuicListenersNoBPF) {
+#ifdef __APPLE__


idle thought, we do this enough I wonder if it's worth a macro like the #ifndef ENVOY_DISABLE_DEPRECATED_FEATURES to just have
TEST_P(QuicHttpIntegrationTest, DISABLE_FOR_APPLE(MultipleQuicListenersNoBPF))

pros: it saves test setup and teardown on the builds we don't want. cons: the macros don't stack so we might have to one-off any which used both. Optional - add it in if you think it's a good idea :-)

It'll be a really helpful macro, but since it's not widely used in this PR, I would prefer to defer it to a stand alone change.

alyssawilk · 2020-01-08T16:39:54Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

+}
+
+TEST_P(QuicHttpIntegrationTest, ConnectionMigration) {
+#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)


might as well put the TEST_P {} in this block too, so we don't create and run an empty test if it's not testable, yeah?

good point! done

alyssawilk · 2020-01-08T16:40:57Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

+  Network::Address::InstanceConstSharedPtr local_addr =
+      Network::Test::getCanonicalLoopbackAddress(version_);
+  quic_connection_->switchConnectionSocket(
+      createConnectionSocket(server_addr_, local_addr, nullptr));


Are there any stats we can check to make sure this actually migrated, and that we didn't somehow end up with a testing no-op?

Not stats on server side yet. But I added logic to check client port has been changed.

stale · 2020-01-15T17:21:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-01-22T18:19:27Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

danzh2010 · 2020-02-03T16:41:24Z

How to reopen this PR? I just come back from vacation.

alyssawilk · 2020-02-03T16:54:22Z

opened!

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

danzh2010 · 2020-02-05T19:41:53Z

Sync'ed with master and addressed all comments. PTAL!

mattklein123

LGTM, thanks. I suggest we ship/iterate. Awesome work! Will defer to @alyssawilk for further review/merge.

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

add BPF

23aa5d4

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners December 19, 2019 18:10

jmarantz assigned mattklein123 Dec 19, 2019

more logging and spell dict

b66e2cd

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

repokitteh-read-only bot assigned alyssawilk Dec 19, 2019

danzh1989 added 6 commits December 19, 2019 15:20

document BPF code

f83b723

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

revert test change

af647d0

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

fix macos

5743920

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

format

db39858

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

move definition to cc file

885902d

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

add loggable inheritance

9be69fa

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

fix override

fffe9d6

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

mattklein123 requested changes Dec 20, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 20, 2019

add test and comment

db4ea19

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

repokitteh-read-only bot removed the waiting label Dec 23, 2019

repokitteh-read-only bot added the waiting label Dec 26, 2019

add NO BPF test

a3c9720

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

repokitteh-read-only bot removed the waiting label Dec 26, 2019

limit usage of BPF to linux

289bce0

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

repokitteh-read-only bot added the waiting label Dec 29, 2019

debug logging on mac

fb065db

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

repokitteh-read-only bot removed the waiting label Dec 30, 2019

repokitteh-read-only bot added the waiting label Jan 3, 2020

danzh1989 added 2 commits January 3, 2020 13:05

fix preprocessor

09ae009

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

Merge branch 'master' into multiworker

7c1309f

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

repokitteh-read-only bot removed the waiting label Jan 3, 2020

mattklein123 requested changes Jan 5, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 5, 2020

alyssawilk reviewed Jan 6, 2020

View reviewed changes

zuercher mentioned this pull request Jan 8, 2020

tools: ability to exclude file regions from spell checking #9594

Closed

alyssawilk reviewed Jan 8, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 15, 2020

stale bot closed this Jan 22, 2020

alyssawilk reopened this Feb 3, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 3, 2020

danzh1989 added 2 commits February 4, 2020 15:53

Merge branch 'master' into multiworker

2e27fce

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

address comments

b08fd52

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

repokitteh-read-only bot removed the waiting label Feb 5, 2020

typo

9e121fb

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

mattklein123 previously approved these changes Feb 5, 2020

View reviewed changes

address comment

aaa9336

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

danzh2010 dismissed mattklein123’s stale review via aaa9336 February 6, 2020 21:07

alyssawilk approved these changes Feb 6, 2020

View reviewed changes

mattklein123 merged commit e176b30 into envoyproxy:master Feb 7, 2020

danzh2010 mentioned this pull request Feb 10, 2020

L4 consistent hashing QUIC proxy #1193

Open

quiche: add multi-worker support for QUIC via BPF #9424

quiche: add multi-worker support for QUIC via BPF #9424

Conversation

danzh2010 commented Dec 19, 2019 • edited Loading

danzh2010 commented Dec 19, 2019

moderation commented Dec 20, 2019

mattklein123 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

mattklein123 commented Dec 26, 2019

danzh2010 commented Dec 26, 2019 • edited Loading

mattklein123 commented Dec 29, 2019

danzh2010 commented Dec 30, 2019

mattklein123 commented Jan 3, 2020

mattklein123 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

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

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

stale bot commented Jan 15, 2020

stale bot commented Jan 22, 2020

danzh2010 commented Feb 3, 2020

alyssawilk commented Feb 3, 2020

danzh2010 commented Feb 5, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

danzh2010 commented Dec 19, 2019 •

edited

Loading

danzh2010 commented Dec 26, 2019 •

edited

Loading