Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
36 changes: 36 additions & 0 deletions test/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,42 @@ envoy_benchmark_test(
benchmark_binary = "eds_speed_test",
)

envoy_cc_benchmark_binary(
name = "cds_speed_test",
srcs = ["cds_speed_test.cc"],
external_deps = [
"benchmark",
],
deps = [
":utility_lib",
"//source/common/config:grpc_mux_lib",
"//source/common/config:grpc_subscription_lib",
"//source/common/config:protobuf_link_hacks",
"//source/common/config:utility_lib",
"//source/common/upstream:eds_lib",
"//source/extensions/transport_sockets/raw_buffer:config",
"//source/server:transport_socket_config_lib",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:admin_mocks",
"//test/mocks/server:instance_mocks",
"//test/mocks/ssl:ssl_mocks",
"//test/mocks/upstream:cluster_manager_mocks",
"//test/test_common:test_runtime_lib",
"//test/test_common:utility_lib",
Comment thread
htuch marked this conversation as resolved.
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto",
"@envoy_api//envoy/service/discovery/v3:pkg_cc_proto",
],
)

envoy_benchmark_test(
name = "cds_speed_test_benchmark_test",
benchmark_binary = "cds_speed_test",
)

envoy_cc_test_library(
name = "health_check_fuzz_utils_lib",
srcs = [
Expand Down
191 changes: 191 additions & 0 deletions test/common/upstream/cds_speed_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
// Note: this should be run with --compilation_mode=opt, and would benefit from a
// quiescent system with disabled cstate power management.

#include "envoy/config/cluster/v3/cluster.pb.h"
#include "envoy/config/core/v3/health_check.pb.h"
#include "envoy/config/endpoint/v3/endpoint.pb.h"
#include "envoy/config/endpoint/v3/endpoint_components.pb.h"
#include "envoy/service/discovery/v3/discovery.pb.h"
#include "envoy/stats/scope.h"

#include "common/config/grpc_mux_impl.h"
#include "common/config/grpc_subscription_impl.h"
#include "common/config/utility.h"
#include "common/singleton/manager_impl.h"
#include "common/upstream/eds.h"

#include "server/transport_socket_config_impl.h"

#include "test/benchmark/main.h"
#include "test/common/upstream/utility.h"
#include "test/mocks/local_info/mocks.h"
#include "test/mocks/protobuf/mocks.h"
#include "test/mocks/runtime/mocks.h"
#include "test/mocks/server/admin.h"
#include "test/mocks/server/instance.h"
#include "test/mocks/ssl/mocks.h"
#include "test/mocks/upstream/cluster_manager.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/utility.h"

#include "benchmark/benchmark.h"

using ::benchmark::State;
using Envoy::benchmark::skipExpensiveBenchmarks;

namespace Envoy {
namespace Upstream {

class CdsSpeedTest {
public:
CdsSpeedTest(State& state, bool v2_config)
: state_(state), v2_config_(v2_config),
type_url_(v2_config_
? "type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"
: "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"),
subscription_stats_(Config::Utility::generateStats(stats_)),
api_(Api::createApiForTest(stats_)), async_client_(new Grpc::MockAsyncClient()),
grpc_mux_(new Config::GrpcMuxImpl(
local_info_, std::unique_ptr<Grpc::MockAsyncClient>(async_client_), dispatcher_,
*Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
"envoy.service.endpoint.v3.EndpointDiscoveryService.StreamEndpoints"),
Comment thread
pgenera marked this conversation as resolved.
Outdated
envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, {}, true)) {

resetCluster(R"EOF(
name: name
connect_timeout: 0.25s
type: EDS

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.

In terms of Cluster configuration, I think there are ultimately many dimensions to consider, but the two I'd start with are number of clusters, number of endpoints per cluster and the choice of load balancing algorithm (everything from WRR to Maglev etc.). This will provide a pretty interesting microbenchmark.

@pgenera pgenera Dec 7, 2020

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.

Right now this exercises the number of clusters for both the v2 & v3 APIs; I propose taking out the v2 part and adding LB algorithm & endpoints per cluster, all for STATIC clusters. Does that sound like a good next revision?

In the interim I've separated out the v2 & v3 tests to make the output more readable and give the Complexity Computing Magic a better chance at working.

I haven't generated interesting output, perf or otherwise, as I just got this working again. I'm happy to do so once we know I'm testing the right things.

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.

Yes, let's drop v2 as it's on the way out. Agree on the changes proposed.

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.

FWIW, I think we could also explore LB algorithm and endpoints in the EDS test if it was able to fully simulate what is going on in eds_speed_test.cc, so maybe we can make CDS test even simpler and exercise some other cluster attributes in the parameter space. For now just dropping v2 sounds good.

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.

I've dropped v2; maybe adding a larger set of test parameters to this or cds should go in a followup?

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.

Let's do the additions in followup PRs.

eds_cluster_config:
Comment thread
pgenera marked this conversation as resolved.
Outdated
service_name: fare
eds_config:
api_config_source:
cluster_names:
- eds
refresh_delay: 1s
)EOF",
Envoy::Upstream::Cluster::InitializePhase::Secondary);

EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_, _));
cluster_->initialize([this] { initialized_ = true; });
EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(testing::Return(&async_stream_));
subscription_->start({"fare"});
}

void resetCluster(const std::string& yaml_config, Cluster::InitializePhase initialize_phase) {
local_info_.node_.mutable_locality()->set_zone("us-east-1a");
eds_cluster_ = parseClusterFromV3Yaml(yaml_config);
Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format(
"cluster.{}.",
eds_cluster_.alt_stat_name().empty() ? eds_cluster_.name() : eds_cluster_.alt_stat_name()));
Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context(
admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_,
singleton_manager_, tls_, validation_visitor_, *api_);
cluster_ = std::make_shared<EdsClusterImpl>(eds_cluster_, runtime_, factory_context,
std::move(scope), false);
EXPECT_EQ(initialize_phase, cluster_->initializePhase());
eds_callbacks_ = cm_.subscription_factory_.callbacks_;
subscription_ = std::make_unique<Config::GrpcSubscriptionImpl>(
grpc_mux_, *eds_callbacks_, resource_decoder_, subscription_stats_, type_url_, dispatcher_,
std::chrono::milliseconds(), false);
}

void clusterHelper(bool ignore_unknown_dynamic_fields, size_t num_clusters) {
state_.PauseTiming();

auto response = std::make_unique<envoy::service::discovery::v3::DiscoveryResponse>();
response->set_type_url(type_url_);
response->set_version_info(fmt::format("version-{}", version_++));

// make a pile of dynamic clusters and add them to the response
for (size_t i = 0; i < num_clusters; ++i) {
envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
cluster_load_assignment.set_cluster_name("fare_" + std::to_string(i));

auto* endpoints = cluster_load_assignment.add_endpoints();
endpoints->set_priority(1);
auto* locality = endpoints->mutable_locality();
locality->set_region("region");
locality->set_zone("zone");
locality->set_sub_zone("sub_zone");

auto* resource = response->mutable_resources()->Add();
resource->PackFrom(cluster_load_assignment);
if (v2_config_) {
RELEASE_ASSERT(resource->type_url() ==
"type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
"");
resource->set_type_url("type.googleapis.com/envoy.api.v2.ClusterLoadAssignment");
}
}
validation_visitor_.setSkipValidation(ignore_unknown_dynamic_fields);

state_.SetComplexityN(num_clusters);
state_.ResumeTiming();
grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response));
}

TestDeprecatedV2Api _deprecated_v2_api_;
Comment thread
pgenera marked this conversation as resolved.
Outdated
State& state_;
const bool v2_config_;
const std::string type_url_;
uint64_t version_{};
bool initialized_{};
Stats::IsolatedStoreImpl stats_;
Config::SubscriptionStats subscription_stats_;
Ssl::MockContextManager ssl_context_manager_;
envoy::config::cluster::v3::Cluster eds_cluster_;
NiceMock<MockClusterManager> cm_;
NiceMock<Event::MockDispatcher> dispatcher_;
EdsClusterImplSharedPtr cluster_;
Config::SubscriptionCallbacks* eds_callbacks_{};
Config::OpaqueResourceDecoderImpl<envoy::config::endpoint::v3::ClusterLoadAssignment>
resource_decoder_{validation_visitor_, "cluster_name"};
NiceMock<Random::MockRandomGenerator> random_;
NiceMock<Runtime::MockLoader> runtime_;
NiceMock<LocalInfo::MockLocalInfo> local_info_;
NiceMock<Server::MockAdmin> admin_;
Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
NiceMock<ThreadLocal::MockInstance> tls_;
ProtobufMessage::MockValidationVisitor validation_visitor_;
Api::ApiPtr api_;
Grpc::MockAsyncClient* async_client_;
NiceMock<Grpc::MockAsyncStream> async_stream_;
Config::GrpcMuxImplSharedPtr grpc_mux_;
Config::GrpcSubscriptionImplPtr subscription_;
};

} // namespace Upstream
} // namespace Envoy

