Skip to content

hot restart: conditionally build hot restart #1365

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

Merged
mattklein123 merged 10 commits into from
Aug 5, 2017
Merged

hot restart: conditionally build hot restart #1365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b9e1553
hot restart: conditionally remove hot restart based on build time fla…
zuercher Jul 31, 2017
9b8bc5b
cleanup conditional exec of hotrestart_test
zuercher Aug 1, 2017
f594a5f
remove unused rule
zuercher Aug 1, 2017
a1fc2eb
fix file formatting
zuercher Aug 2, 2017
06a7976
move HotRestartImpl to source/common/hot_restart
zuercher Aug 3, 2017
2186752
cleanup namespaces
zuercher Aug 3, 2017
b6cd328
fix format
zuercher Aug 3, 2017
81f84d5
move HotRestart[Nop]Impl to Envoy::Server
zuercher Aug 4, 2017
a758256
Merge branch 'master' into osx-2-hot_restart
zuercher Aug 4, 2017
f6484ae
remove stray namespaces
zuercher Aug 4, 2017
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: 5 additions & 0 deletions bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,8 @@ config_setting(
name = "disable_signal_trace",
values = {"define": "signal_trace=disabled"},
)

config_setting(
name = "disable_hot_restart",
values = {"define": "hot_restart=disabled"},
)
6 changes: 5 additions & 1 deletion bazel/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ You can use the `-c <compilation_mode>` flag to control this, e.g.
bazel build -c opt //source/exe:envoy-static
```


## Sanitizers

To build and run tests with the gcc compiler's [address sanitizer
Expand Down Expand Up @@ -230,6 +229,11 @@ remove log statements of lower importance during compilation to enhance performa
bazel build --copt=-DNVLOG //source/exe:envoy-static
```

## Hot Restart

Hot restart can be disabled in any build by specifying `--define=hot_restart=disabled`
on the Bazel command line.


# Release builds

Expand Down
14 changes: 11 additions & 3 deletions bazel/envoy_build_system.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def envoy_copts(repository, test = False):
# TCLAP command line parser needs this to support int64_t/uint64_t
"@bazel_tools//tools/osx:darwin": ["-DHAVE_LONG_LONG"],
"//conditions:default": [],
})
}) + envoy_select_hot_restart(["-DENVOY_HOT_RESTART"])

# Compute the final linkopts based on various options.
def envoy_linkopts():
Expand Down Expand Up @@ -267,7 +267,7 @@ def envoy_sh_test(name,
name = name + "_gen_test_runner",
srcs = srcs,
outs = [test_runner_cc],
cmd = "$(location //bazel:gen_sh_test_runner.sh) $(location " + srcs[0] + ") >> $@",
cmd = "$(location //bazel:gen_sh_test_runner.sh) $(SRCS) >> $@",
tools = ["//bazel:gen_sh_test_runner.sh"],
)
envoy_cc_test_library(
Expand All @@ -281,7 +281,7 @@ def envoy_sh_test(name,
name = name,
srcs = ["//bazel:sh_test_wrapper.sh"],
data = srcs + data,
args = ["$(location " + srcs[0] + ")"],
args = srcs,
**kargs
)

Expand Down Expand Up @@ -338,3 +338,11 @@ def envoy_proto_descriptor(name, out, srcs = [], external_deps = []):
cmd = cmd,
tools = ["//external:protoc"],
)

# Selects the given values if hot restart is enabled in the current build.
def envoy_select_hot_restart(xs):
return select({
"//bazel:disable_hot_restart": [],
"@bazel_tools//tools/osx:darwin": [],
"//conditions:default": xs,
})
2 changes: 2 additions & 0 deletions bazel/sh_test_wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@
# Where the runfiles are for tests.
export TEST_RUNDIR="${TEST_SRCDIR}/${TEST_WORKSPACE}"

cd $(dirname "$0")
Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 1, 2017

Choose a reason for hiding this comment

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

Because the envoy_sh_test srcs don't get run through $(location) anymore, I did this. There's probably a better way.

Copy link
Copy Markdown
Member

@htuch htuch Aug 2, 2017

Choose a reason for hiding this comment

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

How come envoy_sh_test needed modification in this PR?

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 3, 2017

Choose a reason for hiding this comment

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

I wanted to pass the output of bazel select() to envoy_sh_test's srcs parameter to conditionally disable the the hot restart test. However, select only functions when it's passed to a rule. envoy_sh_test is a macro and was trying to inspect srcs (something like "$(location " + srcs[0] + ")"), which doesn't work.

I switched those lines to use "$(SRCS)" or to just pass srcs directly, but the different is the absence of the relative path on the front of the filenames.

Copy link
Copy Markdown
Member

@htuch htuch Aug 4, 2017

Choose a reason for hiding this comment

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

So, a few of things:

  1. This might break the Google import, as this is making an assumption about file layout relative to where the source file is. Does something like cd "${TEST_RUNDIR}" work instead?

  2. There should be a cleaner way to disable the test than totally removing its sources. I still think tags is one way to manage this, if there's a way in envoy_sh_test to figure out if we're on OS X or not. https://github.com/lyft/envoy/blob/master/bazel/cc_configure.bzl#L30 does this basically, but does so in a repository rule context. So it's possible in Bazel..

  3. Nit: `cd "$(dirname "$0")"

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 4, 2017

Choose a reason for hiding this comment

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

I can rewrite the cd command as cd "$(dirname "${TEST_RUNDIR}"/${TEST_BINARY}")".

Ultimately, the files specified in srcs end up in "${TEST_SRCDIR}"/"${TEST_WORKSPACE}"/path/to/file (see https://docs.bazel.build/versions/master/build-ref.html#data). The script sets TEST_RUNDIR to "${TEST_SRCDIR}"/"${TEST_WORKSPACE}". The path/to/file here is test/integration. But envoy_sh_test is also used for //test/tools/router_check/test:router_tool_test, which will use a different path.

I've no doubt bazel is capable of doing what you want, I just don't understand how to do it. I've spent quite a bit of time reading Bazel docs. The only way to access the repository context, that I know of, is to write a custom rule, so I guess I'll try that again.

Copy link
Copy Markdown
Member

@htuch htuch Aug 4, 2017

Choose a reason for hiding this comment

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

Let's go with what you have there right now and then we can try and figure this out during the Google import, fix upstream if necessary.

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 4, 2017

Choose a reason for hiding this comment

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

So, cc_configure.bzl defines a repository_rule, which gets a repository_ctx object that contains the os information.

sh_test (which is the rule envoy_sh_test eventually creates) is a regular rule. Regular rules get a ctx object, which does not contain os information. In any event, it's not really OS that we want to key on but whether or not hot restart has been disabled for the build.

The ctx object does have a vars dict which contains "hot_restart": "disabled" when building hot restart is disabled, but entry that won't exist on OS X, since I'm trying to disable hot restart there by default (as it won't even compile). I suppose we could say that building on OS X requires explicitly disabling the feature with a command line flag.

At that point, I'm left writing a rule that duplicates the functionality of sh_test with the features available in ctx. And at that point I'm stuck. I can get it to run the wrapper script and am left concatenating strings to try to reproduce the TEST_XXX environment variables that the test depends on. In particular, a value for TEST_TMPDIR just doesn't seem to be available.

In the end, even if I got that to work, it's not at all clear to me that it's better.

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 5, 2017

Choose a reason for hiding this comment

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

I tried one other way, which is to use the fact that a test with tags = ["manual"] will not be included when running tests with wildcards (e.g. bazel test //test/...).

My idea was to pass a select() to tags and add the manual tag when hot restart is disabled. But that's not supported either:

/Users/stephan/workspace/turbinelabs-envoy/test/integration/BUILD:14:1: //test/integration:hotrestart_test: attribute "tags" is not configurable.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Aug 5, 2017

Choose a reason for hiding this comment

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

@zuercher thanks for trying so hard to figure this out, but I think you have done enough. If this causes issues for the Google import they have the expertise to fix if needed as @htuch said.


"$@"
22 changes: 3 additions & 19 deletions source/exe/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ envoy_cc_library(
srcs = ["main.cc"],
deps = [
":envoy_main_common_lib",
":hot_restart_lib",
"//source/server:hot_restart_lib",
"//source/server:options_lib",
"//source/server/config/http:lightstep_lib",
"//source/server/config/http:zipkin_lib",
Expand All @@ -73,29 +73,13 @@ envoy_cc_library(
hdrs = ["main_common.h"],
deps = [
":envoy_common_lib",
":hot_restart_lib",
"//source/common/common:compiler_requirements_lib",
"//source/server:hot_restart_lib",
"//source/server:hot_restart_nop_lib",
"//source/server/config_validation:server_lib",
],
)

envoy_cc_library(
name = "hot_restart_lib",
srcs = ["hot_restart.cc"],
hdrs = ["hot_restart.h"],
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/event:file_event_interface",
"//include/envoy/server:hot_restart_interface",
"//include/envoy/server:instance_interface",
"//include/envoy/server:options_interface",
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"//source/common/network:utility_lib",
"//source/common/stats:stats_lib",
],
)

envoy_cc_library(
name = "sigaction_lib",
srcs = ["signal_action.cc"],
Expand Down
25 changes: 13 additions & 12 deletions source/exe/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
#include <iostream>
#include <memory>

#include "exe/hot_restart.h"
#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"
Expand All @@ -19,24 +22,22 @@
*
* This should be used to do setup tasks specific to a particular site's
* deployment such as initializing signal handling. It calls main_common
* after setting up command line options and the hot restarter.
* 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

Envoy::OptionsImpl options(argc, argv, Envoy::Server::SharedMemory::version(),
spdlog::level::warn);
#ifdef ENVOY_HOT_RESTART
// Enabled by default, except on OS X. Control with "bazel --define=hot_restart=disabled"
const std::string shared_mem_version = Envoy::Server::SharedMemory::version();
#else
const std::string shared_mem_version = "disabled";
#endif

std::unique_ptr<Envoy::Server::HotRestartImpl> restarter;
try {
restarter.reset(new Envoy::Server::HotRestartImpl(options));
} catch (Envoy::EnvoyException& e) {
std::cerr << "unable to initialize hot restart: " << e.what() << std::endl;
return 1;
}
Envoy::OptionsImpl options(argc, argv, shared_mem_version, spdlog::level::warn);

return Envoy::main_common(options, *restarter);
return Envoy::main_common(options);
}
37 changes: 30 additions & 7 deletions source/exe/main_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@
#include "common/stats/stats_impl.h"
#include "common/stats/thread_local_store.h"

#include "exe/hot_restart.h"

#include "server/config_validation/server.h"
#include "server/drain_manager_impl.h"
#include "server/hot_restart_nop_impl.h"
#include "server/options_impl.h"
#include "server/server.h"
#include "server/test_hooks.h"

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

#include "ares.h"

namespace Envoy {
Expand All @@ -36,7 +39,27 @@ class ProdComponentFactory : public ComponentFactory {

} // namespace Server

int main_common(OptionsImpl& options, Server::HotRestartImpl& restarter) {
int main_common(OptionsImpl& options) {
#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;
}

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

Event::Libevent::Global::initialize();
Server::ProdComponentFactory component_factory;
LocalInfo::LocalInfoImpl local_info(
Expand All @@ -54,12 +77,12 @@ int main_common(OptionsImpl& options, Server::HotRestartImpl& restarter) {

ares_library_init(ARES_LIB_INIT_ALL);

Logger::Registry::initialize(options.logLevel(), restarter.logLock());
Logger::Registry::initialize(options.logLevel(), log_lock);
DefaultTestHooks default_test_hooks;
ThreadLocal::InstanceImpl tls;
Stats::ThreadLocalStoreImpl stats_store(restarter);
Server::InstanceImpl server(options, default_test_hooks, restarter, stats_store,
restarter.accessLogLock(), component_factory, local_info, tls);
Stats::ThreadLocalStoreImpl stats_store(stats_allocator);
Server::InstanceImpl server(options, default_test_hooks, *restarter, stats_store, access_log_lock,
component_factory, local_info, tls);
server.run();
ares_library_cleanup();
return 0;
Expand Down
5 changes: 1 addition & 4 deletions source/exe/main_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#pragma once

#include "exe/hot_restart.h"

#include "server/options_impl.h"

namespace Envoy {
Expand All @@ -10,9 +8,8 @@ namespace Envoy {
* main() runs.
*
* @param options Options object initialized by site-specific code
* @param restarter HotRestart object initialized in site-specific code.
* @return int Return code that should be returned from the actual main()
*/
int main_common(Envoy::OptionsImpl& options, Envoy::Server::HotRestartImpl& restarter);
int main_common(Envoy::OptionsImpl& options);

} // namespace Envoy
26 changes: 26 additions & 0 deletions source/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
"envoy_select_hot_restart",
)

envoy_package()
Expand Down Expand Up @@ -95,6 +96,31 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "hot_restart_lib",
srcs = envoy_select_hot_restart(["hot_restart_impl.cc"]),
hdrs = envoy_select_hot_restart(["hot_restart_impl.h"]),
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/event:file_event_interface",
"//include/envoy/server:hot_restart_interface",
"//include/envoy/server:instance_interface",
"//include/envoy/server:options_interface",
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"//source/common/network:utility_lib",
"//source/common/stats:stats_lib",
],
)

envoy_cc_library(
name = "hot_restart_nop_lib",
hdrs = ["hot_restart_nop_impl.h"],
deps = [
"//include/envoy/server:hot_restart_interface",
],
)

envoy_cc_library(
name = "init_manager_lib",
srcs = ["init_manager_impl.cc"],
Expand Down
2 changes: 1 addition & 1 deletion source/exe/hot_restart.cc → source/server/hot_restart_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "exe/hot_restart.h"
#include "server/hot_restart_impl.h"

#include <signal.h>
#include <sys/mman.h>
Expand Down
0 source/exe/hot_restart.h → source/server/hot_restart_impl.h
View file
Open in desktop
File renamed without changes.
28 changes: 28 additions & 0 deletions source/server/hot_restart_nop_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#pragma once

#include <string>

#include "envoy/server/hot_restart.h"

namespace Envoy {
namespace Server {

/**
* No-op implementation of HotRestart.
*/
class HotRestartNopImpl : public Server::HotRestart {
public:
HotRestartNopImpl(){};

void drainParentListeners() override {}
int duplicateParentListenSocket(const std::string&) override { return -1; }
void getParentStats(GetParentStatsInfo& info) override { memset(&info, 0, sizeof(info)); }
void initialize(Event::Dispatcher&, Server::Instance&) override {}
void shutdownParentAdmin(ShutdownParentAdminInfo&) override {}
void terminateParent() override {}
void shutdown() override {}
std::string version() override { return "disabled"; }
};

} // namespace Server
} // namespace Envoy
4 changes: 3 additions & 1 deletion test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ load(
"envoy_cc_test_library",
"envoy_package",
"envoy_sh_test",
"envoy_select_hot_restart",
)

envoy_package()

envoy_sh_test(
name = "hotrestart_test",
srcs = ["hotrestart_test.sh"],
srcs = envoy_select_hot_restart(["hotrestart_test.sh"]),
data = [
"//source/exe:envoy-static",
"//test/common/runtime:filesystem_setup.sh",
Expand Down Expand Up @@ -143,6 +144,7 @@ envoy_cc_test_library(
"//source/common/upstream:upstream_includes",
"//source/common/upstream:upstream_lib",
"//source/server:connection_handler_lib",
"//source/server:hot_restart_nop_lib",
"//source/server:server_lib",
"//source/server:test_hooks_lib",
"//source/server/config/http:buffer_lib",
Expand Down
21 changes: 3 additions & 18 deletions test/integration/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,21 @@
#include <string>

#include "envoy/http/header_map.h"
#include "envoy/server/hot_restart.h"

#include "common/local_info/local_info_impl.h"
#include "common/network/utility.h"
#include "common/stats/thread_local_store.h"
#include "common/thread_local/thread_local_impl.h"

#include "server/hot_restart_nop_impl.h"

#include "test/integration/integration.h"
#include "test/integration/utility.h"
#include "test/test_common/environment.h"

#include "gtest/gtest.h"

namespace Envoy {
namespace Server {

class TestHotRestart : public HotRestart {
public:
// Server::HotRestart
void drainParentListeners() override {}
int duplicateParentListenSocket(const std::string&) override { return -1; }
void getParentStats(GetParentStatsInfo& info) override { memset(&info, 0, sizeof(info)); }
void initialize(Event::Dispatcher&, Server::Instance&) override {}
void shutdownParentAdmin(ShutdownParentAdminInfo&) override {}
void terminateParent() override {}
void shutdown() override {}
std::string version() override { return "1"; }
};

} // namespace Server

IntegrationTestServerPtr IntegrationTestServer::create(const std::string& config_path,
const std::string& bootstrap_path,
Expand Down Expand Up @@ -91,7 +76,7 @@ void IntegrationTestServer::onWorkerListenerRemoved() {

void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion version) {
Server::TestOptionsImpl options(config_path_, bootstrap_path_);
Server::TestHotRestart restarter;
Server::HotRestartNopImpl restarter;
Thread::MutexBasicLockable lock;
LocalInfo::LocalInfoImpl local_info(Network::Utility::getLocalAddress(version), "zone_name",
"cluster_name", "node_name");
Expand Down