ci: fix for Bazel issues seen in PR #5767 by mpwarres · Pull Request #5868 · envoyproxy/envoy

mpwarres · 2019-02-06T23:08:09Z

Description:

Proposed fix for issues seen in #5767

Update envoy/ci/WORKSPACE.filter.example, which is used in CI tests (see setup here), to more closely match current envoy-filter-example WORKSPACE file. In particular, remove the dual use of "envoy" as both the name of envoy-filter-example workspace as well as the envoy local_repository, and load //bazel:repositories.bzl and //bazel:cc_configure.bzl using fully qualified target names, which removes the need to set --package_path in ci/build_setup.sh.

Risk Level: low: errors should result in CI failure
Testing: existing CI tests
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Michael Warres mpw@google.com

Signed-off-by: Michael Warres <mpw@google.com>

ci/build_setup.sh

.bazelrc

Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-02-07T12:59:48Z

/retest

repokitteh-read-only · 2019-02-07T12:59:52Z

🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5868 (comment) was created by @mpwarres.

see: more, trace.

htuch · 2019-02-07T18:46:31Z

/retest

I'm surprised at the ASAN errors, they are consistent across tests, but seem to indicate some file I/O issue.

repokitteh-read-only · 2019-02-07T18:46:36Z

🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5868 (comment) was created by @htuch.

see: more, trace.

lizan · 2019-02-07T19:00:09Z

ci/WORKSPACE.filter.example

@@ -1,12 +1,12 @@
-workspace(name = "envoy")
+workspace(name = "envoy_filter_example")


The ASAN/TSAN test failures are real, CI run ASAN/TSAN from envoy-filter-example though our test setup depends on TEST_WORKSPACE env. Before this change it is identical so wasn't a problem though now it is.

Addressed by splitting into separate 'bazel test' invocations, so the @envoy//... tests are run with TEST_WORKSPACE=envoy, while the @envoy-filter-example//... tests are run with TEST_WORKSPACE=envoy-filter-example. All CI tests pass now.

@lizan PTAL again, thanks!

Needed to pick up #5850.

Run tests defined in envoy workspace in one bazel test invocation, and tests defined in envoy-filter-example workspace in a separate bazel test invocation, done from $ENVOY_FILTER_EXAMPLE_SRCDIR. This is so that the $TEST_WORKSPACE environment variable, which plays a role in locating test runtime file dependencies, will be set properly for each grouping of tests. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-02-09T15:08:30Z

/retest

repokitteh-read-only · 2019-02-09T15:08:33Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5868 (comment) was created by @mpwarres.

see: more, trace.

…ffic tapping. Signed-off-by: Michael Warres <mpw@google.com>

htuch · 2019-02-10T22:55:10Z

ci/do_ci.sh

+  echo "bazel ASAN/UBSAN debug build with tests"
+  echo "Building and testing envoy tests..."
+  cd "${ENVOY_SRCDIR}"
+  bazel_with_collection test ${BAZEL_TEST_OPTIONS} -c dbg --config=clang-asan //test/...


How come we can't build/test @envoy directly anymore? I get that we're in a different workspace etc, but don't see why we should be able to use these paths.

It's a combination of a few things:

bazel test automatically sets the TEST_WORKSPACE environment variable to the local repository's workspace name.

Without this PR, when bazel test is run from $ENVOY_FILTER_EXAMPLE_SRCDIR, TEST_WORKSPACE would evaluate to "envoy", since that was the workspace name set by WORKSPACE.filter.example.

With this PR, when bazel test is run from $ENVOY_FILTER_EXAMPLE_SRCDIR, TEST_WORKSPACE evaluates to "envoy_filter_example".

TEST_WORKSPACE affects TEST_RUNDIR. Here's where envoy/test/main.cc sets TEST_RUNDIR to $TEST_SRCDIR/$TEST_WORKSPACE. And TEST_RUNDIR is how Envoy::TestEnvironment finds test input data: TestEnvironment::runfilesPath() just appends the path it is given to TEST_RUNDIR.

Empirically, bazel places runfiles for build targets in $TEST_SRCDIR/<workspace-name>, where <workspace-name> is the name of the (maybe external) workspace containing the build target. With this PR, prior to the latest changes that split bazel test invocations, runfiles for @envoy tests were in $TEST_SRCDIR/envoy, while bazel test invoked from $ENVOY_FILTER_EXAMPLE_SRCDIR was setting TEST_RUNDIR to $TEST_SRCDIR/envoy_filter_example and looking for them there.

Splitting the bazel test invocations ensures that TEST_WORKSPACE is always the same as the <workspace-name> in which the runfiles for tests can be found. This issue didn't arise before, since we used the same workspace name "envoy" in both cases.

This seems to be masking an underlying issue in how we or Bazel are setting up the rundir path for tests. Shouldn't we be able to bazel test @foo//:bar for some external repository?

Here is a very basic, self-contained repro that illustrates the behavior: https://gist.github.com/mpwarres/b83539d17f92d63d9c25794a84a734ab

@dkelmer / @dslomov, is this supposed to work, or is running a test with runfiles that belongs to external dependency A from some other workspace B not a supported use case?

