-
Notifications
You must be signed in to change notification settings - Fork 5.5k
server: factor out MainCommon as a class, with a run method #2568
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.
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
Changes from 21 commits
e696fe1
d796786
622e5d2
6a87f69
de78e95
3e5e52e
53f3ec0
b9d284b
2ff4f4a
b28fa8e
ccd38ad
4d2bc12
4bd2da7
c84e9b4
8e0262a
928deb7
94e9b12
1763e71
2ba3207
a9237fe
57ef61f
3b469a4
f45099c
552cb2a
4914e62
58a0180
8cb20e7
ada35db
ce29a45
c2bb454
92a9544
22a2e80
d1cf0b9
6372516
1e29195
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| { | ||
| "listeners": [{ | ||
| "address": "tcp://127.0.0.1:0", | ||
| "filters": [{ | ||
| "name": "http_connection_manager", | ||
| "config": { | ||
| "codec_type": "auto", | ||
| "stat_prefix": "ingress_http", | ||
| "route_config": { | ||
| "virtual_hosts": [{ | ||
| "name": "local_service", | ||
| "domains": [ | ||
| "*" | ||
| ], | ||
| "routes": [{ | ||
| "timeout_ms": 0, | ||
| "prefix": "/", | ||
| "host_rewrite": "www.google.com", | ||
| "cluster": "service_google" | ||
| }] | ||
| }] | ||
| }, | ||
| "filters": [{ | ||
| "name": "router", | ||
| "config": {} | ||
| }] | ||
| } | ||
| }] | ||
| }], | ||
| "admin": { | ||
| "access_log_path": "/tmp/admin_access.log", | ||
| "address": "tcp://127.0.0.1:0" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we're using a v1 config here? In general, unless verifying v1 regression, we're trying to turn down v1 and only add new stuff as v2.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was being lazy and copied the first plausible config I found. Done. |
||
| }, | ||
| "cluster_manager": { | ||
| "clusters": [{ | ||
| "name": "service_google", | ||
| "connect_timeout_ms": 250, | ||
| "type": "logical_dns", | ||
| "lb_type": "round_robin", | ||
| "hosts": [{ | ||
| "url": "tcp://google.com:443" | ||
| }], | ||
| "ssl_context": { | ||
| "sni": "www.google.com" | ||
| } | ||
| }] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
|
@@ -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& stats_allocator() PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| }; | ||
|
|
||
| } // namespace Server | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 may return a reference counted data location | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, I think s/may return/should return/. We have an outstanding issue related to this at #2453 |
||
| * 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
|
|
@@ -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": [], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you test with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the |
||
| "//conditions:default": [":sigaction_lib"], | ||
| }), | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
|
|
||
| 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) | ||
|
|
||
| /** | ||
|
|
@@ -24,32 +9,22 @@ | |
| * deployment such as initializing signal handling. It calls main_common | ||
| * 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 | ||
|
|
||
| int main(int argc, const char** argv) { | ||
| try { | ||
| #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); | ||
| }; | ||
| Envoy::MainCommon main_common(argc, argv, true); | ||
| #else | ||
| const Envoy::OptionsImpl::HotRestartVersionCb hot_restart_version_cb = [](uint64_t, uint64_t) { | ||
| return "disabled"; | ||
| }; | ||
| Envoy::MainCommon main_common(argc, argv, false); | ||
| #endif | ||
|
|
||
| std::unique_ptr<Envoy::OptionsImpl> options; | ||
| try { | ||
| options = std::make_unique<Envoy::OptionsImpl>(argc, argv, hot_restart_version_cb, | ||
| spdlog::level::info); | ||
| return main_common.run() ? 0 : 1; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EXIT_SUCCESS/EXIT_FAILURE here also?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } catch (const Envoy::NoServingException& e) { | ||
| return 0; | ||
| } catch (const Envoy::MalformedArgvException& e) { | ||
| std::cerr << "MalformedArgvException: " << e.what() << std::endl; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning behind these new catch blocks? Were these things caught elsewhere and now are caught here? My main comment is that the errors we print should be user friendly so I don't think we should print the name of the exception which the user doesn't care about. Perhaps just "Envoy startup failure: ..."?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's possible these were necessary only when I had some other problem deeper in the code, and now it's not necessary. I'll investigate & follow up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't extra; it's just moved. It was previously on main_common.cc:92. I removed the extra printing though, and handles the case in Server::InstanceImpl::InstanceImpl where the config fails to parse, e.g. because it is empty, which is tested in hotrestart_integration_test. I guess an invalid json config could also cause this, or failing to open the log file. I think it makes sense to handle EnvoyException in main.cc. Or, we could re-cast the exception in MainCommon's ctor as one that was already handled in main.cc. WDYT?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine either way. The main thing I would watch out for is that we don't catch exceptions when the run loop starts running. I realize that wasn't really clearly codified, and feel free to add more comments, but the idea is that we can throw and exit(1) during startup, but once the main loop starts up any exceptions which aren't explicitly handled should crash the process w/ core dump. That's the way the code works today so I would just be careful that we maintain that behavior. |
||
| return 1; | ||
| } catch (const Envoy::EnvoyException& e) { | ||
| std::cerr << "EnvoyException: " << e.what() << std::endl; | ||
| return 1; | ||
| } | ||
| return Envoy::main_common(*options); | ||
| return 0; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best practice is to write new example configs in YAML; this the preferred human readable config representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done