static void addClusters(State& state) {
Envoy::Thread::MutexBasicLockable lock;
Envoy::Logger::Context logging_state(spdlog::level::warn,
Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false);
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
Envoy::Upstream::CdsSpeedTest speed_test(state, state.range(0));
// if we've been instructed to skip tests, only run once no matter the argument:
uint32_t clusters = skipExpensiveBenchmarks() ? 1 : state.range(2);
Comment thread
pgenera marked this conversation as resolved.
Outdated
speed_test.clusterHelper(state.range(1), clusters);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't this also include the time it took to build the response proto in the benchmark?

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.

I don't believe so; the calls to state_.PauseTiming() and ResumeTiming() in clusterHelper should exclude everything but the grpc callback?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be clearer I think what we were benchmarking if we put the PauseTiming calls at the start of the loop here (line 162) and Resume at the end, and then put Resume/Pause calls in the helper functions surrounding the code we really want to test.

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.

I think this is what you mean.

}
}

BENCHMARK(addClusters)
->Ranges({{false, true}, {false, true}, {64, 100000}})
->Unit(benchmark::kMillisecond)
->Complexity();

static void duplicateUpdate(State& state) {
Envoy::Thread::MutexBasicLockable lock;
Envoy::Logger::Context logging_state(spdlog::level::warn,
Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false);

for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
Envoy::Upstream::CdsSpeedTest speed_test(state, false);
uint32_t clusters = skipExpensiveBenchmarks() ? 1 : state.range(0);
Comment thread
pgenera marked this conversation as resolved.
Outdated

speed_test.clusterHelper(true, clusters);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you comment a bit on a few choices here:

  1. Why call speed_test.clusterHelper(true, clusters); twice?
  2. Why not construct CdsSpeedTest outside the loop?
  3. Related: should we put the Envoy::Logger::Context into the class to avoid repeating that code for each test?

I'm not pushing back against those decisions I'm just trying to understand them.

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.

  1. that's the difference between the two test cases; I added a comment (hopefully) explaining.
  2. I assume the loop here is modifying state in a meaningful way, which is then passed to the newly constructed speed_test. This auto _ : state is directly from the example in https://github.com/google/benchmark
  3. y'know, I don't know what the Logger Context was doing here, and removing it has no ill effect...

This was mostly copy & pasted from eds_speed_test (I don't understand github's copy detection and why it didn't notice this); I've backported the changes to eds_speed_test as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RE 1 and 3: cool, all set, thanks.

Re 2: the auto _ state : state pattern is fine, but I think state.range(x) won't change within a call, and it would be safe to construct the CdsSpeedTest object outside the loop. I think that will result in a more focused perf benchmark. WDYT?

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.

Testing bears out your hypothesis, I see what I missed there.

speed_test.clusterHelper(true, clusters);
}
}

BENCHMARK(duplicateUpdate)->Range(64, 100000)->Unit(benchmark::kMillisecond)->Complexity();
6 changes: 3 additions & 3 deletions test/common/upstream/eds_speed_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ static void priorityAndLocalityWeighted(State& state) {
Envoy::Thread::MutexBasicLockable lock;
Envoy::Logger::Context logging_state(spdlog::level::warn,
Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false);
for (auto _ : state) {
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
Envoy::Upstream::EdsSpeedTest speed_test(state, state.range(0));
// if we've been instructed to skip tests, only run once no matter the argument:
uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(2);
Expand All @@ -195,7 +195,7 @@ static void duplicateUpdate(State& state) {
Envoy::Logger::Context logging_state(spdlog::level::warn,
Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false);

for (auto _ : state) {
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
Envoy::Upstream::EdsSpeedTest speed_test(state, false);
uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(0);

Expand All @@ -210,7 +210,7 @@ static void healthOnlyUpdate(State& state) {
Envoy::Thread::MutexBasicLockable lock;
Envoy::Logger::Context logging_state(spdlog::level::warn,
Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false);
for (auto _ : state) {
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
Envoy::Upstream::EdsSpeedTest speed_test(state, false);
uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(0);

Expand Down
1 change: 1 addition & 0 deletions tools/spelling/spelling_dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ deallocate
deallocated
deallocating
deallocation
deadcode
dec
dechunk
dechunked
Expand Down