Skip to content

Update Envoy to f84440dc4f95890f14e2e0686b07258f030b54b3#321

Merged
mum4k merged 20 commits intoenvoyproxy:masterfrom
oschaaf:envoy-dep-update-15
Apr 10, 2020
Merged

Update Envoy to f84440dc4f95890f14e2e0686b07258f030b54b3#321
mum4k merged 20 commits intoenvoyproxy:masterfrom
oschaaf:envoy-dep-update-15

Conversation

@oschaaf
Copy link
Copy Markdown
Member

@oschaaf oschaaf commented Apr 6, 2020

  • Trace id generation broke. This was amended by directly
    using the new request id uid implementation. It would be
    nice to leverage the new request id extension capability
    later for this.
  • Amended calls to get counters by name
  • Synced .bazelrc
  • Histogram value formatting subtly changed in some cases, probably caused by an update
    to Envoy's dependency on libfmt.
    Introduce an explicit formatting specifier in the affected output formatting code to address that.
  • Tweaks for resolving new OOM issues with ASAN/TSAN in CI that started with the updated Envoy
    revision. This affected both the build process as well as the testing process itself. Notably, the
    python-based integration tests would get OOM-killed in asan runs.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

- Trace id generation broke. This was amended by directly
using the new request id uid implementation. It would be
nice to leverage the new request id extension capability
later for this.
- Amended calls to get counters by name
- Synced .bazelrc
- Amended test expectations for response size histograms
  which now report in with '.0' postfixed.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 6, 2020

For this PR, ASAN failed for times in a row with:

Server terminated abruptly (error code: 14, error message: 'Socket closed', log file: '/build/tmp/_bazel_bazel/f85b6fb5740e6e8c7efea142eec4b6e8/server/jvm.out')

oschaaf added 2 commits April 7, 2020 17:14
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf changed the title Update Envoy to 4c26d6ea3d60a1d3720c6e0cfe5a04563d4bd141 Update Envoy to 4b6e81c112e204ef04445318d2cbfa168cf22ca7 Apr 8, 2020
oschaaf added 14 commits April 8, 2020 10:02
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Output formatting subtly changed, probably because of an updated
dependency brought in by the new Envoy revision. Use an explicit
formatting directive to amend and ensure that stays put.

This also backs out the updated expectations, cleaning up the diff.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf changed the title Update Envoy to 4b6e81c112e204ef04445318d2cbfa168cf22ca7 Update Envoy to f84440dc4f95890f14e2e0686b07258f030b54b3 Apr 9, 2020
@oschaaf oschaaf marked this pull request as ready for review April 9, 2020 12:09
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 9, 2020

Readying this up for review. I plan to respin the tests a couple of times to make sure, but I think
the new out-of-memory issues in CI are addressed in here sufficiently now.

@oschaaf oschaaf added P1 waiting-for-review A PR waiting for a review. labels Apr 9, 2020
mum4k
mum4k previously approved these changes Apr 9, 2020
echo "bazel TSAN debug build with tests"
echo "Building and testing envoy tests..."
cd "${SRCDIR}"
[ -z "$CIRCLECI" ] || export BAZEL_BUILD_OPTIONS="${BAZEL_TEST_OPTIONS} --local_ram_resources=12288"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Curios: what was the default value of the local_ram_resources flag?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From https://docs.bazel.build/versions/master/user-manual.html:

By default Bazel will estimate amount of RAM and number of CPU cores directly
from system configuration.

Envoy::Protobuf::util::TimeUtil::DurationToMicroseconds(percentile.duration()));
} else {
ss << fmt::format("{}.value: {}", percentile_prefix, percentile.raw_value());
ss << fmt::format("{}.value: {:.{}f}", percentile_prefix, percentile.raw_value(), 0);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure I follow this, what is the ", 0" for ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Basically the intent is to avoid trailing .0 by specifying zero precision. However, I changed this to use a static cast to an integer instead, which I'm hoping is more intuitive?

We want to avoid trailing '.0's in the output of percentile
values. Effectively we want to represent integer values.
So static cast the input to libfmt to int, instead of specifying a
precision of 0.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 9, 2020

Ugh:

ASAN ran out of memory again. It seems this now happens when building the Envoy
dependency:

-o bazel-out/k8-dbg/bin/external/envoy/test/mocks/server/_objs/server_mocks/mocks.pic.o)
Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox
clang: error: unable to execute command: Killed

It should be possible to scale back --jobs some more to resolve this.

TSAN reported a different kind of failure. Looking into that, we're tracking that one over at #317.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Copy link
Copy Markdown
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Thank you for the changes.

@mum4k mum4k merged commit adc3df5 into envoyproxy:master Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 waiting-for-review A PR waiting for a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants