From 6d6b5a93f406da77690b2d1d6ac81221a5119132 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 9 Sep 2019 15:33:55 -0400 Subject: [PATCH 1/4] Add per-host memory usage test cast to stats_integration_test Signed-off-by: Antonio Vicente --- test/config/utility.cc | 4 +- test/config/utility.h | 9 +++ test/integration/integration.h | 3 + test/integration/stats_integration_test.cc | 77 ++++++++++++++++++---- 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/test/config/utility.cc b/test/config/utility.cc index 11efd7a79da7..c230b4c772c3 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -365,8 +365,8 @@ void ConfigHelper::finalize(const std::vector& ports) { *cluster->mutable_transport_socket(), tls_config); } } - ASSERT(port_idx == ports.size() || eds_hosts || original_dst_cluster || custom_cluster || - bootstrap_.dynamic_resources().has_cds_config()); + ASSERT(skip_port_usage_validation_ || port_idx == ports.size() || eds_hosts || + original_dst_cluster || custom_cluster || bootstrap_.dynamic_resources().has_cds_config()); if (!connect_timeout_set_) { #ifdef __APPLE__ diff --git a/test/config/utility.h b/test/config/utility.h index 0abf01c3bf60..574160651aa3 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -158,6 +158,10 @@ class ConfigHelper { // Allow a finalized configuration to be edited for generating xDS responses void applyConfigModifiers(); + // Skip validation that ensures that all upstream ports are referenced by the + // configuration generated in ConfigHelper::finalize. + void skipPortUsageValidation() { skip_port_usage_validation_ = true; } + private: // Load the first HCM struct from the first listener into a parsed proto. bool loadHttpConnectionManager( @@ -186,6 +190,11 @@ class ConfigHelper { // default). bool connect_timeout_set_{false}; + // Option to disable port usage validation for cases where the number of + // upstream ports created is expected to be larger than the number of + // upstreams in the config. + bool skip_port_usage_validation_{false}; + // A sanity check guard to make sure config is not modified after handing it to Envoy. bool finalized_{false}; }; diff --git a/test/integration/integration.h b/test/integration/integration.h index c61dfb0fc006..7cfa3b726435 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -173,6 +173,9 @@ class BaseIntegrationTest : Logger::Loggable { void setUpstreamProtocol(FakeHttpConnection::Type protocol); // Sets fake_upstreams_count_ and alters the upstream protocol in the config_helper_ void setUpstreamCount(uint32_t count) { fake_upstreams_count_ = count; } + // Skip validation that ensures that all upstream ports are referenced by the + // configuration generated in ConfigHelper::finalize. + void skipPortUsageValidation() { config_helper_.skipPortUsageValidation(); } // Make test more deterministic by using a fixed RNG value. void setDeterministic() { deterministic_ = true; } diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 4cfacefe2d4a..6f6789ee148a 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -146,9 +146,25 @@ class ClusterMemoryTestHelper : public BaseIntegrationTest { ClusterMemoryTestHelper() : BaseIntegrationTest(testing::TestWithParam::GetParam()) {} - static size_t computeMemory(int num_clusters) { + static size_t computeMemoryDelta(int initial_num_clusters, int initial_num_hosts, + int final_num_clusters, int final_num_hosts, bool allow_stats) { + // Use the same number of fake upstreams for both helpers in order to exclude memory overhead + // added by the fake upstreams. + int fake_upstreams_count = 1 + final_num_clusters * final_num_hosts; + + size_t initial_memory; + { + ClusterMemoryTestHelper helper; + helper.setUpstreamCount(fake_upstreams_count); + helper.skipPortUsageValidation(); + initial_memory = + helper.clusterMemoryHelper(initial_num_clusters, initial_num_hosts, allow_stats); + } + ClusterMemoryTestHelper helper; - return helper.clusterMemoryHelper(num_clusters, true); + helper.setUpstreamCount(fake_upstreams_count); + return helper.clusterMemoryHelper(final_num_clusters, final_num_hosts, allow_stats) - + initial_memory; } private: @@ -157,15 +173,26 @@ class ClusterMemoryTestHelper : public BaseIntegrationTest { * @param allow_stats if false, enable set_reject_all in stats_config * @return size_t the total memory allocated */ - size_t clusterMemoryHelper(int num_clusters, bool allow_stats) { + size_t clusterMemoryHelper(int num_clusters, int num_hosts, bool allow_stats) { Stats::TestUtil::MemoryTest memory_test; config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { if (!allow_stats) { bootstrap.mutable_stats_config()->mutable_stats_matcher()->set_reject_all(true); } - for (int i = 1; i < num_clusters; i++) { - auto* c = bootstrap.mutable_static_resources()->add_clusters(); - c->set_name(fmt::format("cluster_{}", i)); + for (int i = 1; i < num_clusters; ++i) { + auto* cluster = bootstrap.mutable_static_resources()->add_clusters(); + cluster->set_name(fmt::format("cluster_{}", i)); + } + + for (int i = 0; i < num_clusters; ++i) { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(i); + for (int j = 0; j < num_hosts; ++j) { + auto* host = cluster->add_hosts(); + auto* socket_address = host->mutable_socket_address(); + socket_address->set_protocol(envoy::api::v2::core::SocketAddress::TCP); + socket_address->set_address("0.0.0.0"); + socket_address->set_port_value(80); + } } }); initialize(); @@ -194,9 +221,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // A unique instance of ClusterMemoryTest allows for multiple runs of Envoy with // differing configuration. This is necessary for measuring the memory consumption // between the different instances within the same test. - const size_t m1 = ClusterMemoryTestHelper::computeMemory(1); - const size_t m1001 = ClusterMemoryTestHelper::computeMemory(1001); - const size_t m_per_cluster = (m1001 - m1) / 1000; + const size_t m1000 = ClusterMemoryTestHelper::computeMemoryDelta(1, 0, 1001, 0, true); + const size_t m_per_cluster = (m1000) / 1000; // Note: if you are increasing this golden value because you are adding a // stat, please confirm that this will be generally useful to most Envoy @@ -245,9 +271,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // A unique instance of ClusterMemoryTest allows for multiple runs of Envoy with // differing configuration. This is necessary for measuring the memory consumption // between the different instances within the same test. - const size_t m1 = ClusterMemoryTestHelper::computeMemory(1); - const size_t m1001 = ClusterMemoryTestHelper::computeMemory(1001); - const size_t m_per_cluster = (m1001 - m1) / 1000; + const size_t m1000 = ClusterMemoryTestHelper::computeMemoryDelta(1, 0, 1001, 0, true); + const size_t m_per_cluster = (m1000) / 1000; // Note: if you are increasing this golden value because you are adding a // stat, please confirm that this will be generally useful to most Envoy @@ -275,5 +300,33 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { EXPECT_MEMORY_LE(m_per_cluster, 36000); } +TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { + // A unique instance of ClusterMemoryTest allows for multiple runs of Envoy with + // differing configuration. This is necessary for measuring the memory consumption + // between the different instances within the same test. + const size_t m1000 = ClusterMemoryTestHelper::computeMemoryDelta(1, 1, 1, 1001, true); + const size_t m_per_host = (m1000) / 1000; + + // Note: if you are increasing this golden value because you are adding a + // stat, please confirm that this will be generally useful to most Envoy + // users. Otherwise you are adding to the per-host memory overhead, which + // will be significant for Envoy installations configured to talk to large + // numbers of hosts. + // + // History of golden values: + // + // Date PR Bytes Per Host Notes + // exact upper-bound + // ---------- ----- ----------------- ----- + // 2019/09/09 xxx 2883 3000 Initial per-host memory snapshot + + // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI + // 'release' builds, where we control the platform and tool-chain. So you + // will need to find the correct value only after failing CI and looking + // at the logs. + EXPECT_MEMORY_EQ(m_per_host, 2883); + EXPECT_MEMORY_LE(m_per_host, 3000); +} + } // namespace } // namespace Envoy From 9f720f44202fd16a663ae55d89539a7b8dc2cc69 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 9 Sep 2019 19:23:28 -0400 Subject: [PATCH 2/4] Add PR number to per-host memory golden value history Signed-off-by: Antonio Vicente --- test/integration/stats_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 6f6789ee148a..b05fbe7982cd 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -318,7 +318,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // Date PR Bytes Per Host Notes // exact upper-bound // ---------- ----- ----------------- ----- - // 2019/09/09 xxx 2883 3000 Initial per-host memory snapshot + // 2019/09/09 8189 2883 3000 Initial per-host memory snapshot // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you From b27a9cfc48d8c00c0ec30bf35ea648a654e5de4a Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 9 Sep 2019 20:16:16 -0400 Subject: [PATCH 3/4] Fix CI failures due to memory usage on some configs and timeout under tsan. Signed-off-by: Antonio Vicente --- test/integration/BUILD | 3 +++ test/integration/stats_integration_test.cc | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/integration/BUILD b/test/integration/BUILD index 70082725f8d9..12640ff1a654 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -548,6 +548,9 @@ envoy_cc_test( envoy_cc_test( name = "stats_integration_test", srcs = ["stats_integration_test.cc"], + # The symbol table cluster memory tests take a while to run specially under tsan. + # Shard it to avoid test timeout. + shard_count = 2, deps = [ ":integration_lib", "//source/common/memory:stats_lib", diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index b05fbe7982cd..553100c13f89 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -318,14 +318,14 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // Date PR Bytes Per Host Notes // exact upper-bound // ---------- ----- ----------------- ----- - // 2019/09/09 8189 2883 3000 Initial per-host memory snapshot + // 2019/09/09 8189 2883 3100 Initial per-host memory snapshot // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you // will need to find the correct value only after failing CI and looking // at the logs. EXPECT_MEMORY_EQ(m_per_host, 2883); - EXPECT_MEMORY_LE(m_per_host, 3000); + EXPECT_MEMORY_LE(m_per_host, 3100); } } // namespace From c7970994d108887a759a3becb41a07df8f84cb94 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 10 Sep 2019 19:13:36 -0400 Subject: [PATCH 4/4] Use real stats symbol tables on the per-host stats memory usage test case. Signed-off-by: Antonio Vicente --- test/integration/stats_integration_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 553100c13f89..2118a0e1aaa2 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -301,6 +301,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { + Stats::TestUtil::SymbolTableCreatorTestPeer::setUseFakeSymbolTables(false); + // A unique instance of ClusterMemoryTest allows for multiple runs of Envoy with // differing configuration. This is necessary for measuring the memory consumption // between the different instances within the same test. @@ -318,13 +320,13 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // Date PR Bytes Per Host Notes // exact upper-bound // ---------- ----- ----------------- ----- - // 2019/09/09 8189 2883 3100 Initial per-host memory snapshot + // 2019/09/09 8189 2739 3100 Initial per-host memory snapshot // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you // will need to find the correct value only after failing CI and looking // at the logs. - EXPECT_MEMORY_EQ(m_per_host, 2883); + EXPECT_MEMORY_EQ(m_per_host, 2739); EXPECT_MEMORY_LE(m_per_host, 3100); }