From 70c6dd0923f5f1e3d0c7f10d552b81222c4beef6 Mon Sep 17 00:00:00 2001 From: Zach Date: Sat, 24 Oct 2020 21:33:23 +0000 Subject: [PATCH 1/4] Made health check fuzz more efficient Signed-off-by: Zach --- test/common/upstream/health_check_fuzz.cc | 89 ++++++++++++++----- test/common/upstream/health_check_fuzz.h | 29 +++++- .../health_checker_impl_test_utils.cc | 4 +- .../upstream/health_checker_impl_test_utils.h | 4 +- 4 files changed, 100 insertions(+), 26 deletions(-) diff --git a/test/common/upstream/health_check_fuzz.cc b/test/common/upstream/health_check_fuzz.cc index fa21636cb335..603846f3c2ed 100644 --- a/test/common/upstream/health_check_fuzz.cc +++ b/test/common/upstream/health_check_fuzz.cc @@ -314,16 +314,17 @@ void GrpcHealthCheckFuzz::allocGrpcHealthCheckerFromProto( void GrpcHealthCheckFuzz::initialize(test::common::upstream::HealthCheckTestCase input) { allocGrpcHealthCheckerFromProto(input.health_check_config()); + test_session_ = std::make_unique(); cluster_->prioritySet().getMockHostSet(0)->hosts_ = { makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; expectSessionCreate(); - expectStreamCreate(0); + expectStreamCreate(); health_checker_->start(); ON_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) .WillByDefault(testing::Return(45000)); if (DurationUtil::durationToMilliseconds(input.health_check_config().initial_jitter()) != 0) { - test_sessions_[0]->interval_timer_->invokeCallback(); + test_session_->interval_timer_->invokeCallback(); } reuse_connection_ = @@ -333,7 +334,7 @@ void GrpcHealthCheckFuzz::initialize(test::common::upstream::HealthCheckTestCase // Logic from respondResponseSpec() in unit tests void GrpcHealthCheckFuzz::respond(test::common::upstream::Respond respond, bool last_action) { const test::common::upstream::GrpcRespond& grpc_respond = respond.grpc_respond(); - if (!test_sessions_[0]->timeout_timer_->enabled_) { + if (!test_session_->timeout_timer_->enabled_) { ENVOY_LOG_MISC(trace, "Timeout timer is disabled. Skipping response."); return; } @@ -359,32 +360,31 @@ void GrpcHealthCheckFuzz::respond(test::common::upstream::Respond respond, bool response_headers->setStatus(grpc_respond.grpc_respond_headers().status()); ENVOY_LOG_MISC(trace, "Responded headers {}", *response_headers.get()); - test_sessions_[0]->stream_response_callbacks_->decodeHeaders(std::move(response_headers), - end_stream_on_headers); + test_session_->stream_response_callbacks_->decodeHeaders(std::move(response_headers), + end_stream_on_headers); // If the interval timer is enabled, that means that the rpc is complete, as decodeHeaders hit a // certain branch that called onRpcComplete(), logically representing a completed rpc call. Thus, // skip the next responses until explicitly invoking interval timer as cleanup. - if (has_data && !test_sessions_[0]->interval_timer_->enabled_) { + if (has_data && !test_session_->interval_timer_->enabled_) { std::vector> bufferList = makeBufferListToRespondWith(grpc_respond.grpc_respond_bytes()); // If the interval timer is enabled, that means that the rpc is complete, as decodeData hit a // certain branch that called onRpcComplete(), logically representing a completed rpc call. // Thus, skip the next responses until explicitly invoking interval timer as cleanup. - for (size_t i = 0; i < bufferList.size() && !test_sessions_[0]->interval_timer_->enabled_; - ++i) { + for (size_t i = 0; i < bufferList.size() && !test_session_->interval_timer_->enabled_; ++i) { const bool end_stream_on_data = !has_trailers && i == bufferList.size() - 1; const auto data = std::make_unique(bufferList[i].data(), bufferList[i].size()); ENVOY_LOG_MISC(trace, "Responded with data"); - test_sessions_[0]->stream_response_callbacks_->decodeData(*data, end_stream_on_data); + test_session_->stream_response_callbacks_->decodeData(*data, end_stream_on_data); } } // If the interval timer is enabled, that means that the rpc is complete, as decodeData hit a // certain branch that called onRpcComplete(), logically representing a completed rpc call. Thus, // skip responding with trailers until explicitly invoking interval timer as cleanup. - if (has_trailers && !test_sessions_[0]->interval_timer_->enabled_) { + if (has_trailers && !test_session_->interval_timer_->enabled_) { std::unique_ptr response_trailers = std::make_unique( Fuzz::fromHeaders( @@ -392,11 +392,11 @@ void GrpcHealthCheckFuzz::respond(test::common::upstream::Respond respond, bool ENVOY_LOG_MISC(trace, "Responded trailers {}", *response_trailers.get()); - test_sessions_[0]->stream_response_callbacks_->decodeTrailers(std::move(response_trailers)); + test_session_->stream_response_callbacks_->decodeTrailers(std::move(response_trailers)); } // This means that the response did not represent a full rpc response. - if (!test_sessions_[0]->interval_timer_->enabled_) { + if (!test_session_->interval_timer_->enabled_) { return; } @@ -411,7 +411,7 @@ void GrpcHealthCheckFuzz::respond(test::common::upstream::Respond respond, bool } void GrpcHealthCheckFuzz::triggerIntervalTimer(bool expect_client_create) { - if (!test_sessions_[0]->interval_timer_->enabled_) { + if (!test_session_->interval_timer_->enabled_) { ENVOY_LOG_MISC(trace, "Interval timer is disabled. Skipping trigger interval timer."); return; } @@ -419,19 +419,19 @@ void GrpcHealthCheckFuzz::triggerIntervalTimer(bool expect_client_create) { expectClientCreate(0); ENVOY_LOG_MISC(trace, "Created client"); } - expectStreamCreate(0); + expectStreamCreate(); ENVOY_LOG_MISC(trace, "Created stream"); - test_sessions_[0]->interval_timer_->invokeCallback(); + test_session_->interval_timer_->invokeCallback(); } void GrpcHealthCheckFuzz::triggerTimeoutTimer(bool last_action) { - if (!test_sessions_[0]->timeout_timer_->enabled_) { + if (!test_session_->timeout_timer_->enabled_) { ENVOY_LOG_MISC(trace, "Timeout timer is disabled. Skipping trigger timeout timer."); return; } ENVOY_LOG_MISC(trace, "Triggered timeout timer"); - test_sessions_[0]->timeout_timer_->invokeCallback(); // This closes the client, turns off - // timeout and enables interval + test_session_->timeout_timer_->invokeCallback(); // This closes the client, turns off + // timeout and enables interval if ((!reuse_connection_ || received_no_error_goaway_) && !last_action) { ENVOY_LOG_MISC(trace, "Triggering interval timer after timeout."); @@ -442,7 +442,7 @@ void GrpcHealthCheckFuzz::triggerTimeoutTimer(bool last_action) { } void GrpcHealthCheckFuzz::raiseEvent(const Network::ConnectionEvent& event_type, bool last_action) { - test_sessions_[0]->client_connection_->raiseEvent(event_type); + test_session_->client_connection_->raiseEvent(event_type); if (!last_action && event_type != Network::ConnectionEvent::Connected) { // Close events will always blow away the client ENVOY_LOG_MISC(trace, "Triggering interval timer after close event"); @@ -454,16 +454,63 @@ void GrpcHealthCheckFuzz::raiseEvent(const Network::ConnectionEvent& event_type, void GrpcHealthCheckFuzz::raiseGoAway(bool no_error) { if (no_error) { - test_sessions_[0]->codec_client_->raiseGoAway(Http::GoAwayErrorCode::NoError); + test_session_->codec_client_->raiseGoAway(Http::GoAwayErrorCode::NoError); // Will cause other events to blow away client, because this is a "graceful" go away received_no_error_goaway_ = true; } else { // go away events without no error flag explicitly blow away client - test_sessions_[0]->codec_client_->raiseGoAway(Http::GoAwayErrorCode::Other); + test_session_->codec_client_->raiseGoAway(Http::GoAwayErrorCode::Other); triggerIntervalTimer(true); } } +void GrpcHealthCheckFuzz::expectSessionCreate() { + test_session_->timeout_timer_ = new NiceMock(&dispatcher_); + test_session_->interval_timer_ = new NiceMock(&dispatcher_); + test_session_->request_encoder_.stream_.callbacks_.clear(); + expectClientCreate(0); +} + +void GrpcHealthCheckFuzz::expectClientCreate(size_t index) { + TestSession& test_session = *test_session_; + test_session.codec_ = new NiceMock(); + test_session.client_connection_ = new NiceMock(); + connection_index_.push_back(index); + codec_index_.push_back(index); + + EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _)) + .Times(testing::AnyNumber()) + .WillRepeatedly(testing::InvokeWithoutArgs([&]() -> Network::ClientConnection* { + connection_index_.front(); + connection_index_.pop_front(); + return test_session_->client_connection_; + })); + + EXPECT_CALL(*health_checker_, createCodecClient_(_)) + .WillRepeatedly( + Invoke([&](Upstream::Host::CreateConnectionData& conn_data) -> Http::CodecClient* { + codec_index_.front(); + codec_index_.pop_front(); + TestSession& test_session = *test_session_; + std::shared_ptr cluster{ + new NiceMock()}; + Event::MockDispatcher dispatcher_; + + test_session.codec_client_ = new CodecClientForTest( + Http::CodecClient::Type::HTTP1, std::move(conn_data.connection_), + test_session.codec_, nullptr, + Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); + return test_session.codec_client_; + })); +} + +void GrpcHealthCheckFuzz::expectStreamCreate() { + test_session_->request_encoder_.stream_.callbacks_.clear(); + EXPECT_CALL(*test_session_->codec_, newStream(_)) + .WillOnce(DoAll(SaveArgAddress(&test_session_->stream_response_callbacks_), + ReturnRef(test_session_->request_encoder_))); +} + Network::ConnectionEvent HealthCheckFuzz::getEventTypeFromProto(const test::common::upstream::RaiseEvent& event) { switch (event) { diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index ea17615e1270..39ded92694b3 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -69,7 +69,7 @@ class TcpHealthCheckFuzz : public HealthCheckFuzz, TcpHealthCheckerImplTestBase bool empty_response_ = true; }; -class GrpcHealthCheckFuzz : public HealthCheckFuzz, GrpcHealthCheckerImplTestBaseUtils { +class GrpcHealthCheckFuzz : public HealthCheckFuzz, public HealthCheckerTestBase { public: void allocGrpcHealthCheckerFromProto(const envoy::config::core::v3::HealthCheck& config); void initialize(test::common::upstream::HealthCheckTestCase input) override; @@ -87,6 +87,33 @@ class GrpcHealthCheckFuzz : public HealthCheckFuzz, GrpcHealthCheckerImplTestBas // Determines whether a client closes after responds and timeouts. Exactly maps to // received_no_error_goaway_ in source code. bool received_no_error_goaway_ = false; + + // In order to not affect unit tests, move state here. Since only one test session, we don't need + // a vector. Also, we should make Timers NiceMocks. + struct TestSession { + TestSession() = default; + + NiceMock* interval_timer_{}; + NiceMock* timeout_timer_{}; + Http::MockClientConnection* codec_{}; + Stats::IsolatedStoreImpl stats_store_; + Network::MockClientConnection* client_connection_{}; + NiceMock request_encoder_; + Http::ResponseDecoder* stream_response_callbacks_{}; + CodecClientForTest* codec_client_{}; + }; + + using TestSessionPtr = std::unique_ptr; + TestSessionPtr test_session_; + + void expectSessionCreate(); + void expectClientCreate(size_t index); + void expectStreamCreate(); + + std::vector test_sessions_; + std::shared_ptr health_checker_; + std::list connection_index_{}; + std::list codec_index_{}; }; } // namespace Upstream diff --git a/test/common/upstream/health_checker_impl_test_utils.cc b/test/common/upstream/health_checker_impl_test_utils.cc index 3bdd709d43e0..e724ccbb7482 100644 --- a/test/common/upstream/health_checker_impl_test_utils.cc +++ b/test/common/upstream/health_checker_impl_test_utils.cc @@ -75,8 +75,8 @@ void HttpHealthCheckerImplTestBase::expectClientCreate(size_t index) { // This is needed to put expectations in LIFO order. The unit tests use inSequence, which makes // expectations FIFO. void TcpHealthCheckerImplTestBase::expectSessionCreate() { - timeout_timer_ = new Event::MockTimer(&dispatcher_); - interval_timer_ = new Event::MockTimer(&dispatcher_); + timeout_timer_ = new NiceMock(&dispatcher_); + interval_timer_ = new NiceMock(&dispatcher_); } void TcpHealthCheckerImplTestBase::expectClientCreate() { diff --git a/test/common/upstream/health_checker_impl_test_utils.h b/test/common/upstream/health_checker_impl_test_utils.h index 5c67c7371909..7b95c2a285ea 100644 --- a/test/common/upstream/health_checker_impl_test_utils.h +++ b/test/common/upstream/health_checker_impl_test_utils.h @@ -83,8 +83,8 @@ class TcpHealthCheckerImplTestBase : public HealthCheckerTestBase { std::shared_ptr health_checker_; Network::MockClientConnection* connection_{}; - Event::MockTimer* timeout_timer_{}; - Event::MockTimer* interval_timer_{}; + NiceMock* timeout_timer_{}; + NiceMock* interval_timer_{}; Network::ReadFilterSharedPtr read_filter_; }; From 50477882abb53b4a48e4d35a51d65e87bdc5171f Mon Sep 17 00:00:00 2001 From: Zach Date: Mon, 26 Oct 2020 20:34:48 +0000 Subject: [PATCH 2/4] Responded to Asra's comments Signed-off-by: Zach --- test/common/upstream/health_check_fuzz.cc | 53 ++++++++++------------- test/common/upstream/health_check_fuzz.h | 4 +- 2 files changed, 23 insertions(+), 34 deletions(-) diff --git a/test/common/upstream/health_check_fuzz.cc b/test/common/upstream/health_check_fuzz.cc index 603846f3c2ed..b6cd1171a8f2 100644 --- a/test/common/upstream/health_check_fuzz.cc +++ b/test/common/upstream/health_check_fuzz.cc @@ -313,11 +313,29 @@ void GrpcHealthCheckFuzz::allocGrpcHealthCheckerFromProto( } void GrpcHealthCheckFuzz::initialize(test::common::upstream::HealthCheckTestCase input) { - allocGrpcHealthCheckerFromProto(input.health_check_config()); test_session_ = std::make_unique(); + allocGrpcHealthCheckerFromProto(input.health_check_config()); cluster_->prioritySet().getMockHostSet(0)->hosts_ = { makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; expectSessionCreate(); + ON_CALL(dispatcher_, createClientConnection_(_, _, _, _)) + .WillByDefault(testing::InvokeWithoutArgs( + [&]() -> Network::ClientConnection* { return test_session_->client_connection_; })); + + ON_CALL(*health_checker_, createCodecClient_(_)) + .WillByDefault( + Invoke([&](Upstream::Host::CreateConnectionData& conn_data) -> Http::CodecClient* { + TestSession& test_session = *test_session_; + std::shared_ptr cluster{ + new NiceMock()}; + Event::MockDispatcher dispatcher_; + + test_session.codec_client_ = new CodecClientForTest( + Http::CodecClient::Type::HTTP1, std::move(conn_data.connection_), + test_session.codec_, nullptr, + Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); + return test_session.codec_client_; + })); expectStreamCreate(); health_checker_->start(); ON_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) @@ -416,7 +434,7 @@ void GrpcHealthCheckFuzz::triggerIntervalTimer(bool expect_client_create) { return; } if (expect_client_create) { - expectClientCreate(0); + expectClientCreate(); ENVOY_LOG_MISC(trace, "Created client"); } expectStreamCreate(); @@ -468,40 +486,13 @@ void GrpcHealthCheckFuzz::expectSessionCreate() { test_session_->timeout_timer_ = new NiceMock(&dispatcher_); test_session_->interval_timer_ = new NiceMock(&dispatcher_); test_session_->request_encoder_.stream_.callbacks_.clear(); - expectClientCreate(0); + expectClientCreate(); } -void GrpcHealthCheckFuzz::expectClientCreate(size_t index) { +void GrpcHealthCheckFuzz::expectClientCreate() { TestSession& test_session = *test_session_; test_session.codec_ = new NiceMock(); test_session.client_connection_ = new NiceMock(); - connection_index_.push_back(index); - codec_index_.push_back(index); - - EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _)) - .Times(testing::AnyNumber()) - .WillRepeatedly(testing::InvokeWithoutArgs([&]() -> Network::ClientConnection* { - connection_index_.front(); - connection_index_.pop_front(); - return test_session_->client_connection_; - })); - - EXPECT_CALL(*health_checker_, createCodecClient_(_)) - .WillRepeatedly( - Invoke([&](Upstream::Host::CreateConnectionData& conn_data) -> Http::CodecClient* { - codec_index_.front(); - codec_index_.pop_front(); - TestSession& test_session = *test_session_; - std::shared_ptr cluster{ - new NiceMock()}; - Event::MockDispatcher dispatcher_; - - test_session.codec_client_ = new CodecClientForTest( - Http::CodecClient::Type::HTTP1, std::move(conn_data.connection_), - test_session.codec_, nullptr, - Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); - return test_session.codec_client_; - })); } void GrpcHealthCheckFuzz::expectStreamCreate() { diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index 39ded92694b3..ef1b05e87789 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -107,13 +107,11 @@ class GrpcHealthCheckFuzz : public HealthCheckFuzz, public HealthCheckerTestBase TestSessionPtr test_session_; void expectSessionCreate(); - void expectClientCreate(size_t index); + void expectClientCreate(); void expectStreamCreate(); std::vector test_sessions_; std::shared_ptr health_checker_; - std::list connection_index_{}; - std::list codec_index_{}; }; } // namespace Upstream From fdbe7077194e3b5f4215e2ea56381543197e5020 Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 27 Oct 2020 18:48:55 +0000 Subject: [PATCH 3/4] Deleted default constructor Signed-off-by: Zach --- test/common/upstream/health_check_fuzz.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index ef1b05e87789..7c6c3cbedc83 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -91,8 +91,6 @@ class GrpcHealthCheckFuzz : public HealthCheckFuzz, public HealthCheckerTestBase // In order to not affect unit tests, move state here. Since only one test session, we don't need // a vector. Also, we should make Timers NiceMocks. struct TestSession { - TestSession() = default; - NiceMock* interval_timer_{}; NiceMock* timeout_timer_{}; Http::MockClientConnection* codec_{}; From 8772d98c734228176fdb41871c09958298c2ee8c Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 28 Oct 2020 22:30:20 +0000 Subject: [PATCH 4/4] Deleted unused field Signed-off-by: Zach --- test/common/upstream/health_check_fuzz.h | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index 7c6c3cbedc83..627e44ea1bcc 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -108,7 +108,6 @@ class GrpcHealthCheckFuzz : public HealthCheckFuzz, public HealthCheckerTestBase void expectClientCreate(); void expectStreamCreate(); - std::vector test_sessions_; std::shared_ptr health_checker_; };