Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
e696fe1
Factor out most of the contents of main() into a new MainCommon class…
jmarantz Feb 6, 2018
d796786
Formatting fixups.
jmarantz Feb 6, 2018
622e5d2
Get all tests working with minimal changes.
jmarantz Feb 8, 2018
6a87f69
formatting fixups
jmarantz Feb 8, 2018
de78e95
streamline the validation flow.
jmarantz Feb 9, 2018
3e5e52e
Merge branch 'master' into main-common-as-class
jmarantz Feb 9, 2018
53f3ec0
mutex-protect Server::InstanceImpl::terminate_
jmarantz Feb 9, 2018
b9d284b
formatting fixups
jmarantz Feb 9, 2018
2ff4f4a
Fixes problems found after after retesting new main_common with old m…
jmarantz Feb 9, 2018
b28fa8e
fix struct/class conflict which I saw only in CI
jmarantz Feb 9, 2018
ccd38ad
add missing override.
jmarantz Feb 9, 2018
4d2bc12
Use TEST_RUNDIR rather than TEST_SRCDIR.
jmarantz Feb 9, 2018
4bd2da7
Specify envoy-standard return comment.
jmarantz Feb 9, 2018
c84e9b4
Test the legacy-main past using an old copy of main() in a test.
jmarantz Feb 9, 2018
8e0262a
formatting fixup.
jmarantz Feb 9, 2018
928deb7
Just get a writable char* out of std::string by taking a pointer to c…
jmarantz Feb 9, 2018
94e9b12
Try to get CI coverage tests passing by disabling main_common_test.
jmarantz Feb 10, 2018
1763e71
Try using #if 0 to disable a test for coverage, as ifndef ENVOY_CONFI…
jmarantz Feb 10, 2018
2ba3207
Try using #if !ENVOY_CONFIG_COVERAGE rather than #ifndef to skip this…
jmarantz Feb 10, 2018
a9237fe
Define ENVOY_CONFIG_COVERAGE as a -D flag in addition to a bazel sett…
jmarantz Feb 11, 2018
57ef61f
Fix the --cxxopt syntax.
jmarantz Feb 11, 2018
3b469a4
Raise coverage timeout to 1000s and make test output streamed.
jmarantz Feb 11, 2018
f45099c
use EXIT_SUCCESS/EXIT_FAILURE rather than 0/1 for main() return codes.
jmarantz Feb 11, 2018
552cb2a
use EXIT_SUCCESS/FAILURE in the test too.
jmarantz Feb 11, 2018
4914e62
Catch one more EXIT_xxx xform.
jmarantz Feb 11, 2018
58a0180
Remove mutex around terminate_ after convincing myself it wasn't nece…
jmarantz Feb 11, 2018
8cb20e7
Switch the new test config to yaml format.
jmarantz Feb 11, 2018
ada35db
Cleanups as suggested by mattklein123.
jmarantz Feb 11, 2018
ce29a45
Move test yaml to test/config/integration, clarify try/catch discipli…
jmarantz Feb 11, 2018
c2bb454
Move overrides into the existing group of overrides.
jmarantz Feb 11, 2018
92a9544
add missing file.
jmarantz Feb 11, 2018
22a2e80
Attempt to get main_common_test into coverage by just disabling HANDL…
jmarantz Feb 11, 2018
d1cf0b9
Revert "Attempt to get main_common_test into coverage by just disabli…
jmarantz Feb 11, 2018
6372516
Clean up argv, which is surprisingly messy.
jmarantz Feb 12, 2018
1e29195
Address comments from hutch.
jmarantz Feb 13, 2018
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
5 changes: 4 additions & 1 deletion include/envoy/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ envoy_cc_library(
envoy_cc_library(
name = "hot_restart_interface",
hdrs = ["hot_restart.h"],
deps = ["//include/envoy/event:dispatcher_interface"],
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/thread:thread_interface",
],
)

envoy_cc_library(
Expand Down
17 changes: 17 additions & 0 deletions include/envoy/server/hot_restart.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#include "envoy/common/pure.h"
#include "envoy/event/dispatcher.h"
#include "envoy/stats/stats.h"
#include "envoy/thread/thread.h"

namespace Envoy {
namespace Server {
Expand Down Expand Up @@ -78,6 +80,21 @@ class HotRestart {
* perform a full or hot restart.
*/
virtual std::string version() PURE;

/**
* @return Thread::BasicLockable& a lock for logging.
*/
virtual Thread::BasicLockable& logLock() PURE;

/**
* @return Thread::BasicLockable& a lock for access logs.
*/
virtual Thread::BasicLockable& accessLogLock() PURE;

/**
* @returns an allocator for stats.
*/
virtual Stats::RawStatDataAllocator& statsAllocator() PURE;
};

} // namespace Server
Expand Down
24 changes: 24 additions & 0 deletions include/envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,5 +281,29 @@ class StoreRoot : public Store {

typedef std::unique_ptr<StoreRoot> StoreRootPtr;

struct RawStatData;

/**
* Abstract interface for allocating a RawStatData.
*/
class RawStatDataAllocator {
public:
virtual ~RawStatDataAllocator() {}

/**
* @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no
* more memory available for stats. The allocator should return a reference counted
* data location by name if one already exists with the same name. This is used for
* intra-process scope swapping as well as inter-process hot restart.
*/
virtual RawStatData* alloc(const std::string& name) PURE;

/**
* Free a raw stat data block. The allocator should handle reference counting and only truly
* free the block if it is no longer needed.
*/
virtual void free(RawStatData& data) PURE;
};

} // namespace Stats
} // namespace Envoy
22 changes: 0 additions & 22 deletions source/common/stats/stats_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,28 +191,6 @@ struct RawStatData {
static size_t& initializeAndGetMutableMaxObjNameLength(size_t configured_size);
};

/**
* Abstract interface for allocating a RawStatData.
*/
class RawStatDataAllocator {
public:
virtual ~RawStatDataAllocator() {}

/**
* @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no more
* memory available for stats. The allocator may return a reference counted data location
* by name if one already exists with the same name. This is used for intra-process
* scope swapping as well as inter-process hot restart.
*/
virtual RawStatData* alloc(const std::string& name) PURE;

/**
* Free a raw stat data block. The allocator should handle reference counting and only truly
* free the block if it is no longer needed.
*/
virtual void free(RawStatData& data) PURE;
};

/**
* Implementation of the Metric interface. Virtual inheritance is used because the interfaces that
* will inherit from Metric will have other base classes that will also inherit from Metric.
Expand Down
12 changes: 5 additions & 7 deletions source/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,9 @@ envoy_cc_library(
deps = [
":envoy_main_common_lib",
":extra_protocol_proxies_lib",
"//source/server:hot_restart_lib",
"//source/server:options_lib",
"//source/server/config/http:lightstep_lib",
"//source/server/config/http:zipkin_lib",
] + select({
"//bazel:disable_signal_trace": [],
"//conditions:default": [":sigaction_lib"],
}),
],
)

envoy_cc_library(
Expand All @@ -100,7 +95,10 @@ envoy_cc_library(
"//source/server:hot_restart_nop_lib",
"//source/server:proto_descriptors_lib",
"//source/server/config_validation:server_lib",
],
] + select({
"//bazel:disable_signal_trace": [],
Copy link
Member

Choose a reason for hiding this comment

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

Did you test with --define signal_trace=disabled? We don't have any CI tests to catch these variants of Bazel build, so when moving things around it's probably a good idea to manually verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Doing so now. Actually, why is this a compile-time decision rather than an option? Then it would be easier to test without rebuilding the entire tree. Same with HotRestart, though that's addressed in #2568.

Copy link
Member

Choose a reason for hiding this comment

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

Some of these options are compile-time as we want to squash dependencies (e.g. Google gRPC) or demonstrate that site-local replacement or build time removal of features is possible (e.g. we replace during import on the Google side). Hot restart is probably a better contender to be fully dynamic, as it doesn't introduce any deps.

There's a wider conversation happening right now in #2069. Basically, we need some way to provide greater build time flexibility to allow very minimal Envoy binary deployments while also support kitchen sink builds with dynamic configuration. The former might be interesting in resource constrained embedded or container environments, the latter on a traditional fat server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't arguing against any compile-time switches, but it wasn't obvious what dependency this one had. I was thinking that if we were compiling on Windows and the APIs didn't exist, we'd just #if them out based on platform support as opposed to config, but keep the class API above that consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the signal_trace deps are the Backtrace library, an external dependency.

"//conditions:default": [":sigaction_lib"],
}),
)

envoy_cc_library(
Expand Down
50 changes: 16 additions & 34 deletions source/exe/main.cc
Original file line number Diff line number Diff line change
@@ -1,20 +1,5 @@
#include <iostream>
#include <memory>

#include "exe/main_common.h"

#ifdef ENVOY_HANDLE_SIGNALS
#include "exe/signal_action.h"
#endif

#ifdef ENVOY_HOT_RESTART
#include "server/hot_restart_impl.h"
#endif

#include "server/options_impl.h"

#include "spdlog/spdlog.h"

// NOLINT(namespace-envoy)

/**
Expand All @@ -25,31 +10,28 @@
* after setting up command line options.
*/
int main(int argc, char** argv) {
#ifdef ENVOY_HANDLE_SIGNALS
// Enabled by default. Control with "bazel --define=signal_trace=disabled"
Envoy::SignalAction handle_sigs;
#endif

#ifdef ENVOY_HOT_RESTART
// Enabled by default, except on OS X. Control with "bazel --define=hot_restart=disabled"
const Envoy::OptionsImpl::HotRestartVersionCb hot_restart_version_cb =
[](uint64_t max_num_stats, uint64_t max_stat_name_len) {
return Envoy::Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len);
};
constexpr bool enable_hot_restart = true;
#else
const Envoy::OptionsImpl::HotRestartVersionCb hot_restart_version_cb = [](uint64_t, uint64_t) {
return "disabled";
};
constexpr bool enable_hot_restart = false;
#endif

std::unique_ptr<Envoy::OptionsImpl> options;
std::unique_ptr<Envoy::MainCommon> main_common;

// Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE
// as needed. Whatever code in the initialization path that fails is expected to log an error
// message so the user can diagnose.
try {
options = std::make_unique<Envoy::OptionsImpl>(argc, argv, hot_restart_version_cb,
spdlog::level::info);
main_common = std::make_unique<Envoy::MainCommon>(argc, argv, enable_hot_restart);
} catch (const Envoy::NoServingException& e) {
return 0;
return EXIT_SUCCESS;
} catch (const Envoy::MalformedArgvException& e) {
return 1;
return EXIT_FAILURE;
} catch (const Envoy::EnvoyException& e) {
return EXIT_FAILURE;
}
return Envoy::main_common(*options);

// Run the server listener loop outside try/catch blocks, so that unexpected exceptions
// show up as a core-dumps for easier diagnostis.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: diagnostics

return main_common->run() ? EXIT_SUCCESS : EXIT_FAILURE;
}
149 changes: 89 additions & 60 deletions source/exe/main_common.cc
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
#include "exe/main_common.h"

