diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 21dff258f..dc2e006f6 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -1,7 +1,7 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") -ENVOY_COMMIT = "582ab4a353ac2c19f4326115b471cebeabe7ae8a" # April 17th, 2020 -ENVOY_SHA = "fd019d30f45bbc781e5e95324e3b16abcd23beca19baf0aa762bd39c602ddac1" +ENVOY_COMMIT = "888e0e28900a470df448c65d7b99d8065fd60251" # May 9th, 2020 +ENVOY_SHA = "9a4e2342d1ddaf2c9ff6f819f2010d35871f8c19a93b58ee63f2e0fc57fc9fe6" RULES_PYTHON_COMMIT = "dd7f9c5f01bafbfea08c44092b6b0c8fc8fcb77f" # Feb 22nd, 2020 RULES_PYTHON_SHA = "0aa9ec790a58053e3ab5af397879b267a625955f8297c239b2d8559c6773397b" diff --git a/source/client/BUILD b/source/client/BUILD index 3d65f3640..7d4ede8f4 100644 --- a/source/client/BUILD +++ b/source/client/BUILD @@ -57,7 +57,7 @@ envoy_cc_library( "@envoy//source/common/http:header_map_lib_with_external_headers", "@envoy//source/common/http:headers_lib_with_external_headers", "@envoy//source/common/http/http1:codec_lib_with_external_headers", - "@envoy//source/common/http/http1:conn_pool_legacy_lib_with_external_headers", + "@envoy//source/common/http/http1:conn_pool_lib_with_external_headers", "@envoy//source/common/http/http2:conn_pool_lib_with_external_headers", "@envoy//source/common/init:manager_lib_with_external_headers", "@envoy//source/common/local_info:local_info_lib_with_external_headers", diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 99b5a3a65..37902e58f 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -26,7 +26,11 @@ Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, // In prefetch mode we try to keep the amount of connections at the configured limit. if (prefetch_connections_) { while (host_->cluster().resourceManager(priority_).connections().canCreate()) { - createNewConnection(); + // We cannot rely on ::tryCreateConnection here, because that might decline without + // updating connections().canCreate() above. We would risk an infinite loop. + ActiveClientPtr client = instantiateActiveClient(); + connecting_request_capacity_ += client->effectiveConcurrentRequestLimit(); + client->moveIntoList(std::move(client), owningList(client->state_)); } } @@ -34,8 +38,7 @@ Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, // of ready_clients_, which will pick the oldest one instead. This makes us cycle through // all the available connections. if (!ready_clients_.empty() && connection_reuse_strategy_ == ConnectionReuseStrategy::LRU) { - ready_clients_.back()->moveBetweenLists(ready_clients_, busy_clients_); - attachRequestToClient(*busy_clients_.front(), response_decoder, callbacks); + attachRequestToClient(*ready_clients_.back(), response_decoder, callbacks); return nullptr; } @@ -43,52 +46,6 @@ Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, return ConnPoolImpl::newStream(response_decoder, callbacks); } -Http2PoolImpl::Http2PoolImpl( - Envoy::Event::Dispatcher& dispatcher, Envoy::Upstream::HostConstSharedPtr host, - Envoy::Upstream::ResourcePriority priority, - const Envoy::Network::ConnectionSocket::OptionsSharedPtr& options, // NOLINT - const Envoy::Network::TransportSocketOptionsSharedPtr& transport_socket_options) // NOLINT - : Envoy::Http::Legacy::ConnPoolImplBase(std::move(host), priority), dispatcher_(dispatcher), - socket_options_(options), transport_socket_options_(transport_socket_options) { - for (uint32_t i = 0; i < host_->cluster().resourceManager(priority_).connections().max(); i++) { - pools_.push_back(std::make_unique( - dispatcher_, host_, priority_, socket_options_, transport_socket_options_)); - } -} - -void Http2PoolImpl::addDrainedCallback(Envoy::Http::ConnectionPool::Instance::DrainedCb cb) { - for (auto& pool : pools_) { - pool->addDrainedCallback(cb); - } -} - -void Http2PoolImpl::drainConnections() { - for (auto& pool : pools_) { - pool->drainConnections(); - } -} - -bool Http2PoolImpl::hasActiveConnections() const { - for (auto& pool : pools_) { - if (pool->hasActiveConnections()) { - return true; - } - } - return false; -} - -Envoy::Http::ConnectionPool::Cancellable* -Http2PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, - Envoy::Http::ConnectionPool::Callbacks& callbacks) { - // Use the simplest but probably naive approach of rotating over the available pool instances - // / connections to distribute requests accross them. - return pools_[pool_round_robin_index_++ % pools_.size()]->newStream(response_decoder, callbacks); -}; - -void Http2PoolImpl::checkForDrained() { - // TODO(oschaaf): this one is protected, can't forward it. -} - BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, @@ -200,9 +157,12 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai case Envoy::Http::ConnectionPool::PoolFailureReason::Overflow: benchmark_client_stats_.pool_overflow_.inc(); break; - case Envoy::Http::ConnectionPool::PoolFailureReason::ConnectionFailure: + case Envoy::Http::ConnectionPool::PoolFailureReason::LocalConnectionFailure: + case Envoy::Http::ConnectionPool::PoolFailureReason::RemoteConnectionFailure: benchmark_client_stats_.pool_connection_failure_.inc(); break; + case Envoy::Http::ConnectionPool::PoolFailureReason::Timeout: + break; default: NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 2a42aebdf..a40e90a19 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -15,7 +15,7 @@ #include "nighthawk/common/statistic.h" #include "external/envoy/source/common/common/logger.h" -#include "external/envoy/source/common/http/http1/conn_pool_legacy.h" +#include "external/envoy/source/common/http/http1/conn_pool.h" #include "external/envoy/source/common/http/http2/conn_pool.h" #include "external/envoy/source/common/runtime/runtime_impl.h" @@ -45,13 +45,13 @@ struct BenchmarkClientStats { ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) }; -class Http1PoolImpl : public Envoy::Http::Legacy::Http1::ProdConnPoolImpl { +class Http1PoolImpl : public Envoy::Http::Http1::ProdConnPoolImpl { public: enum class ConnectionReuseStrategy { MRU, LRU, }; - using Envoy::Http::Legacy::Http1::ProdConnPoolImpl::ProdConnPoolImpl; + using Envoy::Http::Http1::ProdConnPoolImpl::ProdConnPoolImpl; Envoy::Http::ConnectionPool::Cancellable* newStream(Envoy::Http::ResponseDecoder& response_decoder, Envoy::Http::ConnectionPool::Callbacks& callbacks) override; @@ -67,49 +67,6 @@ class Http1PoolImpl : public Envoy::Http::Legacy::Http1::ProdConnPoolImpl { bool prefetch_connections_{}; }; -// Vanilla Envoy's HTTP/2 pool is single connection only (or actually sometimes dual in connection -// drainage scenarios). Http2PoolImpl is an experimental pool, which uses multiple vanilla Envoy -// HTTP/2 pools under the hood. Using multiple connections is useful when testing backends that need -// multiple connections to distribute load internally. Combining multiple connections with -// --max-requests-per-connection may help as well, as doing periodically initiating new connections -// may help the benchmark target by giving it an opportunity to rebalance. -class Http2PoolImpl : public Envoy::Http::ConnectionPool::Instance, - public Envoy::Http::Legacy::ConnPoolImplBase { -public: - // For doc comments, see Envoy::Http::ConnectionPool::Instance & Envoy::Http::ConnPoolImplBase - Http2PoolImpl( - Envoy::Event::Dispatcher& dispatcher, Envoy::Upstream::HostConstSharedPtr host, - Envoy::Upstream::ResourcePriority priority, - const Envoy::Network::ConnectionSocket::OptionsSharedPtr& options, // NOLINT - const Envoy::Network::TransportSocketOptionsSharedPtr& transport_socket_options); // NOLINT - - // Envoy::Http::ConnectionPool::Instance - Envoy::Http::Protocol protocol() const override { return Envoy::Http::Protocol::Http2; } - - void addDrainedCallback(Envoy::Http::ConnectionPool::Instance::DrainedCb cb) override; - - void drainConnections() override; - - bool hasActiveConnections() const override; - - Envoy::Upstream::HostDescriptionConstSharedPtr host() const override { return host_; }; - - Envoy::Http::ConnectionPool::Cancellable* - newStream(Envoy::Http::ResponseDecoder& response_decoder, - Envoy::Http::ConnectionPool::Callbacks& callbacks) override; - -protected: - // Envoy::Http::ConnPoolImplBase - void checkForDrained() override; - -private: - Envoy::Event::Dispatcher& dispatcher_; - const Envoy::Network::ConnectionSocket::OptionsSharedPtr socket_options_; - const Envoy::Network::TransportSocketOptionsSharedPtr transport_socket_options_; - std::vector> pools_; - uint64_t pool_round_robin_index_{0}; -}; - class BenchmarkClientHttpImpl : public BenchmarkClient, public StreamDecoderCompletionCallback, public Envoy::Logger::Loggable { diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 816d063f3..df5845bed 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -74,9 +74,6 @@ class ClusterManagerFactory : public Envoy::Upstream::ProdClusterManagerFactory h1_pool->setConnectionReuseStrategy(connection_reuse_strategy_); h1_pool->setPrefetchConnections(prefetch_connections_); return Envoy::Http::ConnectionPool::InstancePtr{h1_pool}; - } else if (use_multi_conn_h2_pool_ && protocol == Envoy::Http::Protocol::Http2) { - return Envoy::Http::ConnectionPool::InstancePtr{ - new Http2PoolImpl(dispatcher, host, priority, options, transport_socket_options)}; } return Envoy::Upstream::ProdClusterManagerFactory::allocateConnPool( dispatcher, host, priority, protocol, options, transport_socket_options); @@ -89,12 +86,10 @@ class ClusterManagerFactory : public Envoy::Upstream::ProdClusterManagerFactory void setPrefetchConnections(const bool prefetch_connections) { prefetch_connections_ = prefetch_connections; } - void enableMultiConnectionH2Pool() { use_multi_conn_h2_pool_ = true; } private: Http1PoolImpl::ConnectionReuseStrategy connection_reuse_strategy_{}; bool prefetch_connections_{}; - bool use_multi_conn_h2_pool_{}; }; ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system) @@ -278,6 +273,9 @@ void ProcessImpl::createBootstrapConfiguration(envoy::config::bootstrap::v3::Boo cluster->set_name(fmt::format("{}", i)); cluster->mutable_connect_timeout()->set_seconds(options_.timeout().count()); cluster->mutable_max_requests_per_connection()->set_value(options_.maxRequestsPerConnection()); + if (options_.h2() && options_.h2UseMultipleConnections()) { + cluster->mutable_http2_protocol_options()->mutable_max_concurrent_streams()->set_value(1); + } auto thresholds = cluster->mutable_circuit_breakers()->add_thresholds(); // We do not support any retrying. @@ -386,7 +384,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector( Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl( - *dispatcher_, tls_, {}, *local_info_, init_manager_, store_root_, generator_, + *dispatcher_, tls_, {}, *local_info_, store_root_, generator_, Envoy::ProtobufMessage::getStrictValidationVisitor(), *api_)}); ssl_context_manager_ = std::make_unique(time_system_); @@ -400,9 +398,6 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vectorsetPrefetchConnections(options_.prefetchConnections()); - if (options_.h2UseMultipleConnections()) { - cluster_manager_factory_->enableMultiConnectionH2Pool(); - } envoy::config::bootstrap::v3::Bootstrap bootstrap; createBootstrapConfiguration(bootstrap, uris, request_source_uri, number_of_workers); if (tracing_uri != nullptr) { diff --git a/source/server/README.md b/source/server/README.md index 010023690..e3dda42f4 100644 --- a/source/server/README.md +++ b/source/server/README.md @@ -100,17 +100,20 @@ bazel-bin/nighthawk_test_server [--disable-extensions ] [--hot-restart-version] [--restart-epoch ] [--log-path ] -[--log-format-escaped] [--log-format -] [--component-log-level -] [-l ] -[--local-address-ip-version ] -[--admin-address-path ] +[--log-format-prefix-with-location +] [--log-format-escaped] +[--log-format ] +[--component-log-level ] [-l +] [--local-address-ip-version +] [--admin-address-path +] [--reject-unknown-dynamic-fields] [--allow-unknown-static-fields] -[--allow-unknown-fields] [--config-yaml -] [-c ] [--concurrency -] [--base-id ] [--] -[--version] [-h] +[--allow-unknown-fields] +[--bootstrap-version ] +[--config-yaml ] [-c ] +[--concurrency ] [--base-id +] [--] [--version] [-h] Where: @@ -167,6 +170,10 @@ hot restart epoch # --log-path Path to logfile +--log-format-prefix-with-location +Prefix all occurrences of '%v' in log format with with '[%g:%#] ' +('[path/to/file.cc:99] '). + --log-format-escaped Escape c-style escape sequences in the application logs @@ -201,6 +208,10 @@ allow unknown fields in static configuration --allow-unknown-fields allow unknown fields in static configuration (DEPRECATED) +--bootstrap-version +API version to parse the bootstrap config as (e.g. 3). If unset, all +known versions will be attempted + --config-yaml Inline YAML configuration, merges with the contents of --config-path diff --git a/test/BUILD b/test/BUILD index 2c9db7a40..29cce7730 100644 --- a/test/BUILD +++ b/test/BUILD @@ -55,7 +55,6 @@ envoy_cc_test( "//test/mocks/common:mock_termination_predicate_factory", "@envoy//source/common/api:api_lib", "@envoy//source/common/stats:isolated_store_lib_with_external_headers", - "@envoy//test/mocks/init:init_mocks", "@envoy//test/mocks/local_info:local_info_mocks", "@envoy//test/mocks/protobuf:protobuf_mocks", "@envoy//test/mocks/thread_local:thread_local_mocks", @@ -238,7 +237,6 @@ envoy_cc_test( deps = [ "//source/common:nighthawk_common_lib", "@envoy//source/common/stats:isolated_store_lib_with_external_headers", - "@envoy//test/mocks/init:init_mocks", "@envoy//test/mocks/local_info:local_info_mocks", "@envoy//test/mocks/protobuf:protobuf_mocks", "@envoy//test/mocks/thread_local:thread_local_mocks", diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 62a65cb60..983995df5 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -223,10 +223,12 @@ TEST_F(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { TEST_F(BenchmarkClientHttpTest, PoolFailures) { setupBenchmarkClient(); - client_->onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason::ConnectionFailure); + client_->onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason::LocalConnectionFailure); + client_->onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); client_->onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason::Overflow); + client_->onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason::Timeout); EXPECT_EQ(1, getCounter("pool_overflow")); - EXPECT_EQ(1, getCounter("pool_connection_failure")); + EXPECT_EQ(2, getCounter("pool_connection_failure")); } TEST_F(BenchmarkClientHttpTest, RequestMethodPost) { diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc index 5da4167bb..4a7b59517 100644 --- a/test/client_worker_test.cc +++ b/test/client_worker_test.cc @@ -5,7 +5,6 @@ #include "external/envoy/source/common/runtime/runtime_impl.h" #include "external/envoy/source/common/stats/isolated_store_impl.h" -#include "external/envoy/test/mocks/init/mocks.h" #include "external/envoy/test/mocks/local_info/mocks.h" #include "external/envoy/test/mocks/protobuf/mocks.h" #include "external/envoy/test/mocks/thread_local/mocks.h" @@ -36,9 +35,9 @@ class ClientWorkerTest : public Test { public: ClientWorkerTest() : api_(Envoy::Api::createApiForTest()), thread_id_(std::this_thread::get_id()) { - loader_ = std::make_unique(Envoy::Runtime::LoaderPtr{ - new Envoy::Runtime::LoaderImpl(dispatcher_, tls_, {}, local_info_, init_manager_, store_, - rand_, validation_visitor_, *api_)}); + loader_ = std::make_unique( + Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl( + dispatcher_, tls_, {}, local_info_, store_, rand_, validation_visitor_, *api_)}); benchmark_client_ = new MockBenchmarkClient(); sequencer_ = new MockSequencer(); request_generator_ = new MockRequestSource(); @@ -95,7 +94,6 @@ class ClientWorkerTest : public Test { NiceMock dispatcher_; std::unique_ptr loader_; NiceMock local_info_; - Envoy::Init::MockManager init_manager_; NiceMock validation_visitor_; Envoy::Upstream::ClusterManagerPtr cluster_manager_ptr_; Envoy::Tracing::HttpTracerSharedPtr http_tracer_; diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 95619bfbf..a34ef52fb 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -264,16 +264,22 @@ def test_https_h2(https_test_server_fixture): def test_https_h2_multiple_connections(https_test_server_fixture): """ Test that the experimental h2 pool uses multiple connections. + The burst we send ensures we will need 10 connections right away, as we + limit max active streams per connection to 1 by setting the experimental + flag to use multiple h2 connections. """ parsed_json, _ = https_test_server_fixture.runNighthawkClient([ "--h2", https_test_server_fixture.getTestServerRootUri(), "--rps", "100", "--duration", "100", - "--termination-predicate", "benchmark.http_2xx:9", "--max-active-requests", "1", - "--experimental-h2-use-multiple-connections" + "--termination-predicate", "benchmark.http_2xx:99", "--max-active-requests", "10", + "--max-pending-requests", "10", "--experimental-h2-use-multiple-connections", "--burst-size", + "10" ]) counters = https_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json) - assertCounterEqual(counters, "benchmark.http_2xx", 10) - assertCounterEqual(counters, "upstream_cx_http2_total", 10) + assertCounterEqual(counters, "benchmark.http_2xx", 100) + # Empirical observation shows we may end up creating more then 10 connections. + # This is stock Envoy h/2 pool behavior. + assertCounterGreaterEqual(counters, "upstream_cx_http2_total", 10) def _do_tls_configuration_test(https_test_server_fixture, cli_parameter, use_h2): diff --git a/test/worker_test.cc b/test/worker_test.cc index 8cdad7aff..30263f0af 100644 --- a/test/worker_test.cc +++ b/test/worker_test.cc @@ -2,7 +2,6 @@ #include "external/envoy/source/common/runtime/runtime_impl.h" #include "external/envoy/source/common/stats/isolated_store_impl.h" -#include "external/envoy/test/mocks/init/mocks.h" #include "external/envoy/test/mocks/local_info/mocks.h" #include "external/envoy/test/mocks/protobuf/mocks.h" #include "external/envoy/test/mocks/thread_local/mocks.h" @@ -39,7 +38,6 @@ class WorkerTest : public Test { Envoy::Stats::IsolatedStoreImpl test_store_; Envoy::Runtime::RandomGeneratorImpl rand_; NiceMock local_info_; - Envoy::Init::MockManager init_manager_; NiceMock validation_visitor_; }; @@ -52,9 +50,9 @@ TEST_F(WorkerTest, WorkerExecutesOnThread) { TestWorker worker(*api_, tls_); NiceMock dispatcher; std::unique_ptr loader = - std::make_unique(Envoy::Runtime::LoaderPtr{ - new Envoy::Runtime::LoaderImpl(dispatcher, tls_, {}, local_info_, init_manager_, - test_store_, rand_, validation_visitor_, *api_)}); + std::make_unique( + Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl( + dispatcher, tls_, {}, local_info_, test_store_, rand_, validation_visitor_, *api_)}); worker.start(); worker.waitForCompletion();