hot restart: conditionally build hot restart by zuercher · Pull Request #1365 · envoyproxy/envoy

zuercher · 2017-08-01T00:10:35Z

(Split out of #1348, part of fixes for #128).

hot restart: conditionally build hot restart based on flag/architecture

Use --define=hot_restart=disabled to disable hot restart in a build.
Hot restart is always disabled for OS X.

…g/architecture Use --define=hotrestart=disabled to disable hot restart in a build. Hot restart is permanently disabled for OS X.

mattklein123

Again thanks for splitting out. Generally LGTM, a few small comments.

mattklein123 · 2017-08-01T02:02:58Z

source/exe/hot_restart_nop.h

@@ -0,0 +1,39 @@
+#pragma once
+
+#include <fcntl.h>


I think many/all of these includes are not needed?

mattklein123 · 2017-08-01T02:03:47Z

source/exe/hot_restart_nop.h

@@ -0,0 +1,39 @@
+#pragma once


in an optimal world, this hot_restart_nop.h would also be used by the test code which does the same thing. You would need to rejigger slightly and maybe put this in source/common/hot_restart/hot_restart_nop.h. It's not a huge deal but if you feel like it.

mattklein123 · 2017-08-01T02:04:53Z

source/exe/main.cc

+#ifdef ENVOY_HOT_RESTART
+  const std::string& shared_mem_version = Envoy::Server::SharedMemory::version();
+#else
+  const std::string& shared_mem_version = "disabled";


technically you are taking a reference to a temporary here. I would probably just define on the previous line and then take a reference to it.

mattklein123 · 2017-08-01T02:05:47Z

source/exe/main_common.cc


+#ifdef ENVOY_HOT_RESTART
 #include "exe/hot_restart.h"
+#endif


I left it in now that it's under common. I could add an #ifndef ENVOY_HOT_RESTART, but I don't think it hurts anything to leave it.

mattklein123 · 2017-08-01T02:06:38Z

test/integration/hotrestart_test.sh


 set -e

+if [[ -f "test/integration/hotrestart_disabled" ]]; then


@htuch @dnoe is there a better way to just exclude this test entirely if hot restart is not being via bazel define?

See my comments on the use of test tags and interpretation in envoy_build_system.bzl as one possible option.

htuch

Thanks for splitting this out and generalizing to non-OS X.

htuch · 2017-08-01T01:44:12Z

test/integration/hotrestart_test.sh


 set -e

+if [[ -f "test/integration/hotrestart_disabled" ]]; then


This should probably be "${TEST_RUNDIR}"/test/integration/hotrestart_disabled

htuch · 2017-08-01T01:55:38Z

test/integration/BUILD

+    name = "hotrestart_test_disabled",
+    outs = ["hotrestart_disabled"],
+    cmd = "touch $@",
+)


I think a possibly more elegant way to handle this might be to use Bazel tags. If you had tags = ["hot_restart"] on the hotrestart_test, you could have the rules in envoy_build_system.bzl pick up on this and completely skip defining the test on OS X. That's provided there is some way in Skylark to figure out which platform you are on.

I can't figure out how to do this with tags.

The closest I've gotten is to rewrite envoy_sh_test to not access srcs in a way that prevents select from working. I'll push it in a bit.

@zuercher please just push what you have. @htuch or @lizan can you look at the branch and maybe help out with specifics on how to do it?

Thanks,
Matt

htuch · 2017-08-01T01:59:27Z

source/exe/BUILD

    name = "hot_restart_lib",
-    srcs = ["hot_restart.cc"],
-    hdrs = ["hot_restart.h"],
+    srcs = select({


Can you define a function in envoy_build_system.bzl which is basically something like:

def select_hotrestart(xs): return select({ "//bazel:disable_hot_restart": [], "@bazel_tools//tools/osx:darwin": [], "//conditions:default": xs, })

This might make the various sites this appears at cleaner.

zuercher · 2017-08-01T23:42:14Z

bazel/sh_test_wrapper.sh

 # Where the runfiles are for tests.
 export TEST_RUNDIR="${TEST_SRCDIR}/${TEST_WORKSPACE}"

+cd $(dirname "$0")


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

How come envoy_sh_test needed modification in this PR?

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.

So, a few of things:

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?

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..

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

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.

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.

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.

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.

@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.

mattklein123

Thank you @zuercher looks great. @dnoe @htuch this LGTM from code perspective. Can you please review from a) bazel perspective to see if any fixes are needed (if so please help with specifics), b) google import perspective on disabling hot restart?

