diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4367cea808065..a4c69d2d1e94f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,6 +9,7 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* +* tcp: setting NODELAY in the base connection class. This should have no effect for TCP or HTTP proxying, but may improve throughput in other areas. This behavior can be temporarily reverted by setting `envoy.reloadable_features.always_nodelay` to false. * upstream: host weight changes now cause a full load balancer rebuild as opposed to happening atomically inline. This change has been made to support load balancer pre-computation of data structures based on host weight, but may have performance implications if host weight changes diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 47a7fd33f8f15..fe9225a48059e 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -49,7 +49,9 @@ CodecClient::CodecClient(Type type, Network::ClientConnectionPtr&& connection, // We just universally set no delay on connections. Theoretically we might at some point want // to make this configurable. - connection_->noDelay(true); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + connection_->noDelay(true); + } } CodecClient::~CodecClient() = default; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 4ce8ec63dd387..ee8b601d9472a 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -20,6 +20,7 @@ #include "common/network/listen_socket_impl.h" #include "common/network/raw_buffer_socket.h" #include "common/network/utility.h" +#include "common/runtime/runtime_features.h" namespace Envoy { namespace Network { @@ -781,7 +782,11 @@ ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher, TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info, bool connected) : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info, - connected) {} + connected) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + noDelay(true); + } +} void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) { if (!transport_connect_pending_) { @@ -860,6 +865,9 @@ void ClientConnectionImpl::connect() { socket_->addressProvider().remoteAddress()->asString()); const Api::SysCallIntResult result = socket_->connect(socket_->addressProvider().remoteAddress()); if (result.rc_ == 0) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + noDelay(true); + } // write will become ready. ASSERT(connecting_); } else { diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 4334fc72c9227..4040542a6bbd9 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -61,6 +61,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_500_after_100", "envoy.reloadable_features.allow_preconnect", "envoy.reloadable_features.allow_response_for_timeout", + "envoy.reloadable_features.always_nodelay", "envoy.reloadable_features.consume_all_retry_headers", "envoy.reloadable_features.check_ocsp_policy", "envoy.reloadable_features.disable_tls_inspector_injection", diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index 90426c794c490..034f647d7f516 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -6,6 +6,7 @@ #include "envoy/event/timer.h" #include "envoy/upstream/upstream.h" +#include "common/runtime/runtime_features.h" #include "common/stats/timespan_impl.h" #include "common/upstream/upstream_impl.h" @@ -31,7 +32,10 @@ ActiveTcpClient::ActiveTcpClient(Envoy::ConnectionPool::ConnPoolImplBase& parent host->cluster().stats().upstream_cx_tx_bytes_total_, host->cluster().stats().upstream_cx_tx_bytes_buffered_, &host->cluster().stats().bind_errors_, nullptr}); - connection_->noDelay(true); + + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + connection_->noDelay(true); + } connection_->connect(); } diff --git a/source/common/tcp/original_conn_pool.cc b/source/common/tcp/original_conn_pool.cc index b34c31280f89a..9aa1e21d90f48 100644 --- a/source/common/tcp/original_conn_pool.cc +++ b/source/common/tcp/original_conn_pool.cc @@ -6,6 +6,7 @@ #include "envoy/event/timer.h" #include "envoy/upstream/upstream.h" +#include "common/runtime/runtime_features.h" #include "common/stats/timespan_impl.h" #include "common/upstream/upstream_impl.h" @@ -400,7 +401,9 @@ OriginalConnPoolImpl::ActiveConn::ActiveConn(OriginalConnPoolImpl& parent) // We just universally set no delay on connections. Theoretically we might at some point want // to make this configurable. - conn_->noDelay(true); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + conn_->noDelay(true); + } } OriginalConnPoolImpl::ActiveConn::~ActiveConn() { diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 37925e764f327..71228947b8704 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -568,7 +568,9 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() { expect_close_ = false; client_->connect(); - client_->noDelay(true); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + client_->noDelay(true); + } } if (!parent_.send_bytes_.empty()) { diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 6cdc7b8ad0077..289de27c32c1b 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -2,6 +2,8 @@ #include "envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.pb.h" +#include "common/runtime/runtime_features.h" + namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -64,7 +66,9 @@ ClientPtr ClientImpl::create(Upstream::HostConstSharedPtr host, Event::Dispatche client->connection_->addConnectionCallbacks(*client); client->connection_->addReadFilter(Network::ReadFilterSharedPtr{new UpstreamReadFilter(*client)}); client->connection_->connect(); - client->connection_->noDelay(true); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + client->connection_->noDelay(true); + } return client; } diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 98191c58566a0..6e3002f20cf8c 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -12,6 +12,7 @@ #include "common/event/deferred_task.h" #include "common/network/connection_impl.h" #include "common/network/utility.h" +#include "common/runtime/runtime_features.h" #include "common/stats/timespan_impl.h" #include "extensions/transport_sockets/well_known_names.h" @@ -589,7 +590,9 @@ ConnectionHandlerImpl::ActiveTcpConnection::ActiveTcpConnection( active_connections_.listener_.stats_.downstream_cx_length_ms_, time_source)) { // We just universally set no delay on connections. Theoretically we might at some point want // to make this configurable. - connection_->noDelay(true); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) { + connection_->noDelay(true); + } auto& listener = active_connections_.listener_; listener.stats_.downstream_cx_total_.inc(); listener.stats_.downstream_cx_active_.inc(); diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 789915d03cc1e..539ba78aff393 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -86,6 +86,7 @@ envoy_cc_test( "//test/mocks/api:api_mocks", "//test/mocks/buffer:buffer_mocks", "//test/mocks/event:event_mocks", + "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", "//test/mocks/stats:stats_mocks", "//test/test_common:environment_lib", diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 63361228acbe5..f12897dffbbcd 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -122,6 +122,7 @@ class TestClientConnectionImpl : public Network::ClientConnectionImpl { public: using ClientConnectionImpl::ClientConnectionImpl; Buffer::Instance& readBuffer() { return *read_buffer_; } + ConnectionSocketPtr& socket() { return socket_; } }; class ConnectionImplTest : public testing::TestWithParam { @@ -399,11 +400,13 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeout) { ConnectionMocks mocks = createConnectionMocks(false); MockTransportSocket* transport_socket = mocks.transport_socket_.get(); IoHandlePtr io_handle = std::make_unique(0); + // Avoid setting noDelay on the fake fd of 0. + auto local_addr = std::make_shared("/pipe/path"); auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); auto server_connection = std::make_unique( *mocks.dispatcher_, - std::make_unique(std::move(io_handle), nullptr, nullptr), + std::make_unique(std::move(io_handle), local_addr, nullptr), std::move(mocks.transport_socket_), stream_info_, true); EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _)); @@ -418,10 +421,11 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) { ConnectionMocks mocks = createConnectionMocks(false); MockTransportSocket* transport_socket = mocks.transport_socket_.get(); IoHandlePtr io_handle = std::make_unique(0); + auto local_addr = std::make_shared("/pipe/path"); auto server_connection = std::make_unique( *mocks.dispatcher_, - std::make_unique(std::move(io_handle), nullptr, nullptr), + std::make_unique(std::move(io_handle), local_addr, nullptr), std::move(mocks.transport_socket_), stream_info_, true); transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); @@ -436,11 +440,12 @@ TEST_P(ConnectionImplTest, ServerTransportSocketTimeoutDisabledOnConnect) { ConnectionMocks mocks = createConnectionMocks(false); MockTransportSocket* transport_socket = mocks.transport_socket_.get(); IoHandlePtr io_handle = std::make_unique(0); + auto local_addr = std::make_shared("/pipe/path"); auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); auto server_connection = std::make_unique( *mocks.dispatcher_, - std::make_unique(std::move(io_handle), nullptr, nullptr), + std::make_unique(std::move(io_handle), local_addr, nullptr), std::move(mocks.transport_socket_), stream_info_, true); bool timer_destroyed = false; @@ -598,7 +603,24 @@ TEST_P(ConnectionImplTest, ConnectionStats) { MockConnectionStats client_connection_stats; client_connection_->setConnectionStats(client_connection_stats.toBufferStats()); EXPECT_TRUE(client_connection_->connecting()); + + // Make sure that NO_DELAY starts out false, so that the check below verifies that it transitions + // to true actually tests something. + int initial_value = 0; + socklen_t size = sizeof(int); + Api::SysCallIntResult result = testClientConnection()->socket()->getSocketOption( + IPPROTO_TCP, TCP_NODELAY, &initial_value, &size); + ASSERT_EQ(0, result.rc_); + ASSERT_EQ(0, initial_value); + client_connection_->connect(); + + int new_value = 0; + result = testClientConnection()->socket()->getSocketOption(IPPROTO_TCP, TCP_NODELAY, + &initial_value, &size); + ASSERT_EQ(0, result.rc_); + ASSERT_EQ(0, new_value); + // The Network::Connection class oddly uses onWrite as its indicator of if // it's done connection, rather than the Connected event. EXPECT_TRUE(client_connection_->connecting()); @@ -1858,22 +1880,19 @@ TEST_P(ConnectionImplTest, DelayedCloseTimeoutNullStats) { } // Test DumpState methods. -TEST_P(ConnectionImplTest, NetworkSocketDumpsWithoutAllocatingMemory) { +TEST_P(ConnectionImplTest, NetworkAndPipeSocketDumpsWithoutAllocatingMemory) { std::array buffer; OutputBufferStream ostream{buffer.data(), buffer.size()}; IoHandlePtr io_handle = std::make_unique(0); + // Avoid setting noDelay on the fake fd of 0. + auto local_addr = std::make_shared("/pipe/path"); Address::InstanceConstSharedPtr server_addr; - Address::InstanceConstSharedPtr local_addr; if (GetParam() == Network::Address::IpVersion::v4) { server_addr = Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv4Instance("1.1.1.1", 80, nullptr)}; - local_addr = Network::Address::InstanceConstSharedPtr{ - new Network::Address::Ipv4Instance("1.2.3.4", 56789, nullptr)}; } else { server_addr = Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv6Instance("::1", 80, nullptr)}; - local_addr = Network::Address::InstanceConstSharedPtr{ - new Network::Address::Ipv6Instance("::1:2:3:4", 56789, nullptr)}; } auto connection_socket = @@ -1895,12 +1914,12 @@ TEST_P(ConnectionImplTest, NetworkSocketDumpsWithoutAllocatingMemory) { contents, HasSubstr( "remote_address_: 1.1.1.1:80, direct_remote_address_: 1.1.1.1:80, local_address_: " - "1.2.3.4:56789")); + "/pipe/path")); } else { EXPECT_THAT( contents, HasSubstr("remote_address_: [::1]:80, direct_remote_address_: [::1]:80, local_address_: " - "[::1:2:3:4]:56789")); + "/pipe/path")); } } @@ -1909,10 +1928,11 @@ TEST_P(ConnectionImplTest, NetworkConnectionDumpsWithoutAllocatingMemory) { OutputBufferStream ostream{buffer.data(), buffer.size()}; ConnectionMocks mocks = createConnectionMocks(false); IoHandlePtr io_handle = std::make_unique(0); + auto local_addr = std::make_shared("/pipe/path"); auto server_connection = std::make_unique( *mocks.dispatcher_, - std::make_unique(std::move(io_handle), nullptr, nullptr), + std::make_unique(std::move(io_handle), local_addr, nullptr), std::move(mocks.transport_socket_), stream_info_, true); // Start measuring memory and dump state. diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index 7fc3408c2ccad..6eaf1aa815c16 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -319,7 +319,6 @@ class TcpConnPoolImplDestructorTest : public Event::TestUsingSimulatedTime, EXPECT_CALL(*connection_, addReadFilter(_)); EXPECT_CALL(*connection_, connect()); EXPECT_CALL(*connection_, setConnectionStats(_)); - EXPECT_CALL(*connection_, noDelay(true)); EXPECT_CALL(*connection_, streamInfo()).Times(2); EXPECT_CALL(*connection_, id()).Times(AnyNumber()); diff --git a/test/extensions/filters/network/common/redis/BUILD b/test/extensions/filters/network/common/redis/BUILD index 5bb47f0c7e491..0f27100d53fcf 100644 --- a/test/extensions/filters/network/common/redis/BUILD +++ b/test/extensions/filters/network/common/redis/BUILD @@ -58,6 +58,7 @@ envoy_cc_test( "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:host_mocks", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "@envoy_api//envoy/extensions/filters/network/redis_proxy/v3:pkg_cc_proto", ], ) diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc index 953aa9374fbd5..e68f1769c1f6f 100644 --- a/test/extensions/filters/network/common/redis/client_impl_test.cc +++ b/test/extensions/filters/network/common/redis/client_impl_test.cc @@ -14,6 +14,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/upstream/host.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -75,8 +76,9 @@ class RedisClientImplTest : public testing::Test, EXPECT_CALL(*upstream_connection_, addReadFilter(_)) .WillOnce(SaveArg<0>(&upstream_read_filter_)); EXPECT_CALL(*upstream_connection_, connect()); - EXPECT_CALL(*upstream_connection_, noDelay(true)); - + if (legacy_nodelay_) { + EXPECT_CALL(*upstream_connection_, noDelay(true)); + } redis_command_stats_ = Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable()); @@ -145,6 +147,7 @@ class RedisClientImplTest : public testing::Test, Common::Redis::RedisCommandStatsSharedPtr redis_command_stats_; std::string auth_username_; std::string auth_password_; + bool legacy_nodelay_{}; }; TEST_F(RedisClientImplTest, BatchWithZeroBufferAndTimeout) { @@ -338,6 +341,62 @@ TEST_F(RedisClientImplTest, Basic) { client_->close(); } +TEST_F(RedisClientImplTest, BasicLegacyNodelay) { + legacy_nodelay_ = true; + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.always_nodelay", "false"}}); + InSequence s; + + setup(); + + client_->initialize(auth_username_, auth_password_); + + Common::Redis::RespValue request1; + MockClientCallbacks callbacks1; + EXPECT_CALL(*encoder_, encode(Ref(request1), _)); + EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); + PoolRequest* handle1 = client_->makeRequest(request1, callbacks1); + EXPECT_NE(nullptr, handle1); + + onConnected(); + + Common::Redis::RespValue request2; + MockClientCallbacks callbacks2; + EXPECT_CALL(*encoder_, encode(Ref(request2), _)); + EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); + PoolRequest* handle2 = client_->makeRequest(request2, callbacks2); + EXPECT_NE(nullptr, handle2); + + EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_total_.value()); + EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_active_.value()); + EXPECT_EQ(2UL, host_->stats_.rq_total_.value()); + EXPECT_EQ(2UL, host_->stats_.rq_active_.value()); + + Buffer::OwnedImpl fake_data; + EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void { + InSequence s; + Common::Redis::RespValuePtr response1(new Common::Redis::RespValue()); + EXPECT_CALL(callbacks1, onResponse_(Ref(response1))); + EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _)); + EXPECT_CALL(host_->outlier_detector_, + putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _)); + callbacks_->onRespValue(std::move(response1)); + + Common::Redis::RespValuePtr response2(new Common::Redis::RespValue()); + EXPECT_CALL(callbacks2, onResponse_(Ref(response2))); + EXPECT_CALL(*connect_or_op_timer_, disableTimer()); + EXPECT_CALL(host_->outlier_detector_, + putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _)); + callbacks_->onRespValue(std::move(response2)); + })); + upstream_read_filter_->onData(fake_data, false); + + EXPECT_CALL(*upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(*connect_or_op_timer_, disableTimer()); + client_->close(); +} + class ConfigEnableCommandStats : public Config { bool disableOutlierEvents() const override { return false; } std::chrono::milliseconds opTimeout() const override { return std::chrono::milliseconds(25); } diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc index 4c6309620f3da..ce6f3cc1b24f9 100644 --- a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc +++ b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc @@ -310,6 +310,9 @@ TEST_P(QuicHttpIntegrationTest, GetRequestAndEmptyResponse) { } TEST_P(QuicHttpIntegrationTest, GetRequestAndResponseWithBody) { + // Use the old nodelay in a random test for coverage. nodelay is a no-op for QUIC. + config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false"); + initialize(); sendRequestAndVerifyResponse(default_request_headers_, /*request_size=*/0, default_response_headers_, /*response_size=*/1024, diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 348d5f22fc099..2a0275bc823a8 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1261,6 +1261,14 @@ TEST_P(IntegrationTest, ViaAppendWith100Continue) { testEnvoyHandling100Continue(false, "foo"); } +// Pick a random test and use the old nodelay for coverage. This test can be +// removed when the code path is removed. +TEST_P(IntegrationTest, ViaAppendWith100ContinueWithOldNodelay) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false"); + config_helper_.addConfigModifier(setVia("foo")); + testEnvoyHandling100Continue(false, "foo"); +} + // Test delayed close semantics for downstream HTTP/1.1 connections. When an early response is // sent by Envoy, it will wait for response acknowledgment (via FIN/RST) from the client before // closing the socket (with a timeout for ensuring cleanup). diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index e63c9d7d9d3f8..8c9665cd3b15c 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -99,6 +99,8 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamWritesFirst) { // Test TLS upstream. TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamTls) { + // Make sure old style nodelay is covered in at least one integration test. + config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false"); upstream_tls_ = true; setUpstreamProtocol(FakeHttpConnection::Type::HTTP1); config_helper_.configureUpstreamTls();