@htuch I'm not sure how else to address the Bazel behavior within the confines of this PR, any suggestions welcome. In some sense the Bazel behavior was masked even prior to this PR, by virtue of the external dependency having the same name as the workspace using it.

@htuch from a repository consuming envoy as deps (e.g. envoy-filter-example, istio/proxy), bazel test @envoy//test/... will fail some of tests depending on TEST_WORKSPACE, I think it is a separate issue which we should address, but should not block on this PR.

@lizan yep, I've already merged this PR. I'm hoping we can followup with an issue on bazelbuild GH wit @mpwarres 's repro.

htuch

@mpwarres if you can file this repro as an issue at https://github.com/bazelbuild/bazel/issues (and CC me), happy to merge this PR for now. Thanks!

mpwarres · 2019-02-12T03:43:40Z

Sure thing, filed as bazelbuild/bazel#7399 . Thanks for the review!

lizan · 2019-02-12T09:08:51Z

I'm not quite sure it is bazel issue though, seems like we shouldn't use TEST_WORKSPACE but hardcode envoy here?

htuch · 2019-02-12T15:19:25Z

@lizan what if someone sets up Envoy with a different WORKSPACE and chosen name? I guess the Bazel issue is "what are the best practices for referring to your runfiles?".

mpwarres · 2019-02-12T16:09:44Z

+1 to what @htuch said. I updated the repro at https://gist.github.com/mpwarres/b83539d17f92d63d9c25794a84a734ab to illustrate both @lizan's workaround (which does work, so long as the workspace name matches), as well as the failure if the workspace name assumed by the test code doesn't match the name assigned to the external dependency by the importing workspace.

Proposed fix for issues seen in envoyproxy#5767 Update envoy/ci/WORKSPACE.filter.example, which is used in CI tests (see setup [here](https://github.com/envoyproxy/envoy/blob/f2511a39cf2c4fe392d5499e854c39f262712100/ci/build_setup.sh#L92)), to more closely match current envoy-filter-example [WORKSPACE](https://github.com/envoyproxy/envoy-filter-example/blob/master/WORKSPACE) file. In particular, remove the dual use of "envoy" as both the name of envoy-filter-example workspace as well as the envoy local_repository, and load //bazel:repositories.bzl and //bazel:cc_configure.bzl using fully qualified target names, which removes the need to [set --package_path](https://github.com/envoyproxy/envoy/blob/f2511a39cf2c4fe392d5499e854c39f262712100/ci/build_setup.sh#L63) in ci/build_setup.sh. Risk Level: low: errors should result in CI failure Testing: existing CI tests Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Potential fix for issues seen in #5767

2d37bfc

Signed-off-by: Michael Warres <mpw@google.com>

mpwarres mentioned this pull request Feb 6, 2019

For QUICHE integration, add --experimental_remap_main_repo to bazelrc. #5767

Closed

lizan suggested changes Feb 6, 2019

View reviewed changes

ci/build_setup.sh Outdated Show resolved Hide resolved

.bazelrc Outdated Show resolved Hide resolved

htuch assigned lizan Feb 7, 2019

Apply changes suggested by reviews.

892463d

Signed-off-by: Michael Warres <mpw@google.com>

mpwarres changed the title ~~WIP: ci: Potential fix for Bazel issues seen in PR #5767~~ ci: fix for Bazel issues seen in PR #5767 Feb 7, 2019

htuch previously approved these changes Feb 7, 2019

View reviewed changes

lizan reviewed Feb 7, 2019

View reviewed changes

mpwarres added 2 commits February 9, 2019 00:21

Merge branch 'master' into debug-experimental-remap-main-repo

3444627

Needed to pick up #5850.

mpwarres dismissed htuch’s stale review via c989a32 February 9, 2019 05:32

cd back into $ENVOY_SRCDIR to run envoy ssl_integration_test with tra…

a6c7cad

…ffic tapping. Signed-off-by: Michael Warres <mpw@google.com>

htuch reviewed Feb 10, 2019

View reviewed changes

htuch approved these changes Feb 11, 2019

View reviewed changes

htuch merged commit 97d259d into envoyproxy:master Feb 11, 2019

mpwarres mentioned this pull request Feb 12, 2019

test in external dependency cannot find runfile when executed from other workspace bazelbuild/bazel#7399

Closed

		@@ -1,12 +1,12 @@
		workspace(name = "envoy")
		workspace(name = "envoy_filter_example")

Conversation

mpwarres commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpwarres commented Feb 7, 2019

Uh oh!

repokitteh-read-only bot commented Feb 7, 2019

Uh oh!

htuch commented Feb 7, 2019

Uh oh!

repokitteh-read-only bot commented Feb 7, 2019

Uh oh!

lizan Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpwarres Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres commented Feb 9, 2019

Uh oh!

repokitteh-read-only bot commented Feb 9, 2019

Uh oh!

htuch Feb 10, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mpwarres commented Feb 12, 2019

Uh oh!

lizan commented Feb 12, 2019

Uh oh!

htuch commented Feb 12, 2019

Uh oh!

mpwarres commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mpwarres commented Feb 6, 2019 •

edited

Loading

lizan Feb 7, 2019 •

edited

Loading

mpwarres Feb 11, 2019 •

edited

Loading