#include <iostream>
#include <memory>

#include "common/common/compiler_requirements.h"
#include "common/event/libevent.h"
#include "common/network/utility.h"
#include "common/stats/stats_impl.h"
#include "common/stats/thread_local_store.h"

#include "server/config_validation/server.h"
#include "server/drain_manager_impl.h"
Expand All @@ -22,79 +23,107 @@
#include "ares.h"

namespace Envoy {
namespace Server {

class ProdComponentFactory : public ComponentFactory {
public:
// Server::DrainManagerFactory
DrainManagerPtr createDrainManager(Instance& server) override {
return DrainManagerPtr{
// The global drain manager only triggers on listener modification, which effectively is
// hot restart at the global level. The per-listener drain managers decide whether to
// to include /healthcheck/fail status.
new DrainManagerImpl(server, envoy::api::v2::Listener_DrainType_MODIFY_ONLY)};
}

Runtime::LoaderPtr createRuntime(Server::Instance& server,
Server::Configuration::Initial& config) override {
return Server::InstanceUtil::createRuntime(server, config);
}
};
Server::DrainManagerPtr ProdComponentFactory::createDrainManager(Server::Instance& server) {
// The global drain manager only triggers on listener modification, which effectively is
// hot restart at the global level. The per-listener drain managers decide whether to
// to include /healthcheck/fail status.
return std::make_unique<Server::DrainManagerImpl>(server,
envoy::api::v2::Listener_DrainType_MODIFY_ONLY);
}

} // namespace Server
Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server,
Server::Configuration::Initial& config) {
return Server::InstanceUtil::createRuntime(server, config);
}

int main_common(OptionsImpl& options) {
Stats::RawStatData::configure(options);
MainCommonBase::MainCommonBase(OptionsImpl& options, bool hot_restart) : options_(options) {
ares_library_init(ARES_LIB_INIT_ALL);
Event::Libevent::Global::initialize();
RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors());

switch (options_.mode()) {
case Server::Mode::Serve: {
#ifdef ENVOY_HOT_RESTART
std::unique_ptr<Server::HotRestartImpl> restarter;
try {
restarter.reset(new Server::HotRestartImpl(options));
} catch (Envoy::EnvoyException& e) {
std::cerr << "unable to initialize hot restart: " << e.what() << std::endl;
return 1;
if (hot_restart) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah doing the --disable-hot-restart flag will be trivial now.

restarter_.reset(new Server::HotRestartImpl(options_));
}
#endif
if (!hot_restart) {
restarter_.reset(new Server::HotRestartNopImpl());
}

Stats::RawStatData::configure(options_);
tls_.reset(new ThreadLocal::InstanceImpl);
Thread::BasicLockable& log_lock = restarter_->logLock();
Thread::BasicLockable& access_log_lock = restarter_->accessLogLock();
auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion());
Logger::Registry::initialize(options_.logLevel(), log_lock);

stats_store_.reset(new Stats::ThreadLocalStoreImpl(restarter_->statsAllocator()));
server_.reset(new Server::InstanceImpl(options_, local_address, default_test_hooks_,
*restarter_, *stats_store_, access_log_lock,
component_factory_, *tls_));
break;
}
case Server::Mode::Validate:
break;
}
}

