From 4c790085fb64ccc4127b1527a87ba8ade6ae7b95 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 27 Apr 2020 11:13:02 +0200 Subject: [PATCH 1/6] Update Envoy to 0b0213fdc38eb0d0346659cf90bd4c502cadb00c Saving state - work in progress This update is a bit more challenging then usual, as the legacy connection pool has been axed out. This means we need to move towards the new pool implementations, and tackle some challenges associated to that. Signed-off-by: Otto van der Schaaf --- bazel/repositories.bzl | 4 +- source/client/BUILD | 2 +- source/client/benchmark_client_impl.cc | 53 +++----------------------- source/client/benchmark_client_impl.h | 49 ++---------------------- source/client/process_impl.cc | 4 +- test/benchmark_http_client_test.cc | 6 ++- 6 files changed, 16 insertions(+), 102 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 21dff258f..1d4c1be85 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 = "0b0213fdc38eb0d0346659cf90bd4c502cadb00c" # April 27th, 2020 +ENVOY_SHA = "cee73a7d103432d1335f6c554e0a0c8e640280191af38444d1539fe6bb259803" 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..d62b2d0f4 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -26,7 +26,7 @@ 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(); + tryCreateNewConnection(); } } @@ -43,52 +43,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 +154,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..c69eee8ac 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); @@ -278,6 +275,7 @@ 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()); + cluster->mutable_http2_protocol_options()->mutable_max_concurrent_streams()->set_value(2); auto thresholds = cluster->mutable_circuit_breakers()->add_thresholds(); // We do not support any retrying. 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) { From 78d15a5ed5ae78baec4c24d5a169347b440fbde2 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 29 Apr 2020 15:34:18 +0200 Subject: [PATCH 2/6] Fixes to work with the new connection pools - Update to make the new h1 pool work with lru/mru connection re-use & connection prefetching - add todos after analysing what needs to be done for h2 Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 9 ++++++--- source/client/process_impl.cc | 5 ++++- test/integration/test_integration_basics.py | 7 +++++-- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index d62b2d0f4..71387c649 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()) { - tryCreateNewConnection(); + // We cannot rely on ::tryCreateConnection here, because that might decline without + // updating connections().canCreate() above, which would loop indefinitly. + 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; } diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index c69eee8ac..6af4cb9df 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -275,7 +275,10 @@ 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()); - cluster->mutable_http2_protocol_options()->mutable_max_concurrent_streams()->set_value(2); + // TODO(oschaaf): expose this in configuration to allow (easy access to) using multiple h2 + // connections. if (options_.connections() > 1) { + // 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. diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 95619bfbf..3685eeaeb 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -265,11 +265,14 @@ def test_https_h2_multiple_connections(https_test_server_fixture): """ Test that the experimental h2 pool uses multiple connections. """ + # TODO(oschaaf): fix h2 & re-enable this with the new behavior based on the + # updated connection pools. + return 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:9", "--max-active-requests", "100", + "--experimental-h2-use-multiple-connections", "--connections 10" ]) counters = https_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json) assertCounterEqual(counters, "benchmark.http_2xx", 10) From a9e72c4d2083d37f15cda1bd0fd9ebe8015cf38f Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 9 May 2020 23:07:27 +0200 Subject: [PATCH 3/6] README lint + tweak h2 experimental multi-connection test Retains the experimental h2 feature based on the new h2 pool. Behavior has changed. Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 2 +- source/client/process_impl.cc | 12 +++------ source/server/README.md | 29 ++++++++++++++------- test/integration/test_integration_basics.py | 10 +++---- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 71387c649..37902e58f 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -27,7 +27,7 @@ Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, if (prefetch_connections_) { while (host_->cluster().resourceManager(priority_).connections().canCreate()) { // We cannot rely on ::tryCreateConnection here, because that might decline without - // updating connections().canCreate() above, which would loop indefinitly. + // 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_)); diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 6af4cb9df..9d21f7d21 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -86,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) @@ -275,10 +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()); - // TODO(oschaaf): expose this in configuration to allow (easy access to) using multiple h2 - // connections. if (options_.connections() > 1) { - // cluster->mutable_http2_protocol_options()->mutable_max_concurrent_streams()->set_value(1); - //} + 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. @@ -401,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/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 3685eeaeb..44862e531 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -265,17 +265,15 @@ def test_https_h2_multiple_connections(https_test_server_fixture): """ Test that the experimental h2 pool uses multiple connections. """ - # TODO(oschaaf): fix h2 & re-enable this with the new behavior based on the - # updated connection pools. - return 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", "100", - "--experimental-h2-use-multiple-connections", "--connections 10" + "--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, "benchmark.http_2xx", 100) assertCounterEqual(counters, "upstream_cx_http2_total", 10) From 35a79538fce5014d5cd30e60bd456e88acaa97c2 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 9 May 2020 23:22:31 +0200 Subject: [PATCH 4/6] Update Envoy to 888e0e28900a470df448c65d7b99d8065fd60251 Update for changed RuntimeImpl constructor Signed-off-by: Otto van der Schaaf --- bazel/repositories.bzl | 4 ++-- source/client/process_impl.cc | 2 +- test/BUILD | 2 -- test/client_worker_test.cc | 8 +++----- test/worker_test.cc | 8 +++----- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 1d4c1be85..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 = "0b0213fdc38eb0d0346659cf90bd4c502cadb00c" # April 27th, 2020 -ENVOY_SHA = "cee73a7d103432d1335f6c554e0a0c8e640280191af38444d1539fe6bb259803" +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/process_impl.cc b/source/client/process_impl.cc index 9d21f7d21..df5845bed 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -384,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_); 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/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/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(); From c34e2faad8470c34cfb6bfd05b9091cc89a7ac0d Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 9 May 2020 23:33:06 +0200 Subject: [PATCH 5/6] Lint python Signed-off-by: Otto van der Schaaf --- test/integration/test_integration_basics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 44862e531..10c9a4c26 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -269,8 +269,8 @@ def test_https_h2_multiple_connections(https_test_server_fixture): "--h2", https_test_server_fixture.getTestServerRootUri(), "--rps", "100", "--duration", "100", "--termination-predicate", "benchmark.http_2xx:99", "--max-active-requests", "10", - "--max-pending-requests", "10", "--experimental-h2-use-multiple-connections", - "--burst-size", "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", 100) From 571c3990d8329a0f617ec2a1a7a0c3ab467e5e78 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 11 May 2020 00:02:21 +0200 Subject: [PATCH 6/6] Amend test expectation, add code level comments Signed-off-by: Otto van der Schaaf --- test/integration/test_integration_basics.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 10c9a4c26..a34ef52fb 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -264,6 +264,9 @@ 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", @@ -274,7 +277,9 @@ def test_https_h2_multiple_connections(https_test_server_fixture): ]) counters = https_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json) assertCounterEqual(counters, "benchmark.http_2xx", 100) - assertCounterEqual(counters, "upstream_cx_http2_total", 10) + # 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):