Skip to content

Making //test/integration:hds_integration_test test faster and more robust #4164

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

Merged
htuch merged 6 commits into from
Aug 22, 2018
Merged

Making //test/integration:hds_integration_test test faster and more robust #4164

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1d1d77a
Making hds_integration test faster and more robust
markatou Aug 15, 2018
b7d94d4
Addressing comments
markatou Aug 16, 2018
6040189
Addressing comments
markatou Aug 16, 2018
02687d9
Merge remote-tracking branch 'upstream/master' into less_fragile
markatou Aug 21, 2018
a851f9d
Addressing comments
markatou Aug 21, 2018
a4dd166
Addressing comments
markatou Aug 22, 2018
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
67 changes: 53 additions & 14 deletions test/integration/hds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class HdsIntegrationTest : public HttpIntegrationTest,
// one HTTP health_check
envoy::service::discovery::v2::HealthCheckSpecifier makeHttpHealthCheckSpecifier() {
envoy::service::discovery::v2::HealthCheckSpecifier server_health_check_specifier_;
server_health_check_specifier_.mutable_interval()->set_seconds(1);
server_health_check_specifier_.mutable_interval()->set_nanos(100000000);

@mattklein123 mattklein123 Aug 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not still use seconds here for readability? Same elsewhere? Or perhaps a constant such as MaxTimeout to make it clear that we don't care about the timeout?

@markatou markatou Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This interval specifies how often the HdsDelegate sends a message with health checking information back to the management server. Before these changes, I was using time to make sure that the cluster updated before a response was sent. Now, I check the stats to make sure that the cluster processed the change, before I look at what the HdsDelegate sends to the server.

In order to make the interval less than a second, I need to use nanos.

@mattklein123 mattklein123 Aug 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah ok. Alright, some type of constant or comment might still be nice here since it's hard to read how much time this is.


auto* health_check = server_health_check_specifier_.add_cluster_health_checks();

Expand All @@ -130,8 +130,8 @@ class HdsIntegrationTest : public HttpIntegrationTest,
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_zone("some_zone");
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_sub_zone("crete");

health_check->add_health_checks()->mutable_timeout()->set_seconds(2);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(1);
health_check->add_health_checks()->mutable_timeout()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_unhealthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_healthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_grpc_health_check();
Expand All @@ -145,7 +145,7 @@ class HdsIntegrationTest : public HttpIntegrationTest,
// one TCP health_check
envoy::service::discovery::v2::HealthCheckSpecifier makeTcpHealthCheckSpecifier() {
envoy::service::discovery::v2::HealthCheckSpecifier server_health_check_specifier_;
server_health_check_specifier_.mutable_interval()->set_seconds(1);
server_health_check_specifier_.mutable_interval()->set_nanos(100000000);

auto* health_check = server_health_check_specifier_.add_cluster_health_checks();

Expand All @@ -157,8 +157,8 @@ class HdsIntegrationTest : public HttpIntegrationTest,
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_zone("some_zone");
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_sub_zone("crete");

health_check->add_health_checks()->mutable_timeout()->set_seconds(2);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(1);
health_check->add_health_checks()->mutable_timeout()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_unhealthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_healthy_threshold()->set_value(2);
auto* tcp_hc = health_check->mutable_health_checks(0)->mutable_tcp_health_check();
Expand Down Expand Up @@ -234,6 +234,9 @@ TEST_P(HdsIntegrationTest, SingleEndpointHealthyHttp) {
host_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false);
host_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoint's response
test_server_->waitForCounterGe("cluster.anna.health_check.success", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand All @@ -253,14 +256,16 @@ TEST_P(HdsIntegrationTest, SingleEndpointHealthyHttp) {
TEST_P(HdsIntegrationTest, SingleEndpointTimeoutHttp) {
initialize();
server_health_check_specifier_ = makeHttpHealthCheckSpecifier();

server_health_check_specifier_.mutable_cluster_health_checks(0)
->mutable_health_checks(0)
->mutable_timeout()
->set_seconds(0);
server_health_check_specifier_.mutable_cluster_health_checks(0)
->mutable_health_checks(0)
->mutable_timeout()
->set_nanos(800000000);
->set_nanos(100000000);

// Server <--> Envoy
waitForHdsStream();
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, envoy_msg_));
Expand All @@ -275,6 +280,9 @@ TEST_P(HdsIntegrationTest, SingleEndpointTimeoutHttp) {

// Endpoint doesn't repond to the health check

// Make sure the cluster processed the endpoint's (lack of) response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -312,6 +320,9 @@ TEST_P(HdsIntegrationTest, SingleEndpointUnhealthyHttp) {
host_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "404"}}, false);
host_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoint's response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -346,7 +357,7 @@ TEST_P(HdsIntegrationTest, SingleEndpointTimeoutTcp) {
server_health_check_specifier_.mutable_cluster_health_checks(0)
->mutable_health_checks(0)
->mutable_timeout()
->set_nanos(800000000);
->set_nanos(100000000);
hds_stream_->startGrpcStream();
hds_stream_->sendGrpcMessage(server_health_check_specifier_);
test_server_->waitForCounterGe("hds_delegate.requests", ++hds_requests_);
Expand All @@ -355,10 +366,13 @@ TEST_P(HdsIntegrationTest, SingleEndpointTimeoutTcp) {
ASSERT_TRUE(host_upstream_->waitForRawConnection(host_fake_raw_connection_));
ASSERT_TRUE(
host_fake_raw_connection_->waitForData(FakeRawConnection::waitForInexactMatch("Ping")));

host_upstream_->set_allow_unexpected_disconnects(true);

// No response from the endpoint

// Make sure the cluster processed the endpoint's (lack of) response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -396,6 +410,9 @@ TEST_P(HdsIntegrationTest, SingleEndpointHealthyTcp) {
RELEASE_ASSERT(result, result.message());
host_upstream_->set_allow_unexpected_disconnects(true);

// Make sure the cluster processed the endpoint's response
test_server_->waitForCounterGe("cluster.anna.health_check.success", 1);

@htuch htuch Aug 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a bit unclear to me what the win with some of the waits is. We will need to wait for the gRPC response from Envoy to the HDS server below, and those messages are sent independently of what we're doing here wait wise.


// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -433,6 +450,9 @@ TEST_P(HdsIntegrationTest, SingleEndpointUnhealthyTcp) {
RELEASE_ASSERT(result, result.message());
host_upstream_->set_allow_unexpected_disconnects(true);

// TODO(lilika): TcpHealthchecker is not incrementing the stats in failure
// so we can't wait for the cluster to process the endpoint's response

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -476,6 +496,10 @@ TEST_P(HdsIntegrationTest, TwoEndpointsSameLocality) {
host2_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false);
host2_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoints' response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);
test_server_->waitForCounterGe("cluster.anna.health_check.success", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -527,6 +551,10 @@ TEST_P(HdsIntegrationTest, TwoEndpointsDifferentLocality) {
host2_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false);
host2_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoints' response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);
test_server_->waitForCounterGe("cluster.anna.health_check.success", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -561,8 +589,8 @@ TEST_P(HdsIntegrationTest, TwoEndpointsDifferentClusters) {
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_zone("peculiar_zone");
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_sub_zone("paris");

health_check->add_health_checks()->mutable_timeout()->set_seconds(2);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(1);
health_check->add_health_checks()->mutable_timeout()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_unhealthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_healthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_grpc_health_check();
Expand All @@ -587,6 +615,10 @@ TEST_P(HdsIntegrationTest, TwoEndpointsDifferentClusters) {
host2_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false);
host2_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoints' response
test_server_->waitForCounterGe("cluster.anna.health_check.failure", 1);
test_server_->waitForCounterGe("cluster.cat.health_check.success", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down Expand Up @@ -628,6 +660,9 @@ TEST_P(HdsIntegrationTest, TestUpdateMessage) {
host_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false);
host_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoint's response
test_server_->waitForCounterGe("cluster.anna.health_check.success", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand All @@ -641,7 +676,7 @@ TEST_P(HdsIntegrationTest, TestUpdateMessage) {

// New HealthCheckSpecifier message
envoy::service::discovery::v2::HealthCheckSpecifier new_message;
new_message.mutable_interval()->set_seconds(1);
new_message.mutable_interval()->set_nanos(100000000);

auto* health_check = new_message.add_cluster_health_checks();

Expand All @@ -654,8 +689,8 @@ TEST_P(HdsIntegrationTest, TestUpdateMessage) {
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_zone("peculiar_zone");
health_check->mutable_locality_endpoints(0)->mutable_locality()->set_sub_zone("paris");

health_check->add_health_checks()->mutable_timeout()->set_seconds(2);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(1);
health_check->add_health_checks()->mutable_timeout()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(100);
health_check->mutable_health_checks(0)->mutable_unhealthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_healthy_threshold()->set_value(2);
health_check->mutable_health_checks(0)->mutable_grpc_health_check();
Expand All @@ -671,10 +706,14 @@ TEST_P(HdsIntegrationTest, TestUpdateMessage) {
ASSERT_TRUE(host2_fake_connection_->waitForNewStream(*dispatcher_, host2_stream_));
ASSERT_TRUE(host2_stream_->waitForEndStream(*dispatcher_));
host2_upstream_->set_allow_unexpected_disconnects(true);

// Endpoint responds to the health check
host2_stream_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "404"}}, false);
host2_stream_->encodeData(1024, true);

// Make sure the cluster processed the endpoint's response
test_server_->waitForCounterGe("cluster.cat.health_check.failure", 1);

// Envoy reports back to server
ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, response_));

Expand Down