Thanks,
Matt

htuch · 2017-08-02T21:40:03Z

source/exe/BUILD

 )

 envoy_cc_library(
    name = "hot_restart_lib",


Could also move this to source/common/hot_restart.

mattklein123 · 2017-08-03T18:52:48Z

source/common/hot_restart/hot_restart_nop.h

+#include "envoy/server/hot_restart.h"
+
+namespace Envoy {
+namespace Server {


nit: can we change namespace to HotRestart here and in the other moved file?

mattklein123 · 2017-08-03T21:13:42Z

source/common/hot_restart/BUILD

@@ -0,0 +1,35 @@
+licenses(["notice"])  # Apache 2


@zuercher I feel really, really bad asking this, and I should have noticed before, but IMO the hot restart files should actually go in source/server, with namespace moved back to Server. This is so that we don't reach from common code back into server which we have been not doing so far (like for options). If you don't want to do this I can clean it up later. Sorry.

mattklein123 · 2017-08-04T20:15:18Z

source/server/hot_restart_impl.cc

 const uint64_t SharedMemory::VERSION = 8;

-SharedMemory& SharedMemory::initialize(Options& options) {
+SharedMemory& SharedMemory::initialize(Server::Options& options) {


nit: you should be able to revert all of the Server:: changes from these two moved files (h/cc).

htuch · 2017-08-05T16:35:09Z

+1 on thanks for doing the investigative leg work here. @zuercher what you say makes sense, I agree that the complexity is unwarranted. We'll deal with this as needed. I've filed bazelbuild/bazel#3510 to track this with the Bazel team, as it seems something they should support to make life easier for the rest of the world that just want a solution that works out of the box.

) Automatic merge from submit-queue. Add the support of the allow_tls option for Istio authn **What this PR does / why we need it**: This PR adds the support of the allow_tls option for Istio authn. When allow_tls is true, the client certificate does not have to be present. If it is present, it will be extracted after the authentication. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1365 **Special notes for your reviewer**: **Release note**: ```release-note ```

…rs (#1365)

hot restart: conditionally remove hot restart based on build time fla…

b9e1553

…g/architecture Use --define=hotrestart=disabled to disable hot restart in a build. Hot restart is permanently disabled for OS X.

zuercher mentioned this pull request Aug 1, 2017

osx support (fixes #128) #1348

Closed

mattklein123 reviewed Aug 1, 2017

View reviewed changes

htuch reviewed Aug 1, 2017

View reviewed changes

cleanup conditional exec of hotrestart_test

9b8bc5b

zuercher commented Aug 1, 2017

View reviewed changes

zuercher added 2 commits August 1, 2017 16:44

remove unused rule

f594a5f

fix file formatting

a1fc2eb

mattklein123 reviewed Aug 2, 2017

View reviewed changes

htuch reviewed Aug 2, 2017

View reviewed changes

move HotRestartImpl to source/common/hot_restart

06a7976

mattklein123 reviewed Aug 3, 2017

View reviewed changes

zuercher added 2 commits August 3, 2017 12:10

cleanup namespaces

2186752

fix format

b6cd328

mattklein123 reviewed Aug 3, 2017

View reviewed changes

zuercher added 2 commits August 4, 2017 09:46

move HotRestart[Nop]Impl to Envoy::Server

81f84d5

Merge branch 'master' into osx-2-hot_restart

a758256

mattklein123 reviewed Aug 4, 2017

View reviewed changes

remove stray namespaces

f6484ae

mattklein123 approved these changes Aug 5, 2017

View reviewed changes

mattklein123 merged commit ffa2d12 into envoyproxy:master Aug 5, 2017

htuch mentioned this pull request Aug 5, 2017

Provide facility to condition on platform during analysis phase bazelbuild/bazel#3510

Closed

zuercher deleted the osx-2-hot_restart branch August 10, 2017 16:06

mathetake pushed a commit that referenced this pull request Mar 3, 2026

feat: add guided regex, guided choice and guided json for all provide…

a5cd573

…rs (#1365)


		set -e

		if [[ -f "test/integration/hotrestart_disabled" ]]; then

Conversation

zuercher commented Aug 1, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants