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

stats: Add per-host memory usage test case to stats_integration_test #8189

Merged
merged 4 commits into from
Sep 11, 2019
Merged

stats: Add per-host memory usage test case to stats_integration_test #8189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d6b5a9
Add per-host memory usage test cast to stats_integration_test
antoniovicente Sep 9, 2019
9f720f4
Add PR number to per-host memory golden value history
antoniovicente Sep 9, 2019
b27a9cf
Fix CI failures due to memory usage on some configs and timeout under…
antoniovicente Sep 10, 2019
c797099
Use real stats symbol tables on the per-host stats memory usage test …
antoniovicente Sep 10, 2019
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
4 changes: 2 additions & 2 deletions test/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,8 @@ void ConfigHelper::finalize(const std::vector<uint32_t>& 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__
Expand Down
9 changes: 9 additions & 0 deletions test/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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};
};
Expand Down
3 changes: 3 additions & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions test/integration/integration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ class BaseIntegrationTest : Logger::Loggable<Logger::Id::testing> {
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; }

Expand Down
79 changes: 67 additions & 12 deletions test/integration/stats_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,25 @@ class ClusterMemoryTestHelper : public BaseIntegrationTest {
ClusterMemoryTestHelper()
: BaseIntegrationTest(testing::TestWithParam<Network::Address::IpVersion>::GetParam()) {}

static size_t computeMemory(int num_clusters) {
static size_t computeMemoryDelta(int initial_num_clusters, int initial_num_hosts,
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefer explicit unsigned types vs. int, e.g., uint32_t, but not your code and feel free to cleanup in a follow up.

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:
Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -275,5 +300,35 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
EXPECT_MEMORY_LE(m_per_cluster, 36000);
}

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.
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 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, 2739);
EXPECT_MEMORY_LE(m_per_host, 3100);
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for doing this! Can you also make a RealSymbolTable variant, with

  Stats::TestUtil::SymbolTableCreatorTestPeer::setUseFakeSymbolTables(false);

as the first line, and updated target memory values?

FYI if you didn't already know this you can test this locally with:

 --compilation_mode=opt --test_env=ENVOY_MEMORY_TEST_EXACT=true

Copy link
Contributor Author

@antoniovicente antoniovicente Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Memory usage without fake symbol tables is 2739

I rather keep a single variant of this test since it takes a while to run and upcoming changes in the branch below remove use of symbol tables for host stats. With my upcoming change, memory usage per host is 1283 bytes when using either symbol table implementation. How about I have the test disable fake symbol tables?

master...antoniovicente:stats_primitive_host_stats

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Waiting for CI to process the change.

}

} // namespace
} // namespace Envoy