Dockerized benchmarking framework#337

Merged

mum4k merged 70 commits intoenvoyproxy:masterfrom

oschaaf:ci-integration

Jun 26, 2020

Member

oschaaf commented May 31, 2020 •

edited

Loading

This PR has changes that make it easy to consume our integration testing as a benchmarking
tool framework for external repositories, allowing those to write custom tests for running in CI.

This probably needs to be split up into multiple PR's, but this allows reviewers to
have an early peek at what such PR's would lead up to.

The README.md in here has more context, but the primary goals are te be able to:

run the tests against arbitrary Envoy revisions
persist profile dumps, flamegraphs, and latency numbers per test
run the nighthawk tools via docker
offer stock tests, but also allow scaffolding consumer-specific tests

Example of a fully dockerized flow (more example in README.md):

# This script runs the dockerized benchmarking framework, which in
# turn will pull Nighthawk and Envoy in via docker. 

set -eo pipefail
set +x
set -u

# The benchmark logs and artifacts will be dropped here
OUTDIR="/home/oschaaf/code/envoy-perf-vscode/nighthawk/benchmarks/tmp/"
# Used to map the test that we want to see executed into the docker container
TEST_DIR="/home/oschaaf/code/envoy-perf-vscode/nighthawk/benchmarks/test/"

# Rebuild the docker in case something changed.
./docker_build.sh && 
docker run -it --rm \
  -v "/var/run/docker.sock:/var/run/docker.sock:rw" \
  -v "${OUTDIR}:${OUTDIR}:rw" \
  -v "${TEST_DIR}:/usr/local/bin/benchmarks/benchmarks.runfiles/nighthawk/benchmarks/external_tests/" \
  --network=host \
  --env NH_DOCKER_IMAGE="envoyproxy/nighthawk-dev:latest" \
  --env ENVOY_DOCKER_IMAGE_TO_TEST="envoyproxy/envoy-dev:f61b096f6a2dd3a9c74b9a9369a6ea398dbe1f0f" \
  --env TMPDIR="${OUTDIR}" \
  oschaaf/benchmark-dev:latest ./benchmarks --log-cli-level=info -vvvv

Part of #245

oschaaf added 30 commits

November 5, 2019 10:38


          Request header option: fix a bug in handling ':'

7e2c064

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


          WIP: extend benchmarking tests

a750487

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


          Merge remote-tracking branch 'upstream/master' into extend-benchmarking

dbd8aaa

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


          Fix import path in nighthawk_grpc_service.py

8304ee2

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


          Save state on Envoy CI integration work

c1b2660

- Allow overriding some env vars so we can run outside of bazel
- Add a README.md detailing the plan & state

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


          minimize diff

c0f7eb2

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


          update benchmark/README.md

740fc59

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


          update benchmark/README.md

0971a8d

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


          Merge remote-tracking branch 'upstream/master' into ci-integration

82160ab

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


          benchmarking: save state on running with docker and inject Envoy

dfed07c

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


          Expectation for changed host & multiserver feature

46f1a42

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


          Leanify benchmarks/ BUILD. Update README.md

0387f47

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


          remove docker volume we don't need

6ff475a

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


          save state on structing output layout in tmp

c2b34c5

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


          Merge remote-tracking branch 'upstream/master' into ci-integration

14e6ee2


          Save state: small cleanup & simplification

1fd2b8b

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


          save state on some small cleanup

74578ca

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


          Fixes for running benchmark directly via bazel test...

7d428b7

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


          Save state.

f955a1f

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


          Save state

7bdb4b5

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


          Save state

0ae0c60

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


          format & dump nighthawk outputs to disk

1ba6686

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


          Fix python requirements

748d770

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


          Initial dockerization of benchmarks

1dd5448

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


          Improve runfiles path handling

d3ccf66

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


          Fix sni config paths

fe55e0f

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


          Docker build: flatten symlinks

666902f

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


          Add ENV var defaults, fix typo in env var name.

1b373a3

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


          Save fully dockerized example sourcing external benchmarks.

32f4024

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


          Update readme

fd436bd

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

oschaaf added 2 commits

June 19, 2020 14:26


          Formatting fix & make proxy_config private

fa75665

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


          Review feedback: EnvoyProxy - point out an example

08fb5d8

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

Member Author

oschaaf commented Jun 19, 2020

@mum4k thanks, I think I addressed everything. In addition, I added a small tweak to way docker gets run to avoid breakage in the docker flows when they use the latest Envoy revisions:
c24062f#diff-281f9acd4aa73a1b04f644ae61f98febR94

oschaaf added waiting-for-review and removed waiting-for-changes labels


          Merge remote-tracking branch 'upstream/master' into ci-integration

8caf8d5

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

Member Author

oschaaf commented Jun 19, 2020

Another thing I should explicitly call out is that for benchmark/ I added some more python flake8 checks in CI.
Generally check more things, and specifically add docstring (format) checks.

https://github.com/envoyproxy/nighthawk/pull/337/files#diff-ae86159524d9bc451d3e3cc04dc48bb2

oschaaf added 2 commits

June 19, 2020 23:23


          Merge remote-tracking branch 'upstream/master' into ci-integration

9f61f72

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


          Merge remote-tracking branch 'upstream/master' into ci-integration

b1d5634

mum4k requested changes

View reviewed changes

Collaborator

mum4k left a comment

Managed to complete the review now, sorry about any delay incurred.

test/integration/integration_test_fixtures.py Outdated

               from test.integration.nighthawk_test_server import NighthawkTestServer
               from test.integration.nighthawk_grpc_service import NighthawkGrpcService
+              TIMESTAMP = time.strftime('%Y-%m-%d-%H-%M-%S')

Collaborator

mum4k Jun 22, 2020

(nit) Can we either make this private (i.e. _TIMESTAMP), or document it?

test/integration/integration_test_fixtures.py

                 """
-                def __init__(self, ip_version, backend_count=1):
+                def __init__(self, ip_version, server_config, backend_count=1):

Collaborator

mum4k Jun 22, 2020

Can we document server_config in the Args: section of the docstring?

test/integration/integration_test_fixtures.py Outdated

-                  self.nighthawk_test_server_path = os.path.join(self.test_rundir, "nighthawk_test_server")
-                  self.nighthawk_test_config_path = None
-                  self.nighthawk_client_path = os.path.join(self.test_rundir, "nighthawk_client")
+                  self.confdir = "nighthawk/test/integration/configurations/"

Collaborator

mum4k Jun 22, 2020

We are adding some public attributes, can we document them in the class docstring? It will ideally have an "Attributes:" section.

This is partially pre-existing since none of the other attributes are documented there. Up to you if we document all now, or leave a todo for the pre-existing ones. We should still prefer making anything that can be private.

test/integration/integration_test_fixtures.py Outdated

-                      "[", "_").replace("]", "")
-                  self.parameters["test_id"] = test_id
+                  if os.getenv("NH_DOCKER_IMAGE", "") == "":
+                    assert (os.path.exists(self.nighthawk_test_server_path))

Collaborator

mum4k Jun 22, 2020

Can we amend these assertions with some error message that would help the receiver of this error message understand what wen't wrong?

Something like:
assert False, "custom error message"

test/integration/integration_test_fixtures.py Outdated

+                    assert (os.path.exists(self.nighthawk_test_server_path))
+                    assert (os.path.exists(self.nighthawk_client_path))
+                  self.test_id = os.environ.get('PYTEST_CURRENT_TEST').split(':')[-1].split(' ')[0].replace(

Collaborator

mum4k Jun 22, 2020

Ideally attributes on classes are all initialized (and documented if public) inside the init method. Crating attributes "dynamically" in other methods makes it harder to find the complete list (API) of the class.

Can we initialize self.test_id to some useful value inside init? If nothing else, we can set it to the zero value of the same type (an empty string?).

test/integration/nighthawk_test_server.py Outdated

-                  args = [
+                  args = []
+                  if self.docker_image != "":
+                    # TODO(XXX): As of https://github.com/envoyproxy/envoy/commit/e8a2d1e24dc9a0da5273442204ec3cdfad1e7ca8

Collaborator

mum4k Jun 22, 2020

Did we mean to replace XXX with a value?

test/integration/nighthawk_test_server.py


		Path(self.tmpdir).mkdir(parents=True, exist_ok=True)

		with tempfile.NamedTemporaryFile(

Collaborator

mum4k Jun 22, 2020

Are there any parts of this function that we could move out so that we make init shorter and easier to follow?

test/integration/nighthawk_test_server.py

-                                                            ip_version, "--config-path", parameters)
+                                                            ip_version, "--config-path", parameters, tag)
+                def getCliVersionString(self):

Collaborator

mum4k Jun 22, 2020

Can we document this method?

test/integration/test_grpc_service.py

               import pytest
-              from test.integration.integration_test_fixtures import http_test_server_fixture
+              from test.integration.integration_test_fixtures import (http_test_server_fixture, server_config)

Collaborator

mum4k Jun 22, 2020

(optional for this PR) comment about only importing modules / packages applies here too.

Optional, because it is pre-existing and we probably don't want to increase the scope of this PR.

test/integration/test_integration_basics.py Show resolved Hide resolved

mum4k added waiting-for-changes and removed waiting-for-review labels

oschaaf mentioned this pull request

Benchmark framework docker flow should not run as root #383

Open


          Review feedback

a294f29

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

oschaaf added waiting-for-review and removed waiting-for-changes labels

oschaaf added 2 commits

June 24, 2020 23:44


          Merge remote-tracking branch 'upstream/master' into ci-integration

23d9090

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


          Expose nighthawk_client_path as we need it now

9e10892

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

mum4k requested changes

View reviewed changes

Collaborator

mum4k left a comment

Last comment and a complete review.

test/integration/nighthawk_test_server.py

                 """
                   Base class for running a server in a separate process.
