diff --git a/docs/root/intro/arch_overview/intro/threading_model.rst b/docs/root/intro/arch_overview/intro/threading_model.rst index ca83cb92e92c..18f2e822498f 100644 --- a/docs/root/intro/arch_overview/intro/threading_model.rst +++ b/docs/root/intro/arch_overview/intro/threading_model.rst @@ -24,3 +24,7 @@ to have Envoy forcibly balance connections between worker threads. To support th Envoy allows for different types of :ref:`connection balancing ` to be configured on each :ref:`listener `. + +On Windows the kernel is not able to balance the connections properly with the async IO model that Envoy is using. +Until this is fixed by the platfrom, Envoy will enforce listener connection balancing on Windows. This allows us to +balance connections between different worker threads. This behavior comes with a performance penalty. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c8e691ba050e..12e06bccefe5 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,6 +41,7 @@ Bug Fixes * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. * aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion `_. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. +* listener: fixed an issue on Windows where connections are not handled by all worker threads. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. Removed Config or Runtime @@ -70,4 +71,3 @@ Deprecated * listener: :ref:`reuse_port ` has been deprecated in favor of :ref:`enable_reuse_port `. At the same time, the default has been changed from false to true. See above for more information. - diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index cd36b2662737..b4d800e2e7e0 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -168,25 +168,33 @@ void ListenSocketFactoryImpl::doFinalPreWorkerInit() { return; } - for (auto& socket : sockets_) { - const auto rc = socket->ioHandle().listen(tcp_backlog_size_); -#ifndef WIN32 + ASSERT(!sockets_.empty()); + auto listen_and_apply_options = [](Envoy::Network::SocketSharedPtr socket, int tcp_backlog_size) { + const auto rc = socket->ioHandle().listen(tcp_backlog_size); if (rc.return_value_ != 0) { throw EnvoyException(fmt::format("cannot listen() errno={}", rc.errno_)); } -#else - // TODO(davinci26): listen() error handling and generally listening on multiple workers - // is broken right now. This needs follow up to do something better on Windows. - UNREFERENCED_PARAMETER(rc); -#endif - if (!Network::Socket::applyOptions(socket->options(), *socket, envoy::config::core::v3::SocketOption::STATE_LISTENING)) { throw Network::SocketOptionException( fmt::format("cannot set post-listen socket option on socket: {}", socket->addressProvider().localAddress()->asString())); } + }; + // On all platforms we should listen on the first socket. + auto iterator = sockets_.begin(); + listen_and_apply_options(*iterator, tcp_backlog_size_); + ++iterator; +#ifndef WIN32 + // With this implementation on Windows we only accept + // connections on Worker 1 and then we use the `ExactConnectionBalancer` + // to balance these connections to all workers. + // TODO(davinci26): We should update the behavior when socket duplication + // does not cause accepts to hang in the OS. + for (; iterator != sockets_.end(); ++iterator) { + listen_and_apply_options(*iterator, tcp_backlog_size_); } +#endif } ListenerFactoryContextBaseImpl::ListenerFactoryContextBaseImpl( @@ -545,6 +553,19 @@ void ListenerImpl::buildFilterChains() { void ListenerImpl::buildSocketOptions() { // TCP specific setup. if (connection_balancer_ == nullptr) { +#ifdef WIN32 + // On Windows we use the exact connection balancer to dispatch connections + // from worker 1 to all workers. This is a perf hit but it is the only way + // to make all the workers do work. + // TODO(davinci26): We can be faster here if we create a balancer implementation + // that dispatches the connection to a random thread. + ENVOY_LOG(warn, + "ExactBalance was forced enabled for TCP listener '{}' because " + "Envoy is running on Windows." + "ExactBalance is used to load balance connections between workers on Windows.", + config_.name()); + connection_balancer_ = std::make_shared(); +#else // Not in place listener update. if (config_.has_connection_balance_config()) { // Currently exact balance is the only supported type and there are no options. @@ -553,6 +574,7 @@ void ListenerImpl::buildSocketOptions() { } else { connection_balancer_ = std::make_shared(); } +#endif } if (config_.has_tcp_fast_open_queue_length()) { diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 48f68e41e49a..d62c9c520f32 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -157,6 +157,53 @@ TEST_P(IntegrationTest, PerWorkerStatsAndBalancing) { check_listener_stats(0, 1); } +// Make sure all workers pick up connections +TEST_P(IntegrationTest, AllWorkersAreHandlingLoad) { + concurrency_ = 2; + initialize(); + + std::string worker0_stat_name, worker1_stat_name; + if (GetParam() == Network::Address::IpVersion::v4) { + worker0_stat_name = "listener.127.0.0.1_0.worker_0.downstream_cx_total"; + worker1_stat_name = "listener.127.0.0.1_0.worker_1.downstream_cx_total"; + } else { + worker0_stat_name = "listener.[__1]_0.worker_0.downstream_cx_total"; + worker1_stat_name = "listener.[__1]_0.worker_1.downstream_cx_total"; + } + + test_server_->waitForCounterEq(worker0_stat_name, 0); + test_server_->waitForCounterEq(worker1_stat_name, 0); + + // We set the counters for the two workers to see how many connections each handles. + uint64_t w0_ctr = 0; + uint64_t w1_ctr = 0; + constexpr int loops = 5; + for (int i = 0; i < loops; i++) { + constexpr int requests_per_loop = 4; + std::array connections; + for (int j = 0; j < requests_per_loop; j++) { + connections[j] = makeHttpConnection(lookupPort("http")); + } + + auto worker0_ctr = test_server_->counter(worker0_stat_name); + auto worker1_ctr = test_server_->counter(worker1_stat_name); + auto target = w0_ctr + w1_ctr + requests_per_loop; + while (test_server_->counter(worker0_stat_name)->value() + + test_server_->counter(worker1_stat_name)->value() < + target) { + timeSystem().advanceTimeWait(std::chrono::milliseconds(10)); + } + w0_ctr = test_server_->counter(worker0_stat_name)->value(); + w1_ctr = test_server_->counter(worker1_stat_name)->value(); + for (int j = 0; j < requests_per_loop; j++) { + connections[j]->close(); + } + } + + EXPECT_TRUE(w0_ctr > 1); + EXPECT_TRUE(w1_ctr > 1); +} + TEST_P(IntegrationTest, RouterDirectResponseWithBody) { const std::string body = "Response body"; const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", body); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index c2c61c695c03..dd1d25cff1b6 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1837,9 +1837,6 @@ TEST_F(ListenerManagerImplTest, NotSupportedDatagramUds) { "socket type SocketType::Datagram not supported for pipes"); } -// TODO(davinci26): See ListenSocketFactoryImpl::doFinalPreWorkerInit() for why this test is -// not run on Windows. -#ifndef WIN32 TEST_F(ListenerManagerImplTest, CantListen) { InSequence s; @@ -1870,7 +1867,6 @@ name: foo 1UL, server_.stats_store_.counterFromString("listener_manager.listener_create_failure").value()); } -#endif TEST_F(ListenerManagerImplTest, CantBindSocket) { time_system_.setSystemTime(std::chrono::milliseconds(1001001001001));