test: deflaking */LoadStatsIntegrationTest.LocalityWeighted/* (envoyp…

…roxy#7225) For posterity envoyproxy#6875 got us the information it was a "waiting for stats" bug, which is then debuggable by lowering load stats interval to 1ms. This exposed two issues with merge. When request 1 arrived in a separate interval from request 2, the stats protos could arrive in the opposite order (fixed by ignoring order) and that if there was a stats-update with a request in progress it never resolved because we were always adding fields (oops!) I'm tempted to leave the interval low now that this is thoroughly debugged, but there's expectations on the time being within a range which wouldn't work if we lower it so meh? Risk Level: n/a (test only) Testing: yes. very much so. Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#6874 Signed-off-by: Alyssa Wilk <[email protected]>
hango-io · Jun 19, 2019 · b166f11 · b166f11
1 parent 1dc5f64
commit b166f11
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 29 deletions.
diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h
@@ -142,22 +142,22 @@ class RepeatedPtrUtil {
   }
 
   /**
-   * Converts a proto repeated field into a generic vector of const Protobuf::Message unique_ptr's.
+   * Converts a proto repeated field into a container of const Protobuf::Message unique_ptr's.
    *
    * @param repeated_field the proto repeated field to convert.
-   * @return ProtobufType::ConstMessagePtrVector the vector of const Message pointers.
+   * @return ReturnType the container of const Message pointers.
    */
-  template <typename ProtoType>
-  static ProtobufTypes::ConstMessagePtrVector
-  convertToConstMessagePtrVector(const Protobuf::RepeatedPtrField<ProtoType>& repeated_field) {
-    ProtobufTypes::ConstMessagePtrVector ret_vector;
-    std::transform(repeated_field.begin(), repeated_field.end(), std::back_inserter(ret_vector),
+  template <typename ProtoType, typename ReturnType>
+  static ReturnType
+  convertToConstMessagePtrContainer(const Protobuf::RepeatedPtrField<ProtoType>& repeated_field) {
+    ReturnType ret_container;
+    std::transform(repeated_field.begin(), repeated_field.end(), std::back_inserter(ret_container),
                    [](const ProtoType& proto_message) -> std::unique_ptr<const Protobuf::Message> {
                      Protobuf::Message* clone = proto_message.New();
                      clone->MergeFrom(proto_message);
                      return std::unique_ptr<const Protobuf::Message>(clone);
                    });
-    return ret_vector;
+    return ret_container;
   }
 };
 

diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc
@@ -35,7 +35,8 @@ create(const envoy::config::filter::network::http_connection_manager::v2::HttpCo
         ScopedRouteConfigurationsList& scoped_route_list =
             config.scoped_routes().scoped_route_configurations_list();
     return scoped_routes_config_provider_manager.createStaticConfigProvider(
-        RepeatedPtrUtil::convertToConstMessagePtrVector(
+        RepeatedPtrUtil::convertToConstMessagePtrContainer<envoy::api::v2::ScopedRouteConfiguration,
+                                                           ProtobufTypes::ConstMessagePtrVector>(
             scoped_route_list.scoped_route_configurations()),
         factory_context,
         ScopedRoutesConfigProviderManagerOptArg(config.scoped_routes().name(),

diff --git a/test/integration/load_stats_integration_test.cc b/test/integration/load_stats_integration_test.cc
@@ -191,15 +191,13 @@ class LoadStatsIntegrationTest : public testing::TestWithParam<Network::Address:
           upstream_locality_stats->set_total_successful_requests(
               upstream_locality_stats->total_successful_requests() +
               local_upstream_locality_stats.total_successful_requests());
-          upstream_locality_stats->set_total_requests_in_progress(
-              upstream_locality_stats->total_requests_in_progress() +
-              local_upstream_locality_stats.total_requests_in_progress());
           upstream_locality_stats->set_total_error_requests(
               upstream_locality_stats->total_error_requests() +
               local_upstream_locality_stats.total_error_requests());
           upstream_locality_stats->set_total_issued_requests(
               upstream_locality_stats->total_issued_requests() +
               local_upstream_locality_stats.total_issued_requests());
+          // Unlike most stats, current requests in progress replaces old requests in progress.
           break;
         }
       }
@@ -208,6 +206,25 @@ class LoadStatsIntegrationTest : public testing::TestWithParam<Network::Address:
         upstream_locality_stats->CopyFrom(local_upstream_locality_stats);
       }
     }
+
+    // Unfortunately because we don't issue an update when total_requests_in_progress goes from
+    // non-zero to zero, we have to go through and zero it out for any locality stats we didn't see.
+    for (int i = 0; i < cluster_stats->upstream_locality_stats_size(); ++i) {
+      auto upstream_locality_stats = cluster_stats->mutable_upstream_locality_stats(i);
+      bool found = false;
+      for (int j = 0; j < local_cluster_stats.upstream_locality_stats_size(); ++j) {
+        auto& local_upstream_locality_stats = local_cluster_stats.upstream_locality_stats(j);
+        if (TestUtility::protoEqual(upstream_locality_stats->locality(),
+                                    local_upstream_locality_stats.locality()) &&
+            upstream_locality_stats->priority() == local_upstream_locality_stats.priority()) {
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
+        upstream_locality_stats->set_total_requests_in_progress(0);
+      }
+    }
   }
 
   void waitForLoadStatsRequest(
@@ -257,7 +274,7 @@ class LoadStatsIntegrationTest : public testing::TestWithParam<Network::Address:
       EXPECT_EQ("application/grpc",
                 loadstats_stream_->headers().ContentType()->value().getStringView());
     } while (!TestUtility::assertRepeatedPtrFieldEqual(expected_cluster_stats,
-                                                       loadstats_request.cluster_stats()));
+                                                       loadstats_request.cluster_stats(), true));
   }
 
   void waitForUpstreamResponse(uint32_t endpoint_index, uint32_t response_code = 200) {
@@ -460,23 +477,16 @@ TEST_P(LoadStatsIntegrationTest, LocalityWeighted) {
   locality_weighted_lb_ = true;
   initialize();
 
-  // Debug logs for #6874
-  std::cerr << "Waiting for load stats stream." << std::endl;
   waitForLoadStatsStream();
-  std::cerr << "Waiting for load stats request." << std::endl;
   waitForLoadStatsRequest({});
 
-  std::cerr << "Done waiting." << std::endl;
   loadstats_stream_->startGrpcStream();
-  std::cerr << "Starting response." << std::endl;
   requestLoadStatsResponse({"cluster_0"});
-  std::cerr << "Updating assignments." << std::endl;
 
   // Simple 33%/67% split between dragon/winter localities.
   // Even though there are more endpoints in the dragon locality, the winter locality gets the
   // expected weighting in the WRR locality schedule.
   updateClusterLoadAssignment({{0}, 2}, {{1, 2}, 1}, {}, {});
-  std::cerr << "Sending traffic." << std::endl;
 
   sendAndReceiveUpstream(0);
   sendAndReceiveUpstream(1);
@@ -486,10 +496,8 @@ TEST_P(LoadStatsIntegrationTest, LocalityWeighted) {
   sendAndReceiveUpstream(0);
 
   // Verify we get the expect request distribution.
-  std::cerr << "Waiting for load stats request 2." << std::endl;
   waitForLoadStatsRequest(
       {localityStats("winter", 4, 0, 0, 4), localityStats("dragon", 2, 0, 0, 2)});
-  std::cerr << "Done waiting." << std::endl;
 
   EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
   // On slow machines, more than one load stats response may be pushed while we are simulating load.

diff --git a/test/test_common/utility.h b/test/test_common/utility.h
@@ -264,30 +264,59 @@ class TestUtility {
    *
    * @param lhs RepeatedPtrField on LHS.
    * @param rhs RepeatedPtrField on RHS.
+   * @param ignore_ordering if ordering should be ignored. Note if true this turns
+   *   comparison into an N^2 operation.
    * @return bool indicating whether the RepeatedPtrField are equal. TestUtility::protoEqual() is
    *              used for individual element testing.
    */
-  template <class ProtoType>
+  template <typename ProtoType>
   static bool repeatedPtrFieldEqual(const Protobuf::RepeatedPtrField<ProtoType>& lhs,
-                                    const Protobuf::RepeatedPtrField<ProtoType>& rhs) {
+                                    const Protobuf::RepeatedPtrField<ProtoType>& rhs,
+                                    bool ignore_ordering = false) {
     if (lhs.size() != rhs.size()) {
       return false;
     }
 
-    for (int i = 0; i < lhs.size(); ++i) {
-      if (!TestUtility::protoEqual(lhs[i], rhs[i], /*ignore_repeated_field_ordering=*/false)) {
+    if (!ignore_ordering) {
+      for (int i = 0; i < lhs.size(); ++i) {
+        if (!TestUtility::protoEqual(lhs[i], rhs[i], /*ignore_ordering=*/false)) {
+          return false;
+        }
+      }
+
+      return true;
+    }
+    typedef std::list<std::unique_ptr<const Protobuf::Message>> ProtoList;
+    // Iterate through using protoEqual as ignore_ordering is true, and fields
+    // in the sub-protos may also be out of order.
+    ProtoList lhs_list =
+        RepeatedPtrUtil::convertToConstMessagePtrContainer<ProtoType, ProtoList>(lhs);
+    ProtoList rhs_list =
+        RepeatedPtrUtil::convertToConstMessagePtrContainer<ProtoType, ProtoList>(rhs);
+    while (!lhs_list.empty()) {
+      bool found = false;
+      for (auto it = rhs_list.begin(); it != rhs_list.end(); ++it) {
+        if (TestUtility::protoEqual(*lhs_list.front(), **it,
+                                    /*ignore_ordering=*/true)) {
+          lhs_list.pop_front();
+          rhs_list.erase(it);
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
         return false;
       }
     }
-
     return true;
   }
 
   template <class ProtoType>
   static AssertionResult
   assertRepeatedPtrFieldEqual(const Protobuf::RepeatedPtrField<ProtoType>& lhs,
-                              const Protobuf::RepeatedPtrField<ProtoType>& rhs) {
-    if (!repeatedPtrFieldEqual(lhs, rhs)) {
+                              const Protobuf::RepeatedPtrField<ProtoType>& rhs,
+                              bool ignore_ordering = false) {
+    if (!repeatedPtrFieldEqual(lhs, rhs, ignore_ordering)) {
       return AssertionFailure() << RepeatedPtrUtil::debugString(lhs) << " does not match "
                                 << RepeatedPtrUtil::debugString(rhs);
     }