+                  Arguments:

Collaborator

mum4k Jun 25, 2020

(nit) The "Args:" section of a class docstring actually belongs to the docstring of the initializer (init). Would you mind moving it there?

I am also happy to do this in a follow-up PR since this is minor, just let me know.

Member Author

oschaaf Jun 25, 2020

I prefer to do it in a follow up: this file exists out of benchmarks/, I'm hoping to tighten up the format checking so that I can get all the python to comply with the check format rules that are now specific to benchmark/. Being backed by that makes it easier, as the doc string format isn't in my muscle memory yet. Added a note to #371 as a reminder.

Member Author

oschaaf Jun 26, 2020

Forgot to mark this is ready for review after posting the comment above - just did so.

mum4k added waiting-for-changes and removed waiting-for-review labels

oschaaf mentioned this pull request

Replace occurrences of "importing * from .." in python code #371

Closed

oschaaf added waiting-for-review and removed waiting-for-changes labels

mum4k approved these changes

View reviewed changes

mum4k merged commit 6747458 into envoyproxy:master

oschaaf mentioned this pull request

Stricter and more checks for python code #395

Merged

wjuan-AFK pushed a commit to wjuan-AFK/nighthawk that referenced this pull request


          Dockerized benchmarking framework (envoyproxy#337)

41dd36e

This PR has changes that make it easy to consume our integration testing as a benchmarking
tool framework for external repositories, allowing those to write custom tests for running in CI.

This probably needs to be split up into multiple PR's, but this allows reviewers to
have an early peek at what such PR's would lead up to.

The `README.md` in here has more context, but the primary goals are te be able to:
- run the tests against arbitrary Envoy revisions
- persist profile dumps, flamegraphs, and latency numbers per test
- run the nighthawk tools via docker
- offer stock tests, but also allow scaffolding consumer-specific tests

Example of a fully dockerized flow (more example in `README.md`):

```

set -eo pipefail
set +x
set -u

OUTDIR="/home/oschaaf/code/envoy-perf-vscode/nighthawk/benchmarks/tmp/"
TEST_DIR="/home/oschaaf/code/envoy-perf-vscode/nighthawk/benchmarks/test/"

./docker_build.sh &&
docker run -it --rm \
  -v "/var/run/docker.sock:/var/run/docker.sock:rw" \
  -v "${OUTDIR}:${OUTDIR}:rw" \
  -v "${TEST_DIR}:/usr/local/bin/benchmarks/benchmarks.runfiles/nighthawk/benchmarks/external_tests/" \
  --network=host \
  --env NH_DOCKER_IMAGE="envoyproxy/nighthawk-dev:latest" \
  --env ENVOY_DOCKER_IMAGE_TO_TEST="envoyproxy/envoy-dev:f61b096f6a2dd3a9c74b9a9369a6ea398dbe1f0f" \
  --env TMPDIR="${OUTDIR}" \
  oschaaf/benchmark-dev:latest ./benchmarks --log-cli-level=info -vvvv
```

Part of envoyproxy#245

oschaaf mentioned this pull request

Make it easy to integrate Nighthawk into CI of other projects #29

Open

oschaaf added a commit that referenced this pull request


          Python linting enhancements + 3.8 compat fix (#395)

44ff257

In #337 some review comments related to doc comments have been postponed to a followup.

- Adds more flake8 convention checks. This broadens the checks we introduced in the
benchmarking flow to cover the global code base, and exclude a few
files that we either don't own or care about.
- Updates yapf & flake8 version requirements, which enhances automatic formatting,
as well as fixes a python 3.8 compatibility bug.

Fixes #371
Fixes #400

Summary of compliance failures under the proposed checks before this change:

13    D100 Missing docstring in public module
2     D101 Missing docstring in public class
9     D102 Missing docstring in public method
46    D103 Missing docstring in public function
4     D107 Missing docstring in __init__
29    D200 One-line docstring should fit on one line with quotes
9     D202 No blank lines allowed after function docstring
32    D205 1 blank line required between summary line and description
3     D208 Docstring is over-indented
1     D209 Multi-line docstring closing quotes should be on a separate line
1     D210 No whitespaces allowed surrounding docstring text
1     D300 Use """triple double quotes"""
39    D400 First line should end with a period
16    D401 First line should be in imperative mood
1     E124 closing bracket does not match visual indentation
2     E125 continuation line with same indent as next logical line
7     E126 continuation line over-indented for hanging indent
1     E401 multiple imports on one line
2     E714 test for object identity should be 'is not'
4     F403 'from test.integration.utility import *' used; unable to detect undefined names
152   F405 'assertCounterEqual' may be undefined, or defined from star imports: test.integration.utility
2     F841 local variable 'logs' is assigned to but never used
8     W291 trailing whitespace
1     W293 blank line contains whitespace
3     W504 line break after binary operator
1     W605 invalid escape sequence '\:'
389

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 waiting-for-review