From 6d0285f2c5e51be609a4feebddbbd6954c49b2d7 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 6 Mar 2024 17:35:43 +0000 Subject: [PATCH] restart flag Signed-off-by: Ali Beyad --- source/common/quic/BUILD | 3 +-- .../quic/client_connection_factory_impl.cc | 5 ++++- .../quic/envoy_quic_client_connection.cc | 20 +++++++++---------- .../quic/envoy_quic_client_connection.h | 7 ++++--- source/common/quic/envoy_quic_utils.cc | 7 +++---- source/common/quic/envoy_quic_utils.h | 3 ++- source/common/runtime/runtime_features.cc | 2 +- test/common/network/utility_test.cc | 6 ++++-- .../quic/envoy_quic_client_session_test.cc | 9 ++++++--- .../quic/envoy_quic_client_stream_test.cc | 3 ++- test/common/quic/test_utils.h | 2 +- .../integration/quic_http_integration_test.cc | 7 ++++--- 12 files changed, 42 insertions(+), 32 deletions(-) diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index 50346c554fa6..7b4beec3b832 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -180,6 +180,7 @@ envoy_cc_library( "//envoy/http:codec_interface", "//envoy/http:persistent_quic_info_interface", "//envoy/registry", + "//source/common/runtime:runtime_lib", "//source/extensions/quic/crypto_stream:envoy_quic_crypto_client_stream_lib", "//source/extensions/transport_sockets/tls:ssl_socket_lib", "@com_github_google_quiche//:quic_core_http_spdy_session_lib", @@ -362,7 +363,6 @@ envoy_cc_library( "//envoy/event:dispatcher_interface", "//source/common/network:socket_option_factory_lib", "//source/common/network:udp_packet_writer_handler_lib", - "//source/common/runtime:runtime_lib", "@com_github_google_quiche//:quic_core_connection_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], @@ -436,7 +436,6 @@ envoy_cc_library( "//source/common/network:socket_option_factory_lib", "//source/common/protobuf:utility_lib", "//source/common/quic:quic_io_handle_wrapper_lib", - "//source/common/runtime:runtime_lib", "@com_github_google_quiche//:quic_core_config_lib", "@com_github_google_quiche//:quic_core_http_header_list_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index 3317e11463bc..bf43e62fdbda 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -1,5 +1,7 @@ #include "source/common/quic/client_connection_factory_impl.h" +#include "source/common/runtime/runtime_features.h" + namespace Envoy { namespace Quic { @@ -45,7 +47,8 @@ std::unique_ptr createQuicNetworkConnection( ASSERT(!quic_versions.empty()); auto connection = std::make_unique( quic::QuicUtils::CreateRandomConnectionId(), server_addr, info_impl->conn_helper_, - info_impl->alarm_factory_, quic_versions, local_addr, dispatcher, options, generator); + info_impl->alarm_factory_, quic_versions, local_addr, dispatcher, options, generator, + Runtime::runtimeFeatureEnabled("envoy.restart_features.prefer_quic_client_udp_gro")); // TODO (danzh) move this temporary config and initial RTT configuration to h3 pool. quic::QuicConfig config = info_impl->quic_config_; diff --git a/source/common/quic/envoy_quic_client_connection.cc b/source/common/quic/envoy_quic_client_connection.cc index 27c5a87229d2..7d929a0d2744 100644 --- a/source/common/quic/envoy_quic_client_connection.cc +++ b/source/common/quic/envoy_quic_client_connection.cc @@ -7,7 +7,6 @@ #include "source/common/network/socket_option_factory.h" #include "source/common/network/udp_packet_writer_handler_impl.h" #include "source/common/quic/envoy_quic_utils.h" -#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Quic { @@ -31,35 +30,37 @@ EnvoyQuicClientConnection::EnvoyQuicClientConnection( const quic::ParsedQuicVersionVector& supported_versions, Network::Address::InstanceConstSharedPtr local_addr, Event::Dispatcher& dispatcher, const Network::ConnectionSocket::OptionsSharedPtr& options, - quic::ConnectionIdGeneratorInterface& generator) + quic::ConnectionIdGeneratorInterface& generator, const bool prefer_gro) : EnvoyQuicClientConnection( server_connection_id, helper, alarm_factory, supported_versions, dispatcher, - createConnectionSocket(initial_peer_address, local_addr, options), generator) {} + createConnectionSocket(initial_peer_address, local_addr, options, prefer_gro), generator, + prefer_gro) {} EnvoyQuicClientConnection::EnvoyQuicClientConnection( const quic::QuicConnectionId& server_connection_id, quic::QuicConnectionHelperInterface& helper, quic::QuicAlarmFactory& alarm_factory, const quic::ParsedQuicVersionVector& supported_versions, Event::Dispatcher& dispatcher, Network::ConnectionSocketPtr&& connection_socket, - quic::ConnectionIdGeneratorInterface& generator) + quic::ConnectionIdGeneratorInterface& generator, const bool prefer_gro) : EnvoyQuicClientConnection( server_connection_id, helper, alarm_factory, new EnvoyQuicPacketWriter( std::make_unique(connection_socket->ioHandle())), /*owns_writer=*/true, supported_versions, dispatcher, std::move(connection_socket), - generator) {} + generator, prefer_gro) {} EnvoyQuicClientConnection::EnvoyQuicClientConnection( const quic::QuicConnectionId& server_connection_id, quic::QuicConnectionHelperInterface& helper, quic::QuicAlarmFactory& alarm_factory, quic::QuicPacketWriter* writer, bool owns_writer, const quic::ParsedQuicVersionVector& supported_versions, Event::Dispatcher& dispatcher, Network::ConnectionSocketPtr&& connection_socket, - quic::ConnectionIdGeneratorInterface& generator) + quic::ConnectionIdGeneratorInterface& generator, const bool prefer_gro) : quic::QuicConnection(server_connection_id, quic::QuicSocketAddress(), envoyIpAddressToQuicSocketAddress( connection_socket->connectionInfoProvider().remoteAddress()->ip()), &helper, &alarm_factory, writer, owns_writer, quic::Perspective::IS_CLIENT, supported_versions, generator), - QuicNetworkConnection(std::move(connection_socket)), dispatcher_(dispatcher) {} + QuicNetworkConnection(std::move(connection_socket)), dispatcher_(dispatcher), + prefer_gro_(prefer_gro) {} void EnvoyQuicClientConnection::processPacket( Network::Address::InstanceConstSharedPtr local_address, @@ -156,7 +157,7 @@ void EnvoyQuicClientConnection::probeWithNewPort(const quic::QuicSocketAddress& // The probing socket will have the same host but a different port. auto probing_socket = createConnectionSocket(connectionSocket()->connectionInfoProvider().remoteAddress(), - new_local_address, connectionSocket()->options()); + new_local_address, connectionSocket()->options(), prefer_gro_); setUpConnectionSocket(*probing_socket, delegate_); auto writer = std::make_unique( std::make_unique(probing_socket->ioHandle())); @@ -230,8 +231,7 @@ void EnvoyQuicClientConnection::onFileEvent(uint32_t events, if (connected() && (events & Event::FileReadyType::Read)) { Api::IoErrorPtr err = Network::Utility::readPacketsFromSocket( connection_socket.ioHandle(), *connection_socket.connectionInfoProvider().localAddress(), - *this, dispatcher_.timeSource(), - Runtime::runtimeFeatureEnabled("envoy.restart_features.prefer_quic_client_udp_gro"), + *this, dispatcher_.timeSource(), prefer_gro_, // For client connections, recvmmsg consumes more CPU with no performance improvement. /*allow_mmsg=*/false, packets_dropped_); if (err == nullptr) { diff --git a/source/common/quic/envoy_quic_client_connection.h b/source/common/quic/envoy_quic_client_connection.h index d8b917e0f4dd..07a5f45c345b 100644 --- a/source/common/quic/envoy_quic_client_connection.h +++ b/source/common/quic/envoy_quic_client_connection.h @@ -60,7 +60,7 @@ class EnvoyQuicClientConnection : public quic::QuicConnection, Network::Address::InstanceConstSharedPtr local_addr, Event::Dispatcher& dispatcher, const Network::ConnectionSocket::OptionsSharedPtr& options, - quic::ConnectionIdGeneratorInterface& generator); + quic::ConnectionIdGeneratorInterface& generator, bool prefer_gro); EnvoyQuicClientConnection(const quic::QuicConnectionId& server_connection_id, quic::QuicConnectionHelperInterface& helper, @@ -69,7 +69,7 @@ class EnvoyQuicClientConnection : public quic::QuicConnection, const quic::ParsedQuicVersionVector& supported_versions, Event::Dispatcher& dispatcher, Network::ConnectionSocketPtr&& connection_socket, - quic::ConnectionIdGeneratorInterface& generator); + quic::ConnectionIdGeneratorInterface& generator, bool prefer_gro); // Network::UdpPacketProcessor void processPacket(Network::Address::InstanceConstSharedPtr local_address, @@ -133,7 +133,7 @@ class EnvoyQuicClientConnection : public quic::QuicConnection, const quic::ParsedQuicVersionVector& supported_versions, Event::Dispatcher& dispatcher, Network::ConnectionSocketPtr&& connection_socket, - quic::ConnectionIdGeneratorInterface& generator); + quic::ConnectionIdGeneratorInterface& generator, bool prefer_gro); void onFileEvent(uint32_t events, Network::ConnectionSocket& connection_socket); @@ -147,6 +147,7 @@ class EnvoyQuicClientConnection : public quic::QuicConnection, Event::Dispatcher& dispatcher_; bool migrate_port_on_path_degrading_{false}; uint8_t num_socket_switches_{0}; + const bool prefer_gro_; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 0974ced68a5a..2d111361c45c 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -10,7 +10,6 @@ #include "source/common/network/socket_option_factory.h" #include "source/common/network/utility.h" #include "source/common/protobuf/utility.h" -#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Quic { @@ -139,7 +138,8 @@ Http::StreamResetReason quicErrorCodeToEnvoyRemoteResetReason(quic::QuicErrorCod Network::ConnectionSocketPtr createConnectionSocket(const Network::Address::InstanceConstSharedPtr& peer_addr, Network::Address::InstanceConstSharedPtr& local_addr, - const Network::ConnectionSocket::OptionsSharedPtr& options) { + const Network::ConnectionSocket::OptionsSharedPtr& options, + const bool prefer_gro) { if (local_addr == nullptr) { local_addr = Network::Utility::getLocalAddress(peer_addr->ip()->version()); } @@ -151,8 +151,7 @@ createConnectionSocket(const Network::Address::InstanceConstSharedPtr& peer_addr } connection_socket->addOptions(Network::SocketOptionFactory::buildIpPacketInfoOptions()); connection_socket->addOptions(Network::SocketOptionFactory::buildRxQueueOverFlowOptions()); - if (Runtime::runtimeFeatureEnabled("envoy.restart_features.prefer_quic_client_udp_gro") && - Api::OsSysCallsSingleton::get().supportsUdpGro()) { + if (prefer_gro && Api::OsSysCallsSingleton::get().supportsUdpGro()) { connection_socket->addOptions(Network::SocketOptionFactory::buildUdpGroOptions()); } if (options != nullptr) { diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index ae63d9a78297..1dc6991ca3d3 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -161,7 +161,8 @@ Http::StreamResetReason quicErrorCodeToEnvoyRemoteResetReason(quic::QuicErrorCod Network::ConnectionSocketPtr createConnectionSocket(const Network::Address::InstanceConstSharedPtr& peer_addr, Network::Address::InstanceConstSharedPtr& local_addr, - const Network::ConnectionSocket::OptionsSharedPtr& options); + const Network::ConnectionSocket::OptionsSharedPtr& options, + bool prefer_gro = false); // Convert a cert in string form to X509 object. // Return nullptr if the bytes passed cannot be passed. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 825d9451a5e8..6cf30d6c8417 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -102,10 +102,10 @@ RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); RUNTIME_GUARD(envoy_restart_features_allow_client_socket_creation_failure); +RUNTIME_GUARD(envoy_restart_features_prefer_quic_client_udp_gro); RUNTIME_GUARD(envoy_restart_features_send_goaway_for_premature_rst_streams); RUNTIME_GUARD(envoy_restart_features_udp_read_normalize_addresses); RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads); -RUNTIME_GUARD(envoy_restart_features_prefer_quic_client_udp_gro); // Begin false flags. Most of them should come with a TODO to flip true. diff --git a/test/common/network/utility_test.cc b/test/common/network/utility_test.cc index ba84c75920f3..496226865f9a 100644 --- a/test/common/network/utility_test.cc +++ b/test/common/network/utility_test.cc @@ -829,7 +829,8 @@ TEST(PacketLoss, LossTest) { NiceMock processor; MonotonicTime time(std::chrono::seconds(0)); uint32_t packets_dropped = 0; - Utility::readFromSocket(handle, *address, processor, time, UdpRecvMsgMethod::RecvMmsg, &packets_dropped); + Utility::readFromSocket(handle, *address, processor, time, UdpRecvMsgMethod::RecvMmsg, + &packets_dropped); EXPECT_EQ(1, packets_dropped); // Send another packet. @@ -837,7 +838,8 @@ TEST(PacketLoss, LossTest) { reinterpret_cast(&storage), sizeof(storage))); // Make sure the drop count is now 2. - Utility::readFromSocket(handle, *address, processor, time, UdpRecvMsgMethod::RecvMmsg, &packets_dropped); + Utility::readFromSocket(handle, *address, processor, time, UdpRecvMsgMethod::RecvMmsg, + &packets_dropped); EXPECT_EQ(2, packets_dropped); } #endif diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index f8d13ab6bd24..99e3cdbf45ce 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -47,7 +47,7 @@ class TestEnvoyQuicClientConnection : public EnvoyQuicClientConnection { quic::ConnectionIdGeneratorInterface& generator) : EnvoyQuicClientConnection(server_connection_id, helper, alarm_factory, &writer, false, supported_versions, dispatcher, std::move(connection_socket), - generator) { + generator, /*prefer_gro=*/true) { SetEncrypter(quic::ENCRYPTION_FORWARD_SECURE, std::make_unique(quic::ENCRYPTION_FORWARD_SECURE)); InstallDecrypter(quic::ENCRYPTION_FORWARD_SECURE, @@ -77,7 +77,8 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam( quic::test::crypto_test_utils::ProofVerifierForTesting())), @@ -502,7 +503,9 @@ TEST_P(EnvoyQuicClientSessionTest, UsesUdpGro) { // Make sure the option for GRO is set on the socket. int sock_opt; socklen_t sock_len = sizeof(int); - EXPECT_EQ(0, peer_socket_->getSocketOption(SOL_UDP, UDP_GRO, &sock_opt, &sock_len).return_value_); + EXPECT_EQ(0, quic_connection_->connectionSocket() + ->getSocketOption(SOL_UDP, UDP_GRO, &sock_opt, &sock_len) + .return_value_); EXPECT_EQ(1, sock_opt); // GRO uses `recvmsg`, not `recvmmsg`. diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 7daf1fe0db1c..598db9685442 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -47,7 +47,8 @@ class EnvoyQuicClientStreamTest : public testing::Test { quic_connection_(new MockEnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, &writer_, /*owns_writer=*/false, {quic_version_}, *dispatcher_, - createConnectionSocket(peer_addr_, self_addr_, nullptr), connection_id_generator_)), + createConnectionSocket(peer_addr_, self_addr_, nullptr, /*prefer_gro=*/true), + connection_id_generator_)), quic_session_(quic_config_, {quic_version_}, std::unique_ptr(quic_connection_), *dispatcher_, quic_config_.GetInitialStreamFlowControlWindowToSend() * 2, diff --git a/test/common/quic/test_utils.h b/test/common/quic/test_utils.h index 0a1a5938eab4..fdf3a1d0e587 100644 --- a/test/common/quic/test_utils.h +++ b/test/common/quic/test_utils.h @@ -75,7 +75,7 @@ class MockEnvoyQuicClientConnection : public EnvoyQuicClientConnection { quic::ConnectionIdGeneratorInterface& generator) : EnvoyQuicClientConnection(server_connection_id, helper, alarm_factory, writer, owns_writer, supported_versions, dispatcher, std::move(connection_socket), - generator) {} + generator, /*prefer_gro=*/true) {} MOCK_METHOD(quic::MessageStatus, SendMessage, (quic::QuicMessageId, absl::Span, bool)); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 17f3242cfb06..88f545e7acd6 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -73,7 +73,8 @@ class TestEnvoyQuicClientConnection : public EnvoyQuicClientConnection { bool validation_failure_on_path_response, quic::ConnectionIdGeneratorInterface& generator) : EnvoyQuicClientConnection(server_connection_id, initial_peer_address, helper, alarm_factory, - supported_versions, local_addr, dispatcher, options, generator), + supported_versions, local_addr, dispatcher, options, generator, + /*prefer_gro=*/true), dispatcher_(dispatcher), validation_failure_on_path_response_(validation_failure_on_path_response) {} @@ -772,7 +773,7 @@ TEST_P(QuicHttpIntegrationTest, PortMigration) { Network::Address::InstanceConstSharedPtr local_addr = Network::Test::getCanonicalLoopbackAddress(version_); quic_connection_->switchConnectionSocket( - createConnectionSocket(server_addr_, local_addr, nullptr)); + createConnectionSocket(server_addr_, local_addr, nullptr, /*prefer_gro=*/true)); EXPECT_NE(old_port, local_addr->ip()->port()); // Send the rest data. codec_client_->sendData(*request_encoder_, 1024u, true); @@ -801,7 +802,7 @@ TEST_P(QuicHttpIntegrationTest, PortMigration) { auto options = std::make_shared(); options->push_back(option); quic_connection_->switchConnectionSocket( - createConnectionSocket(server_addr_, local_addr, options)); + createConnectionSocket(server_addr_, local_addr, options, /*prefer_gro=*/true)); EXPECT_TRUE(codec_client_->disconnected()); cleanupUpstreamAndDownstream(); }