From 1695b23720cd474a3c3e6e4e5658008d5288bb1b Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 10:04:37 +0200 Subject: [PATCH 01/29] Update Envoy + Python integration testing - Switch to Envoy@ec47fee6604468bc392937967415c736f19fb22129929881270a1635ad216d87 - Fixes to build with this version - Remove C++ integration tests that broke unrepairably - Add Python-based integration testing - Sets us up with some basic orchestration to run servers (like the one from Nighthawk in these tests) - Add python format checking (CI) / fixing - Update grpc stub generation to a more sane approach Fixes https://github.com/envoyproxy/nighthawk/issues/50 Signed-off-by: Otto van der Schaaf --- .gitignore | 5 +- api/client/BUILD | 35 +---- bazel/repositories.bzl | 4 +- ci/do_ci.sh | 8 +- integration/BUILD | 15 ++ integration/common.py | 9 ++ .../configurations/nighthawk_http_origin.yaml | 28 ++++ .../nighthawk_https_origin.yaml | 35 +++++ integration/integration_test.py | 42 +++++ integration/integration_test_fixtures.py | 147 ++++++++++++++++++ integration/nighthawk_test_server.py | 90 +++++++++++ integration/test_integration_basics.py | 86 ++++++++++ source/client/benchmark_client_impl.cc | 16 +- source/client/process_impl.cc | 3 +- source/common/ssl.h | 11 +- source/server/http_test_server_filter.cc | 5 +- test/BUILD | 1 - test/benchmark_http_client_test.cc | 13 +- test/client_test.cc | 135 ++-------------- test/client_worker_test.cc | 5 +- test/service_test.cc | 2 +- test/statistic_test.cc | 2 +- test/worker_test.cc | 6 +- tools/.style.yapf | 6 + tools/format_python_tools.py | 71 +++++++++ tools/format_python_tools.sh | 18 +++ tools/gen_compilation_database.py | 101 ++++++------ tools/requirements.txt | 2 + tools/shell_utils.sh | 11 ++ 29 files changed, 683 insertions(+), 229 deletions(-) create mode 100644 integration/BUILD create mode 100644 integration/common.py create mode 100644 integration/configurations/nighthawk_http_origin.yaml create mode 100644 integration/configurations/nighthawk_https_origin.yaml create mode 100644 integration/integration_test.py create mode 100644 integration/integration_test_fixtures.py create mode 100644 integration/nighthawk_test_server.py create mode 100644 integration/test_integration_basics.py create mode 100644 tools/.style.yapf create mode 100755 tools/format_python_tools.py create mode 100755 tools/format_python_tools.sh create mode 100644 tools/requirements.txt create mode 100755 tools/shell_utils.sh diff --git a/.gitignore b/.gitignore index 00aa5c993..8b53bf503 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,7 @@ generated/ .rnd .vscode/ bazel.output.txt -envoy/ \ No newline at end of file +envoy/ +*.pyc +__pycache__ +tools/pyformat \ No newline at end of file diff --git a/api/client/BUILD b/api/client/BUILD index ea6534a42..9b7dbc10f 100644 --- a/api/client/BUILD +++ b/api/client/BUILD @@ -7,53 +7,32 @@ load( envoy_package() -load("@com_github_grpc_grpc//bazel:cc_grpc_library.bzl", "cc_grpc_library") load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library") +load("@com_github_grpc_grpc//bazel:cc_grpc_library.bzl", "cc_grpc_library") api_proto_library( name = "base", - srcs = [ - "options.proto", - "output.proto", - ], - visibility = ["//visibility:public"], - deps = [ - "@envoy_api//envoy/api/v2/core:base_export", - ], -) - -# TODO(oschaaf): This is a bit of a hack, we conjure the expected _base_cc_only ourselves here -# because it doesn't get generated for us. Also see https://github.com/grpc/grpc/issues/14145 -proto_library( - name = "_base_cc_only", srcs = [ "options.proto", "output.proto", "service.proto", ], + visibility = ["//visibility:public"], deps = [ - "@com_github_gogo_protobuf//:gogo_proto", - "@com_google_protobuf//:any_proto", - "@com_google_protobuf//:descriptor_proto", - "@com_google_protobuf//:duration_proto", - "@com_google_protobuf//:empty_proto", - "@com_google_protobuf//:struct_proto", - "@com_google_protobuf//:timestamp_proto", - "@com_google_protobuf//:wrappers_proto", - "@com_lyft_protoc_gen_validate//validate:validate_proto", + "@envoy_api//envoy/api/v2/auth:cert_export", + "@envoy_api//envoy/api/v2/cluster:circuit_breaker_export", "@envoy_api//envoy/api/v2/core:base_export", - "@googleapis//:http_api_protos_proto", - "@googleapis//:rpc_status_protos_lib", ], ) cc_grpc_library( name = "grpc_service_lib", srcs = [ - "service.proto", + ":base", ], + grpc_only = True, proto_only = False, - use_external = True, + use_external = False, visibility = ["//visibility:public"], well_known_protos = True, deps = [ diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 6c6c8f109..df0a01441 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 = "cd910bc54f8bbc24061aaeaf666b39422666ee58" -ENVOY_SHA = "2614c4d24269017ded547577d5ee226351de96c7d3fbbf29e4dfd3a4b7159f82" +ENVOY_COMMIT = "2f569b9a8d3f0d7a43ffa69e3e5ba947cd3a9f8b" +ENVOY_SHA = "ec47fee6604468bc392937967415c736f19fb22129929881270a1635ad216d87" def nighthawk_dependencies(): http_archive( diff --git a/ci/do_ci.sh b/ci/do_ci.sh index bf53cea09..9ebee2aa5 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -9,8 +9,10 @@ function do_build () { } function do_test() { - bazel test $BAZEL_BUILD_OPTIONS $BAZEL_TEST_OPTIONS --test_output=all \ - //test:nighthawk_test //test/server:http_test_server_filter_integration_test + bazel test $BAZEL_BUILD_OPTIONS $BAZEL_TEST_OPTIONS \ + --test_output=all \ + //test:nighthawk_test //test/server:http_test_server_filter_integration_test \ + //integration:integration_test } function do_test_with_valgrind() { @@ -76,12 +78,14 @@ function do_check_format() { echo "check_format..." cd "${SRCDIR}" ./tools/check_format.sh check + ./tools/format_python_tools.sh check } function do_fix_format() { echo "fix_format..." cd "${SRCDIR}" ./tools/check_format.sh fix + ./tools/format_python_tools.sh fix } [ -z "${NUM_CPUS}" ] && export NUM_CPUS=`grep -c ^processor /proc/cpuinfo` diff --git a/integration/BUILD b/integration/BUILD new file mode 100644 index 000000000..9a85db832 --- /dev/null +++ b/integration/BUILD @@ -0,0 +1,15 @@ +licenses(["notice"]) # Apache 2 + +py_test( + name = "integration_test", + srcs = [ + "integration_test.py", + ], + data = [ + "configurations/nighthawk_http_origin.yaml", + "configurations/nighthawk_https_origin.yaml", + "//:nighthawk_client", + "//:nighthawk_test_server", + "@envoy//test/config/integration/certs", + ], +) diff --git a/integration/common.py b/integration/common.py new file mode 100644 index 000000000..72d760435 --- /dev/null +++ b/integration/common.py @@ -0,0 +1,9 @@ +class NighthawkException(Exception): + pass + + +# TODO(oschaaf): When we're on python 3 teach IpVersion below how to render itself to a string. +class IpVersion: + UNKNOWN = 1 + IPV4 = 2 + IPV6 = 3 diff --git a/integration/configurations/nighthawk_http_origin.yaml b/integration/configurations/nighthawk_http_origin.yaml new file mode 100644 index 000000000..a38478282 --- /dev/null +++ b/integration/configurations/nighthawk_http_origin.yaml @@ -0,0 +1,28 @@ +static_resources: + listeners: + - address: + socket_address: + address: $server_ip + port_value: $server_port + filter_chains: + - filters: + - name: envoy.http_connection_manager + config: + generate_request_id: false + codec_type: auto + stat_prefix: ingress_http + route_config: + name: local_route + virtual_hosts: + - name: service + domains: + - "*" + http_filters: + - name: test-server + config: + response_body_size: 10 + response_headers: + - { header: { key: "x-nh", value: "1"}} + - name: envoy.router + config: + dynamic_stats: false diff --git a/integration/configurations/nighthawk_https_origin.yaml b/integration/configurations/nighthawk_https_origin.yaml new file mode 100644 index 000000000..b3b69f631 --- /dev/null +++ b/integration/configurations/nighthawk_https_origin.yaml @@ -0,0 +1,35 @@ +static_resources: + listeners: + - address: + socket_address: + address: $server_ip + port_value: $server_port + filter_chains: + - filters: + - name: envoy.http_connection_manager + config: + generate_request_id: false + codec_type: auto + stat_prefix: ingress_http + route_config: + name: local_route + virtual_hosts: + - name: service + domains: + - "*" + http_filters: + - name: test-server + config: + response_body_size: 10 + response_headers: + - { header: { key: "x-nh", value: "1"}} + - name: envoy.router + config: + dynamic_stats: false + tls_context: + common_tls_context: + tls_certificates: + - certificate_chain: + filename: $ssl_cert_path + private_key: + filename: $ssl_key_path \ No newline at end of file diff --git a/integration/integration_test.py b/integration/integration_test.py new file mode 100644 index 000000000..f2dda4d14 --- /dev/null +++ b/integration/integration_test.py @@ -0,0 +1,42 @@ +"""@package integration_test.py +Entry point for our integration testing +""" + +import logging +import os +import sys +import unittest + +import test_integration_basics +from common import IpVersion, NighthawkException +from integration_test_fixtures import IntegrationTestBase + + +def determineIpVersionsFromEnvironment(): + env_versions = os.environ.get("ENVOY_IP_TEST_VERSIONS", "all") + if env_versions == "v4only": + versions = [IpVersion.IPV4] + elif env_versions == "v6only": + versions = [IpVersion.IPV6] + elif env_versions == "all": + versions = [IpVersion.IPV4, IpVersion.IPV6] + else: + raise NighthawkException("Unknown ip version: '%s'" % versions) + return versions + + +def runTests(ip_version): + IntegrationTestBase.ip_version = ip_version + logging.info("Running %s integration tests." % + ("ipv6" if IntegrationTestBase.ip_version == IpVersion.IPV6 else "ipv4")) + res = unittest.TextTestRunner(verbosity=2).run( + unittest.TestLoader().loadTestsFromModule(test_integration_basics)) + return res.wasSuccessful() + + +if __name__ == '__main__': + logging.basicConfig(stream=sys.stderr, level=logging.DEBUG) + ip_versions = determineIpVersionsFromEnvironment() + assert (len(ip_versions) > 0) + ok = all(map(runTests, ip_versions)) + exit(0 if ok else 1) diff --git a/integration/integration_test_fixtures.py b/integration/integration_test_fixtures.py new file mode 100644 index 000000000..3fa295c84 --- /dev/null +++ b/integration/integration_test_fixtures.py @@ -0,0 +1,147 @@ +"""@package integration_test_fixtures +Base classes for Nighthawk integration tests +""" + +import json +import logging +import os +import socket +import subprocess +import sys +import threading +import time +import unittest + +from common import IpVersion +from nighthawk_test_server import NighthawkTestServer + + +class IntegrationTestBase(unittest.TestCase): + """ + IntegrationTestBase facilitates testing against the Nighthawk test server, by determining a free port, and starting it up in a separate process in setUp(). + """ + + def __init__(self, *args, **kwargs): + super(IntegrationTestBase, self).__init__(*args, **kwargs) + self.test_rundir = os.path.join(os.environ["TEST_SRCDIR"], os.environ["TEST_WORKSPACE"]) + + self.nighthawk_test_server_path = os.path.join(self.test_rundir, "nighthawk_test_server") + self.nighthawk_test_config_path = None + self.nighthawk_client_path = os.path.join(self.test_rundir, "nighthawk_client") + self.server_ip = "::1" if IntegrationTestBase.ip_version == IpVersion.IPV6 else "127.0.0.1" + self.socket_type = socket.AF_INET6 if IntegrationTestBase.ip_version == IpVersion.IPV6 else socket.AF_INET + self.test_server = None + self.server_port = -1 + self.parameters = dict() + + # TODO(oschaaf): For the NH test server, add a way to let it determine a port by itself and pull that + # out. + def getFreeListenerPortForAddress(self, address): + """ + Determines a free port and returns that. Theoretically it is possible that another process + will steal the port before our caller is able to leverage it, but we take that chance. + The upside is that we can push the port upon the server we are about to start through configuration + which is compatible accross servers. + """ + # TODO(oschaaf): When we are on python 3 use RAII here. + sock = socket.socket(self.socket_type, socket.SOCK_STREAM) + sock.bind((address, 0)) + port = sock.getsockname()[1] + sock.close() + return port + + def setUp(self): + """ + Performs sanity checks and starts up the server. Upon exit the server is ready to accept connections. + """ + self.assertTrue(os.path.exists(self.nighthawk_test_server_path)) + self.assertTrue(os.path.exists(self.nighthawk_client_path)) + self.server_port = self.getFreeListenerPortForAddress(self.server_ip) + self.test_server = NighthawkTestServer( + self.nighthawk_test_server_path, self.nighthawk_test_config_path, self.server_ip, + self.server_port, IntegrationTestBase.ip_version, self.parameters) + self.assertTrue(self.test_server.start()) + + def tearDown(self): + """ + Stops the server. + """ + self.assertEqual(0, self.test_server.stop()) + + def getNighthawkCounterMapFromJson(self, parsed_json): + """ + Utility method to get the counters from the json indexed by name. + Note:the `client.` prefix will be stripped from the index. + """ + return { + counter["name"][len("client."):]: int(counter["value"]) + for counter in parsed_json["results"][0]["counters"] + } + + def getTestServerRootUri(self, https=False): + """ + Utility for getting the http://host:port/ that can be used to query the server we started in setUp() + """ + uri_host = self.server_ip + if IntegrationTestBase.ip_version == IpVersion.IPV6: + uri_host = "[%s]" % self.server_ip + + uri = "%s://%s:%s/" % ("https" if https else "http", uri_host, self.test_server.server_port) + return uri + + def runNighthawk(self, args, expect_failure=False, timeout=30): + """ + Runs Nighthawk against the test server, returning a json-formatted result. + If the timeout is exceeded an exception will be raised. + """ + if IntegrationTestBase.ip_version == IpVersion.IPV6: + args.insert(0, "--address-family v6") + args.insert(0, "--output-format json") + args.insert(0, self.nighthawk_client_path) + logging.info("Nighthawk client popen() args: [%s]" % args) + client_process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + stdout, stderr = client_process.communicate() + logging.info(stderr.decode('utf-8')) + json_string = stdout.decode('utf-8') + if expect_failure: + self.assertNotEqual(0, client_process.returncode) + else: + self.assertEqual(0, client_process.returncode) + return json.loads(json_string) + + +# We don't have a straightforward way to do parameterized tests yet. +# But the tests can be run twice if desired so, and setting this to true will enable ipv6 mode. +IntegrationTestBase.ip_version = IpVersion.UNKNOWN + + +class HttpIntegrationTestBase(IntegrationTestBase): + """ + Base for running plain http tests against the Nighthawk test server + """ + + def __init__(self, *args, **kwargs): + super(HttpIntegrationTestBase, self).__init__(*args, **kwargs) + self.nighthawk_test_config_path = os.path.join( + self.test_rundir, "integration/configurations/nighthawk_http_origin.yaml") + + def getTestServerRootUri(self): + return super(HttpIntegrationTestBase, self).getTestServerRootUri(False) + + +class HttpsIntegrationTestBase(IntegrationTestBase): + """ + Base for https tests against the Nighthawk test server + """ + + def __init__(self, *args, **kwargs): + super(HttpsIntegrationTestBase, self).__init__(*args, **kwargs) + self.parameters["ssl_key_path"] = os.path.join( + self.test_rundir, "external/envoy/test/config/integration/certs/serverkey.pem") + self.parameters["ssl_cert_path"] = os.path.join( + self.test_rundir, "external/envoy/test/config/integration/certs/servercert.pem") + self.nighthawk_test_config_path = os.path.join( + self.test_rundir, "integration/configurations/nighthawk_https_origin.yaml") + + def getTestServerRootUri(self): + return super(HttpsIntegrationTestBase, self).getTestServerRootUri(True) diff --git a/integration/nighthawk_test_server.py b/integration/nighthawk_test_server.py new file mode 100644 index 000000000..635cd7162 --- /dev/null +++ b/integration/nighthawk_test_server.py @@ -0,0 +1,90 @@ +import json +import logging +import os +import socket +import subprocess +import sys +import tempfile +import threading +import time +from string import Template + +from common import IpVersion + + +class TestServerBase(object): + """ + Base class for running a server in a separate process. + """ + + def __init__(self, server_binary_path, config_template_path, server_ip, server_port, ip_version, + server_binary_config_path_arg, parameters): + self.ip_version = ip_version + self.server_binary_path = server_binary_path + self.config_template_path = config_template_path + self.server_thread = threading.Thread(target=self.serverThreadRunner) + self.server_process = None + self.server_ip = server_ip + self.socket_type = socket.AF_INET6 if ip_version == IpVersion.IPV6 else socket.AF_INET + self.server_port = server_port + self.parameters = parameters + self.server_binary_config_path_arg = server_binary_config_path_arg + + def serverThreadRunner(self): + self.parameters["server_ip"] = self.server_ip + self.parameters["server_port"] = self.server_port + + with open(self.config_template_path) as f: + config = Template(f.read()) + config = config.substitute(self.parameters) + + parameterized_config_path = None + with tempfile.NamedTemporaryFile(mode="w", delete=False, suffix=".yaml") as tmp: + parameterized_config_path = tmp.name + tmp.write(config) + + args = [self.server_binary_path, self.server_binary_config_path_arg, parameterized_config_path] + logging.info("Test server popen() args: [%s]" % args) + self.server_process = subprocess.Popen(args) + self.server_process.communicate() + + def waitUntillServerListening(self): + sock = socket.socket(self.socket_type, socket.SOCK_STREAM) + sock.settimeout(1) + tries = 10 + while tries > 0: + time.sleep(0.5) + tries -= 1 + if sock.connect_ex((self.server_ip, self.server_port)) == 0: + sock.close() + return True + logging.error("Timeout while waiting for server listener at %s:%s to accept connections.", + self.server_ip, self.server_port) + return False + + def start(self): + self.server_thread.daemon = True + self.server_thread.start() + return self.waitUntillServerListening() + + def stop(self): + self.server_process.terminate() + self.server_thread.join() + return self.server_process.returncode + + +class NighthawkTestServer(TestServerBase): + """ + Will run the Nighthawk test server in a separate process. Passes in the right cli-arg to point it to its + configuration. For, say, NGINX this would be '-c' instead. + """ + + def __init__(self, + server_binary_path, + config_template_path, + server_ip, + server_port, + ip_version, + parameters=dict()): + super(NighthawkTestServer, self).__init__(server_binary_path, config_template_path, server_ip, + server_port, ip_version, "--config-path", parameters) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py new file mode 100644 index 000000000..6ce0ce12a --- /dev/null +++ b/integration/test_integration_basics.py @@ -0,0 +1,86 @@ +#!/usr/bin/env python + +import logging +import os +import sys +import unittest + +from common import IpVersion +from integration_test_fixtures import (HttpIntegrationTestBase, HttpsIntegrationTestBase, + IntegrationTestBase) + +# TODO(oschaaf): rewrite the tests so we can just hand a map of expected key values to it. + + +class TestHttp(HttpIntegrationTestBase): + + def test_h1(self): + parsed_json = self.runNighthawk([self.getTestServerRootUri()]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["benchmark.http_2xx"], 25) + self.assertEqual(counters["upstream_cx_destroy"], 1) + self.assertEqual(counters["upstream_cx_destroy_local"], 1) + self.assertEqual(counters["upstream_cx_http1_total"], 1) + self.assertEqual(counters["upstream_cx_rx_bytes_total"], 3400) + self.assertEqual(counters["upstream_cx_total"], 1) + self.assertEqual(counters["upstream_cx_tx_bytes_total"], + 1400 if IntegrationTestBase.ip_version == IpVersion.IPV6 else 1500) + self.assertEqual(counters["upstream_rq_pending_total"], 1) + self.assertEqual(counters["upstream_rq_total"], 25) + self.assertEqual(len(counters), 9) + + def test_h2(self): + parsed_json = self.runNighthawk(["--h2", self.getTestServerRootUri()]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["benchmark.http_2xx"], 25) + self.assertEqual(counters["upstream_cx_destroy"], 1) + self.assertEqual(counters["upstream_cx_destroy_local"], 1) + self.assertEqual(counters["upstream_cx_http2_total"], 1) + self.assertGreaterEqual(counters["upstream_cx_rx_bytes_total"], 1145) + self.assertEqual(counters["upstream_cx_total"], 1) + self.assertGreaterEqual(counters["upstream_cx_tx_bytes_total"], 403) + self.assertEqual(counters["upstream_rq_pending_total"], 1) + self.assertEqual(counters["upstream_rq_total"], 25) + self.assertEqual(len(counters), 9) + + +class TestHttps(HttpsIntegrationTestBase): + + def test_h1(self): + parsed_json = self.runNighthawk([self.getTestServerRootUri()]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["benchmark.http_2xx"], 25) + self.assertEqual(counters["upstream_cx_destroy"], 1) + self.assertEqual(counters["upstream_cx_destroy_local"], 1) + self.assertEqual(counters["upstream_cx_http1_total"], 1) + self.assertEqual(counters["upstream_cx_rx_bytes_total"], 3400) + self.assertEqual(counters["upstream_cx_total"], 1) + self.assertEqual(counters["upstream_cx_tx_bytes_total"], + 1400 if IntegrationTestBase.ip_version == IpVersion.IPV6 else 1500) + self.assertEqual(counters["upstream_rq_pending_total"], 1) + self.assertEqual(counters["upstream_rq_total"], 25) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-GCM-SHA256"], 1) + self.assertEqual(counters["ssl.curves.X25519"], 1) + self.assertEqual(counters["ssl.handshake"], 1) + self.assertEqual(counters["ssl.sigalgs.rsa_pss_rsae_sha256"], 1) + self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) + self.assertEqual(len(counters), 14) + + def test_h2(self): + parsed_json = self.runNighthawk(["--h2", self.getTestServerRootUri()]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["benchmark.http_2xx"], 25) + self.assertEqual(counters["upstream_cx_destroy"], 1) + self.assertEqual(counters["upstream_cx_destroy_local"], 1) + self.assertEqual(counters["upstream_cx_http2_total"], 1) + self.assertGreaterEqual(counters["upstream_cx_rx_bytes_total"], 1145) + self.assertEqual(counters["upstream_cx_total"], 1) + self.assertGreaterEqual(counters["upstream_cx_tx_bytes_total"], 403) + self.assertEqual(counters["upstream_rq_pending_total"], 1) + self.assertEqual(counters["upstream_rq_total"], 25) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-GCM-SHA256"], 1) + self.assertEqual(counters["ssl.curves.X25519"], 1) + self.assertEqual(counters["ssl.handshake"], 1) + self.assertEqual(counters["ssl.sigalgs.rsa_pss_rsae_sha256"], 1) + self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) + self.assertEqual(len(counters), 14) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index cd39322f0..655bd05d5 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -14,6 +14,7 @@ #include "common/network/dns_impl.h" #include "common/network/raw_buffer_socket.h" #include "common/network/utility.h" +#include "common/protobuf/message_validator_impl.h" #include "common/upstream/cluster_manager_impl.h" #include "common/upstream/upstream_impl.h" @@ -118,15 +119,18 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { auto& config_factory = Envoy::Config::Utility::getAndCheckFactory< Envoy::Server::Configuration::UpstreamTransportSocketConfigFactory>( transport_socket.name()); - Envoy::ProtobufTypes::MessagePtr message = - Envoy::Config::Utility::translateToFactoryConfig(transport_socket, config_factory); ssl_context_manager_ = std::make_unique( api_.timeSource()); + // TODO: pass in the right validation visitor transport_socket_factory_context_ = std::make_unique( - store_.createScope("client."), dispatcher_, generator_, store_, api_, - *ssl_context_manager_); + store_.createScope("client."), dispatcher_, generator_, store_, api_, *ssl_context_manager_, + Envoy::ProtobufMessage::getNullValidationVisitor()); + + Envoy::ProtobufTypes::MessagePtr message = Envoy::Config::Utility::translateToFactoryConfig( + transport_socket, transport_socket_factory_context_->messageValidationVisitor(), + config_factory); auto client_config = std::make_unique( @@ -140,9 +144,11 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { socket_factory = std::make_unique(); }; + // TODO(oschaaf): pass in the right validation visitor. cluster_ = std::make_unique( cluster_config, bind_config, runtime, std::move(socket_factory), - store_.createScope("client."), false /*added_via_api*/); + store_.createScope("client."), false /*added_via_api*/, + Envoy::ProtobufMessage::getNullValidationVisitor()); ASSERT(uri_->address() != nullptr); diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index c62871647..a03f6a647 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -186,7 +186,8 @@ bool ProcessImpl::run(OutputCollector& collector) { bool ok = true; Envoy::Runtime::RandomGeneratorImpl generator; Envoy::Runtime::ScopedLoaderSingleton loader( - Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl({}, generator, *store_, tls_)}); + Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl( + *dispatcher_, tls_, {}, "foo-cluster", *store_, generator, api_)}); for (auto& w : workers_) { w->start(); diff --git a/source/common/ssl.h b/source/common/ssl.h index bd780921a..2eff0bd21 100644 --- a/source/common/ssl.h +++ b/source/common/ssl.h @@ -23,9 +23,11 @@ class MinimalTransportSocketFactoryContext MinimalTransportSocketFactoryContext( Envoy::Stats::ScopePtr&& stats_scope, Envoy::Event::Dispatcher& dispatcher, Envoy::Runtime::RandomGenerator& random, Envoy::Stats::Store& stats, Envoy::Api::Api& api, - Envoy::Extensions::TransportSockets::Tls::ContextManagerImpl& ssl_context_manager) + Envoy::Extensions::TransportSockets::Tls::ContextManagerImpl& ssl_context_manager, + Envoy::ProtobufMessage::ValidationVisitor& validation_visitor) : ssl_context_manager_(ssl_context_manager), stats_scope_(std::move(stats_scope)), - dispatcher_(dispatcher), random_(random), stats_(stats), api_(api) {} + dispatcher_(dispatcher), random_(random), stats_(stats), api_(api), + validation_visitor_(validation_visitor) {} Envoy::Server::Admin& admin() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } @@ -55,6 +57,10 @@ class MinimalTransportSocketFactoryContext Envoy::Api::Api& api() override { return api_; } + Envoy::ProtobufMessage::ValidationVisitor& messageValidationVisitor() override { + return validation_visitor_; + } + private: Envoy::Extensions::TransportSockets::Tls::ContextManagerImpl& ssl_context_manager_; Envoy::Stats::ScopePtr stats_scope_; @@ -63,6 +69,7 @@ class MinimalTransportSocketFactoryContext Envoy::Runtime::RandomGenerator& random_; Envoy::Stats::Store& stats_; Envoy::Api::Api& api_; + Envoy::ProtobufMessage::ValidationVisitor& validation_visitor_; }; } // namespace Ssl diff --git a/source/server/http_test_server_filter.cc b/source/server/http_test_server_filter.cc index fdd6a1d82..5faefb848 100644 --- a/source/server/http_test_server_filter.cc +++ b/source/server/http_test_server_filter.cc @@ -4,6 +4,7 @@ #include "envoy/server/filter_config.h" +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" #include "absl/strings/numbers.h" @@ -28,7 +29,9 @@ bool HttpTestServerDecoderFilter::mergeJsonConfig(absl::string_view json, error_message = absl::nullopt; try { nighthawk::server::ResponseOptions json_config; - Envoy::MessageUtil::loadFromJson(std::string(json), json_config); + // TODO(oschaaf): pass in the right ValidationVisitor type. + Envoy::MessageUtil::loadFromJson(std::string(json), json_config, + Envoy::ProtobufMessage::getNullValidationVisitor()); config.MergeFrom(json_config); Envoy::MessageUtil::validate(config); } catch (Envoy::EnvoyException exception) { diff --git a/test/BUILD b/test/BUILD index 0434de64b..bf67b28bd 100644 --- a/test/BUILD +++ b/test/BUILD @@ -61,7 +61,6 @@ envoy_cc_test( "@envoy//source/common/network:dns_lib", "@envoy//source/common/protobuf:utility_lib", "@envoy//source/common/stats:isolated_store_lib", - "@envoy//test/integration:http_integration_lib", "@envoy//test/integration:integration_lib", "@envoy//test/mocks/event:event_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 b804303bd..76d2c591a 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -22,7 +22,6 @@ #include "client/benchmark_client_impl.h" -#include "test/integration/http_integration.h" #include "test/integration/integration.h" #include "test/integration/utility.h" #include "test/mocks/runtime/mocks.h" @@ -300,7 +299,9 @@ TEST_P(BenchmarkClientHttpTest, H1ConnectionFailure) { EXPECT_EQ(1, getCounter("upstream_cx_total")); EXPECT_LE(1, getCounter("upstream_rq_pending_failure_eject")); EXPECT_EQ(1, getCounter("upstream_rq_pending_total")); - EXPECT_EQ(5, nonZeroValuedCounterCount()); + EXPECT_EQ(1, getCounter("upstream_cx_destroy_remote")); + EXPECT_EQ(1, getCounter("upstream_cx_destroy")); + EXPECT_EQ(7, nonZeroValuedCounterCount()); } TEST_P(BenchmarkClientHttpTest, H1MultiConnectionFailure) { @@ -314,7 +315,9 @@ TEST_P(BenchmarkClientHttpTest, H1MultiConnectionFailure) { EXPECT_EQ(10, getCounter("upstream_cx_total")); EXPECT_LE(10, getCounter("upstream_rq_pending_failure_eject")); EXPECT_EQ(10, getCounter("upstream_rq_pending_total")); - EXPECT_EQ(5, nonZeroValuedCounterCount()); + EXPECT_EQ(10, getCounter("upstream_cx_destroy_remote")); + EXPECT_EQ(10, getCounter("upstream_cx_destroy")); + EXPECT_EQ(7, nonZeroValuedCounterCount()); } TEST_P(BenchmarkClientHttpTest, EnableLatencyMeasurement) { @@ -423,7 +426,9 @@ TEST_P(BenchmarkClientHttpTest, RequestMethodPost) { EXPECT_EQ(1, getCounter("upstream_rq_pending_total")); EXPECT_EQ(1, getCounter("upstream_cx_close_notify")); EXPECT_EQ(1, getCounter("upstream_cx_http1_total")); - EXPECT_EQ(9, nonZeroValuedCounterCount()); + EXPECT_EQ(1, getCounter("upstream_cx_close_notify")); + EXPECT_EQ(1, getCounter("upstream_cx_destroy_local")); + EXPECT_EQ(11, nonZeroValuedCounterCount()); } // TODO(oschaaf): test protocol violations, stream resets, etc. diff --git a/test/client_test.cc b/test/client_test.cc index 12bcc5f79..5da7be324 100644 --- a/test/client_test.cc +++ b/test/client_test.cc @@ -1,34 +1,9 @@ -// This test relies on fork(). Somehow the integration test server excepts when running -// under TSAN because of Envoy not being able to find its configuration file with die_after_fork -// disabled. The code below seems to suggest that TSAN doesn't support this scenario. -// https://github.com/llvm-mirror/compiler-rt/blob/master/lib/tsan/rtl/tsan_interceptors.cc#L999 -// Further root cause analysis is not worth it at this point, because this code will be deprecated -// soon in favor of either https://github.com/envoyproxy/nighthawk/pull/60 or moving this end-to-end -// testing to python. -// Thus we just disable this test when we detect we are running under TSAN. -#ifndef __has_feature -#define __has_feature(x) 0 // Compatibility with non-clang compilers. -#endif -#if defined(__has_feature) && !__has_feature(thread_sanitizer) - #include -#include "common/api/api_impl.h" -#include "common/common/thread_impl.h" -#include "common/filesystem/filesystem_impl.h" - #include "client/client.h" -#include "client/factories_impl.h" -#include "client/options_impl.h" #include "test/client/utility.h" -#include "test/integration/http_integration.h" -#include "test/integration/integration.h" -#include "test/integration/utility.h" -#include "test/mocks.h" -#include "test/mocks/event/mocks.h" -#include "test/mocks/stats/mocks.h" -#include "test/server/utility.h" +#include "test/test_common/environment.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -39,126 +14,38 @@ using namespace testing; namespace Nighthawk { namespace Client { -class ClientTest : public Envoy::BaseIntegrationTest, - public TestWithParam { -public: - ClientTest() - : Envoy::BaseIntegrationTest(GetParam(), realTime(), readEnvoyConfiguration()), - fd_port_(2, 0), fd_confirm_(2, 0) {} - - std::string readEnvoyConfiguration() { - Envoy::Filesystem::InstanceImplPosix file_system; - std::string envoy_config = file_system.fileReadToEnd(Envoy::TestEnvironment::runfilesPath( - "test/test_data/benchmark_http_client_test_envoy.yaml")); - return Envoy::TestEnvironment::substitute(envoy_config); - } - - void SetUp() override { - // We fork the integration test fixture into a child process, to avoid conflicting - // runtimeloaders as both NH and the integration server want to own that and we can have only - // one. The plan is to move to python for this type of testing, so hopefully we can deprecate - // this test and it's peculiar setup with fork/pipe soon. - RELEASE_ASSERT(pipe(fd_port_.data()) == 0, "Failed to open pipe"); - RELEASE_ASSERT(pipe(fd_confirm_.data()) == 0, "Failed to open pipe"); - pid_ = fork(); - RELEASE_ASSERT(pid_ >= 0, "Fork failed"); - - if (pid_ == 0) { - // child process running the integration test server. - ares_library_init(ARES_LIB_INIT_ALL); - Envoy::Event::Libevent::Global::initialize(); - initialize(); - int port = lookupPort("listener_0"); - int parent_message; - RELEASE_ASSERT(write(fd_port_[1], &port, sizeof(port)) == sizeof(port), "write failed"); - // The parent process writes to fd_confirm_ when it has read the port. This call to read - // blocks until that happens. - RELEASE_ASSERT(read(fd_confirm_[0], &parent_message, sizeof(parent_message)) == - sizeof(parent_message), - "Invalid read size"); - RELEASE_ASSERT(parent_message == port, "Failed to confirm port"); - // The parent process closes fd_port_ when the test tears down. The read call blocks until it - // does that. - RELEASE_ASSERT(read(fd_port_[0], &port_, sizeof(port_)) == -1, "read failed"); - GTEST_SKIP(); - } else if (pid_ > 0) { - RELEASE_ASSERT(read(fd_port_[0], &port_, sizeof(port_)) > 0, "read failed"); - RELEASE_ASSERT(port_ > 0, "read unexpected port_ value"); - RELEASE_ASSERT(write(fd_confirm_[1], &port_, sizeof(port_)) == sizeof(port_), "write failed"); - } - } - - void TearDown() override { - if (pid_ == 0) { - test_server_.reset(); - fake_upstreams_.clear(); - ares_library_cleanup(); - } - RELEASE_ASSERT(!close(fd_confirm_[0]), "close failed"); - RELEASE_ASSERT(!close(fd_confirm_[1]), "close failed"); - RELEASE_ASSERT(!close(fd_port_[0]), "close failed"); - RELEASE_ASSERT(!close(fd_port_[1]), "close failed"); - } +class ClientTest : public testing::Test {}; - std::string testUrl() { - RELEASE_ASSERT(pid_ > 0, "Unexpected call to testUrl"); - const std::string address = Envoy::Network::Test::getLoopbackAddressUrlString(GetParam()); - return fmt::format("http://{}:{}/", address, port_); - } - - const char* getAddressFamilyOptionString() { - auto ip_version = GetParam(); - RELEASE_ASSERT(ip_version == Envoy::Network::Address::IpVersion::v4 || - ip_version == Envoy::Network::Address::IpVersion::v6, - "bad ip version"); - return ip_version == Envoy::Network::Address::IpVersion::v6 ? "v6" : "v4"; - } - - int port_; - pid_t pid_; - std::vector fd_port_; - std::vector fd_confirm_; -}; - -INSTANTIATE_TEST_SUITE_P(IpVersions, ClientTest, - ValuesIn(Envoy::TestEnvironment::getIpVersionsForTest()), - Envoy::TestUtility::ipTestParamsToString); - -TEST_P(ClientTest, NormalRun) { +// TODO(oschaaf): revisit this, and improve testability of the Main +// class, so we can mock its dependencies. +// We now have integration tests covering this much better. +TEST_F(ClientTest, NormalRun) { Main program(Nighthawk::Client::TestUtility::createOptionsImpl( - fmt::format("foo --address-family {} --duration 2 --rps 10 {}", - getAddressFamilyOptionString(), testUrl()))); + "foo --duration 1 --rps 10 http://localhost:63657/")); EXPECT_TRUE(program.run()); } -TEST_P(ClientTest, AutoConcurrencyRun) { +TEST_F(ClientTest, AutoConcurrencyRun) { std::vector argv; argv.push_back("foo"); argv.push_back("--concurrency"); argv.push_back("auto"); - argv.push_back("--address-family"); - argv.push_back(getAddressFamilyOptionString()); argv.push_back("--duration"); argv.push_back("1"); argv.push_back("--rps"); argv.push_back("1"); argv.push_back("--verbosity"); argv.push_back("error"); - std::string url = testUrl(); - argv.push_back(url.c_str()); - + argv.push_back("http://localhost:63657/"); Main program(argv.size(), argv.data()); EXPECT_TRUE(program.run()); } -TEST_P(ClientTest, BadRun) { +TEST_F(ClientTest, BadRun) { Main program(Nighthawk::Client::TestUtility::createOptionsImpl( - fmt::format("foo --address-family {} --duration 1 --rps 1 https://unresolveable.host/", - getAddressFamilyOptionString()))); + "foo --duration 1 --rps 1 https://unresolveable.host/")); EXPECT_FALSE(program.run()); } } // namespace Client } // namespace Nighthawk - -#endif \ No newline at end of file diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc index fed3e064c..6f02b2177 100644 --- a/test/client_worker_test.cc +++ b/test/client_worker_test.cc @@ -26,8 +26,8 @@ class ClientWorkerTest : public Test { ClientWorkerTest() : api_(Envoy::Thread::threadFactoryForTest(), store_, time_system_, file_system_), thread_id_(std::this_thread::get_id()), - loader_( - Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl({}, rand_, store_, tls_)}) { + loader_(Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl( + dispatcher_, tls_, {}, "test-cluster", store_, rand_, api_)}) { benchmark_client_ = new MockBenchmarkClient(); sequencer_ = new MockSequencer(); @@ -64,6 +64,7 @@ class ClientWorkerTest : public Test { MockBenchmarkClient* benchmark_client_; MockSequencer* sequencer_; Envoy::Runtime::RandomGeneratorImpl rand_; + NiceMock dispatcher_; Envoy::Runtime::ScopedLoaderSingleton loader_; Envoy::Filesystem::InstanceImplPosix file_system_; }; diff --git a/test/service_test.cc b/test/service_test.cc index d3a5eb61e..41e16ce0d 100644 --- a/test/service_test.cc +++ b/test/service_test.cc @@ -91,7 +91,7 @@ TEST_P(ServiceTest, Basic) { EXPECT_TRUE(r->Read(&response_)); EXPECT_FALSE(response_.has_error_detail()); EXPECT_TRUE(response_.has_output()); - EXPECT_EQ(6, response_.output().results(0).counters().size()); + EXPECT_GE(response_.output().results(0).counters().size(), 8); auto status = r->Finish(); EXPECT_TRUE(status.ok()); } diff --git a/test/statistic_test.cc b/test/statistic_test.cc index ef546d8af..d9e5e7c5d 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -259,7 +259,7 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { Envoy::MessageUtil util; util.loadFromJson( filesystem.fileReadToEnd(TestEnvironment::runfilesPath("test/test_data/hdr_proto_json.gold")), - parsed_json_proto); + parsed_json_proto, Envoy::ProtobufMessage::getNullValidationVisitor()); EXPECT_TRUE(util(parsed_json_proto, statistic.toProto())); } diff --git a/test/worker_test.cc b/test/worker_test.cc index 163819d68..4da9b12e2 100644 --- a/test/worker_test.cc +++ b/test/worker_test.cc @@ -49,8 +49,10 @@ TEST_F(WorkerTest, WorkerExecutesOnThread) { EXPECT_CALL(tls_, allocateSlot()).Times(1); TestWorker worker(api_, tls_); - Envoy::Runtime::ScopedLoaderSingleton loader( - Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl({}, rand_, store_, tls_)}); + NiceMock dispatcher; + Envoy::Runtime::ScopedLoaderSingleton loader(Envoy::Runtime::LoaderPtr{ + new Envoy::Runtime::LoaderImpl(dispatcher, tls_, {}, "test-cluster", store_, rand_, api_)}); + worker.start(); worker.waitForCompletion(); diff --git a/tools/.style.yapf b/tools/.style.yapf new file mode 100644 index 000000000..b07cb7971 --- /dev/null +++ b/tools/.style.yapf @@ -0,0 +1,6 @@ +# The Google Python styles can be found here: https://github.com/google/styleguide/blob/gh-pages/pyguide.md +# TODO: Look into enforcing single vs double quote. +[style] +based_on_style=Google +indent_width=2 +column_limit=100 diff --git a/tools/format_python_tools.py b/tools/format_python_tools.py new file mode 100755 index 000000000..e81cb11a6 --- /dev/null +++ b/tools/format_python_tools.py @@ -0,0 +1,71 @@ +import argparse +import fnmatch +import os +import sys + +from yapf.yapflib.yapf_api import FormatFile + +EXCLUDE_LIST = ['generated', 'venv'] + + +def collectFiles(): + """Collect all Python files in the tools directory. + + Returns: A collection of python files in the tools directory excluding + any directories in the EXCLUDE_LIST constant. + """ + # TODO: Add ability to collect a specific file or files. + matches = [] + path_parts = os.getcwd().split('/') + dirname = '.' + if path_parts[-1] == 'tools': + dirname = '/'.join(path_parts[:-1]) + for root, dirnames, filenames in os.walk(dirname): + dirnames[:] = [d for d in dirnames if d not in EXCLUDE_LIST] + for filename in fnmatch.filter(filenames, '*.py'): + matches.append(os.path.join(root, filename)) + return matches + + +def validateFormat(fix=False): + """Check the format of python files in the tools directory. + + Arguments: + fix: a flag to indicate if fixes should be applied. + """ + fixes_required = False + failed_update_files = set() + successful_update_files = set() + for python_file in collectFiles(): + reformatted_source, encoding, changed = FormatFile( + python_file, style_config='.style.yapf', in_place=fix, print_diff=not fix) + if not fix: + fixes_required = True if changed else fixes_required + if reformatted_source: + print(reformatted_source) + continue + file_list = failed_update_files if reformatted_source else successful_update_files + file_list.add(python_file) + if fix: + displayFixResults(successful_update_files, failed_update_files) + fixes_required = len(failed_update_files) > 0 + return not fixes_required + + +def displayFixResults(successful_files, failed_files): + if successful_files: + print('Successfully fixed {} files'.format(len(successful_files))) + + if failed_files: + print('The following files failed to fix inline:') + for failed_file in failed_files: + print(' - {}'.format(failed_file)) + + +if __name__ == '__main__': + parser = argparse.ArgumentParser(description='Tool to format python files.') + parser.add_argument( + 'action', choices=['check', 'fix'], default='check', help='Fix invalid syntax in files.') + args = parser.parse_args() + is_valid = validateFormat(args.action == 'fix') + sys.exit(0 if is_valid else 1) diff --git a/tools/format_python_tools.sh b/tools/format_python_tools.sh new file mode 100755 index 000000000..c58dccfb7 --- /dev/null +++ b/tools/format_python_tools.sh @@ -0,0 +1,18 @@ +#!/bin/bash + +set -e + +VENV_DIR="pyformat" +SCRIPTPATH=$(realpath "$(dirname $0)") +. $SCRIPTPATH/shell_utils.sh +cd "$SCRIPTPATH" + +source_venv "$VENV_DIR" +echo "Installing requirements..." +pip install -r requirements.txt + +echo "Running Python format check..." +python format_python_tools.py $1 + +echo "Running Python3 flake8 check..." +flake8 . --exclude=*/venv/* --count --select=E901,E999,F821,F822,F823 --show-source --statistics diff --git a/tools/gen_compilation_database.py b/tools/gen_compilation_database.py index 7e2485dd0..53db5f03c 100755 --- a/tools/gen_compilation_database.py +++ b/tools/gen_compilation_database.py @@ -7,81 +7,78 @@ def generateCompilationDatabase(args): - if args.run_bazel_build: - subprocess.check_call(["bazel", "build"] + args.bazel_targets) + if args.run_bazel_build: + subprocess.check_call(["bazel", "build"] + args.bazel_targets) - gen_compilation_database_sh = os.path.join( - os.path.realpath(os.path.dirname(__file__)), "../bazel/gen_compilation_database.sh") - subprocess.check_call([gen_compilation_database_sh] + args.bazel_targets) + gen_compilation_database_sh = os.path.join( + os.path.realpath(os.path.dirname(__file__)), "../bazel/gen_compilation_database.sh") + subprocess.check_call([gen_compilation_database_sh] + args.bazel_targets) def isHeader(filename): - for ext in (".h", ".hh", ".hpp", ".hxx"): - if filename.endswith(ext): - return True - return False + for ext in (".h", ".hh", ".hpp", ".hxx"): + if filename.endswith(ext): + return True + return False def isCompileTarget(target, args): - filename = target["file"] - if not args.include_headers and isHeader(filename): - return False + filename = target["file"] + if not args.include_headers and isHeader(filename): + return False - if not args.include_genfiles: - if filename.startswith("bazel-out/"): - return False + if not args.include_genfiles: + if filename.startswith("bazel-out/"): + return False - if not args.include_external: - if filename.startswith("external/"): - return False + if not args.include_external: + if filename.startswith("external/"): + return False - if filename.startswith("google/protobuf"): - return False + if filename.startswith("google/protobuf"): + return False - if filename.startswith("hdrhistogram_c"): - return False + if filename.startswith("hdrhistogram_c"): + return False - return True + return True def modifyCompileCommand(target): - cxx, options = target["command"].split(" ", 1) + cxx, options = target["command"].split(" ", 1) - # Workaround for bazel added C++11 options, those doesn't affect build itself but - # clang-tidy will misinterpret them. - options = options.replace("-std=c++0x ", "") - options = options.replace("-std=c++11 ", "") + # Workaround for bazel added C++11 options, those doesn't affect build itself but + # clang-tidy will misinterpret them. + options = options.replace("-std=c++0x ", "") + options = options.replace("-std=c++11 ", "") - if isHeader(target["file"]): - options += " -Wno-pragma-once-outside-header -Wno-unused-const-variable" - options += " -Wno-unused-function" + if isHeader(target["file"]): + options += " -Wno-pragma-once-outside-header -Wno-unused-const-variable" + options += " -Wno-unused-function" - target["command"] = " ".join(["clang++", options]) - return target + target["command"] = " ".join(["clang++", options]) + return target def fixCompilationDatabase(args): - with open("compile_commands.json", "r") as db_file: - db = json.load(db_file) + with open("compile_commands.json", "r") as db_file: + db = json.load(db_file) - db = [modifyCompileCommand(target) - for target in db if isCompileTarget(target, args)] + db = [modifyCompileCommand(target) for target in db if isCompileTarget(target, args)] - # Remove to avoid writing into symlink - os.remove("compile_commands.json") - with open("compile_commands.json", "w") as db_file: - json.dump(db, db_file, indent=2) + # Remove to avoid writing into symlink + os.remove("compile_commands.json") + with open("compile_commands.json", "w") as db_file: + json.dump(db, db_file, indent=2) if __name__ == "__main__": - parser = argparse.ArgumentParser( - description='Generate JSON compilation database') - parser.add_argument('--run_bazel_build', action='store_true') - parser.add_argument('--include_external', action='store_true') - parser.add_argument('--include_genfiles', action='store_true') - parser.add_argument('--include_headers', action='store_true') - parser.add_argument( - 'bazel_targets', nargs='*', default=["//source/exe/...", "//test/..."]) - args = parser.parse_args() - generateCompilationDatabase(args) - fixCompilationDatabase(args) + parser = argparse.ArgumentParser(description='Generate JSON compilation database') + parser.add_argument('--run_bazel_build', action='store_true') + parser.add_argument('--include_external', action='store_true') + parser.add_argument('--include_genfiles', action='store_true') + parser.add_argument('--include_headers', action='store_true') + parser.add_argument('bazel_targets', nargs='*', default=["//source/exe/...", "//test/..."]) + args = parser.parse_args() + generateCompilationDatabase(args) + fixCompilationDatabase(args) diff --git a/tools/requirements.txt b/tools/requirements.txt new file mode 100644 index 000000000..8c4405010 --- /dev/null +++ b/tools/requirements.txt @@ -0,0 +1,2 @@ +flake8==3.6.0 +yapf==0.25.0 \ No newline at end of file diff --git a/tools/shell_utils.sh b/tools/shell_utils.sh new file mode 100755 index 000000000..8aa65eed8 --- /dev/null +++ b/tools/shell_utils.sh @@ -0,0 +1,11 @@ +source_venv() { + VENV_DIR=$1 + if [[ "$VIRTUAL_ENV" == "" ]]; then + if [[ ! -d "${VENV_DIR}"/venv ]]; then + virtualenv "${VENV_DIR}"/venv --no-site-packages --python=python2.7 + fi + source "${VENV_DIR}"/venv/bin/activate + else + echo "Found existing virtualenv" + fi +} From 792c61ed1e105452de2f6641ce02e4370a769a19 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 13:26:12 +0200 Subject: [PATCH 02/29] CI image update, Py3, Integration test sync Signed-off-by: Otto van der Schaaf --- WORKSPACE | 18 ++++++++ bazel/repositories.bzl | 10 +++++ integration/BUILD | 20 +++++++++ .../configurations/nighthawk_http_origin.yaml | 5 +++ .../nighthawk_https_origin.yaml | 5 +++ integration/integration_test.py | 3 ++ integration/integration_test_fixtures.py | 41 +++++++++++++++---- integration/nighthawk_test_server.py | 28 ++++++------- integration/test_integration_basics.py | 15 ++++--- requirements.txt | 5 +++ 10 files changed, 122 insertions(+), 28 deletions(-) create mode 100644 requirements.txt diff --git a/WORKSPACE b/WORKSPACE index 7cf02508c..dcde98112 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -24,3 +24,21 @@ load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_depe go_rules_dependencies() go_register_toolchains() + +# For PIP support: +load("@io_bazel_rules_python//python:pip.bzl", "pip_import", "pip_repositories") + +pip_repositories() + +# This rule translates the specified requirements.txt into +# @my_deps//:requirements.bzl, which itself exposes a pip_install method. +pip_import( + name = "python_pip_deps", + requirements = "//:requirements.txt", +) + +# Load the pip_install symbol for my_deps, and create the dependencies' +# repositories. +load("@python_pip_deps//:requirements.bzl", "pip_install") + +pip_install() diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index df0a01441..1007ba0d7 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -3,6 +3,9 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") ENVOY_COMMIT = "2f569b9a8d3f0d7a43ffa69e3e5ba947cd3a9f8b" ENVOY_SHA = "ec47fee6604468bc392937967415c736f19fb22129929881270a1635ad216d87" +RULES_PYTHON_COMMIT = "fdbb17a4118a1728d19e638a5291b4c4266ea5b8" +RULES_PYTHON_SHA = "9a3d71e348da504a9c4c5e8abd4cb822f7afb32c613dc6ee8b8535333a81a938" + def nighthawk_dependencies(): http_archive( name = "envoy", @@ -49,3 +52,10 @@ cc_library( strip_prefix = "HdrHistogram_c-0.9.8", url = "https://github.com/HdrHistogram/HdrHistogram_c/archive/0.9.8.tar.gz", ) + + http_archive( + name = "io_bazel_rules_python", + sha256 = RULES_PYTHON_SHA, + strip_prefix = "rules_python-%s" % RULES_PYTHON_COMMIT, + url = "https://github.com/bazelbuild/rules_python/archive/%s.tar.gz" % RULES_PYTHON_COMMIT, + ) diff --git a/integration/BUILD b/integration/BUILD index 9a85db832..48fb07304 100644 --- a/integration/BUILD +++ b/integration/BUILD @@ -1,5 +1,13 @@ licenses(["notice"]) # Apache 2 +load( + "@envoy//bazel:envoy_build_system.bzl", + "envoy_package", +) +load("@python_pip_deps//:requirements.bzl", "requirement") + +envoy_package() + py_test( name = "integration_test", srcs = [ @@ -12,4 +20,16 @@ py_test( "//:nighthawk_test_server", "@envoy//test/config/integration/certs", ], + # We enforce python 2, but I we may be able to stop doing that when we update + # bazel in CI. (Or we add python3 explicitly as a build dependency somehow). + python_version = "PY3", + srcs_version = "PY3ONLY", + deps = [ + requirement("requests"), + # I think the following are implied by 'request'. + requirement("urllib3"), + requirement("chardet"), + requirement("certifi"), + requirement("idna"), + ], ) diff --git a/integration/configurations/nighthawk_http_origin.yaml b/integration/configurations/nighthawk_http_origin.yaml index a38478282..9c123fa1f 100644 --- a/integration/configurations/nighthawk_http_origin.yaml +++ b/integration/configurations/nighthawk_http_origin.yaml @@ -1,3 +1,8 @@ +admin: + access_log_path: /tmp/admin_access.log + profile_path: /tmp/envoy.prof + address: + socket_address: { address: $server_ip, port_value: $admin_port } static_resources: listeners: - address: diff --git a/integration/configurations/nighthawk_https_origin.yaml b/integration/configurations/nighthawk_https_origin.yaml index b3b69f631..ac16cabcc 100644 --- a/integration/configurations/nighthawk_https_origin.yaml +++ b/integration/configurations/nighthawk_https_origin.yaml @@ -1,3 +1,8 @@ +admin: + access_log_path: /tmp/admin_access.log + profile_path: /tmp/envoy.prof + address: + socket_address: { address: $server_ip, port_value: $admin_port } static_resources: listeners: - address: diff --git a/integration/integration_test.py b/integration/integration_test.py index f2dda4d14..1ec583e0c 100644 --- a/integration/integration_test.py +++ b/integration/integration_test.py @@ -1,3 +1,5 @@ +#!/usr/bin/env python3 + """@package integration_test.py Entry point for our integration testing """ @@ -11,6 +13,7 @@ from common import IpVersion, NighthawkException from integration_test_fixtures import IntegrationTestBase +assert sys.version_info >= (3, 0) def determineIpVersionsFromEnvironment(): env_versions = os.environ.get("ENVOY_IP_TEST_VERSIONS", "all") diff --git a/integration/integration_test_fixtures.py b/integration/integration_test_fixtures.py index 3fa295c84..a58b2329a 100644 --- a/integration/integration_test_fixtures.py +++ b/integration/integration_test_fixtures.py @@ -5,6 +5,7 @@ import json import logging import os +import requests import socket import subprocess import sys @@ -12,14 +13,15 @@ import time import unittest + from common import IpVersion from nighthawk_test_server import NighthawkTestServer class IntegrationTestBase(unittest.TestCase): """ - IntegrationTestBase facilitates testing against the Nighthawk test server, by determining a free port, and starting it up in a separate process in setUp(). - """ + IntegrationTestBase facilitates testing against the Nighthawk test server, by determining a free port, and starting it up in a separate process in setUp(). + """ def __init__(self, *args, **kwargs): super(IntegrationTestBase, self).__init__(*args, **kwargs) @@ -32,7 +34,8 @@ def __init__(self, *args, **kwargs): self.socket_type = socket.AF_INET6 if IntegrationTestBase.ip_version == IpVersion.IPV6 else socket.AF_INET self.test_server = None self.server_port = -1 - self.parameters = dict() + self.admin_port = -1 + self.parameters = {} # TODO(oschaaf): For the NH test server, add a way to let it determine a port by itself and pull that # out. @@ -43,11 +46,10 @@ def getFreeListenerPortForAddress(self, address): The upside is that we can push the port upon the server we are about to start through configuration which is compatible accross servers. """ - # TODO(oschaaf): When we are on python 3 use RAII here. - sock = socket.socket(self.socket_type, socket.SOCK_STREAM) - sock.bind((address, 0)) - port = sock.getsockname()[1] - sock.close() + with socket.socket(self.socket_type, socket.SOCK_STREAM) as sock: + sock.bind((address, 0)) + port = sock.getsockname()[1] + sock.close() return port def setUp(self): @@ -57,6 +59,8 @@ def setUp(self): self.assertTrue(os.path.exists(self.nighthawk_test_server_path)) self.assertTrue(os.path.exists(self.nighthawk_client_path)) self.server_port = self.getFreeListenerPortForAddress(self.server_ip) + self.admin_port = self.getFreeListenerPortForAddress(self.server_ip) + self.parameters["admin_port"] = self.admin_port self.test_server = NighthawkTestServer( self.nighthawk_test_server_path, self.nighthawk_test_config_path, self.server_ip, self.server_port, IntegrationTestBase.ip_version, self.parameters) @@ -89,7 +93,26 @@ def getTestServerRootUri(self, https=False): uri = "%s://%s:%s/" % ("https" if https else "http", uri_host, self.test_server.server_port) return uri - def runNighthawk(self, args, expect_failure=False, timeout=30): + def getTestServerStatisticsJson(self): + """ + Uri to grab a server stats snapshot over http from the admin interface. + """ + uri_host = self.server_ip + if IntegrationTestBase.ip_version == IpVersion.IPV6: + uri_host = "[%s]" % self.server_ip + uri = "http://%s:%s/stats?format=json" % (uri_host, self.admin_port) + r = requests.get(uri) + self.assertEqual(r.status_code, 200) + return r.json() + + def getServerStatFromJson(self, server_stats_json, name): + counters = server_stats_json["stats"] + for counter in counters: + if counter["name"] == name: + return int(counter["value"]) + return None + + def runNighthawkClient(self, args, expect_failure=False, timeout=30): """ Runs Nighthawk against the test server, returning a json-formatted result. If the timeout is exceeded an exception will be raised. diff --git a/integration/nighthawk_test_server.py b/integration/nighthawk_test_server.py index 635cd7162..f5d556620 100644 --- a/integration/nighthawk_test_server.py +++ b/integration/nighthawk_test_server.py @@ -48,24 +48,24 @@ def serverThreadRunner(self): self.server_process = subprocess.Popen(args) self.server_process.communicate() - def waitUntillServerListening(self): - sock = socket.socket(self.socket_type, socket.SOCK_STREAM) - sock.settimeout(1) - tries = 10 - while tries > 0: - time.sleep(0.5) - tries -= 1 - if sock.connect_ex((self.server_ip, self.server_port)) == 0: - sock.close() - return True - logging.error("Timeout while waiting for server listener at %s:%s to accept connections.", - self.server_ip, self.server_port) - return False + def waitUntilServerListening(self): + with socket.socket(self.socket_type, socket.SOCK_STREAM) as sock: + sock.settimeout(1) + tries = 10 + while tries > 0: + time.sleep(0.5) + tries -= 1 + if sock.connect_ex((self.server_ip, self.server_port)) == 0: + sock.close() + return True + logging.error("Timeout while waiting for server listener at %s:%s to accept connections.", + self.server_ip, self.server_port) + return False def start(self): self.server_thread.daemon = True self.server_thread.start() - return self.waitUntillServerListening() + return self.waitUntilServerListening() def stop(self): self.server_process.terminate() diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index 6ce0ce12a..6a9c3ead4 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 import logging import os @@ -15,7 +15,7 @@ class TestHttp(HttpIntegrationTestBase): def test_h1(self): - parsed_json = self.runNighthawk([self.getTestServerRootUri()]) + parsed_json = self.runNighthawkClient([self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) self.assertEqual(counters["upstream_cx_destroy"], 1) @@ -29,8 +29,12 @@ def test_h1(self): self.assertEqual(counters["upstream_rq_total"], 25) self.assertEqual(len(counters), 9) + server_stats = self.getTestServerStatisticsJson() + self.assertEqual( + self.getServerStatFromJson(server_stats, "http.ingress_http.downstream_rq_2xx"), 25) + def test_h2(self): - parsed_json = self.runNighthawk(["--h2", self.getTestServerRootUri()]) + parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) self.assertEqual(counters["upstream_cx_destroy"], 1) @@ -47,7 +51,7 @@ def test_h2(self): class TestHttps(HttpsIntegrationTestBase): def test_h1(self): - parsed_json = self.runNighthawk([self.getTestServerRootUri()]) + parsed_json = self.runNighthawkClient([self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) self.assertEqual(counters["upstream_cx_destroy"], 1) @@ -67,7 +71,7 @@ def test_h1(self): self.assertEqual(len(counters), 14) def test_h2(self): - parsed_json = self.runNighthawk(["--h2", self.getTestServerRootUri()]) + parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) self.assertEqual(counters["upstream_cx_destroy"], 1) @@ -84,3 +88,4 @@ def test_h2(self): self.assertEqual(counters["ssl.sigalgs.rsa_pss_rsae_sha256"], 1) self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) self.assertEqual(len(counters), 14) + \ No newline at end of file diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 000000000..4706afeb9 --- /dev/null +++ b/requirements.txt @@ -0,0 +1,5 @@ +requests +urllib3 +chardet +certifi +idna \ No newline at end of file From b2d3cbb0e493569dc463ec90eedca7d081017800 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 13:38:26 +0200 Subject: [PATCH 03/29] Update CI image version & formatting Signed-off-by: Otto van der Schaaf --- .circleci/config.yml | 2 +- integration/integration_test.py | 2 +- integration/integration_test_fixtures.py | 1 - integration/test_integration_basics.py | 1 - 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index d0fcee53b..94552fad6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,6 +1,6 @@ references: envoy-build-image: &envoy-build-image - envoyproxy/envoy-build:cfc514546bc0284536893cca5fa43d7128edcd35 + envoyproxy/envoy-build:d0cefa7f071dbd4ef24399c2db8656c3a5d8c3ef version: 2 jobs: diff --git a/integration/integration_test.py b/integration/integration_test.py index 1ec583e0c..9a5539b9f 100644 --- a/integration/integration_test.py +++ b/integration/integration_test.py @@ -1,5 +1,4 @@ #!/usr/bin/env python3 - """@package integration_test.py Entry point for our integration testing """ @@ -15,6 +14,7 @@ assert sys.version_info >= (3, 0) + def determineIpVersionsFromEnvironment(): env_versions = os.environ.get("ENVOY_IP_TEST_VERSIONS", "all") if env_versions == "v4only": diff --git a/integration/integration_test_fixtures.py b/integration/integration_test_fixtures.py index a58b2329a..1d683f68c 100644 --- a/integration/integration_test_fixtures.py +++ b/integration/integration_test_fixtures.py @@ -13,7 +13,6 @@ import time import unittest - from common import IpVersion from nighthawk_test_server import NighthawkTestServer diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index 6a9c3ead4..829fd4734 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -88,4 +88,3 @@ def test_h2(self): self.assertEqual(counters["ssl.sigalgs.rsa_pss_rsae_sha256"], 1) self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) self.assertEqual(len(counters), 14) - \ No newline at end of file From be4b41f801a8027030cc4f99638b9167433df192 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 14:02:19 +0200 Subject: [PATCH 04/29] Bump bazel-compilation-database to 0.3.5 Signed-off-by: Otto van der Schaaf --- bazel/gen_compilation_database.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/gen_compilation_database.sh b/bazel/gen_compilation_database.sh index 11492a160..d642ddfd2 100755 --- a/bazel/gen_compilation_database.sh +++ b/bazel/gen_compilation_database.sh @@ -1,6 +1,6 @@ #!/bin/bash -RELEASE_VERSION=0.3.1 +RELEASE_VERSION=0.3.5 if [[ ! -d bazel-compilation-database-${RELEASE_VERSION} ]]; then curl -L https://github.com/grailbio/bazel-compilation-database/archive/${RELEASE_VERSION}.tar.gz | tar -xz From 8d36fabee1add23b155dc8f0dc6c5a7c6ccb857c Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 14:34:26 +0200 Subject: [PATCH 05/29] Sanitize requirements.txt Now that we have progressed I think we can drop a couple of entries. Signed-off-by: Otto van der Schaaf --- requirements.txt | 4 ---- 1 file changed, 4 deletions(-) diff --git a/requirements.txt b/requirements.txt index 4706afeb9..f2293605c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1 @@ requests -urllib3 -chardet -certifi -idna \ No newline at end of file From 03816adbe7963fae60760a074aa2468669312119 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 27 Jun 2019 22:56:34 +0200 Subject: [PATCH 06/29] Add cluster, pool and tls control options - Adds a the following fields to the benchmark client proto (and matching CLI args): ``` envoy.api.v2.auth.UpstreamTlsContext tls_context = 13; uint32 max_pending_requests = 14 [(validate.rules).uint32 = {gte: 1}]; uint32 max_active_requests = 15 [(validate.rules).uint32 = {gte: 1}]; uint32 max_requests_per_connection = 16 [(validate.rules).uint32 = {gte: 1}]; ``` This allows for - cipher suite selection in `https` runs, - tuning of connection re-use, - tuning client side request queueing - tuning the maximum amount of active requests When specifying `max_pending_requests > 1` the benchmark client will stop guarding the exact pacing of requests, allowing open-loop test scenarios. - Unifies validation in the CLI and service paths. - Consolidates default values into a header and improves handling. - Updates the options proto to use 32 bits where relevant, as well as switches fields to WKT's to enable us to make the distinction between `not-set` and type-default. Resolves both https://github.com/envoyproxy/nighthawk/issues/45 Resolves https://github.com/envoyproxy/nighthawk/issues/32 Signed-off-by: Otto van der Schaaf --- README.md | 45 ++-- api/client/options.proto | 39 ++- ci/do_ci.sh | 8 +- include/nighthawk/client/BUILD | 1 + include/nighthawk/client/options.h | 11 +- include/nighthawk/common/sequencer.h | 6 + integration/integration_test.py | 3 - integration/nighthawk_test_server.py | 28 +- requirements.txt | 4 + source/client/BUILD | 1 + source/client/benchmark_client_impl.cc | 57 ++-- source/client/benchmark_client_impl.h | 22 +- source/client/factories_impl.cc | 10 +- source/client/options_impl.cc | 314 ++++++++++++++++------ source/client/options_impl.h | 40 ++- source/client/service_impl.cc | 4 +- source/common/sequencer_impl.cc | 2 + source/common/sequencer_impl.h | 2 + test/BUILD | 1 + test/benchmark_http_client_test.cc | 69 ++++- test/factories_test.cc | 7 + test/mocks.h | 10 +- test/options_test.cc | 67 +++-- test/output_collector_test.cc | 1 + test/sequencer_test.cc | 21 ++ test/service_test.cc | 27 +- test/test_data/output_collector.json.gold | 11 +- test/test_data/output_collector.yaml.gold | 9 - 28 files changed, 568 insertions(+), 252 deletions(-) diff --git a/README.md b/README.md index 431f828f9..fae319315 100644 --- a/README.md +++ b/README.md @@ -41,22 +41,38 @@ bazel build -c opt //:nighthawk_client USAGE: - bazel-bin/nighthawk_client [--request-body-size ] - [--request-header ] ... - [--request-method ] [--address-family - ] [--burst-size ] - [--prefetch-connections] [--output-format - ] [-v ] [--concurrency - ] [--h2] [--timeout ] - [--duration ] [--connections - ] [--rps ] [--] - [--version] [-h] + nighthawk_client [--max-requests-per-connection ] + [--max-active-requests ] + [--max-pending-requests ] [--tls-context + ] [--request-body-size ] + [--request-header ] ... [--request-method + ] + [--address-family ] [--burst-size + ] [--prefetch-connections] [--output-format + ] [-v ] [--concurrency ] [--h2] [--timeout + ] [--duration ] [--connections + ] [--rps ] [--] [--version] [-h] + Where: + --max-requests-per-connection + Max requests per connection (default: 2^31). + + --max-active-requests + Max active requests (default: 2^31). + + --max-pending-requests + Max pending requests (default: 1, no client side queuing. Specifying + any other value will allow client-side queuing of requests). + + --tls-context + Tls context configuration in yaml or json. Example + (json):{common_tls_context:{tls_params:{cipher_suites:["-ALL:ECDHE-RSA + -AES128-SHA"]}}} + --request-body-size Size of the request body to send. NH will send a number of consecutive 'a' characters equal to the number specified here. (default: 0, no @@ -107,7 +123,7 @@ Where: --duration The number of seconds that the test should run. Default: 5. - --connections + --connections The number of connections per event loop that the test should maximally use. HTTP/1 only. Default: 1. @@ -128,8 +144,7 @@ Where: in case of https no certificates are validated. - Nighthawk, a L7 (HTTP/HTTPS/HTTP2) performance characterization tool. - + L7 (HTTP/HTTPS/HTTP2) performance characterization tool. ``` diff --git a/api/client/options.proto b/api/client/options.proto index e6b0c250b..581c19cf3 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -3,44 +3,59 @@ syntax = "proto3"; package nighthawk.client; import "google/protobuf/duration.proto"; -import "validate/validate.proto"; +import "google/protobuf/wrappers.proto"; +import "envoy/api/v2/auth/cert.proto"; import "envoy/api/v2/core/base.proto"; +import "validate/validate.proto"; message RequestOptions { envoy.api.v2.core.RequestMethod request_method = 1; repeated envoy.api.v2.core.HeaderValueOption request_headers = 2; - uint32 request_body_size = 3 [(validate.rules).uint32 = {lte: 4194304}]; + google.protobuf.UInt32Value request_body_size = 3 [(validate.rules).uint32 = {lte: 4194304}]; } // TODO(oschaaf): Ultimately this will be a load test specification. The fact that it // can arrive via CLI is just a concrete detail. Change this to reflect that. message CommandLineOptions { // See :option:`--rps` for details. - uint64 requests_per_second = 1 [(validate.rules).uint64 = {gte: 1, lte: 1000000}]; + google.protobuf.UInt32Value requests_per_second = 1 + [(validate.rules).uint32 = {gte: 1, lte: 1000000}]; // See :option:`--connections` for details. - uint64 connections = 2 [(validate.rules).uint64 = {gte: 1, lte: 1000000}]; + google.protobuf.UInt32Value connections = 2 [(validate.rules).uint32 = {gte: 1, lte: 1000000}]; // See :option:`--duration` for details. google.protobuf.Duration duration = 3 [(validate.rules).duration.gte.nanos = 1]; // See :option:`--timeout` for details. google.protobuf.Duration timeout = 4 [(validate.rules).duration.gte.seconds = 0]; // See :option:`--h2` for details. - bool h2 = 5; + google.protobuf.BoolValue h2 = 5; // See :option:`--concurrency` for details. - string concurrency = 6; // [(validate.rules).string = {pattern: "^([0-9]*|auto)$"}]; + google.protobuf.StringValue concurrency = + 6; // [(validate.rules).string = {pattern: "^([0-9]*|auto)$"}]; // See :option:`--verbosity` for details. - string verbosity = 7 + google.protobuf.StringValue verbosity = 7 [(validate.rules).string = {in: ["trace", "debug", "info", "warn", "error", "critical"]}]; // See :option:`--output-format` for details. - string output_format = 8 [(validate.rules).string = {in: ["human", "yaml", "json"]}]; + google.protobuf.StringValue output_format = 8 + [(validate.rules).string = {in: ["human", "yaml", "json"]}]; // See :option:`--prefetch-connections` for details. - bool prefetch_connections = 9; + google.protobuf.BoolValue prefetch_connections = 9; // See :option:`--burst-size` for details. - uint64 burst_size = 10 [(validate.rules).uint64 = {lte: 1000000}]; + google.protobuf.UInt32Value burst_size = 10 [(validate.rules).uint32 = {lte: 1000000}]; // See :option:`--address-family` for details. - string address_family = 11 [(validate.rules).string = {in: ["v4", "v6"]}]; + google.protobuf.StringValue address_family = 11 + [(validate.rules).string = {in: ["v4", "v6", "auto"]}]; oneof oneof_request_options { // See :option:`--request_options` for details. RequestOptions request_options = 12; } - string uri = 13; // [(validate.rules).string.uri = true]; + // See :option:`--tls_context` for details. + envoy.api.v2.auth.UpstreamTlsContext tls_context = 13; + // See :option:`--max-pending_requests` for details. + google.protobuf.UInt32Value max_pending_requests = 14 [(validate.rules).uint32 = {gte: 1}]; + // See :option:`--max-active_requests` for details. + google.protobuf.UInt32Value max_active_requests = 15 [(validate.rules).uint32 = {gte: 1}]; + // See :option:`--max-requests-per-connection` for details. + google.protobuf.UInt32Value max_requests_per_connection = 16 [(validate.rules).uint32 = {gte: 1}]; + // See :option:`--uri` for details. + google.protobuf.StringValue uri = 17; // [(validate.rules).string.uri = true]; } diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 9ebee2aa5..884e60aa8 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -95,14 +95,14 @@ if [ -n "$CIRCLECI" ]; then mv "${HOME:-/root}/.gitconfig" "${HOME:-/root}/.gitconfig_save" echo 1 fi - + NUM_CPUS=8 if [ "$1" == "coverage" ]; then NUM_CPUS=6 fi fi - if grep 'docker\|lxc' /proc/1/cgroup; then +if grep 'docker\|lxc' /proc/1/cgroup; then # Create a fake home. Python site libs tries to do getpwuid(3) if we don't and the CI # Docker image gets confused as it has no passwd entry when running non-root # unless we do this. @@ -110,14 +110,14 @@ fi mkdir -p "${FAKE_HOME}" export HOME="${FAKE_HOME}" export PYTHONUSERBASE="${FAKE_HOME}" - + export BUILD_DIR=/build if [[ ! -d "${BUILD_DIR}" ]] then echo "${BUILD_DIR} mount missing - did you forget -v :${BUILD_DIR}? Creating." mkdir -p "${BUILD_DIR}" fi - + # Environment setup. export USER=bazel export TEST_TMPDIR=/build/tmp diff --git a/include/nighthawk/client/BUILD b/include/nighthawk/client/BUILD index 31c47a361..90830cd13 100644 --- a/include/nighthawk/client/BUILD +++ b/include/nighthawk/client/BUILD @@ -30,5 +30,6 @@ envoy_basic_cc_library( "@envoy//source/common/network:dns_lib", "@envoy//source/common/protobuf", "@envoy//source/common/runtime:runtime_lib", + "@envoy_api//envoy/api/v2:cds_export_cc", ], ) diff --git a/include/nighthawk/client/options.h b/include/nighthawk/client/options.h index ddaa12ffd..294fa8369 100644 --- a/include/nighthawk/client/options.h +++ b/include/nighthawk/client/options.h @@ -4,6 +4,7 @@ #include #include +#include "envoy/api/v2/cds.pb.h" #include "envoy/common/pure.h" #include "api/client/options.pb.h" @@ -20,8 +21,8 @@ class Options { public: virtual ~Options() = default; - virtual uint64_t requestsPerSecond() const PURE; - virtual uint64_t connections() const PURE; + virtual uint32_t requestsPerSecond() const PURE; + virtual uint32_t connections() const PURE; virtual std::chrono::seconds duration() const PURE; virtual std::chrono::seconds timeout() const PURE; virtual std::string uri() const PURE; @@ -30,11 +31,15 @@ class Options { virtual std::string verbosity() const PURE; virtual std::string outputFormat() const PURE; virtual bool prefetchConnections() const PURE; - virtual uint64_t burstSize() const PURE; + virtual uint32_t burstSize() const PURE; virtual std::string addressFamily() const PURE; virtual std::string requestMethod() const PURE; virtual std::vector requestHeaders() const PURE; virtual uint32_t requestBodySize() const PURE; + virtual const envoy::api::v2::auth::UpstreamTlsContext& tlsContext() const PURE; + virtual uint32_t maxPendingRequests() const PURE; + virtual uint32_t maxActiveRequests() const PURE; + virtual uint32_t maxRequestsPerConnection() const PURE; /** * Converts an Options instance to an equivalent CommandLineOptions instance in terms of option diff --git a/include/nighthawk/common/sequencer.h b/include/nighthawk/common/sequencer.h index a7277262f..f4270c49f 100644 --- a/include/nighthawk/common/sequencer.h +++ b/include/nighthawk/common/sequencer.h @@ -41,6 +41,12 @@ class Sequencer { * other words, time spend while the Sequencer is idle and not blocked by a rate limiter). */ virtual StatisticPtrMap statistics() const PURE; + + /** + * Stops all planned work. Makes pending waitForCompletion() calls return ASAP, disregarding any + * timeouts. + */ + virtual void cancel() PURE; }; using SequencerPtr = std::unique_ptr; diff --git a/integration/integration_test.py b/integration/integration_test.py index 9a5539b9f..f2dda4d14 100644 --- a/integration/integration_test.py +++ b/integration/integration_test.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python3 """@package integration_test.py Entry point for our integration testing """ @@ -12,8 +11,6 @@ from common import IpVersion, NighthawkException from integration_test_fixtures import IntegrationTestBase -assert sys.version_info >= (3, 0) - def determineIpVersionsFromEnvironment(): env_versions = os.environ.get("ENVOY_IP_TEST_VERSIONS", "all") diff --git a/integration/nighthawk_test_server.py b/integration/nighthawk_test_server.py index f5d556620..635cd7162 100644 --- a/integration/nighthawk_test_server.py +++ b/integration/nighthawk_test_server.py @@ -48,24 +48,24 @@ def serverThreadRunner(self): self.server_process = subprocess.Popen(args) self.server_process.communicate() - def waitUntilServerListening(self): - with socket.socket(self.socket_type, socket.SOCK_STREAM) as sock: - sock.settimeout(1) - tries = 10 - while tries > 0: - time.sleep(0.5) - tries -= 1 - if sock.connect_ex((self.server_ip, self.server_port)) == 0: - sock.close() - return True - logging.error("Timeout while waiting for server listener at %s:%s to accept connections.", - self.server_ip, self.server_port) - return False + def waitUntillServerListening(self): + sock = socket.socket(self.socket_type, socket.SOCK_STREAM) + sock.settimeout(1) + tries = 10 + while tries > 0: + time.sleep(0.5) + tries -= 1 + if sock.connect_ex((self.server_ip, self.server_port)) == 0: + sock.close() + return True + logging.error("Timeout while waiting for server listener at %s:%s to accept connections.", + self.server_ip, self.server_port) + return False def start(self): self.server_thread.daemon = True self.server_thread.start() - return self.waitUntilServerListening() + return self.waitUntillServerListening() def stop(self): self.server_process.terminate() diff --git a/requirements.txt b/requirements.txt index f2293605c..de639a6cb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1,5 @@ requests +urllib3 +chardet +certifi +idna diff --git a/source/client/BUILD b/source/client/BUILD index e42783bbd..47b30ccdb 100644 --- a/source/client/BUILD +++ b/source/client/BUILD @@ -47,5 +47,6 @@ envoy_cc_library( "@envoy//source/common/runtime:runtime_lib", "@envoy//source/exe:envoy_common_lib", "@envoy//source/exe:process_wide_lib", + "@envoy_api//envoy/api/v2:cds_export_cc", ], ) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 655bd05d5..385a9f6c3 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -29,18 +29,17 @@ using namespace std::chrono_literals; namespace Nighthawk { namespace Client { -BenchmarkClientHttpImpl::BenchmarkClientHttpImpl(Envoy::Api::Api& api, - Envoy::Event::Dispatcher& dispatcher, - Envoy::Stats::Store& store, - StatisticPtr&& connect_statistic, - StatisticPtr&& response_statistic, UriPtr&& uri, - bool use_h2, bool prefetch_connections) +BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( + Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Store& store, + StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, UriPtr&& uri, bool use_h2, + bool prefetch_connections, envoy::api::v2::auth::UpstreamTlsContext tls_context) : api_(api), dispatcher_(dispatcher), store_(store), scope_(store_.createScope("client.benchmark.")), connect_statistic_(std::move(connect_statistic)), response_statistic_(std::move(response_statistic)), use_h2_(use_h2), prefetch_connections_(prefetch_connections), uri_(std::move(uri)), - benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}) { + benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), + tls_context_(std::move(tls_context)) { connect_statistic_->setId("benchmark_http_client.queue_to_connect"); response_statistic_->setId("benchmark_http_client.request_to_response"); @@ -88,11 +87,14 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { envoy::api::v2::core::BindConfig bind_config; cluster_config.mutable_connect_timeout()->set_seconds(timeout_.count()); + cluster_config.mutable_max_requests_per_connection()->set_value(max_requests_per_connection_); auto thresholds = cluster_config.mutable_circuit_breakers()->add_thresholds(); + // We do not support any retrying. thresholds->mutable_max_retries()->set_value(0); thresholds->mutable_max_connections()->set_value(connection_limit_); thresholds->mutable_max_pending_requests()->set_value(max_pending_requests_); + thresholds->mutable_max_requests()->set_value(max_active_requests_); Envoy::Network::TransportSocketFactoryPtr socket_factory; @@ -106,12 +108,9 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { common_tls_context->add_alpn_protocols("http/1.1"); } auto transport_socket = cluster_config.transport_socket(); - if (!cluster_config.has_transport_socket()) { - ASSERT(cluster_config.has_tls_context()); - transport_socket.set_name( - Envoy::Extensions::TransportSockets::TransportSocketNames::get().Tls); - transport_socket.mutable_typed_config()->PackFrom(cluster_config.tls_context()); - } + ASSERT(!cluster_config.has_transport_socket()); + transport_socket.set_name(Envoy::Extensions::TransportSockets::TransportSocketNames::get().Tls); + transport_socket.mutable_typed_config()->PackFrom(tls_context_); // TODO(oschaaf): Ideally we'd just re-use Tls::Upstream::createTransportFactory(). // But instead of doing that, we need to perform some of what that implements ourselves here, @@ -161,6 +160,7 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { Envoy::Network::ConnectionSocket::OptionsSharedPtr options = std::make_shared(); + if (use_h2_) { pool_ = std::make_unique(dispatcher_, host, Envoy::Upstream::ResourcePriority::Default, options); @@ -172,7 +172,7 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { if (prefetch_connections_) { prefetchPoolConnections(); } -} +} // namespace Client void BenchmarkClientHttpImpl::terminate() { pool_.reset(); } @@ -191,14 +191,16 @@ void BenchmarkClientHttpImpl::setRequestHeader(absl::string_view key, absl::stri } bool BenchmarkClientHttpImpl::tryStartOne(std::function caller_completion_callback) { - if (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default) - .pendingRequests() - .canCreate() || - // In closed loop mode we want to be able to control the pacing as exactly as possible. - // In open-loop mode we probably want to skip this. - // NOTE(oschaaf): We can't consistently rely on resourceManager()::requests() because that - // isn't used for h/1 (it is used in tcp and h2 though). - ((requests_initiated_ - requests_completed_) >= connection_limit_)) { + // When no client side queueing is specified (via max_pending_requests_), we are in closed loop + // mode. In closed loop mode we want to be able to control the pacing as exactly as possible. In + // open-loop mode we probably want to skip this. NOTE(oschaaf): We can't consistently rely on + // resourceManager()::requests() because that isn't used for h/1 (it is used in tcp and h2 + // though). + if (max_pending_requests_ == 1 && + (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default) + .pendingRequests() + .canCreate() || + ((requests_initiated_ - requests_completed_) >= connection_limit_))) { return false; } @@ -236,7 +238,16 @@ void BenchmarkClientHttpImpl::onComplete(bool success, const Envoy::Http::Header } void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) { - ASSERT(reason == Envoy::Http::ConnectionPool::PoolFailureReason::ConnectionFailure); + switch (reason) { + case Envoy::Http::ConnectionPool::PoolFailureReason::Overflow: + benchmark_client_stats_.pool_overflow_.inc(); + break; + case Envoy::Http::ConnectionPool::PoolFailureReason::ConnectionFailure: + benchmark_client_stats_.pool_connection_failure_.inc(); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } } } // namespace Client diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 96a2872ce..c63e7799b 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -24,6 +24,8 @@ #include "client/stream_decoder.h" +#include "api/client/options.pb.h" + namespace Nighthawk { namespace Client { @@ -38,7 +40,9 @@ using namespace Envoy; // We need this because of macro expectations. COUNTER(http_3xx) \ COUNTER(http_4xx) \ COUNTER(http_5xx) \ - COUNTER(http_xxx) + COUNTER(http_xxx) \ + COUNTER(pool_overflow) \ + COUNTER(pool_connection_failure) struct BenchmarkClientStats { ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) @@ -51,13 +55,20 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, BenchmarkClientHttpImpl(Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Store& store, StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, UriPtr&& uri, bool use_h2, - bool prefetch_connections); + bool prefetch_connections, + envoy::api::v2::auth::UpstreamTlsContext tls_context); void setConnectionLimit(uint64_t connection_limit) { connection_limit_ = connection_limit; } void setConnectionTimeout(std::chrono::seconds timeout) { timeout_ = timeout; } void setMaxPendingRequests(uint64_t max_pending_requests) { max_pending_requests_ = max_pending_requests; } + void setMaxActiveRequests(uint64_t max_active_requests) { + max_active_requests_ = max_active_requests; + } + void setMaxRequestsPerConnection(uint64_t max_requests_per_connection) { + max_requests_per_connection_ = max_requests_per_connection; + } // BenchmarkClient void initialize(Envoy::Runtime::Loader& runtime) override; @@ -103,8 +114,10 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, const bool prefetch_connections_; const UriPtr uri_; std::chrono::seconds timeout_{5s}; - uint64_t connection_limit_{1}; - uint64_t max_pending_requests_{1}; + uint32_t connection_limit_{1}; + uint32_t max_pending_requests_{1}; + uint32_t max_active_requests_{UINT32_MAX}; + uint32_t max_requests_per_connection_{UINT32_MAX}; PrefetchablePoolPtr pool_; Envoy::Event::TimerPtr timer_; Envoy::Runtime::RandomGeneratorImpl generator_; @@ -113,6 +126,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, bool measure_latencies_{}; BenchmarkClientStats benchmark_client_stats_; uint32_t request_body_size_{0}; + const envoy::api::v2::auth::UpstreamTlsContext tls_context_; }; } // namespace Client diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index 058466bed..22754427c 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -25,20 +25,20 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create(Envoy::Api::Api& api, StatisticFactoryImpl statistic_factory(options_); auto benchmark_client = std::make_unique( api, dispatcher, store, statistic_factory.create(), statistic_factory.create(), - std::move(uri), options_.h2(), options_.prefetchConnections()); - - benchmark_client->setRequestMethod(options_.requestMethod()); - + std::move(uri), options_.h2(), options_.prefetchConnections(), options_.tlsContext()); auto request_options = options_.toCommandLineOptions()->request_options(); if (request_options.request_headers_size() > 0) { for (const auto& header : request_options.request_headers()) { benchmark_client->setRequestHeader(header.header().key(), header.header().value()); } } - + benchmark_client->setRequestMethod(options_.requestMethod()); benchmark_client->setRequestBodySize(options_.requestBodySize()); benchmark_client->setConnectionTimeout(options_.timeout()); benchmark_client->setConnectionLimit(options_.connections()); + benchmark_client->setMaxPendingRequests(options_.maxPendingRequests()); + benchmark_client->setMaxActiveRequests(options_.maxActiveRequests()); + benchmark_client->setMaxRequestsPerConnection(options_.maxRequestsPerConnection()); return benchmark_client; } diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 4e934926f..ed1e3472d 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -1,7 +1,6 @@ #include "client/options_impl.h" -#include - +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" #include "common/uri_impl.h" #include "common/utility.h" @@ -14,53 +13,69 @@ namespace Nighthawk { namespace Client { OptionsImpl::OptionsImpl(int argc, const char* const* argv) { + setNonTrivialDefaults(); + // Override some defaults, we are in CLI-mode. + verbosity_ = "info"; + output_format_ = "human"; + const char* descr = "L7 (HTTP/HTTPS/HTTP2) performance characterization tool."; TCLAP::CmdLine cmd(descr, ' ', "PoC"); // NOLINT - TCLAP::ValueArg requests_per_second("", "rps", - "The target requests-per-second rate. Default: 5.", - false, 5 /*default qps*/, "uint64_t", cmd); - TCLAP::ValueArg connections( + TCLAP::ValueArg requests_per_second( + "", "rps", + fmt::format("The target requests-per-second rate. Default: {}.", requests_per_second_), false, + 0, "uint32_t", cmd); + TCLAP::ValueArg connections( "", "connections", - "The number of connections per event loop that the test should maximally " - "use. HTTP/1 only. Default: 1.", - false, 1, "uint64_t", cmd); - TCLAP::ValueArg duration("", "duration", - "The number of seconds that the test should run. Default: 5.", - false, 5, "uint64_t", cmd); - TCLAP::ValueArg timeout( + fmt::format("The number of connections per event loop that the test should maximally " + "use. HTTP/1 only. Default: {}.", + connections_), + false, 0, "uint32_t", cmd); + TCLAP::ValueArg duration( + "", "duration", + fmt::format("The number of seconds that the test should run. Default: {}.", duration_), false, + 0, "uint32_t", cmd); + TCLAP::ValueArg timeout( "", "timeout", - "Timeout period in seconds used for both connection timeout and grace period waiting for " - "lagging responses to come in after the test run is done. Default: 5.", - false, 5, "uint64_t", cmd); + fmt::format( + "Timeout period in seconds used for both connection timeout and grace period waiting for " + "lagging responses to come in after the test run is done. Default: {}.", + timeout_), + false, 0, "uint32_t", cmd); TCLAP::SwitchArg h2("", "h2", "Use HTTP/2", cmd); TCLAP::ValueArg concurrency( "", "concurrency", - "The number of concurrent event loops that should be used. Specify 'auto' to let " - "Nighthawk leverage all vCPUs that have affinity to the Nighthawk process.Note that " - "increasing this results in an effective load multiplier combined with the configured-- rps " - "and --connections values.Default : 1. ", - false, "1", "string", cmd); + fmt::format( + "The number of concurrent event loops that should be used. Specify 'auto' to let " + "Nighthawk leverage all vCPUs that have affinity to the Nighthawk process.Note that " + "increasing this results in an effective load multiplier combined with the configured-- " + "rps " + "and --connections values. Default: {}. ", + concurrency_), + false, "", "string", cmd); std::vector log_levels = {"trace", "debug", "info", "warn", "error", "critical"}; TCLAP::ValuesConstraint verbosities_allowed(log_levels); TCLAP::ValueArg verbosity( "v", "verbosity", - "Verbosity of the output. Possible values: [trace, debug, info, warn, error, critical]. The " - "default level is 'info'.", - false, "warn", &verbosities_allowed, cmd); + fmt::format("Verbosity of the output. Possible values: [trace, debug, info, warn, error, " + "critical]. The " + "default level is '{}'.", + verbosity_), + false, "", &verbosities_allowed, cmd); std::vector output_formats = {"human", "yaml", "json"}; TCLAP::ValuesConstraint output_formats_allowed(output_formats); TCLAP::ValueArg output_format( "", "output-format", - "Verbosity of the output. Possible values: [human, yaml, json]. The " - "default output format is 'human'.", - false, "human", &output_formats_allowed, cmd); + fmt::format("Verbosity of the output. Possible values: [human, yaml, json]. The " + "default output format is '{}'.", + output_format_), + false, "", &output_formats_allowed, cmd); TCLAP::SwitchArg prefetch_connections( "", "prefetch-connections", "Prefetch connections before benchmarking (HTTP/1 only).", cmd); @@ -68,18 +83,18 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { // Note: we allow a burst size of 1, which intuitively may not make sense. However, allowing it // doesn't hurt either, and it does allow one to use a the same code-execution-paths in test // series that ramp up burst sizes. - TCLAP::ValueArg burst_size( + TCLAP::ValueArg burst_size( "", "burst-size", - "Release requests in bursts of the specified size (default: 0, no bursting).", false, 0, - "uint64_t", cmd); + fmt::format("Release requests in bursts of the specified size (default: {}).", burst_size_), + false, 0, "uint32_t", cmd); std::vector address_families = {"auto", "v4", "v6"}; TCLAP::ValuesConstraint address_families_allowed(address_families); - TCLAP::ValueArg address_family( "", "address-family", - "Network addres family preference. Possible values: [auto, v4, v6]. The " - "default output format is 'v4'.", - false, "v4", &address_families_allowed, cmd); + fmt::format("Network addres family preference. Possible values: [auto, v4, v6]. The " + "default output format is '{}'.", + address_family_), + false, "", &address_families_allowed, cmd); std::vector request_methods = {"GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE"}; @@ -99,6 +114,28 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { "to the number specified here. (default: 0, no data).", false, 0, "uint32_t", cmd); + TCLAP::ValueArg tls_context( + "", "tls-context", + "Tls context configuration in yaml or json. Example (json):" + "{common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", + false, "", "string", cmd); + + TCLAP::ValueArg max_pending_requests( + "", "max-pending-requests", + "Max pending requests (default: 1, no client side queuing. Specifying any other value will " + "allow client-side queuing of requests).", + false, 0, "uint32_t", cmd); + + TCLAP::ValueArg max_active_requests( + "", "max-active-requests", + fmt::format("Max active requests (default: {}).", max_active_requests_), false, 0, "uint32_t", + cmd); + + TCLAP::ValueArg max_requests_per_connection( + "", "max-requests-per-connection", + fmt::format("Max requests per connection (default: {}).", max_requests_per_connection_), + false, 0, "uint32_t", cmd); + TCLAP::UnlabeledValueArg uri("uri", "uri to benchmark. http:// and https:// are supported, " "but in case of https no certificates are validated.", @@ -121,40 +158,151 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { throw NoServingException(); } - requests_per_second_ = requests_per_second.getValue(); - connections_ = connections.getValue(); - duration_ = duration.getValue(); - timeout_ = timeout.getValue(); + if (requests_per_second.isSet()) { + requests_per_second_ = requests_per_second.getValue(); + } + if (connections.isSet()) { + connections_ = connections.getValue(); + } + if (duration.isSet()) { + duration_ = duration.getValue(); + } + if (timeout.isSet()) { + timeout_ = timeout.getValue(); + } uri_ = uri.getValue(); - h2_ = h2.getValue(); - concurrency_ = concurrency.getValue(); - verbosity_ = verbosity.getValue(); - output_format_ = output_format.getValue(); - prefetch_connections_ = prefetch_connections.getValue(); - burst_size_ = burst_size.getValue(); - address_family_ = address_family.getValue(); - request_method_ = request_method.getValue(); - request_headers_ = request_headers.getValue(); - request_body_size_ = request_body_size.getValue(); - - // We cap on negative values. TCLAP accepts negative values which we will get here as very - // large values. We just cap values to 2^63. - const uint64_t largest_acceptable_uint64_option_value = - static_cast(std::pow(2ull, 63ull)); - - if (requests_per_second_ == 0 || requests_per_second_ > largest_acceptable_uint64_option_value) { + if (h2.isSet()) { + h2_ = h2.getValue(); + } + if (concurrency.isSet()) { + concurrency_ = concurrency.getValue(); + } + if (verbosity.isSet()) { + verbosity_ = verbosity.getValue(); + } + if (output_format.isSet()) { + output_format_ = output_format.getValue(); + } + if (prefetch_connections.isSet()) { + prefetch_connections_ = prefetch_connections.getValue(); + } + if (burst_size.isSet()) { + burst_size_ = burst_size.getValue(); + } + if (address_family.isSet()) { + address_family_ = address_family.getValue(); + } + if (request_method.isSet()) { + request_method_ = request_method.getValue(); + } + if (request_headers.isSet()) { + request_headers_ = request_headers.getValue(); + } + if (request_body_size.isSet()) { + request_body_size_ = request_body_size.getValue(); + } + if (max_pending_requests.isSet()) { + max_pending_requests_ = max_pending_requests.getValue(); + } + if (max_active_requests.isSet()) { + max_active_requests_ = max_active_requests.getValue(); + } + if (max_requests_per_connection.isSet()) { + max_requests_per_connection_ = max_requests_per_connection.getValue(); + } + + // CLI-specific tests. + if (requests_per_second_ > largest_acceptable_uint32_option_value) { throw MalformedArgvException("Invalid value for --rps"); } - if (connections_ == 0 || connections_ > largest_acceptable_uint64_option_value) { + if (connections_ > largest_acceptable_uint32_option_value) { throw MalformedArgvException("Invalid value for --connections"); } - if (duration_ == 0 || duration_ > largest_acceptable_uint64_option_value) { + if (duration_ > largest_acceptable_uint32_option_value) { throw MalformedArgvException("Invalid value for --duration"); } - if (timeout_ == 0 || timeout_ > largest_acceptable_uint64_option_value) { + if (timeout_ > largest_acceptable_uint32_option_value) { throw MalformedArgvException("Invalid value for --timeout"); } + if (request_body_size_ > largest_acceptable_uint32_option_value) { + throw MalformedArgvException("Invalid value for --request-body-size"); + } + if (burst_size_ > largest_acceptable_uint32_option_value) { + throw MalformedArgvException("Invalid value for --burst-size"); + } + if (max_pending_requests_ > largest_acceptable_uint32_option_value) { + throw MalformedArgvException("Invalid value for --max-pending-requests"); + } + if (max_active_requests_ > largest_acceptable_uint32_option_value) { + throw MalformedArgvException("Invalid value for --max-active-requests"); + } + if (max_requests_per_connection_ > largest_acceptable_uint32_option_value) { + throw MalformedArgvException("Invalid value for --max-requests-per-connection"); + } + + if (!tls_context.getValue().empty()) { + // TODO(oschaaf): used to by loadFromJsonEx, which is now gone. + Envoy::MessageUtil::loadFromJson(tls_context.getValue(), tls_context_, + Envoy::ProtobufMessage::getNullValidationVisitor()); + } + validate(); +} + +OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { + // XXX(oschaaf): as default composition isn't trivial, add tests to ensure all constructors + setNonTrivialDefaults(); + for (const auto& header : options.request_options().request_headers()) { + std::string header_string = + fmt::format("{}:{}", header.header().key(), header.header().value()); + request_headers_.push_back(header_string); + } + + requests_per_second_ = + PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, requests_per_second, requests_per_second_); + if (options.has_duration()) { + duration_ = options.duration().seconds(); + } + if (options.has_timeout()) { + timeout_ = options.timeout().seconds(); + } + uri_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, uri, uri_); + h2_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, h2, h2_); + concurrency_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, concurrency, concurrency_); + verbosity_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, verbosity, verbosity_); + output_format_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, output_format, output_format_); + prefetch_connections_ = + PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, prefetch_connections, prefetch_connections_); + burst_size_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, burst_size, burst_size_); + address_family_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, address_family, address_family_); + const auto& request_options = options.request_options(); + if (request_options.request_method() != + ::envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED) { + request_method_ = ::envoy::api::v2::core::RequestMethod_Name(request_options.request_method()); + } + request_body_size_ = + PROTOBUF_GET_WRAPPED_OR_DEFAULT(request_options, request_body_size, request_body_size_); + max_pending_requests_ = + PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, max_pending_requests, max_pending_requests_); + max_active_requests_ = + PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, max_active_requests, max_active_requests_); + max_requests_per_connection_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( + options, max_requests_per_connection, max_requests_per_connection_); + connections_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, connections, connections_); + + tls_context_.MergeFrom(options.tls_context()); + validate(); +} + +void OptionsImpl::setNonTrivialDefaults() { + concurrency_ = "1"; + verbosity_ = "warn"; + output_format_ = "json"; + address_family_ = "v4"; + request_method_ = "GET"; +} + +void OptionsImpl::validate() const { // concurrency must be either 'auto' or a positive integer. if (concurrency_ != "auto") { int parsed_concurrency; @@ -169,29 +317,15 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { throw MalformedArgvException("Value for --concurrency should be greater then 0."); } } - try { UriImpl uri(uri_); } catch (const UriException) { throw MalformedArgvException("Invalid URI"); } -} - -OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) - : requests_per_second_(options.requests_per_second()), connections_(options.connections()), - duration_(options.duration().seconds()), timeout_(options.timeout().seconds()), - uri_(options.uri()), h2_(options.h2()), concurrency_(options.concurrency()), - verbosity_(options.verbosity()), output_format_(options.output_format()), - prefetch_connections_(options.prefetch_connections()), burst_size_(options.burst_size()), - address_family_(options.address_family()), - request_method_( - ::envoy::api::v2::core::RequestMethod_Name(options.request_options().request_method())), - request_body_size_(options.request_options().request_body_size()) { - Envoy::MessageUtil::validate(options); - for (const auto& header : options.request_options().request_headers()) { - std::string header_string = - fmt::format("{}:{}", header.header().key(), header.header().value()); - request_headers_.push_back(header_string); + try { + Envoy::MessageUtil::validate(*toCommandLineOptions()); + } catch (const Envoy::ProtoValidationException& e) { + throw MalformedArgvException(e.what()); } } @@ -199,18 +333,18 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const { CommandLineOptionsPtr command_line_options = std::make_unique(); - command_line_options->set_connections(connections()); + command_line_options->mutable_connections()->set_value(connections()); command_line_options->mutable_duration()->set_seconds(duration().count()); - command_line_options->set_requests_per_second(requestsPerSecond()); + command_line_options->mutable_requests_per_second()->set_value(requestsPerSecond()); command_line_options->mutable_timeout()->set_seconds(timeout().count()); - command_line_options->set_h2(h2()); - command_line_options->set_uri(uri()); - command_line_options->set_concurrency(concurrency()); - command_line_options->set_verbosity(verbosity()); - command_line_options->set_output_format(outputFormat()); - command_line_options->set_prefetch_connections(prefetchConnections()); - command_line_options->set_burst_size(burstSize()); - command_line_options->set_address_family(addressFamily()); + command_line_options->mutable_h2()->set_value(h2()); + command_line_options->mutable_uri()->set_value(uri()); + command_line_options->mutable_concurrency()->set_value(concurrency()); + command_line_options->mutable_verbosity()->set_value(verbosity()); + command_line_options->mutable_output_format()->set_value(outputFormat()); + command_line_options->mutable_prefetch_connections()->set_value(prefetchConnections()); + command_line_options->mutable_burst_size()->set_value(burstSize()); + command_line_options->mutable_address_family()->set_value(addressFamily()); auto request_options = command_line_options->mutable_request_options(); envoy::api::v2::core::RequestMethod method = envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED; @@ -229,8 +363,12 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const { request_header->set_value(split_header[1]); } } - request_options->set_request_body_size(requestBodySize()); - + request_options->mutable_request_body_size()->set_value(requestBodySize()); + *(command_line_options->mutable_tls_context()) = tlsContext(); + command_line_options->mutable_max_pending_requests()->set_value(maxPendingRequests()); + command_line_options->mutable_max_active_requests()->set_value(maxActiveRequests()); + command_line_options->mutable_max_requests_per_connection()->set_value( + maxRequestsPerConnection()); return command_line_options; } diff --git a/source/client/options_impl.h b/source/client/options_impl.h index 5a24760cd..6aad947dd 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -14,11 +15,10 @@ class OptionsImpl : public Options { public: OptionsImpl(int argc, const char* const* argv); OptionsImpl(const nighthawk::client::CommandLineOptions& options); - Client::CommandLineOptionsPtr toCommandLineOptions() const override; - uint64_t requestsPerSecond() const override { return requests_per_second_; } - uint64_t connections() const override { return connections_; } + uint32_t requestsPerSecond() const override { return requests_per_second_; } + uint32_t connections() const override { return connections_; } std::chrono::seconds duration() const override { return std::chrono::seconds(duration_); } std::chrono::seconds timeout() const override { return std::chrono::seconds(timeout_); } std::string uri() const override { return uri_; } @@ -27,28 +27,44 @@ class OptionsImpl : public Options { std::string verbosity() const override { return verbosity_; }; std::string outputFormat() const override { return output_format_; }; bool prefetchConnections() const override { return prefetch_connections_; } - uint64_t burstSize() const override { return burst_size_; } + uint32_t burstSize() const override { return burst_size_; } std::string addressFamily() const override { return address_family_; }; std::string requestMethod() const override { return request_method_; }; std::vector requestHeaders() const override { return request_headers_; }; uint32_t requestBodySize() const override { return request_body_size_; }; + const envoy::api::v2::auth::UpstreamTlsContext& tlsContext() const override { + return tls_context_; + }; + uint32_t maxPendingRequests() const override { return max_pending_requests_; } + uint32_t maxActiveRequests() const override { return max_active_requests_; } + uint32_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } private: - uint64_t requests_per_second_; - uint64_t connections_; - uint64_t duration_; - uint64_t timeout_; + void setNonTrivialDefaults(); + void validate() const; + + // We cap on negative values. TCLAP accepts negative values which we will get here as very + // large values. We just cap values, hoping we catch accidental wraparound to a reasonable extent. + static constexpr uint32_t largest_acceptable_uint32_option_value = UINT32_MAX - 30000; + uint32_t requests_per_second_{5}; + uint32_t connections_{1}; + uint32_t duration_{5}; + uint32_t timeout_{30}; std::string uri_; - bool h2_; + bool h2_{false}; std::string concurrency_; std::string verbosity_; std::string output_format_; - bool prefetch_connections_; - uint64_t burst_size_; + bool prefetch_connections_{false}; + uint32_t burst_size_{0}; std::string address_family_; std::string request_method_; std::vector request_headers_; - uint32_t request_body_size_; + uint32_t request_body_size_{0}; + envoy::api::v2::auth::UpstreamTlsContext tls_context_; + uint32_t max_pending_requests_{1}; + uint32_t max_active_requests_{largest_acceptable_uint32_option_value}; + uint32_t max_requests_per_connection_{largest_acceptable_uint32_option_value}; }; } // namespace Client diff --git a/source/client/service_impl.cc b/source/client/service_impl.cc index 90ff480d9..fd38a3a1b 100644 --- a/source/client/service_impl.cc +++ b/source/client/service_impl.cc @@ -15,8 +15,8 @@ void ServiceImpl::handleExecutionRequest(const nighthawk::client::ExecutionReque OptionsPtr options; try { options = std::make_unique(request.start_request().options()); - } catch (Envoy::EnvoyException exception) { - response.mutable_error_detail()->set_message(exception.what()); + } catch (MalformedArgvException e) { + response.mutable_error_detail()->set_message(e.what()); writeResponseAndFinish(response); return; } diff --git a/source/common/sequencer_impl.cc b/source/common/sequencer_impl.cc index fe7b20f70..7d791a2b1 100644 --- a/source/common/sequencer_impl.cc +++ b/source/common/sequencer_impl.cc @@ -157,6 +157,8 @@ void SequencerImpl::waitForCompletion() { ASSERT(!running_); } +void SequencerImpl::cancel() { cancelled_ = true; } + StatisticPtrMap SequencerImpl::statistics() const { StatisticPtrMap statistics; statistics[latency_statistic_->id()] = latency_statistic_.get(); diff --git a/source/common/sequencer_impl.h b/source/common/sequencer_impl.h index e94e057b2..5ca400afd 100644 --- a/source/common/sequencer_impl.h +++ b/source/common/sequencer_impl.h @@ -68,6 +68,8 @@ class SequencerImpl : public Sequencer, public Envoy::Logger::Loggableresolve(*dispatcher_, GetParam() == Envoy::Network::Address::IpVersion::v4 ? Envoy::Network::DnsLookupFamily::V4Only : Envoy::Network::DnsLookupFamily::V6Only); + envoy::api::v2::auth::UpstreamTlsContext tls_context; client_ = std::make_unique( api_, *dispatcher_, store_, std::make_unique(), - std::make_unique(), std::move(uri), use_h2, prefetch_connections); + std::make_unique(), std::move(uri), use_h2, prefetch_connections, + tls_context); } uint64_t nonZeroValuedCounterCount() { @@ -301,7 +303,7 @@ TEST_P(BenchmarkClientHttpTest, H1ConnectionFailure) { EXPECT_EQ(1, getCounter("upstream_rq_pending_total")); EXPECT_EQ(1, getCounter("upstream_cx_destroy_remote")); EXPECT_EQ(1, getCounter("upstream_cx_destroy")); - EXPECT_EQ(7, nonZeroValuedCounterCount()); + EXPECT_EQ(8, nonZeroValuedCounterCount()); } TEST_P(BenchmarkClientHttpTest, H1MultiConnectionFailure) { @@ -317,7 +319,7 @@ TEST_P(BenchmarkClientHttpTest, H1MultiConnectionFailure) { EXPECT_EQ(10, getCounter("upstream_rq_pending_total")); EXPECT_EQ(10, getCounter("upstream_cx_destroy_remote")); EXPECT_EQ(10, getCounter("upstream_cx_destroy")); - EXPECT_EQ(7, nonZeroValuedCounterCount()); + EXPECT_EQ(8, nonZeroValuedCounterCount()); } TEST_P(BenchmarkClientHttpTest, EnableLatencyMeasurement) { @@ -351,9 +353,10 @@ TEST_P(BenchmarkClientHttpTest, EnableLatencyMeasurement) { TEST_P(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { auto uri = std::make_unique("http://foo/"); auto store = std::make_unique(); + envoy::api::v2::auth::UpstreamTlsContext tls_context; client_ = std::make_unique( api_, *dispatcher_, *store, std::make_unique(), - std::make_unique(), std::move(uri), false, false); + std::make_unique(), std::move(uri), false, false, tls_context); Envoy::Http::HeaderMapImpl header; auto& status = header.insertStatus(); @@ -395,6 +398,64 @@ TEST_P(BenchmarkClientHttpTest, ConnectionPrefetching) { EXPECT_EQ(50, getCounter("upstream_cx_total")); } +// XXX(oschaaf): Ok; so turns out this feature is h/2 only atm. Maybe want need to log a warning +// if someone configures this for h1. +// XXX(oschaaf): maybe we want to run parameterized tests running over +// h1/h2 and tls vs plain +TEST_P(BenchmarkClientHttpTest, CapRequestConcurrency) { + setupBenchmarkClient("/lorem-ipsum-status-200", true, false); + const uint64_t requests = 4; + uint64_t inflight_response_count = requests; + + // We configure so that max requests is the only thing that can be throttling. + client_->setMaxPendingRequests(requests); + client_->setConnectionLimit(requests); + client_->setMaxActiveRequests(1); + client_->initialize(runtime_); + + std::function f = [this, &inflight_response_count]() { + --inflight_response_count; + if (inflight_response_count == 3) { + dispatcher_->exit(); + } + }; + for (uint64_t i = 0; i < requests; i++) { + EXPECT_EQ(true, client_->tryStartOne(f)); + } + dispatcher_->run(Envoy::Event::Dispatcher::RunType::Block); + EXPECT_EQ(1, getCounter("benchmark.http_2xx")); + EXPECT_EQ(1, getCounter("upstream_rq_total")); + EXPECT_EQ(3, getCounter("upstream_rq_pending_overflow")); + EXPECT_EQ(3, getCounter("benchmark.pool_overflow")); +} + +TEST_P(BenchmarkClientHttpsTest, MaxRequestsPerConnection) { + setupBenchmarkClient("/lorem-ipsum-status-200", false, false); + const uint64_t requests = 4; + uint64_t inflight_response_count = requests; + + // We configure so that max requests is the only thing that can be throttling. + client_->setMaxPendingRequests(requests); + client_->setConnectionLimit(requests); + client_->setMaxActiveRequests(1024); + client_->setMaxRequestsPerConnection(1); + client_->initialize(runtime_); + + std::function f = [this, &inflight_response_count]() { + --inflight_response_count; + if (inflight_response_count == 0) { + dispatcher_->exit(); + } + }; + for (uint64_t i = 0; i < requests; i++) { + EXPECT_EQ(true, client_->tryStartOne(f)); + } + dispatcher_->run(Envoy::Event::Dispatcher::RunType::Block); + + EXPECT_EQ(requests, getCounter("benchmark.http_2xx")); + EXPECT_EQ(requests, getCounter("upstream_cx_http1_total")); +} + TEST_P(BenchmarkClientHttpTest, RequestMethodPost) { setupBenchmarkClient("/", false, true); EXPECT_EQ("GET", client_->requestHeaders().Method()->value().getStringView()); diff --git a/test/factories_test.cc b/test/factories_test.cc index 61c9362a5..652a98f0b 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -31,12 +31,19 @@ class FactoriesTest : public Test { TEST_F(FactoriesTest, CreateBenchmarkClient) { BenchmarkClientFactoryImpl factory(options_); + envoy::api::v2::auth::UpstreamTlsContext tls_context; + EXPECT_CALL(options_, timeout()).Times(1); EXPECT_CALL(options_, connections()).Times(1); EXPECT_CALL(options_, h2()).Times(1); EXPECT_CALL(options_, prefetchConnections()).Times(1); EXPECT_CALL(options_, requestMethod()).Times(1); EXPECT_CALL(options_, requestBodySize()).Times(1); + EXPECT_CALL(options_, tlsContext()).Times(1).WillOnce(ReturnRef(tls_context)); + EXPECT_CALL(options_, maxPendingRequests()).Times(1); + EXPECT_CALL(options_, maxActiveRequests()).Times(1); + EXPECT_CALL(options_, maxRequestsPerConnection()).Times(1); + auto cmd = std::make_unique(); auto request_headers = cmd->mutable_request_options()->add_request_headers(); request_headers->mutable_header()->set_key("foo"); diff --git a/test/mocks.h b/test/mocks.h index f870e322c..21d8e0cdc 100644 --- a/test/mocks.h +++ b/test/mocks.h @@ -61,8 +61,8 @@ class MockOptions : public Client::Options { public: MockOptions(); - MOCK_CONST_METHOD0(requestsPerSecond, uint64_t()); - MOCK_CONST_METHOD0(connections, uint64_t()); + MOCK_CONST_METHOD0(requestsPerSecond, uint32_t()); + MOCK_CONST_METHOD0(connections, uint32_t()); MOCK_CONST_METHOD0(duration, std::chrono::seconds()); MOCK_CONST_METHOD0(timeout, std::chrono::seconds()); MOCK_CONST_METHOD0(uri, std::string()); @@ -71,11 +71,15 @@ class MockOptions : public Client::Options { MOCK_CONST_METHOD0(verbosity, std::string()); MOCK_CONST_METHOD0(outputFormat, std::string()); MOCK_CONST_METHOD0(prefetchConnections, bool()); - MOCK_CONST_METHOD0(burstSize, uint64_t()); + MOCK_CONST_METHOD0(burstSize, uint32_t()); MOCK_CONST_METHOD0(addressFamily, std::string()); MOCK_CONST_METHOD0(requestMethod, std::string()); MOCK_CONST_METHOD0(requestHeaders, std::vector()); MOCK_CONST_METHOD0(requestBodySize, uint32_t()); + MOCK_CONST_METHOD0(tlsContext, envoy::api::v2::auth::UpstreamTlsContext&()); + MOCK_CONST_METHOD0(maxPendingRequests, uint32_t()); + MOCK_CONST_METHOD0(maxActiveRequests, uint32_t()); + MOCK_CONST_METHOD0(maxRequestsPerConnection, uint32_t()); MOCK_CONST_METHOD0(toCommandLineOptions, Client::CommandLineOptionsPtr()); }; diff --git a/test/options_test.cc b/test/options_test.cc index 5d0342487..b21f6546e 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -23,6 +23,8 @@ class OptionsImplTest : public Test { }; class OptionsImplIntTest : public OptionsImplTest, public WithParamInterface {}; +class OptionsImplIntTestNonZeroable : public OptionsImplTest, + public WithParamInterface {}; TEST_F(OptionsImplTest, BogusInput) { // When just passing the non-existing argument --foo it would be interpreted as a @@ -34,12 +36,15 @@ TEST_F(OptionsImplTest, BogusInput) { // This test should cover every option we offer. TEST_F(OptionsImplTest, All) { - std::unique_ptr options = TestUtility::createOptionsImpl( - fmt::format("{} --rps 4 --connections 5 --duration 6 --timeout 7 --h2 " - "--concurrency 8 --verbosity error --output-format json --prefetch-connections " - "--burst-size 13 --address-family v6 --request-method POST --request-body-size " - "1234 --request-header f1:b1 --request-header f2:b2 {}", - client_name_, good_test_uri_)); + Envoy::MessageUtil util; + std::unique_ptr options = TestUtility::createOptionsImpl(fmt::format( + "{} --rps 4 --connections 5 --duration 6 --timeout 7 --h2 " + "--concurrency 8 --verbosity error --output-format json --prefetch-connections " + "--burst-size 13 --address-family v6 --request-method POST --request-body-size 1234 " + "--tls-context {} --request-header f1:b1 --request-header f2:b2 {}", + client_name_, + "{common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", + good_test_uri_)); EXPECT_EQ(4, options->requestsPerSecond()); EXPECT_EQ(5, options->connections()); @@ -57,21 +62,24 @@ TEST_F(OptionsImplTest, All) { const std::vector expected_headers = {"f1:b1", "f2:b2"}; EXPECT_EQ(expected_headers, options->requestHeaders()); EXPECT_EQ(1234, options->requestBodySize()); + EXPECT_EQ("common_tls_context {\n tls_params {\n cipher_suites: " + "\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"\n }\n}\n", + options->tlsContext().DebugString()); // Check that our conversion to CommandLineOptionsPtr makes sense. CommandLineOptionsPtr cmd = options->toCommandLineOptions(); - EXPECT_EQ(cmd->requests_per_second(), options->requestsPerSecond()); - EXPECT_EQ(cmd->connections(), options->connections()); + EXPECT_EQ(cmd->requests_per_second().value(), options->requestsPerSecond()); + EXPECT_EQ(cmd->connections().value(), options->connections()); EXPECT_EQ(cmd->duration().seconds(), options->duration().count()); EXPECT_EQ(cmd->timeout().seconds(), options->timeout().count()); - EXPECT_EQ(cmd->h2(), options->h2()); - EXPECT_EQ(cmd->concurrency(), options->concurrency()); - EXPECT_EQ(cmd->verbosity(), options->verbosity()); - EXPECT_EQ(cmd->output_format(), options->outputFormat()); - EXPECT_EQ(cmd->prefetch_connections(), options->prefetchConnections()); - EXPECT_EQ(cmd->burst_size(), options->burstSize()); - EXPECT_EQ(cmd->address_family(), options->addressFamily()); - EXPECT_EQ(cmd->uri(), options->uri()); + EXPECT_EQ(cmd->h2().value(), options->h2()); + EXPECT_EQ(cmd->concurrency().value(), options->concurrency()); + EXPECT_EQ(cmd->verbosity().value(), options->verbosity()); + EXPECT_EQ(cmd->output_format().value(), options->outputFormat()); + EXPECT_EQ(cmd->prefetch_connections().value(), options->prefetchConnections()); + EXPECT_EQ(cmd->burst_size().value(), options->burstSize()); + EXPECT_EQ(cmd->address_family().value(), options->addressFamily()); + EXPECT_EQ(cmd->uri().value(), options->uri()); auto request_options = cmd->request_options(); envoy::api::v2::core::RequestMethod method = envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED; @@ -83,14 +91,14 @@ TEST_F(OptionsImplTest, All) { EXPECT_EQ(expected_headers[i++], header.header().key() + ":" + header.header().value()); } - EXPECT_EQ(request_options.request_body_size(), options->requestBodySize()); + EXPECT_EQ(request_options.request_body_size().value(), options->requestBodySize()); + EXPECT_TRUE(util(cmd->tls_context(), options->tlsContext())); // Now we construct a new options from the proto we created above. This should result in an // OptionsImpl instance equivalent to options. We test that by converting both to yaml strings, // expecting them to be equal. This should provide helpful output when the test fails by showing // the unexpected (yaml) diff. OptionsImpl options_from_proto(*cmd); - Envoy::MessageUtil util; std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( *(options_from_proto.toCommandLineOptions()), true, true); std::string s2 = Envoy::MessageUtil::getYamlStringFromMessage(*cmd, true, true); @@ -119,13 +127,20 @@ TEST_F(OptionsImplTest, NoArguments) { MalformedArgvException, "Required argument missing: uri"); } -// Check standard expectations for any integer values options we offer. -TEST_P(OptionsImplIntTest, IntOptionsBadValuesThrow) { +TEST_P(OptionsImplIntTestNonZeroable, NonZeroableOptions) { const char* option_name = GetParam(); EXPECT_THROW_WITH_REGEX(TestUtility::createOptionsImpl(fmt::format("{} {} --{} 0", client_name_, good_test_uri_, option_name)), - MalformedArgvException, - fmt::format("Invalid value for --{}", option_name)); + std::exception, "Proto constraint validation failed"); +} + +INSTANTIATE_TEST_SUITE_P(NonZeroableIntOptionTests, OptionsImplIntTestNonZeroable, + Values("rps", "connections", "duration", "max-pending-requests", + "max-active-requests", "max-requests-per-connection")); + +// Check standard expectations for any integer values options we offer. +TEST_P(OptionsImplIntTest, IntOptionsBadValuesTest) { + const char* option_name = GetParam(); EXPECT_THROW_WITH_REGEX(TestUtility::createOptionsImpl(fmt::format("{} {} --{} -1", client_name_, good_test_uri_, option_name)), MalformedArgvException, @@ -139,7 +154,9 @@ TEST_P(OptionsImplIntTest, IntOptionsBadValuesThrow) { } INSTANTIATE_TEST_SUITE_P(IntOptionTests, OptionsImplIntTest, - Values("rps", "connections", "duration", "timeout")); + Values("rps", "connections", "duration", "timeout", "request-body-size", + "burst-size", "max-pending-requests", "max-active-requests", + "max-requests-per-connection")); // Test behaviour of the boolean valued --h2 flag. TEST_F(OptionsImplTest, H2Flag) { @@ -272,8 +289,8 @@ TEST_F(OptionsImplTest, ProtoConstructorValidation) { const auto option = TestUtility::createOptionsImpl(fmt::format("{} http://127.0.0.1/", client_name_)); auto proto = option->toCommandLineOptions(); - proto->set_requests_per_second(0); - EXPECT_THROW_WITH_REGEX(std::make_unique(*proto), Envoy::EnvoyException, + proto->mutable_requests_per_second()->set_value(0); + EXPECT_THROW_WITH_REGEX(std::make_unique(*proto), MalformedArgvException, "CommandLineOptionsValidationError.RequestsPerSecond"); } diff --git a/test/output_collector_test.cc b/test/output_collector_test.cc index 304d97c23..938d74489 100644 --- a/test/output_collector_test.cc +++ b/test/output_collector_test.cc @@ -33,6 +33,7 @@ class OutputCollectorTest : public Test { counters_["bar"] = 2; time_system_.setSystemTime(std::chrono::milliseconds(1234567891567)); command_line_options_.mutable_duration()->set_seconds(1); + command_line_options_.mutable_connections()->set_value(0); EXPECT_CALL(options_, toCommandLineOptions()) .WillOnce(Return(ByMove( std::make_unique(command_line_options_)))); diff --git a/test/sequencer_test.cc b/test/sequencer_test.cc index 9682d5ac5..b53a5bcd2 100644 --- a/test/sequencer_test.cc +++ b/test/sequencer_test.cc @@ -238,6 +238,27 @@ TEST_F(SequencerIntegrationTest, TheHappyFlow) { EXPECT_EQ(2, sequencer.statistics().size()); } +// TODO(oschaaf): would be good to have a mid-run cancellation test as well. +TEST_F(SequencerIntegrationTest, CancelEarly) { + SequencerImpl sequencer( + platform_util_, *dispatcher_, time_system_, time_system_.monotonicTime(), + std::move(rate_limiter_), sequencer_target_, std::make_unique(), + std::make_unique(), test_number_of_intervals_ * interval_, 1s); + + EXPECT_EQ(0, callback_test_count_); + EXPECT_EQ(0, sequencer.latencyStatistic().count()); + + sequencer.start(); + sequencer.cancel(); + sequencer.waitForCompletion(); + + EXPECT_EQ(0, callback_test_count_); + EXPECT_EQ(0, sequencer.latencyStatistic().count()); + EXPECT_EQ(0, sequencer.blockedStatistic().count()); + + EXPECT_EQ(2, sequencer.statistics().size()); +} + // Test an always saturated sequencer target. A concrete example would be a http benchmark client // not being able to start any requests, for example due to misconfiguration or system conditions. TEST_F(SequencerIntegrationTest, AlwaysSaturatedTargetTest) { diff --git a/test/service_test.cc b/test/service_test.cc index 41e16ce0d..ef528c557 100644 --- a/test/service_test.cc +++ b/test/service_test.cc @@ -43,17 +43,12 @@ class ServiceTest : public TestWithParam { void setBasicRequestOptions() { auto options = request_.mutable_start_request()->mutable_options(); - // TODO(oschaaf): Work on mocking so we can avoid sending actual traffic here. - options->set_uri("http://127.0.0.1:10001/"); - options->set_verbosity("info"); - options->set_connections(1); - options->set_concurrency("1"); + // XXX(oschaaf): Set defaults in the options header. + // See if we can get rid of the ones in TCLAP to disambiguate + // how we handle default values. + options->mutable_uri()->set_value("http://127.0.0.1:10001/"); options->mutable_duration()->set_seconds(3); - options->set_output_format("human"); - options->set_requests_per_second(3); - options->mutable_request_options()->set_request_method( - envoy::api::v2::core::RequestMethod::GET); - options->set_address_family("v4"); + options->mutable_requests_per_second()->set_value(3); } void runWithFailingValidationExpectations(absl::string_view match_error = "") { @@ -89,7 +84,7 @@ TEST_P(ServiceTest, Basic) { r->Write(request_, {}); r->WritesDone(); EXPECT_TRUE(r->Read(&response_)); - EXPECT_FALSE(response_.has_error_detail()); + ASSERT_FALSE(response_.has_error_detail()); EXPECT_TRUE(response_.has_output()); EXPECT_GE(response_.output().results(0).counters().size(), 8); auto status = r->Finish(); @@ -104,7 +99,7 @@ TEST_P(ServiceTest, NoConcurrentStart) { EXPECT_TRUE(r->Write(request_, {})); EXPECT_TRUE(r->WritesDone()); EXPECT_TRUE(r->Read(&response_)); - EXPECT_FALSE(response_.has_error_detail()); + ASSERT_FALSE(response_.has_error_detail()); EXPECT_TRUE(response_.has_output()); EXPECT_FALSE(r->Read(&response_)); auto status = r->Finish(); @@ -116,7 +111,7 @@ TEST_P(ServiceTest, BackToBackExecution) { auto r = stub_->ExecutionStream(&context_); EXPECT_TRUE(r->Write(request_, {})); EXPECT_TRUE(r->Read(&response_)); - EXPECT_FALSE(response_.has_error_detail()); + ASSERT_FALSE(response_.has_error_detail()); EXPECT_TRUE(response_.has_output()); EXPECT_TRUE(r->Write(request_, {})); EXPECT_TRUE(r->Read(&response_)); @@ -131,7 +126,7 @@ TEST_P(ServiceTest, BackToBackExecution) { // TODO(oschaaf): functional coverage of all the options / validations. TEST_P(ServiceTest, InvalidRps) { auto options = request_.mutable_start_request()->mutable_options(); - options->set_requests_per_second(0); + options->mutable_requests_per_second()->set_value(0); runWithFailingValidationExpectations( "CommandLineOptionsValidationError.RequestsPerSecond: [\"value must be inside range"); } @@ -162,9 +157,9 @@ TEST_P(ServiceTest, CancelNotSupported) { EXPECT_FALSE(status.ok()); } -TEST_P(ServiceTest, Unresolveable) { +TEST_P(ServiceTest, Unresolvable) { auto options = request_.mutable_start_request()->mutable_options(); - options->set_uri("http://unresolvable-host/"); + options->mutable_uri()->set_value("http://unresolvable-host/"); runWithFailingValidationExpectations("Unknown failure"); } diff --git a/test/test_data/output_collector.json.gold b/test/test_data/output_collector.json.gold index 5f1f7f855..23145884e 100644 --- a/test/test_data/output_collector.json.gold +++ b/test/test_data/output_collector.json.gold @@ -1,15 +1,6 @@ { "options": { - "requests_per_second": "0", - "connections": "0", - "h2": false, - "concurrency": "", - "verbosity": "", - "output_format": "", - "prefetch_connections": false, - "burst_size": "0", - "address_family": "", - "uri": "", + "connections": 0, "duration": "1s" }, "results": [ diff --git a/test/test_data/output_collector.yaml.gold b/test/test_data/output_collector.yaml.gold index 0719d2cfc..82db072d3 100644 --- a/test/test_data/output_collector.yaml.gold +++ b/test/test_data/output_collector.yaml.gold @@ -1,14 +1,5 @@ options: - requests_per_second: 0 connections: 0 - h2: false - concurrency: "" - verbosity: "" - output_format: "" - prefetch_connections: false - burst_size: 0 - address_family: "" - uri: "" duration: 1s results: - name: worker_0 From 4566492b20d6ccfdc2edbb81c9ed74d6dd98262d Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 12:49:24 +0200 Subject: [PATCH 07/29] Back out Sequencer::cancel() Signed-off-by: Otto van der Schaaf --- .bazelrc | 1 + include/nighthawk/common/sequencer.h | 6 ------ source/common/sequencer_impl.cc | 2 -- source/common/sequencer_impl.h | 2 -- test/sequencer_test.cc | 21 --------------------- 5 files changed, 1 insertion(+), 31 deletions(-) diff --git a/.bazelrc b/.bazelrc index 0b9d8ff54..1aea57686 100644 --- a/.bazelrc +++ b/.bazelrc @@ -66,3 +66,4 @@ build:libc++ --define force_libcpp=enabled # Test options test --test_env=HEAPCHECK=normal --test_env=PPROF_PATH +build --verbose_failures --action_env=HOME --action_env=PYTHONUSERBASE --jobs=80 --show_task_finish --experimental_generate_json_trace_profile diff --git a/include/nighthawk/common/sequencer.h b/include/nighthawk/common/sequencer.h index f4270c49f..a7277262f 100644 --- a/include/nighthawk/common/sequencer.h +++ b/include/nighthawk/common/sequencer.h @@ -41,12 +41,6 @@ class Sequencer { * other words, time spend while the Sequencer is idle and not blocked by a rate limiter). */ virtual StatisticPtrMap statistics() const PURE; - - /** - * Stops all planned work. Makes pending waitForCompletion() calls return ASAP, disregarding any - * timeouts. - */ - virtual void cancel() PURE; }; using SequencerPtr = std::unique_ptr; diff --git a/source/common/sequencer_impl.cc b/source/common/sequencer_impl.cc index 7d791a2b1..fe7b20f70 100644 --- a/source/common/sequencer_impl.cc +++ b/source/common/sequencer_impl.cc @@ -157,8 +157,6 @@ void SequencerImpl::waitForCompletion() { ASSERT(!running_); } -void SequencerImpl::cancel() { cancelled_ = true; } - StatisticPtrMap SequencerImpl::statistics() const { StatisticPtrMap statistics; statistics[latency_statistic_->id()] = latency_statistic_.get(); diff --git a/source/common/sequencer_impl.h b/source/common/sequencer_impl.h index 5ca400afd..e94e057b2 100644 --- a/source/common/sequencer_impl.h +++ b/source/common/sequencer_impl.h @@ -68,8 +68,6 @@ class SequencerImpl : public Sequencer, public Envoy::Logger::Loggable(), - std::make_unique(), test_number_of_intervals_ * interval_, 1s); - - EXPECT_EQ(0, callback_test_count_); - EXPECT_EQ(0, sequencer.latencyStatistic().count()); - - sequencer.start(); - sequencer.cancel(); - sequencer.waitForCompletion(); - - EXPECT_EQ(0, callback_test_count_); - EXPECT_EQ(0, sequencer.latencyStatistic().count()); - EXPECT_EQ(0, sequencer.blockedStatistic().count()); - - EXPECT_EQ(2, sequencer.statistics().size()); -} - // Test an always saturated sequencer target. A concrete example would be a http benchmark client // not being able to start any requests, for example due to misconfiguration or system conditions. TEST_F(SequencerIntegrationTest, AlwaysSaturatedTargetTest) { From 2c08ff3ea019e8e9df6873590abc1eea2646ebf2 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 13:07:32 +0200 Subject: [PATCH 08/29] Clean up literak string in options_impl.cc Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index ed1e3472d..9098f7abe 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -50,9 +50,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { fmt::format( "The number of concurrent event loops that should be used. Specify 'auto' to let " "Nighthawk leverage all vCPUs that have affinity to the Nighthawk process.Note that " - "increasing this results in an effective load multiplier combined with the configured-- " - "rps " - "and --connections values. Default: {}. ", + "increasing this results in an effective load multiplier combined with the configured " + "--rps and --connections values. Default: {}. ", concurrency_), false, "", "string", cmd); From 6b592b545e384beb9862abf3da5ba79686b7a494 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 16:07:40 +0200 Subject: [PATCH 09/29] Test update - Add sample server stats counter expectations - Add tests for cipher suite selection Signed-off-by: Otto van der Schaaf --- integration/test_integration_basics.py | 39 ++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index c3aa7ce2c..e0652445c 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -66,6 +66,11 @@ def test_h1(self): self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) self.assertEqual(len(counters), 14) + server_stats = self.getTestServerStatisticsJson() + self.assertEqual( + self.getServerStatFromJson(server_stats, "http.ingress_http.downstream_rq_2xx"), 25) + + def test_h2(self): parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) @@ -84,3 +89,37 @@ def test_h2(self): self.assertEqual(counters["ssl.sigalgs.rsa_pss_rsae_sha256"], 1) self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) self.assertEqual(len(counters), 14) + +def test_h1_tls_context_configuration(self): + parsed_json = self.runNighthawkClient([ + "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) + + parsed_json = self.runNighthawkClient([ + "--h2", "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) + +def test_h2_tls_context_configuration(self): + parsed_json = self.runNighthawkClient([ + "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) + + parsed_json = self.runNighthawkClient([ + "--h2", "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) From b58796e26cc495e852cd4e7525755c3004cc1487 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 17:00:00 +0200 Subject: [PATCH 10/29] Fix format Signed-off-by: Otto van der Schaaf --- integration/test_integration_basics.py | 61 +++++++++++++------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index e0652445c..84d34902f 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -70,7 +70,6 @@ def test_h1(self): self.assertEqual( self.getServerStatFromJson(server_stats, "http.ingress_http.downstream_rq_2xx"), 25) - def test_h2(self): parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) @@ -90,36 +89,38 @@ def test_h2(self): self.assertEqual(counters["ssl.versions.TLSv1.2"], 1) self.assertEqual(len(counters), 14) + def test_h1_tls_context_configuration(self): - parsed_json = self.runNighthawkClient([ - "--duration 1", - "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", - self.getTestServerRootUri() - ]) - counters = self.getNighthawkCounterMapFromJson(parsed_json) - self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) + parsed_json = self.runNighthawkClient([ + "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) + + parsed_json = self.runNighthawkClient([ + "--h2", "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) - parsed_json = self.runNighthawkClient([ - "--h2", "--duration 1", - "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", - self.getTestServerRootUri() - ]) - counters = self.getNighthawkCounterMapFromJson(parsed_json) - self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) def test_h2_tls_context_configuration(self): - parsed_json = self.runNighthawkClient([ - "--duration 1", - "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", - self.getTestServerRootUri() - ]) - counters = self.getNighthawkCounterMapFromJson(parsed_json) - self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) - - parsed_json = self.runNighthawkClient([ - "--h2", "--duration 1", - "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", - self.getTestServerRootUri() - ]) - counters = self.getNighthawkCounterMapFromJson(parsed_json) - self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) + parsed_json = self.runNighthawkClient([ + "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES128-SHA"], 1) + + parsed_json = self.runNighthawkClient([ + "--h2", "--duration 1", + "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES256-GCM-SHA384\"]}}}", + self.getTestServerRootUri() + ]) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertEqual(counters["ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"], 1) From eb47562b0cba647a007d1edbb79a621363c8b9e7 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 17:10:40 +0200 Subject: [PATCH 11/29] Back out accidentally committed .bazelrc change Signed-off-by: Otto van der Schaaf --- .bazelrc | 1 - 1 file changed, 1 deletion(-) diff --git a/.bazelrc b/.bazelrc index 1aea57686..0b9d8ff54 100644 --- a/.bazelrc +++ b/.bazelrc @@ -66,4 +66,3 @@ build:libc++ --define force_libcpp=enabled # Test options test --test_env=HEAPCHECK=normal --test_env=PPROF_PATH -build --verbose_failures --action_env=HOME --action_env=PYTHONUSERBASE --jobs=80 --show_task_finish --experimental_generate_json_trace_profile From 21ccb95a4f71af0409b5f58fa97f640e9290ee18 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jun 2019 17:27:01 +0200 Subject: [PATCH 12/29] NOLINT clang-tidy warning originating from TCLAP Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 9098f7abe..fea914819 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -76,8 +76,10 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { output_format_), false, "", &output_formats_allowed, cmd); - TCLAP::SwitchArg prefetch_connections( - "", "prefetch-connections", "Prefetch connections before benchmarking (HTTP/1 only).", cmd); + TCLAP::SwitchArg prefetch_connections( // NOLINT + "", "prefetch-connections", // NOLINT + "Prefetch connections before benchmarking (HTTP/1 only).", // NOLINT + cmd); // NOLINT // Note: we allow a burst size of 1, which intuitively may not make sense. However, allowing it // doesn't hurt either, and it does allow one to use a the same code-execution-paths in test From 261cfd5c1d342e66eb809b69865012d5aeca9ca4 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 11:21:34 +0200 Subject: [PATCH 13/29] do_ci.sh: clean dif, revert whitespace changes Signed-off-by: Otto van der Schaaf --- ci/do_ci.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 884e60aa8..9ebee2aa5 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -95,14 +95,14 @@ if [ -n "$CIRCLECI" ]; then mv "${HOME:-/root}/.gitconfig" "${HOME:-/root}/.gitconfig_save" echo 1 fi - + NUM_CPUS=8 if [ "$1" == "coverage" ]; then NUM_CPUS=6 fi fi -if grep 'docker\|lxc' /proc/1/cgroup; then + if grep 'docker\|lxc' /proc/1/cgroup; then # Create a fake home. Python site libs tries to do getpwuid(3) if we don't and the CI # Docker image gets confused as it has no passwd entry when running non-root # unless we do this. @@ -110,14 +110,14 @@ if grep 'docker\|lxc' /proc/1/cgroup; then mkdir -p "${FAKE_HOME}" export HOME="${FAKE_HOME}" export PYTHONUSERBASE="${FAKE_HOME}" - + export BUILD_DIR=/build if [[ ! -d "${BUILD_DIR}" ]] then echo "${BUILD_DIR} mount missing - did you forget -v :${BUILD_DIR}? Creating." mkdir -p "${BUILD_DIR}" fi - + # Environment setup. export USER=bazel export TEST_TMPDIR=/build/tmp From d98b80dd7c25f3f2c522947be07056ba68079179 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 11:23:23 +0200 Subject: [PATCH 14/29] Update README.md Signed-off-by: Otto van der Schaaf --- README.md | 50 ++++++++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index fae319315..e07afbd2f 100644 --- a/README.md +++ b/README.md @@ -41,28 +41,31 @@ bazel build -c opt //:nighthawk_client USAGE: - nighthawk_client [--max-requests-per-connection ] - [--max-active-requests ] - [--max-pending-requests ] [--tls-context - ] [--request-body-size ] - [--request-header ] ... [--request-method - ] - [--address-family ] [--burst-size - ] [--prefetch-connections] [--output-format - ] [-v ] [--concurrency ] [--h2] [--timeout - ] [--duration ] [--connections - ] [--rps ] [--] [--version] [-h] - + bazel-bin/nighthawk_client [--max-requests-per-connection ] + [--max-active-requests ] + [--max-pending-requests ] + [--tls-context ] + [--request-body-size ] + [--request-header ] ... + [--request-method ] [--address-family + ] [--burst-size ] + [--prefetch-connections] [--output-format + ] [-v ] [--concurrency + ] [--h2] [--timeout ] + [--duration ] [--connections + ] [--rps ] [--] + [--version] [-h] Where: --max-requests-per-connection - Max requests per connection (default: 2^31). + Max requests per connection (default: 4294937295). --max-active-requests - Max active requests (default: 2^31). + Max active requests (default: 4294937295). --max-pending-requests Max pending requests (default: 1, no client side queuing. Specifying @@ -89,9 +92,8 @@ Where: Network addres family preference. Possible values: [auto, v4, v6]. The default output format is 'v4'. - --burst-size - Release requests in bursts of the specified size (default: 0, no - bursting). + --burst-size + Release requests in bursts of the specified size (default: 0). --prefetch-connections Prefetch connections before benchmarking (HTTP/1 only). @@ -109,25 +111,25 @@ Where: The number of concurrent event loops that should be used. Specify 'auto' to let Nighthawk leverage all vCPUs that have affinity to the Nighthawk process.Note that increasing this results in an effective - load multiplier combined with the configured-- rps and --connections - values.Default : 1. + load multiplier combined with the configured --rps and --connections + values. Default: 1. --h2 Use HTTP/2 - --timeout + --timeout Timeout period in seconds used for both connection timeout and grace period waiting for lagging responses to come in after the test run is - done. Default: 5. + done. Default: 30. - --duration + --duration The number of seconds that the test should run. Default: 5. --connections The number of connections per event loop that the test should maximally use. HTTP/1 only. Default: 1. - --rps + --rps The target requests-per-second rate. Default: 5. --, --ignore_rest From bbd115575d505b0cf22f587e7a32676f856c29b6 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 11:26:06 +0200 Subject: [PATCH 15/29] benchmark_client_impl.cc: clean up diff Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 385a9f6c3..5bb50b0f4 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -160,7 +160,6 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { Envoy::Network::ConnectionSocket::OptionsSharedPtr options = std::make_shared(); - if (use_h2_) { pool_ = std::make_unique(dispatcher_, host, Envoy::Upstream::ResourcePriority::Default, options); @@ -172,7 +171,7 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { if (prefetch_connections_) { prefetchPoolConnections(); } -} // namespace Client +} void BenchmarkClientHttpImpl::terminate() { pool_.reset(); } From ad8b8e1ecb485a57e06ae8bcaa47bc790392f1a9 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 12:41:14 +0200 Subject: [PATCH 16/29] Another cleanup round Signed-off-by: Otto van der Schaaf --- test/factories_test.cc | 2 -- test/service_test.cc | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/test/factories_test.cc b/test/factories_test.cc index 652a98f0b..7bed5641a 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -32,7 +32,6 @@ class FactoriesTest : public Test { TEST_F(FactoriesTest, CreateBenchmarkClient) { BenchmarkClientFactoryImpl factory(options_); envoy::api::v2::auth::UpstreamTlsContext tls_context; - EXPECT_CALL(options_, timeout()).Times(1); EXPECT_CALL(options_, connections()).Times(1); EXPECT_CALL(options_, h2()).Times(1); @@ -43,7 +42,6 @@ TEST_F(FactoriesTest, CreateBenchmarkClient) { EXPECT_CALL(options_, maxPendingRequests()).Times(1); EXPECT_CALL(options_, maxActiveRequests()).Times(1); EXPECT_CALL(options_, maxRequestsPerConnection()).Times(1); - auto cmd = std::make_unique(); auto request_headers = cmd->mutable_request_options()->add_request_headers(); request_headers->mutable_header()->set_key("foo"); diff --git a/test/service_test.cc b/test/service_test.cc index ef528c557..ebb9ed660 100644 --- a/test/service_test.cc +++ b/test/service_test.cc @@ -43,9 +43,9 @@ class ServiceTest : public TestWithParam { void setBasicRequestOptions() { auto options = request_.mutable_start_request()->mutable_options(); - // XXX(oschaaf): Set defaults in the options header. - // See if we can get rid of the ones in TCLAP to disambiguate - // how we handle default values. + // TODO(oschaaf): this sends actual traffic, which isn't relevant for the tests + // we are about to perform. However, it would be nice to be able to mock out things + // to clean this up. options->mutable_uri()->set_value("http://127.0.0.1:10001/"); options->mutable_duration()->set_seconds(3); options->mutable_requests_per_second()->set_value(3); From ca64cfe27881ff80786011e63d87ff17f91bb0fd Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 13:28:33 +0200 Subject: [PATCH 17/29] Make sure we catch exception when parsing json Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 12 ++++++++---- test/benchmark_http_client_test.cc | 7 +++---- test/options_test.cc | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index fea914819..50772d899 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -18,6 +18,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { verbosity_ = "info"; output_format_ = "human"; + // TODO(oschaaf): Purge the validation we perform here. Most of it should have become + // redundant now that we also perform validation of the resulting proto. const char* descr = "L7 (HTTP/HTTPS/HTTP2) performance characterization tool."; TCLAP::CmdLine cmd(descr, ' ', "PoC"); // NOLINT @@ -242,15 +244,17 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { } if (!tls_context.getValue().empty()) { - // TODO(oschaaf): used to by loadFromJsonEx, which is now gone. - Envoy::MessageUtil::loadFromJson(tls_context.getValue(), tls_context_, - Envoy::ProtobufMessage::getNullValidationVisitor()); + try { + Envoy::MessageUtil::loadFromJson(tls_context.getValue(), tls_context_, + Envoy::ProtobufMessage::getNullValidationVisitor()); + } catch (Envoy::EnvoyException e) { + throw MalformedArgvException(e.what()); + } } validate(); } OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { - // XXX(oschaaf): as default composition isn't trivial, add tests to ensure all constructors setNonTrivialDefaults(); for (const auto& header : options.request_options().request_headers()) { diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index fca970ff2..28b34921b 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -398,10 +398,9 @@ TEST_P(BenchmarkClientHttpTest, ConnectionPrefetching) { EXPECT_EQ(50, getCounter("upstream_cx_total")); } -// XXX(oschaaf): Ok; so turns out this feature is h/2 only atm. Maybe want need to log a warning -// if someone configures this for h1. -// XXX(oschaaf): maybe we want to run parameterized tests running over -// h1/h2 and tls vs plain +// TODO(oschaaf): Turns out this feature is h/2 only as of writing this. +// Consider logging a warning/error when attempting to configure this with +// H1 enabled. TEST_P(BenchmarkClientHttpTest, CapRequestConcurrency) { setupBenchmarkClient("/lorem-ipsum-status-200", true, false); const uint64_t requests = 4; diff --git a/test/options_test.cc b/test/options_test.cc index b21f6546e..894165ce7 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -294,5 +294,21 @@ TEST_F(OptionsImplTest, ProtoConstructorValidation) { "CommandLineOptionsValidationError.RequestsPerSecond"); } +TEST_F(OptionsImplTest, BadTlsContextSpecification) { + // Bad JSON + EXPECT_THROW_WITH_REGEX(TestUtility::createOptionsImpl(fmt::format( + "{} --tls-context {} http://foo/", client_name_, "{broken_json:")), + MalformedArgvException, "Unable to parse JSON as proto"); + + // TODO(oschaaf): This should fail, but does not. Probably because we pass in a + // NullValidationVisitor. Correct that later on. + + // Correct JSON, but contents not according to spec. + // EXPECT_THROW_WITH_REGEX( + // TestUtility::createOptionsImpl(fmt::format("{} --tls-context {} http://foo/", client_name_, + // "{misspelled_tls_context:{}}")), + // MalformedArgvException, "Unable to parse JSON as proto"); +} + } // namespace Client } // namespace Nighthawk From e84cc0b34fa71a6069bf1ac44eaf38644b804538 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 23:20:14 +0200 Subject: [PATCH 18/29] Fix uint64_t's that pight to be be uint32_t Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index c63e7799b..8b96e964f 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -58,15 +58,15 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, bool prefetch_connections, envoy::api::v2::auth::UpstreamTlsContext tls_context); - void setConnectionLimit(uint64_t connection_limit) { connection_limit_ = connection_limit; } + void setConnectionLimit(uint32_t connection_limit) { connection_limit_ = connection_limit; } void setConnectionTimeout(std::chrono::seconds timeout) { timeout_ = timeout; } - void setMaxPendingRequests(uint64_t max_pending_requests) { + void setMaxPendingRequests(uint32_t max_pending_requests) { max_pending_requests_ = max_pending_requests; } - void setMaxActiveRequests(uint64_t max_active_requests) { + void setMaxActiveRequests(uint32_t max_active_requests) { max_active_requests_ = max_active_requests; } - void setMaxRequestsPerConnection(uint64_t max_requests_per_connection) { + void setMaxRequestsPerConnection(uint32_t max_requests_per_connection) { max_requests_per_connection_ = max_requests_per_connection; } From 698ede03e98a6d5cc874bb503e0e45c1b9774584 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 2 Jul 2019 23:59:44 +0200 Subject: [PATCH 19/29] Add & use TCLAP_SET_IF_SPECIFIED macro Plus some other minor changes addressing review feedback Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 73 +++++++++--------------------- test/benchmark_http_client_test.cc | 4 +- 2 files changed, 24 insertions(+), 53 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 50772d899..2a0e9919b 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -12,6 +12,9 @@ namespace Nighthawk { namespace Client { +#define TCLAP_SET_IF_SPECIFIED(command, value_member) \ + (value_member = (command).isSet() ? (command).getValue() : (value_member)) + OptionsImpl::OptionsImpl(int argc, const char* const* argv) { setNonTrivialDefaults(); // Override some defaults, we are in CLI-mode. @@ -161,60 +164,28 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { throw NoServingException(); } - if (requests_per_second.isSet()) { - requests_per_second_ = requests_per_second.getValue(); - } - if (connections.isSet()) { - connections_ = connections.getValue(); - } - if (duration.isSet()) { - duration_ = duration.getValue(); - } - if (timeout.isSet()) { - timeout_ = timeout.getValue(); - } + TCLAP_SET_IF_SPECIFIED(requests_per_second, requests_per_second_); + TCLAP_SET_IF_SPECIFIED(connections, connections_); + TCLAP_SET_IF_SPECIFIED(duration, duration_); + TCLAP_SET_IF_SPECIFIED(timeout, timeout_); uri_ = uri.getValue(); - if (h2.isSet()) { - h2_ = h2.getValue(); - } - if (concurrency.isSet()) { - concurrency_ = concurrency.getValue(); - } - if (verbosity.isSet()) { - verbosity_ = verbosity.getValue(); - } - if (output_format.isSet()) { - output_format_ = output_format.getValue(); - } - if (prefetch_connections.isSet()) { - prefetch_connections_ = prefetch_connections.getValue(); - } - if (burst_size.isSet()) { - burst_size_ = burst_size.getValue(); - } - if (address_family.isSet()) { - address_family_ = address_family.getValue(); - } - if (request_method.isSet()) { - request_method_ = request_method.getValue(); - } - if (request_headers.isSet()) { - request_headers_ = request_headers.getValue(); - } - if (request_body_size.isSet()) { - request_body_size_ = request_body_size.getValue(); - } - if (max_pending_requests.isSet()) { - max_pending_requests_ = max_pending_requests.getValue(); - } - if (max_active_requests.isSet()) { - max_active_requests_ = max_active_requests.getValue(); - } - if (max_requests_per_connection.isSet()) { - max_requests_per_connection_ = max_requests_per_connection.getValue(); - } + TCLAP_SET_IF_SPECIFIED(h2, h2_); + TCLAP_SET_IF_SPECIFIED(concurrency, concurrency_); + TCLAP_SET_IF_SPECIFIED(verbosity, verbosity_); + TCLAP_SET_IF_SPECIFIED(output_format, output_format_); + TCLAP_SET_IF_SPECIFIED(prefetch_connections, prefetch_connections_); + TCLAP_SET_IF_SPECIFIED(burst_size, burst_size_); + TCLAP_SET_IF_SPECIFIED(address_family, address_family_); + TCLAP_SET_IF_SPECIFIED(request_method, request_method_); + TCLAP_SET_IF_SPECIFIED(request_headers, request_headers_); + TCLAP_SET_IF_SPECIFIED(request_body_size, request_body_size_); + TCLAP_SET_IF_SPECIFIED(max_pending_requests, max_pending_requests_); + TCLAP_SET_IF_SPECIFIED(max_active_requests, max_active_requests_); + TCLAP_SET_IF_SPECIFIED(max_requests_per_connection, max_requests_per_connection_); // CLI-specific tests. + // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate + // these and present everything we couldn't understand to the CLI user in on go. if (requests_per_second_ > largest_acceptable_uint32_option_value) { throw MalformedArgvException("Invalid value for --rps"); } diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 28b34921b..aa3cc9248 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -406,7 +406,7 @@ TEST_P(BenchmarkClientHttpTest, CapRequestConcurrency) { const uint64_t requests = 4; uint64_t inflight_response_count = requests; - // We configure so that max requests is the only thing that can be throttling. + // We configure so that max requests is the only thing that can be throttling. client_->setMaxPendingRequests(requests); client_->setConnectionLimit(requests); client_->setMaxActiveRequests(1); @@ -433,7 +433,7 @@ TEST_P(BenchmarkClientHttpsTest, MaxRequestsPerConnection) { const uint64_t requests = 4; uint64_t inflight_response_count = requests; - // We configure so that max requests is the only thing that can be throttling. + // We configure so that max requests is the only thing that can be throttling. client_->setMaxPendingRequests(requests); client_->setConnectionLimit(requests); client_->setMaxActiveRequests(1024); From 7bfa2fc40ffc26ab7a03fbffec16a4e0afa27ecc Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 3 Jul 2019 09:16:06 +0200 Subject: [PATCH 20/29] Process review feedback - Pass in the strict validation visitor instead of the NOP implementation - Uncomment a test to prove validation works - Audit NullValidationVisitor everywhere & fix Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 5 ++--- source/client/options_impl.cc | 4 ++-- source/server/http_test_server_filter.cc | 3 +-- test/options_test.cc | 12 ++++-------- test/statistic_test.cc | 2 +- 5 files changed, 10 insertions(+), 16 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 5bb50b0f4..6bafa5e9f 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -122,10 +122,9 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { ssl_context_manager_ = std::make_unique( api_.timeSource()); - // TODO: pass in the right validation visitor transport_socket_factory_context_ = std::make_unique( store_.createScope("client."), dispatcher_, generator_, store_, api_, *ssl_context_manager_, - Envoy::ProtobufMessage::getNullValidationVisitor()); + Envoy::ProtobufMessage::getStrictValidationVisitor()); Envoy::ProtobufTypes::MessagePtr message = Envoy::Config::Utility::translateToFactoryConfig( transport_socket, transport_socket_factory_context_->messageValidationVisitor(), @@ -147,7 +146,7 @@ void BenchmarkClientHttpImpl::initialize(Envoy::Runtime::Loader& runtime) { cluster_ = std::make_unique( cluster_config, bind_config, runtime, std::move(socket_factory), store_.createScope("client."), false /*added_via_api*/, - Envoy::ProtobufMessage::getNullValidationVisitor()); + Envoy::ProtobufMessage::getStrictValidationVisitor()); ASSERT(uri_->address() != nullptr); diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 2a0e9919b..0e3e91821 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -13,7 +13,7 @@ namespace Nighthawk { namespace Client { #define TCLAP_SET_IF_SPECIFIED(command, value_member) \ - (value_member = (command).isSet() ? (command).getValue() : (value_member)) + (value_member = ((command.isSet()) ? (command.getValue()) : (value_member))) OptionsImpl::OptionsImpl(int argc, const char* const* argv) { setNonTrivialDefaults(); @@ -217,7 +217,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { if (!tls_context.getValue().empty()) { try { Envoy::MessageUtil::loadFromJson(tls_context.getValue(), tls_context_, - Envoy::ProtobufMessage::getNullValidationVisitor()); + Envoy::ProtobufMessage::getStrictValidationVisitor()); } catch (Envoy::EnvoyException e) { throw MalformedArgvException(e.what()); } diff --git a/source/server/http_test_server_filter.cc b/source/server/http_test_server_filter.cc index 5faefb848..2db6fedd1 100644 --- a/source/server/http_test_server_filter.cc +++ b/source/server/http_test_server_filter.cc @@ -29,9 +29,8 @@ bool HttpTestServerDecoderFilter::mergeJsonConfig(absl::string_view json, error_message = absl::nullopt; try { nighthawk::server::ResponseOptions json_config; - // TODO(oschaaf): pass in the right ValidationVisitor type. Envoy::MessageUtil::loadFromJson(std::string(json), json_config, - Envoy::ProtobufMessage::getNullValidationVisitor()); + Envoy::ProtobufMessage::getStrictValidationVisitor()); config.MergeFrom(json_config); Envoy::MessageUtil::validate(config); } catch (Envoy::EnvoyException exception) { diff --git a/test/options_test.cc b/test/options_test.cc index 894165ce7..ba48be62f 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -299,15 +299,11 @@ TEST_F(OptionsImplTest, BadTlsContextSpecification) { EXPECT_THROW_WITH_REGEX(TestUtility::createOptionsImpl(fmt::format( "{} --tls-context {} http://foo/", client_name_, "{broken_json:")), MalformedArgvException, "Unable to parse JSON as proto"); - - // TODO(oschaaf): This should fail, but does not. Probably because we pass in a - // NullValidationVisitor. Correct that later on. - // Correct JSON, but contents not according to spec. - // EXPECT_THROW_WITH_REGEX( - // TestUtility::createOptionsImpl(fmt::format("{} --tls-context {} http://foo/", client_name_, - // "{misspelled_tls_context:{}}")), - // MalformedArgvException, "Unable to parse JSON as proto"); + EXPECT_THROW_WITH_REGEX( + TestUtility::createOptionsImpl(fmt::format("{} --tls-context {} http://foo/", client_name_, + "{misspelled_tls_context:{}}")), + MalformedArgvException, "envoy.api.v2.auth.UpstreamTlsContext reason INVALID_ARGUMENT"); } } // namespace Client diff --git a/test/statistic_test.cc b/test/statistic_test.cc index d9e5e7c5d..01176d888 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -259,7 +259,7 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { Envoy::MessageUtil util; util.loadFromJson( filesystem.fileReadToEnd(TestEnvironment::runfilesPath("test/test_data/hdr_proto_json.gold")), - parsed_json_proto, Envoy::ProtobufMessage::getNullValidationVisitor()); + parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); EXPECT_TRUE(util(parsed_json_proto, statistic.toProto())); } From a1cf135107d2b6b4c5d181b0716629be8721cb89 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 3 Jul 2019 10:08:56 +0200 Subject: [PATCH 21/29] Tidy up macro definition Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 0e3e91821..90207d25c 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -13,7 +13,7 @@ namespace Nighthawk { namespace Client { #define TCLAP_SET_IF_SPECIFIED(command, value_member) \ - (value_member = ((command.isSet()) ? (command.getValue()) : (value_member))) + ((value_member) = (((command).isSet()) ? ((command).getValue()) : (value_member))) OptionsImpl::OptionsImpl(int argc, const char* const* argv) { setNonTrivialDefaults(); From 94c5ffbe54f31a1979ebe259c6800cd12a4285bd Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 3 Jul 2019 10:57:16 +0200 Subject: [PATCH 22/29] Add test for uint32_t TCLAP parsing range Signed-off-by: Otto van der Schaaf --- source/client/options_impl.h | 7 ++++--- test/options_test.cc | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/source/client/options_impl.h b/source/client/options_impl.h index 6aad947dd..ca84ff3eb 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -13,6 +13,10 @@ namespace Client { class OptionsImpl : public Options { public: + // We cap on negative values. TCLAP accepts negative values which we will get here as very + // large values. We just cap values, hoping we catch accidental wraparound to a reasonable extent. + static constexpr uint32_t largest_acceptable_uint32_option_value = UINT32_MAX - 30000; + OptionsImpl(int argc, const char* const* argv); OptionsImpl(const nighthawk::client::CommandLineOptions& options); Client::CommandLineOptionsPtr toCommandLineOptions() const override; @@ -43,9 +47,6 @@ class OptionsImpl : public Options { void setNonTrivialDefaults(); void validate() const; - // We cap on negative values. TCLAP accepts negative values which we will get here as very - // large values. We just cap values, hoping we catch accidental wraparound to a reasonable extent. - static constexpr uint32_t largest_acceptable_uint32_option_value = UINT32_MAX - 30000; uint32_t requests_per_second_{5}; uint32_t connections_{1}; uint32_t duration_{5}; diff --git a/test/options_test.cc b/test/options_test.cc index ba48be62f..d762a49a5 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -211,6 +211,16 @@ TEST_F(OptionsImplTest, BadConcurrencyValuesThrow) { MalformedArgvException, "Value out of range: --concurrency"); } +// Test a relatively large uint value to see if we can get reasonable range +// when we specced a uint32_t +// See https://github.com/envoyproxy/nighthawk/pull/88/files#r299572672 +TEST_F(OptionsImplTest, ParserIntRangeTest) { + const uint32_t test_value = OptionsImpl::largest_acceptable_uint32_option_value; + std::unique_ptr options = TestUtility::createOptionsImpl(fmt::format( + "{} --max-requests-per-connection {} {} ", client_name_, test_value, good_test_uri_)); + EXPECT_EQ(test_value, options->maxRequestsPerConnection()); +} + // Test we accept --concurrency auto TEST_F(OptionsImplTest, AutoConcurrencyValueParsedOK) { std::unique_ptr options = TestUtility::createOptionsImpl( From 8cdbb733fe45b0bcc1060cc700287e8c6bd879d8 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 8 Jul 2019 12:14:01 +0200 Subject: [PATCH 23/29] Open- & closed-loop: measure blocking Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 6bafa5e9f..cda1038fd 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -189,16 +189,18 @@ void BenchmarkClientHttpImpl::setRequestHeader(absl::string_view key, absl::stri } bool BenchmarkClientHttpImpl::tryStartOne(std::function caller_completion_callback) { + if (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default) + .pendingRequests() + .canCreate()) { + return false; + } // When no client side queueing is specified (via max_pending_requests_), we are in closed loop // mode. In closed loop mode we want to be able to control the pacing as exactly as possible. In // open-loop mode we probably want to skip this. NOTE(oschaaf): We can't consistently rely on // resourceManager()::requests() because that isn't used for h/1 (it is used in tcp and h2 // though). - if (max_pending_requests_ == 1 && - (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default) - .pendingRequests() - .canCreate() || - ((requests_initiated_ - requests_completed_) >= connection_limit_))) { + if ((max_pending_requests_ == 1 && + (requests_initiated_ - requests_completed_) >= connection_limit_)) { return false; } From 08ba684d1bbb0652778a88ecea1cd5fde9a45b3c Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 20:18:30 +0200 Subject: [PATCH 24/29] Update comments Signed-off-by: Otto van der Schaaf --- source/client/benchmark_client_impl.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index cda1038fd..1de45bbea 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -189,16 +189,17 @@ void BenchmarkClientHttpImpl::setRequestHeader(absl::string_view key, absl::stri } bool BenchmarkClientHttpImpl::tryStartOne(std::function caller_completion_callback) { + // When we allow client-side queuing, we want to have a sense of time spend waiting on that queue. + // So we return false here to indicate we couldn't initiate a new request. if (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default) .pendingRequests() .canCreate()) { return false; } - // When no client side queueing is specified (via max_pending_requests_), we are in closed loop - // mode. In closed loop mode we want to be able to control the pacing as exactly as possible. In - // open-loop mode we probably want to skip this. NOTE(oschaaf): We can't consistently rely on - // resourceManager()::requests() because that isn't used for h/1 (it is used in tcp and h2 - // though). + // When no client side queueing is disabled (max_pending equals 1) we control the pacing as + // exactly as possible here. + // NOTE: We can't consistently rely on resourceManager()::requests() + // because that isn't used for h/1 (it is used in tcp and h2 though). if ((max_pending_requests_ == 1 && (requests_initiated_ - requests_completed_) >= connection_limit_)) { return false; From 3eff8714bf5e0f072af48e3f9e8dbcabe2ea019c Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 21:04:06 +0200 Subject: [PATCH 25/29] Add integration test Signed-off-by: Otto van der Schaaf --- integration/integration_test_fixtures.py | 9 +++++++++ integration/test_integration_basics.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/integration/integration_test_fixtures.py b/integration/integration_test_fixtures.py index 9b134f576..342a7e7d9 100644 --- a/integration/integration_test_fixtures.py +++ b/integration/integration_test_fixtures.py @@ -80,6 +80,15 @@ def getNighthawkCounterMapFromJson(self, parsed_json): for counter in parsed_json["results"][0]["counters"] } + def getNighthawkGlobalHistogramsbyIdFromJson(self, parsed_json): + """ + Utility method to get the global histograms from the json indexed by id. + """ + return { + statistic["id"]: statistic + for statistic in parsed_json["results"][0]["statistics"] + } + def getTestServerRootUri(self, https=False): """ Utility for getting the http://host:port/ that can be used to query the server we started in setUp() diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index 84d34902f..8dfeeddae 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -29,6 +29,26 @@ def test_h1(self): self.assertEqual(counters["upstream_rq_total"], 25) self.assertEqual(len(counters), 9) + def mini_stress_test_h1(self, args): + # run a test with more rps then we can handle, and a very small client-side queue. + # we should observe both lots of successfull requests as well as time spend in blocking mode., + parsed_json = self.runNighthawkClient(args) + counters = self.getNighthawkCounterMapFromJson(parsed_json) + self.assertGreater(counters["benchmark.http_2xx"], 1000) + self.assertEqual(counters["upstream_cx_http1_total"], 1) + global_histograms = self.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) + self.assertGreater(int(global_histograms["sequencer.blocking"]["count"]), 1000) + self.assertGreater(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1000) + return counters + + def test_h1_mini_stress_test_with_client_side_queueing(self): + counters = self.mini_stress_test_h1([self.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", "--duration 2"]) + self.assertGreater(counters["upstream_rq_pending_total"], 100) + + def test_h1_mini_stress_test_without_client_side_queueing(self): + counters = self.mini_stress_test_h1([self.getTestServerRootUri(), "--rps", "999999", "--duration 2"]) + self.assertEqual(counters["upstream_rq_pending_total"], 1) + def test_h2(self): parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) From 204f3336cc716107e188188822f50c8a3a4e4ed7 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 21:06:53 +0200 Subject: [PATCH 26/29] Python formatting Signed-off-by: Otto van der Schaaf --- integration/integration_test_fixtures.py | 5 +---- integration/test_integration_basics.py | 15 ++++++++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/integration/integration_test_fixtures.py b/integration/integration_test_fixtures.py index 342a7e7d9..79f3bbf5f 100644 --- a/integration/integration_test_fixtures.py +++ b/integration/integration_test_fixtures.py @@ -84,10 +84,7 @@ def getNighthawkGlobalHistogramsbyIdFromJson(self, parsed_json): """ Utility method to get the global histograms from the json indexed by id. """ - return { - statistic["id"]: statistic - for statistic in parsed_json["results"][0]["statistics"] - } + return {statistic["id"]: statistic for statistic in parsed_json["results"][0]["statistics"]} def getTestServerRootUri(self, https=False): """ diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index 8dfeeddae..aac0e0d3c 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -31,22 +31,27 @@ def test_h1(self): def mini_stress_test_h1(self, args): # run a test with more rps then we can handle, and a very small client-side queue. - # we should observe both lots of successfull requests as well as time spend in blocking mode., + # we should observe both lots of successfull requests as well as time spend in blocking mode., parsed_json = self.runNighthawkClient(args) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertGreater(counters["benchmark.http_2xx"], 1000) self.assertEqual(counters["upstream_cx_http1_total"], 1) global_histograms = self.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) self.assertGreater(int(global_histograms["sequencer.blocking"]["count"]), 1000) - self.assertGreater(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1000) + self.assertGreater( + int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1000) return counters def test_h1_mini_stress_test_with_client_side_queueing(self): - counters = self.mini_stress_test_h1([self.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", "--duration 2"]) + counters = self.mini_stress_test_h1([ + self.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", + "--duration 2" + ]) self.assertGreater(counters["upstream_rq_pending_total"], 100) - + def test_h1_mini_stress_test_without_client_side_queueing(self): - counters = self.mini_stress_test_h1([self.getTestServerRootUri(), "--rps", "999999", "--duration 2"]) + counters = self.mini_stress_test_h1( + [self.getTestServerRootUri(), "--rps", "999999", "--duration 2"]) self.assertEqual(counters["upstream_rq_pending_total"], 1) def test_h2(self): From f4dc3de19faf9313058cd05b0b58bfc91abe580d Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 23:47:18 +0200 Subject: [PATCH 27/29] Add PyDoc comments, add TODO + more of expecations Signed-off-by: Otto van der Schaaf --- integration/test_integration_basics.py | 44 +++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index aac0e0d3c..aa1a76ef8 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -10,11 +10,16 @@ IntegrationTestBase) # TODO(oschaaf): rewrite the tests so we can just hand a map of expected key values to it. - +# TODO(oschaaf): we mostly verify stats observed from the client-side. Add expectations +# for the server side as well. class TestHttp(HttpIntegrationTestBase): def test_h1(self): + """ + Runs the CLI configured to use plain HTTP/1 against our test server, and sanity + checks statistics from both client and server. + """ parsed_json = self.runNighthawkClient([self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) @@ -43,18 +48,39 @@ def mini_stress_test_h1(self, args): return counters def test_h1_mini_stress_test_with_client_side_queueing(self): + """ + Run a max rps test with the h1 pool against our test server, using a small client-side + queue. We expect to observe: + - upstream_rq_pending_total increasing + - upstream_cx_overflow overflows + - blocking to be reported by the sequencer + """ counters = self.mini_stress_test_h1([ self.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", "--duration 2" ]) self.assertGreater(counters["upstream_rq_pending_total"], 100) + self.assertGreater(counters["upstream_cx_overflow"], 0) def test_h1_mini_stress_test_without_client_side_queueing(self): + """ + Run a max rps test with the h1 pool against our test server, with no client-side + queueing. We expect to observe: + - upstream_rq_pending_total to be equal to 1 + - blocking to be reported by the sequencer + - no upstream_cx_overflows + """ counters = self.mini_stress_test_h1( [self.getTestServerRootUri(), "--rps", "999999", "--duration 2"]) self.assertEqual(counters["upstream_rq_pending_total"], 1) + self.assertFalse("upstream_cx_overflow" in counters) + def test_h2(self): + """ + Runs the CLI configured to use h2c against our test server, and sanity + checks statistics from both client and server. + """ parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) @@ -72,6 +98,10 @@ def test_h2(self): class TestHttps(HttpsIntegrationTestBase): def test_h1(self): + """ + Runs the CLI configured to use HTTP/1 over https against our test server, and sanity + checks statistics from both client and server. + """ parsed_json = self.runNighthawkClient([self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) @@ -96,6 +126,11 @@ def test_h1(self): self.getServerStatFromJson(server_stats, "http.ingress_http.downstream_rq_2xx"), 25) def test_h2(self): + """ + Runs the CLI configured to use HTTP/2 (using https) against our test server, and sanity + checks statistics from both client and server. + """ + parsed_json = self.runNighthawkClient(["--h2", self.getTestServerRootUri()]) counters = self.getNighthawkCounterMapFromJson(parsed_json) self.assertEqual(counters["benchmark.http_2xx"], 25) @@ -116,6 +151,10 @@ def test_h2(self): def test_h1_tls_context_configuration(self): + """ + Verifies specifying tls cipher suites works with the h1 pool + """ + parsed_json = self.runNighthawkClient([ "--duration 1", "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", @@ -134,6 +173,9 @@ def test_h1_tls_context_configuration(self): def test_h2_tls_context_configuration(self): + """ + Verifies specifying tls cipher suites works with the h2 pool + """ parsed_json = self.runNighthawkClient([ "--duration 1", "--tls-context {common_tls_context:{tls_params:{cipher_suites:[\"-ALL:ECDHE-RSA-AES128-SHA\"]}}}", From f7e3cdaab01eb5abfa997091353db410e5001090 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 23:55:11 +0200 Subject: [PATCH 28/29] Define a constant for reuse x-expecations Signed-off-by: Otto van der Schaaf --- integration/test_integration_basics.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index aa1a76ef8..0974c2fc7 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -39,12 +39,15 @@ def mini_stress_test_h1(self, args): # we should observe both lots of successfull requests as well as time spend in blocking mode., parsed_json = self.runNighthawkClient(args) counters = self.getNighthawkCounterMapFromJson(parsed_json) - self.assertGreater(counters["benchmark.http_2xx"], 1000) + # We set a reasonably low expecation of 1000 requests. We set it low, because we want this + # test to succeed on a reasonable share of setups (hopefully practically all). + MIN_EXPECTED_REQUESTS = 1000 + self.assertGreater(counters["benchmark.http_2xx"], MIN_EXPECTED_REQUESTS) self.assertEqual(counters["upstream_cx_http1_total"], 1) global_histograms = self.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) - self.assertGreater(int(global_histograms["sequencer.blocking"]["count"]), 1000) + self.assertGreater(int(global_histograms["sequencer.blocking"]["count"]), MIN_EXPECTED_REQUESTS) self.assertGreater( - int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1000) + int(global_histograms["benchmark_http_client.request_to_response"]["count"]), MIN_EXPECTED_REQUESTS) return counters def test_h1_mini_stress_test_with_client_side_queueing(self): From a25af0763e7408c4aa121cd684146e23b02d23d2 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 11 Jul 2019 23:59:37 +0200 Subject: [PATCH 29/29] Amend formatting issues Signed-off-by: Otto van der Schaaf --- integration/test_integration_basics.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/integration/test_integration_basics.py b/integration/test_integration_basics.py index 0974c2fc7..ad90ae9f9 100644 --- a/integration/test_integration_basics.py +++ b/integration/test_integration_basics.py @@ -13,6 +13,7 @@ # TODO(oschaaf): we mostly verify stats observed from the client-side. Add expectations # for the server side as well. + class TestHttp(HttpIntegrationTestBase): def test_h1(self): @@ -47,7 +48,8 @@ def mini_stress_test_h1(self, args): global_histograms = self.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) self.assertGreater(int(global_histograms["sequencer.blocking"]["count"]), MIN_EXPECTED_REQUESTS) self.assertGreater( - int(global_histograms["benchmark_http_client.request_to_response"]["count"]), MIN_EXPECTED_REQUESTS) + int(global_histograms["benchmark_http_client.request_to_response"]["count"]), + MIN_EXPECTED_REQUESTS) return counters def test_h1_mini_stress_test_with_client_side_queueing(self): @@ -78,7 +80,6 @@ def test_h1_mini_stress_test_without_client_side_queueing(self): self.assertEqual(counters["upstream_rq_pending_total"], 1) self.assertFalse("upstream_cx_overflow" in counters) - def test_h2(self): """ Runs the CLI configured to use h2c against our test server, and sanity