Update Envoy to 888e0e28900a470df448c65d7b99d8065fd60251

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"

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",

source/client/benchmark_client_impl.cc

@@ -26,69 +26,26 @@ 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_));
     }
   }
 
   // By default, Envoy re-uses the most recent free connection. Here we pop from the back
   // 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;
   }
 
   // Vanilla Envoy pool behavior.
   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<Envoy::Http::Http2::ProdConnPoolImpl>(
-        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;
   }

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<std::unique_ptr<Envoy::Http::Http2::ProdConnPoolImpl>> pools_;
-  uint64_t pool_round_robin_index_{0};
-};
-
 class BenchmarkClientHttpImpl : public BenchmarkClient,
                                 public StreamDecoderCompletionCallback,
                                 public Envoy::Logger::Loggable<Envoy::Logger::Id::main> {

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<UriP
   store_root_.initializeThreading(*dispatcher_, tls_);
   runtime_singleton_ = std::make_unique<Envoy::Runtime::ScopedLoaderSingleton>(
       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<Extensions::TransportSockets::Tls::ContextManagerImpl>(time_system_);
@@ -400,9 +398,6 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP
           ? Http1PoolImpl::ConnectionReuseStrategy::LRU
           : Http1PoolImpl::ConnectionReuseStrategy::MRU);
   cluster_manager_factory_->setPrefetchConnections(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) {

source/server/README.md

@@ -100,17 +100,20 @@ bazel-bin/nighthawk_test_server  [--disable-extensions <string>]
 [--hot-restart-version]
 [--restart-epoch <uint32_t>]
 [--log-path <string>]
-[--log-format-escaped] [--log-format
-<string>] [--component-log-level
-<string>] [-l <string>]
-[--local-address-ip-version <string>]
-[--admin-address-path <string>]
+[--log-format-prefix-with-location
+<bool>] [--log-format-escaped]
+[--log-format <string>]
+[--component-log-level <string>] [-l
+<string>] [--local-address-ip-version
+<string>] [--admin-address-path
+<string>]
 [--reject-unknown-dynamic-fields]
 [--allow-unknown-static-fields]
-[--allow-unknown-fields] [--config-yaml
-<string>] [-c <string>] [--concurrency
-<uint32_t>] [--base-id <uint32_t>] [--]
-[--version] [-h]
+[--allow-unknown-fields]
+[--bootstrap-version <string>]
+[--config-yaml <string>] [-c <string>]
+[--concurrency <uint32_t>] [--base-id
+<uint32_t>] [--] [--version] [-h]
 
 
 Where:
@@ -167,6 +170,10 @@ hot restart epoch #
 --log-path <string>
 Path to logfile
 
+--log-format-prefix-with-location <bool>
+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 <string>
+API version to parse the bootstrap config as (e.g. 3). If unset, all
+known versions will be attempted
+
 --config-yaml <string>
 Inline YAML configuration, merges with the contents of --config-path
 

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",

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) {

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::ScopedLoaderSingleton>(Envoy::Runtime::LoaderPtr{
-        new Envoy::Runtime::LoaderImpl(dispatcher_, tls_, {}, local_info_, init_manager_, store_,
-                                       rand_, validation_visitor_, *api_)});
+    loader_ = std::make_unique<Envoy::Runtime::ScopedLoaderSingleton>(
+        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<Envoy::Event::MockDispatcher> dispatcher_;
   std::unique_ptr<Envoy::Runtime::ScopedLoaderSingleton> loader_;
   NiceMock<Envoy::LocalInfo::MockLocalInfo> local_info_;
-  Envoy::Init::MockManager init_manager_;
   NiceMock<Envoy::ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Envoy::Upstream::ClusterManagerPtr cluster_manager_ptr_;
   Envoy::Tracing::HttpTracerSharedPtr http_tracer_;

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):