Thread::BasicLockable& log_lock = restarter->logLock();
Thread::BasicLockable& access_log_lock = restarter->accessLogLock();
Stats::RawStatDataAllocator& stats_allocator = *restarter;
#else
std::unique_ptr<Server::HotRestartNopImpl> restarter;
restarter.reset(new Server::HotRestartNopImpl());

Thread::MutexBasicLockable log_lock, access_log_lock;
Stats::HeapRawStatDataAllocator stats_allocator;
#endif
MainCommonBase::~MainCommonBase() { ares_library_cleanup(); }

RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors());
Event::Libevent::Global::initialize();
Server::ProdComponentFactory component_factory;
auto local_address = Network::Utility::getLocalAddress(options.localAddressIpVersion());
switch (options.mode()) {
bool MainCommonBase::run() {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a better name for this? This capture historically what was in the common main, but I think logically it's about overall server setup, teardown and run. We already have a Server::Instance, so maybe Server::Cli, Server::Launcher, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK renaming it but that would also lose any kind of a/b correlation in the review. AFAIK there's no way to tell git that a file was renamed; it either figures it out or it doesn't, and mostly it doesn't :)

Can this be a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Either later or in a final pass before merging, either works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks; I'll put this in a follow-up.

switch (options_.mode()) {
case Server::Mode::Serve:
break;
case Server::Mode::Validate:
Thread::MutexBasicLockable log_lock;
Logger::Registry::initialize(options.logLevel(), log_lock);
return Server::validateConfig(options, local_address, component_factory) ? 0 : 1;
server_->run();
return true;
case Server::Mode::Validate: {
auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion());
return Server::validateConfig(options_, local_address, component_factory_);
}
}
NOT_REACHED;
}

ares_library_init(ARES_LIB_INIT_ALL);
MainCommon::MainCommon(int argc, char** argv, bool hot_restart)
: options_(computeOptions(argc, argv, hot_restart)), base_(*options_, hot_restart) {}

std::unique_ptr<OptionsImpl> MainCommon::computeOptions(int argc, char** argv, bool hot_restart) {
OptionsImpl::HotRestartVersionCb hot_restart_version_cb = [](uint64_t, uint64_t) {
return "disabled";
};

Logger::Registry::initialize(options.logLevel(), log_lock);
DefaultTestHooks default_test_hooks;
ThreadLocal::InstanceImpl tls;
Stats::ThreadLocalStoreImpl stats_store(stats_allocator);
#ifdef ENVOY_HOT_RESTART
if (hot_restart) {
// Enabled by default, except on OS X. Control with "bazel --define=hot_restart=disabled"
hot_restart_version_cb = [](uint64_t max_num_stats, uint64_t max_stat_name_len) {
return Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len);
};
}
#else
// Hot-restart should not be specified if the support is not compiled in.
RELEASE_ASSERT(!hot_restart);
#endif
return std::make_unique<OptionsImpl>(argc, argv, hot_restart_version_cb, spdlog::level::info);
}

// Legacy implementation of main_common.
//
// TODO(jmarantz): Remove this when all callers are removed. At that time, MainCommonBase
// and MainCommon can be merged. The current theory is that only Google calls this.
int main_common(OptionsImpl& options) {
try {
Server::InstanceImpl server(options, local_address, default_test_hooks, *restarter, stats_store,
access_log_lock, component_factory, tls);
server.run();
} catch (const EnvoyException& e) {
ares_library_cleanup();
return 1;
#if ENVOY_HOT_RESTART
MainCommonBase main_common(options, true);
#else
MainCommonBase main_common(options, false);
#endif
return main_common.run() ? EXIT_SUCCESS : EXIT_FAILURE;
} catch (EnvoyException& e) {
return EXIT_FAILURE;
}
ares_library_cleanup();
return 0;
return EXIT_SUCCESS;
}

} // namespace Envoy
Loading