Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fuzz] Made health check fuzz more efficient #13747

Merged
merged 4 commits into from
Oct 29, 2020
Merged

[fuzz] Made health check fuzz more efficient #13747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
70c6dd0
Made health check fuzz more efficient
zasweq Oct 24, 2020
5047788
Responded to Asra's comments
zasweq Oct 26, 2020
fdbe707
Deleted default constructor
zasweq Oct 27, 2020
8772d98
Deleted unused field
zasweq Oct 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 68 additions & 21 deletions test/common/upstream/health_check_fuzz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<TestSession>();
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_ =
Expand All @@ -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;
}
Expand All @@ -359,44 +360,43 @@ 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<std::vector<uint8_t>> 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<Buffer::OwnedImpl>(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<Http::TestResponseTrailerMapImpl> response_trailers =
std::make_unique<Http::TestResponseTrailerMapImpl>(
Fuzz::fromHeaders<Http::TestResponseTrailerMapImpl>(
grpc_respond.grpc_respond_trailers().trailers(), {}, {}));

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;
}

Expand All @@ -411,27 +411,27 @@ 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;
}
if (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.");
Expand All @@ -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");
Expand All @@ -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<Event::MockTimer>(&dispatcher_);
test_session_->interval_timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
test_session_->request_encoder_.stream_.callbacks_.clear();
expectClientCreate(0);
}

void GrpcHealthCheckFuzz::expectClientCreate(size_t index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the given index argument always 0?
If that's the case should we keep this argument, or just remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Asra mentioned that as well :). Removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In regards to your deallocation question, there is no explicit code that deallocates memory. I don't think the memory scales with each fuzz iteration though, as it gets destructed each time.

TestSession& test_session = *test_session_;
test_session.codec_ = new NiceMock<Http::MockClientConnection>();
test_session.client_connection_ = new NiceMock<Network::MockClientConnection>();
connection_index_.push_back(index);
codec_index_.push_back(index);

EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _))
zasweq marked this conversation as resolved.
Show resolved Hide resolved
.Times(testing::AnyNumber())
.WillRepeatedly(testing::InvokeWithoutArgs([&]() -> Network::ClientConnection* {
connection_index_.front();
connection_index_.pop_front();
zasweq marked this conversation as resolved.
Show resolved Hide resolved
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<Upstream::MockClusterInfo> cluster{
new NiceMock<Upstream::MockClusterInfo>()};
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) {
Expand Down
29 changes: 28 additions & 1 deletion test/common/upstream/health_check_fuzz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
htuch marked this conversation as resolved.
Show resolved Hide resolved

NiceMock<Event::MockTimer>* interval_timer_{};
NiceMock<Event::MockTimer>* timeout_timer_{};
Http::MockClientConnection* codec_{};
Stats::IsolatedStoreImpl stats_store_;
Network::MockClientConnection* client_connection_{};
NiceMock<Http::MockRequestEncoder> request_encoder_;
Http::ResponseDecoder* stream_response_callbacks_{};
CodecClientForTest* codec_client_{};
};

using TestSessionPtr = std::unique_ptr<TestSession>;
TestSessionPtr test_session_;

void expectSessionCreate();
void expectClientCreate(size_t index);
void expectStreamCreate();

std::vector<TestSessionPtr> test_sessions_;
zasweq marked this conversation as resolved.
Show resolved Hide resolved
std::shared_ptr<TestGrpcHealthCheckerImpl> health_checker_;
std::list<uint32_t> connection_index_{};
std::list<uint32_t> codec_index_{};
};

} // namespace Upstream
Expand Down
4 changes: 2 additions & 2 deletions test/common/upstream/health_checker_impl_test_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Event::MockTimer>(&dispatcher_);
zasweq marked this conversation as resolved.
Show resolved Hide resolved
interval_timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
}

void TcpHealthCheckerImplTestBase::expectClientCreate() {
Expand Down
4 changes: 2 additions & 2 deletions test/common/upstream/health_checker_impl_test_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ class TcpHealthCheckerImplTestBase : public HealthCheckerTestBase {

std::shared_ptr<TcpHealthCheckerImpl> health_checker_;
Network::MockClientConnection* connection_{};
Event::MockTimer* timeout_timer_{};
Event::MockTimer* interval_timer_{};
NiceMock<Event::MockTimer>* timeout_timer_{};
NiceMock<Event::MockTimer>* interval_timer_{};
Network::ReadFilterSharedPtr read_filter_;
};

Expand Down