Allow disabling default failure predicates. (#997)

Works on #826 

Signed-off-by: Dan Rosen <mergeconflict@google.com>

README.md

@@ -168,11 +168,12 @@ bazel-bin/nighthawk_client  [--user-defined-plugin-config <string>] ...
 [--nighthawk-service <uri format>]
 [--jitter-uniform <duration>] [--open-loop]
 [--experimental-h1-connection-reuse-strategy
-<mru|lru>] [--failure-predicate <string,
-uint64_t>] ... [--termination-predicate
-<string, uint64_t>] ... [--trace <uri
-format>] [--sequencer-idle-strategy <spin
-|poll|sleep>] [--max-concurrent-streams
+<mru|lru>] [--no-default-failure-predicates]
+[--failure-predicate <string, uint64_t>] ...
+[--termination-predicate <string, uint64_t>]
+... [--trace <uri format>]
+[--sequencer-idle-strategy <spin|poll
+|sleep>] [--max-concurrent-streams
 <uint32_t>] [--max-requests-per-connection
 <uint32_t>] [--max-active-requests
 <uint32_t>] [--max-pending-requests
@@ -295,6 +296,11 @@ Choose picking the most recently used, or least-recently-used
 connections for re-use.(default: mru). WARNING: this option is
 experimental and may be removed or changed in the future!
 
+--no-default-failure-predicates
+Disables the default failure predicates, indicating that Nighthawk
+should continue sending load after observing error status codes and
+connection errors.
+
 --failure-predicate <string, uint64_t>  (accepted multiple times)
 Failure predicate. Allows specifying a counter name plus threshold
 value for failing execution. Defaults to not tolerating error status

api/client/options.proto

@@ -129,7 +129,7 @@ message Protocol {
 
 // 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.
-// Next unused number is 113.
+// Next unused number is 114.
 message CommandLineOptions {
   // The target requests-per-second rate. Default: 5.
   google.protobuf.UInt32Value requests_per_second = 1
@@ -228,6 +228,9 @@ message CommandLineOptions {
   // Allows specifying a counter name plus threshold value for failing execution. Defaults to not
   // tolerating error status codes and connection errors.
   map<string, uint64> failure_predicates = 24;
+  // Disables the default failure predicates, indicating that Nighthawk should continue sending load
+  // after observing error status codes and connection errors.
+  google.protobuf.BoolValue no_default_failure_predicates = 113;
   // Enable open loop mode. When enabled, the benchmark client will not provide backpressure when
   // resource limits are hit.
   google.protobuf.BoolValue open_loop = 21;

include/nighthawk/client/options.h

@@ -82,6 +82,7 @@ class Options {
   h1ConnectionReuseStrategy() const PURE;
   virtual TerminationPredicateMap terminationPredicates() const PURE;
   virtual TerminationPredicateMap failurePredicates() const PURE;
+  virtual bool noDefaultFailurePredicates() const PURE;
   virtual bool openLoop() const PURE;
   virtual std::chrono::nanoseconds jitterUniform() const PURE;
   virtual std::string nighthawkService() const PURE;

source/client/options_impl.cc

@@ -236,6 +236,11 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
       "failing execution. Defaults to not tolerating error status codes and connection errors. "
       "Example: benchmark.http_5xx:4294967295.",
       false, "string, uint64_t", cmd);
+  TCLAP::SwitchArg no_default_failure_predicates(
+      "", "no-default-failure-predicates",
+      "Disables the default failure predicates, indicating that Nighthawk should continue sending "
+      "load after observing error status codes and connection errors.",
+      cmd);
 
   std::vector<std::string> h1_connection_reuse_strategies = {"mru", "lru"};
   TCLAP::ValuesConstraint<std::string> h1_connection_reuse_strategies_allowed(
@@ -505,6 +510,10 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
 
   TCLAP_SET_IF_SPECIFIED(trace, trace_);
   parsePredicates(termination_predicates, termination_predicates_);
+  TCLAP_SET_IF_SPECIFIED(no_default_failure_predicates, no_default_failure_predicates_);
+  if (no_default_failure_predicates_) {
+    failure_predicates_.clear();
+  }
   parsePredicates(failure_predicates, failure_predicates_);
   TCLAP_SET_IF_SPECIFIED(open_loop, open_loop_);
   if (jitter_uniform.isSet()) {
@@ -793,7 +802,10 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
     transport_socket_.value().MergeFrom(options.transport_socket());
   }
 
-  if (!options.failure_predicates().empty()) {
+  if (options.has_no_default_failure_predicates()) {
+    no_default_failure_predicates_ = options.no_default_failure_predicates().value();
+  }
+  if (no_default_failure_predicates_ || !options.failure_predicates().empty()) {
     failure_predicates_.clear();
   }
   for (const auto& predicate : options.failure_predicates()) {
@@ -1014,6 +1026,8 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const {
   for (const auto& predicate : failure_predicates_) {
     failure_predicates_option->insert({predicate.first, predicate.second});
   }
+  command_line_options->mutable_no_default_failure_predicates()->set_value(
+      no_default_failure_predicates_);
   command_line_options->mutable_open_loop()->set_value(open_loop_);
   if (jitter_uniform_.count() > 0) {
     *command_line_options->mutable_jitter_uniform() =

source/client/options_impl.h

@@ -88,6 +88,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   }
   TerminationPredicateMap terminationPredicates() const override { return termination_predicates_; }
   TerminationPredicateMap failurePredicates() const override { return failure_predicates_; }
+  bool noDefaultFailurePredicates() const override { return no_default_failure_predicates_; }
   bool openLoop() const override { return open_loop_; }
 
   std::chrono::nanoseconds jitterUniform() const override { return jitter_uniform_; }
@@ -167,6 +168,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
       experimental_h1_connection_reuse_strategy_{nighthawk::client::H1ConnectionReuseStrategy::MRU};
   TerminationPredicateMap termination_predicates_;
   TerminationPredicateMap failure_predicates_;
+  bool no_default_failure_predicates_{false};
   bool open_loop_{false};
   std::chrono::nanoseconds jitter_uniform_;
   std::string nighthawk_service_;

test/integration/test_integration_basics.py

@@ -647,6 +647,23 @@ def test_http_h1_failure_predicate(http_test_server_fixture):
   asserts.assertCounterEqual(counters, "benchmark.http_2xx", 1)
 
 
+def test_http_h1_no_default_failure_predicates(http_test_server_fixture):
+  """Test with no default failure predicates.
+
+  Point the client at a bogus port, which causes an immediate pool connection failure, but disable
+  the default failure predicates. By default, without --no-default-failure-predicates, this would
+  cause the Nighthawk client to fail immediately. With the flag set, we expect successful execution
+  despite the error.
+  """
+  root_uri = utility.replace_port(http_test_server_fixture.getTestServerRootUri(), 123)
+  parsed_json, _ = http_test_server_fixture.runNighthawkClient([
+      root_uri, "--duration", "5", "--rps", "500", "--connections", "1",
+      "--no-default-failure-predicates"
+  ])
+  counters = http_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json)
+  asserts.assertCounterEqual(counters, "benchmark.pool_connection_failure", 1)
+
+
 def test_bad_arg_error_messages(http_test_server_fixture):
   """Test arguments that pass proto validation, but are found to be no good nonetheless, result in reasonable error messages."""
   _, err = http_test_server_fixture.runNighthawkClient(

test/integration/unit_tests/test_utility.py

@@ -102,5 +102,11 @@ def test_parse_uris_to_socket_address():
   assert v6_address.ip == "2001:db8:3333:4444:CCCC:DDDD:EEEE:FFFF" and v6_address.port == 9001
 
 
+def test_replace_port():
+  """Test replace port for both ipv4 and ipv6."""
+  assert utility.replace_port("http://0.0.0.0:8080/", 123) == "http://0.0.0.0:123/"
+  assert utility.replace_port("http://[::]:8080/", 123) == "http://[::]:123/"
+
+
 if __name__ == "__main__":
   raise SystemExit(pytest.main([__file__]))

test/integration/utility.py

@@ -1,6 +1,7 @@
 """Packages utility methods for tests."""
 
 import os
+import re
 import subprocess
 import string
 from typing import Union
@@ -138,3 +139,16 @@ def substitute_yaml_values(runfiles_instance, obj: Union[dict, list, str], param
       with open(runfiles_instance.Rlocation(obj[len(INJECT_RUNFILE_MARKER):].strip()), 'r') as file:
         return file.read()
   return obj
+
+
+def replace_port(root_uri: str, port: int) -> str:
+  """Replace a port number in an existing root URI with a new port number.
+
+  Args:
+    root_uri: A root uri of a Nighthawk test server, expected to contain a port.
+    port: A new port to substitute.
+
+  Returns:
+    str: A new root uri.
+  """
+  return re.sub(":[0-9]+([^:]*)", ":%d\\1" % port, root_uri)

test/mocks/client/mock_options.h

@@ -52,6 +52,7 @@ class MockOptions : public Options {
               h1ConnectionReuseStrategy, (), (const, override));
   MOCK_METHOD(TerminationPredicateMap, terminationPredicates, (), (const, override));
   MOCK_METHOD(TerminationPredicateMap, failurePredicates, (), (const, override));
+  MOCK_METHOD(bool, noDefaultFailurePredicates, (), (const, override));
   MOCK_METHOD(bool, openLoop, (), (const, override));
   MOCK_METHOD(std::chrono::nanoseconds, jitterUniform, (), (const, override));
   MOCK_METHOD(std::string, nighthawkService, (), (const, override));

test/options_test.cc

@@ -205,7 +205,7 @@ TEST_F(OptionsImplTest, AlmostAll) {
       "--max-pending-requests 10 "
       "--max-active-requests 11 --max-requests-per-connection 12 --sequencer-idle-strategy sleep "
       "--termination-predicate t1:1 --termination-predicate t2:2 --failure-predicate f1:1 "
-      "--failure-predicate f2:2 --jitter-uniform .00001s "
+      "--failure-predicate f2:2 --no-default-failure-predicates --jitter-uniform .00001s "
       "--max-concurrent-streams 42 "
       "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {} "
       "--simple-warmup --stats-sinks {} --stats-sinks {} --stats-flush-interval 10 "
@@ -260,6 +260,7 @@ TEST_F(OptionsImplTest, AlmostAll) {
   ASSERT_EQ(2, options->failurePredicates().size());
   EXPECT_EQ(1, options->failurePredicates()["f1"]);
   EXPECT_EQ(2, options->failurePredicates()["f2"]);
+  EXPECT_TRUE(options->noDefaultFailurePredicates());
   EXPECT_EQ(10us, options->jitterUniform());
   EXPECT_EQ(42, options->maxConcurrentStreams());
   EXPECT_EQ(nighthawk::client::H1ConnectionReuseStrategy::LRU,
@@ -337,6 +338,7 @@ TEST_F(OptionsImplTest, AlmostAll) {
   ASSERT_EQ(2, cmd->failure_predicates_size());
   EXPECT_EQ(cmd->failure_predicates().at("f1"), 1);
   EXPECT_EQ(cmd->failure_predicates().at("f2"), 2);
+  EXPECT_TRUE(cmd->no_default_failure_predicates().value());
 
   // 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,
@@ -905,6 +907,12 @@ TEST_F(OptionsImplTest, AutoConcurrencyValueParsedOK) {
   EXPECT_EQ("auto", options->concurrency());
 }
 
+TEST_F(OptionsImplTest, NoDefaultFailurePredicates) {
+  std::unique_ptr<OptionsImpl> options = TestUtility::createOptionsImpl(
+      fmt::format("{} --no-default-failure-predicates {}", client_name_, good_test_uri_));
+  EXPECT_EQ(0, options->failurePredicates().size());
+}
+
 class OptionsImplVerbosityTest : public OptionsImplTest, public WithParamInterface<const char*> {};
 
 // Test we accept all possible --verbosity values.