diff --git a/include/envoy/server/configuration.h b/include/envoy/server/configuration.h index f68efe6e18cbc..d3e9b3585b43b 100644 --- a/include/envoy/server/configuration.h +++ b/include/envoy/server/configuration.h @@ -146,6 +146,11 @@ class Admin { */ virtual const std::string& accessLogPath() PURE; + /** + * @return const std::string& profiler output path. + */ + virtual const std::string& profilePath() PURE; + /** * @return Network::Address::InstanceConstSharedPtr the server address. */ diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index f40baa6fa7155..00f1269f92794 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -899,6 +899,7 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "properties" : { "access_log_path" : {"type" : "string"}, + "profile_path" : {"type" : "string"}, "address" : {"type" : "string"} }, "required" : ["access_log_path", "address"], diff --git a/source/common/profiler/profiler.cc b/source/common/profiler/profiler.cc index d930e9fb37c02..bf7f6279ef2a3 100644 --- a/source/common/profiler/profiler.cc +++ b/source/common/profiler/profiler.cc @@ -9,7 +9,9 @@ namespace Profiler { bool Cpu::profilerEnabled() { return ProfilingIsEnabledForAllThreads(); } -void Cpu::startProfiler(const std::string& output_path) { ProfilerStart(output_path.c_str()); } +bool Cpu::startProfiler(const std::string& output_path) { + return ProfilerStart(output_path.c_str()); +} void Cpu::stopProfiler() { ProfilerStop(); } @@ -27,7 +29,7 @@ void Heap::forceLink() { namespace Profiler { bool Cpu::profilerEnabled() { return false; } -void Cpu::startProfiler(const std::string&) {} +bool Cpu::startProfiler(const std::string&) { return false; } void Cpu::stopProfiler() {} } // Profiler diff --git a/source/common/profiler/profiler.h b/source/common/profiler/profiler.h index fca61797d4c13..1944ac27d1dd1 100644 --- a/source/common/profiler/profiler.h +++ b/source/common/profiler/profiler.h @@ -14,8 +14,9 @@ class Cpu { /** * Start the profiler and write to the specified path. + * @return bool whether the call to start the profiler succeeded. */ - static void startProfiler(const std::string& output_path); + static bool startProfiler(const std::string& output_path); /** * Stop the profiler. diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 1ffbbb2c9a91d..0e4ce71c355d7 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -185,6 +185,7 @@ bool MainImpl::ListenerConfig::createFilterChain(Network::Connection& connection InitialImpl::InitialImpl(const Json::Object& json) { Json::ObjectPtr admin = json.getObject("admin"); admin_.access_log_path_ = admin->getString("access_log_path"); + admin_.profile_path_ = admin->getString("profile_path", "/var/log/envoy/envoy.prof"); admin_.address_ = Network::Utility::resolveUrl(admin->getString("address")); if (json.hasObject("flags_path")) { diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index f3159697cd1e8..6d3b00d3e334c 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -157,9 +157,11 @@ class InitialImpl : public Initial { struct AdminImpl : public Admin { // Server::Configuration::Initial::Admin const std::string& accessLogPath() override { return access_log_path_; } + const std::string& profilePath() override { return profile_path_; } Network::Address::InstanceConstSharedPtr address() override { return address_; } std::string access_log_path_; + std::string profile_path_; Network::Address::InstanceConstSharedPtr address_; }; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 4e6c0d0cb2aa6..387a535cbe6fe 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -163,7 +163,11 @@ Http::Code AdminImpl::handlerCpuProfiler(const std::string& url, Buffer::Instanc bool enable = query_params.begin()->second == "y"; if (enable && !Profiler::Cpu::profilerEnabled()) { - Profiler::Cpu::startProfiler("/var/log/envoy/envoy.prof"); + if (!Profiler::Cpu::startProfiler(profile_path_)) { + response.add("failure to start the profiler"); + return Http::Code::InternalServerError; + } + } else if (!enable && Profiler::Cpu::profilerEnabled()) { Profiler::Cpu::stopProfiler(); } @@ -295,9 +299,10 @@ void AdminFilter::onComplete() { AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider() : config_(new Router::NullConfigImpl()) {} -AdminImpl::AdminImpl(const std::string& access_log_path, +AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profile_path, Network::Address::InstanceConstSharedPtr address, Server::Instance& server) - : server_(server), socket_(new Network::TcpListenSocket(address, true)), + : server_(server), profile_path_(profile_path), + socket_(new Network::TcpListenSocket(address, true)), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), tracing_stats_(Http::ConnectionManagerImpl::generateTracingStats("http.admin.tracing.", server_.stats())), diff --git a/source/server/http/admin.h b/source/server/http/admin.h index d3dba3cf4488d..b6b29f9a7a806 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -22,8 +22,8 @@ class AdminImpl : public Admin, public Http::ConnectionManagerConfig, Logger::Loggable { public: - AdminImpl(const std::string& access_log_path, Network::Address::InstanceConstSharedPtr address, - Server::Instance& server); + AdminImpl(const std::string& access_log_path, const std::string& profiler_path, + Network::Address::InstanceConstSharedPtr address, Server::Instance& server); Http::Code runCallback(const std::string& path, Buffer::Instance& response); Network::ListenSocket& socket() { return *socket_; } @@ -113,6 +113,7 @@ class AdminImpl : public Admin, Server::Instance& server_; std::list access_logs_; + const std::string profile_path_; Network::ListenSocketPtr socket_; Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; diff --git a/source/server/server.cc b/source/server/server.cc index deef64e89e372..cc9cd2a9deb69 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -170,7 +170,8 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks, drain_manager_->startParentShutdownSequence(); original_start_time_ = info.original_start_time_; admin_.reset(new AdminImpl(initial_config.admin().accessLogPath(), - initial_config.admin().address(), *this)); + initial_config.admin().profilePath(), initial_config.admin().address(), + *this)); admin_scope_ = stats_store_.createScope("listener.admin."); handler_.addListener(*admin_, admin_->socket(), *admin_scope_, Network::ListenerOptions::listenerOptionsWithBindToPort()); diff --git a/test/config/integration/server.json b/test/config/integration/server.json index ce3e987809931..b7c38a1e656ca 100644 --- a/test/config/integration/server.json +++ b/test/config/integration/server.json @@ -244,7 +244,7 @@ }] }], - "admin": { "access_log_path": "/dev/null", "address": "tcp://127.0.0.1:10003" }, + "admin": { "access_log_path": "/dev/null", "profile_path": "/tmp/envoy.prof", "address": "tcp://127.0.0.1:10003" }, "flags_path": "/invalid_flags", "statsd_local_udp_port": 8125, "statsd_tcp_cluster_name": "statsd", diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 16190ae0ae9d4..ead6c6d90c7c3 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -105,28 +105,34 @@ TEST_F(IntegrationTest, Admin) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("400", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/hot_restart_version", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/reset_counters", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/hot_restart_version", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/certs", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); +} - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/reset_counters", "", - Http::CodecClient::Type::HTTP1); +// Successful call to startProfiler requires tcmalloc. +#ifdef TCMALLOC + +TEST_F(IntegrationTest, AdminCpuProfilerStart) { + BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( + ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/certs", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); } +#endif diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 1cb183cc682e0..f2deb3b850f38 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1,4 +1,5 @@ #include "common/http/message_impl.h" +#include "common/profiler/profiler.h" #include "server/http/admin.h" #include "test/mocks/server/mocks.h" @@ -12,8 +13,10 @@ namespace Server { class AdminFilterTest : public testing::Test { public: // TODO(mattklein123): Switch to mocks and do not bind to a real port. + // TODO(htuch): Use proper temporary path allocation method. AdminFilterTest() - : admin_("/dev/null", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), + : admin_("/dev/null", "/tmp/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), + server_), filter_(admin_), request_headers_{{":path", "/"}} { filter_.setDecoderFilterCallbacks(callbacks_); } @@ -45,4 +48,27 @@ TEST_F(AdminFilterTest, Trailers) { filter_.decodeTrailers(request_headers_); } +// Can only get code coverage of AdminImpl::handlerCpuProfiler stopProfiler with +// a real profiler linked in (successful call to startProfiler). startProfiler +// requies tcmalloc. +#ifdef TCMALLOC + +TEST_F(AdminFilterTest, AdminProfiler) { + Buffer::OwnedImpl data; + admin_.runCallback("/cpuprofiler?enable=y", data); + EXPECT_TRUE(Profiler::Cpu::profilerEnabled()); + admin_.runCallback("/cpuprofiler?enable=n", data); + EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); +} + +#endif + +TEST_F(AdminFilterTest, AdminBadProfiler) { + Buffer::OwnedImpl data; + AdminImpl admin_bad_profile_path("/dev/null", "/some/unlikely/bad/path.prof", + Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_); + admin_bad_profile_path.runCallback("/cpuprofiler?enable=y", data); + EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); +} + } // namespace Server