Skip to content
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

Update our Envoy dependency #101

Closed
wants to merge 33 commits into from

Conversation

oschaaf
Copy link
Member

@oschaaf oschaaf commented Jul 26, 2019

  • Updates Envoy to envoyproxy/envoy@8246167
  • Swap out Envoy's integration test server to use our own python integration wrappers to fire up a backend in the cpp benchmark client tests. This was needed to resolve the singleton runtime conflict with Envoy's integration test server rearing its head again. As an a-side this sets us up for testing against multiple types of backends later on.
  • Coverage refresher: syncs the way we do coverage testing up with how Envoy pulls it off, greatly improving the time needed to generate a coverage report as well as improving accuracy tracking coverage of inlined code in headers.
    Notes:
  • Fixed argument handling (as with the new coverage test method the test around it would fail).

oschaaf added 22 commits July 15, 2019 16:43
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
- It caused x-version trouble.
- gperftools reported leaks , seemingly unfixable
- gcovr ran into trouble

In short, probably more trouble then it's worth, though it would have been
a nice alternative to the pipe IPC we rely on with this change.

Signed-off-by: Otto van der Schaaf <[email protected]>
- Update CI image: 8246167b9d238797cbc6c03dccc9e3921c37617d
- Update Envoy: bcc66c6b74c365d1d2834cfe15b847ae13be0eb6

Prelude to updating coverage testing with lcov
Benchmark http client still needs to be fixed

Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
As per Harvey's recommendation, use py_binary and refer
to it as a tool in the cpp_tests to get the pip deps sorted.

Also, clean up integration/BUILD

Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
- sync the way we do coverage testing with Envoy upstream
- sync .bazelrc with Envoy upstream

Notes: disabled `LinearRateLimiterInvalidArgumentTest` as that
somehow specifically failed with the new coverage testing.

Visibility of this target is the single thing from barring this to work:
https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_test.bzl#L147

Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
oschaaf added a commit to oschaaf/envoy that referenced this pull request Jul 29, 2019
In envoyproxy/nighthawk#101
we update coverage to the new methodology. For that to
work we need to be able to consume `_lib_internal_only` targets which
aren't visible today.  This proposes a change so we can make that  happen.
The naming is fairly clear that these targets shouldn't  be used
otherwise.

Signed-off-by: Otto van der Schaaf <[email protected]>
In anticipation of envoyproxy/envoy#7749
it makes sense to further upgrade the Envoy dep, which hereby
is done.

Signed-off-by: Otto van der Schaaf <[email protected]>
htuch pushed a commit to envoyproxy/envoy that referenced this pull request Jul 29, 2019
In envoyproxy/nighthawk#101 we update coverage to the new methodology.
For that to work we need to be able to consume _lib_internal_only targets which aren't visible externally today.
This proposes a change so we can make that happen.
The naming is fairly clear that these targets shouldn't be used otherwise.

Risk Level: low
Testing: N/A

Signed-off-by: Otto van der Schaaf <[email protected]>
oschaaf added 3 commits July 29, 2019 23:56
Signed-off-by: Otto van der Schaaf <[email protected]>
- the updated envoy fixes a clang-tidy issue we run in to
- had to introduce some shim code which reduces the coverage %

Let's see if CI is happy with these changes included.

Signed-off-by: Otto van der Schaaf <[email protected]>
oschaaf added 7 commits July 31, 2019 11:04
…k into update-envoy-state-dump

Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf oschaaf marked this pull request as ready for review August 1, 2019 19:46
@oschaaf oschaaf requested a review from htuch August 1, 2019 19:46
@@ -0,0 +1,90 @@
#include <chrono>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @oschaaf . How much of this can be split into separate PRs? I.e. there is a ton of churn, around Python test infra, SSL objects, coverage, general upgrade? Ideally our Envoy upgrades are lightweight and some of these changes, e.g. test infra, are one-offs. Could you split out some of this easily?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this can be split up into multiple PR's easily; be we may have to temporarily allow a lower coverage threshold at some point. I'll carve off a more fun-sized piece right away into a separate PR.

oschaaf added a commit to oschaaf/nighthawk that referenced this pull request Aug 1, 2019
Split off from envoyproxy#101

- As a step towards upgrading our Envoy dependency,
this swaps out Envoy's c++ integration test server with our own
python wrappers around our own test server. This permanently
avoids future runtime singleton conflicts.
- Also starts setting `--base-id` when starting up our test-server.
This is to support the upcoming test-sharding that will be performed
by updated coverage test methodoly which will land in a future update.

Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf oschaaf mentioned this pull request Aug 1, 2019
@oschaaf
Copy link
Member Author

oschaaf commented Aug 10, 2019

Closing this PR as all has been either split out into separate PR's or been superseded.

@oschaaf oschaaf closed this Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants