PR feedback: remove new stat, use real sleep in integration test

The real sleep helps avoid a ~1/500 chance of a test flake due to a race. Signed-off-by: Michael Puncel <mpuncel@squareup.com>
mpuncel · Dec 16, 2020 · a7cfa59 · a7cfa59
1 parent 49e689d
commit a7cfa59
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 5 deletions.
diff --git a/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst b/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst
@@ -119,7 +119,6 @@ If health check is configured, the cluster has an additional statistics tree roo
   network_failure, Counter, Number of health check failures due to network error
   verify_cluster, Counter, Number of health checks that attempted cluster name verification
   healthy, Gauge, Number of healthy members
-  goaway_error, Counter, Number of health check failures due to a HTTP/2 GOAWAY frame with a code other than NO_ERROR from the upstream (subset of network_failure)
 
 .. _config_cluster_manager_cluster_stats_outlier_detection:
 

diff --git a/source/common/upstream/health_checker_base_impl.h b/source/common/upstream/health_checker_base_impl.h
@@ -23,7 +23,6 @@ namespace Upstream {
 #define ALL_HEALTH_CHECKER_STATS(COUNTER, GAUGE)                                                   \
   COUNTER(attempt)                                                                                 \
   COUNTER(failure)                                                                                 \
-  COUNTER(goaway_error)                                                                            \
   COUNTER(network_failure)                                                                         \
   COUNTER(passive_failure)                                                                         \
   COUNTER(success)                                                                                 \

diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -323,7 +323,6 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onGoAway(
     handleFailure(envoy::data::core::v3::NETWORK);
   }
 
-  parent_.stats_.goaway_error_.inc();
   if (client_) {
     expect_reset_ = true;
     client_->close();

diff --git a/test/integration/health_check_integration_test.cc b/test/integration/health_check_integration_test.cc
@@ -319,9 +319,20 @@ TEST_P(HttpHealthCheckIntegrationTest, SingleEndpointGoAwayError) {
   test_server_->waitForCounterGe("cluster.cluster_1.health_check.failure", 1);
   EXPECT_EQ(0, test_server_->counter("cluster.cluster_1.health_check.success")->value());
   EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.health_check.failure")->value());
-  test_server_->waitForCounterGe("cluster.cluster_1.health_check.goaway_error", 1);
 
-  // Advance time to cause another health check.
+  // Now we want to advance time to cause another health check. Ideally the
+  // waitForDisconnect() step above would guarantee that the health checker has
+  // seen the GOAWAY, closed the health check connection, and finally enabled
+  // the health check interval timer so we can advance simulated time to cause
+  // another check. However, due to special handling for protocol errors, there
+  // is a race where both the downstream (health checker) and upstream (fake
+  // upstream) close the connection, so we may exit waitForDisconnect() before
+  // the health checker has actually seen the GOAWAY and enabled the timer. To
+  // address this, we use a small real sleep before advancing thee simulated
+  // time system to give the health checker time to catch up. If we advance time
+  // before the health check timer is enabled, the next health check will never
+  // happen.
+  timeSystem().realSleepDoNotUseWithoutScrutiny(std::chrono::milliseconds(100));
   timeSystem().advanceTimeWait(std::chrono::milliseconds(500));
 
   ASSERT_TRUE(clusters_[cluster_idx].host_upstream_->waitForHttpConnection(