[salvo] Initial commit for benchmark abstraction framework #73

abaptiste · 2020-10-12T18:51:14Z

This is the initial 'official' commit for the Salvo tool. This aims to abstract the execution of nighthawk to benchmark a given envoy version.

See this issue for some background. The two design docs for this project are referenced here.

In this commit, salvo is placed into a separate directory of the envoy-perf repository, and is referenced from the main README.md.

cc: @oschaaf, @htuch, @landesherr

Signed-off-by: Alvin Baptiste [email protected]

Commit Message: [salvo] Initial commit for benchmark abstraction framework
Testing: Unit tests included, Address as many pylint3 issues as feasible
[#Issue] envoyproxy/envoy#11266

Signed-off-by: Alvin Baptiste <[email protected]>

mattklein123 · 2020-10-12T22:53:45Z

@jmarantz @oschaaf would you be willing to do a lightweight review of this?

oschaaf · 2020-10-13T08:39:18Z

Sure; the first questions that pop up for me are:

Do we want to establish/modify checks in CI for Python conventions? (And maybe proto3 & bazel files too?). We hypothetically could copy over some things from Nighthawk and/or Envoy to create a starting point.
There are build tasks in .circleci/config.yml, can we leverage that in this PR? E.g it seems unit tests are provided in here, we could execute those in CI?

oschaaf

awesome; flushing out a first round of comments; the PR is pretty large so I may need some more takes to get through it all, but overall I have a feeling it looks great. Before going into more depth, I think that establishing automated code-level conventions would be good as that helps smooth the review process by automating away part of it.

salvo/README.md

salvo/salvo.py

salvo/src/lib/api/control.proto

Signed-off-by: Alvin Baptiste <[email protected]>

This reverts commit e42a89c. Signed-off-by: Alvin Baptiste <[email protected]>

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste · 2020-10-19T20:44:48Z

I will work on making this change set a bit smaller.

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k · 2020-11-03T18:30:32Z

Hi @abaptiste, my name is Jakub and I am one of the maintainers of Nighthawk. Recently I have been working on collecting requirements for a benchmarking framework of Envoy and I was very excited to hear that you are ahead and have something ready for review. My hope is that we can cooperate towards our common goals.

It will be my pleasure to start reviewing this PR and I am sure we can gain some traction going forward. Sorry about the delay you have encountered here so far, I was otherwise occupied and couldn't get here sooner. I will start reading the code here today and expect to ask some questions later today or tomorrow. I expect to start the review at a high level and will need you help to work towards the details.

Meanwhile it would help me if you could read the linked requirements and highlight any areas of concern or questions you might have. Looking forward to working with you on this.

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k

Thank you for iterating on this, the BUILD system setup looks much easier to follow. I have a few more improvement suggestions on that front. I have also reviewed the documentation, most of the helper scripts and the proto API in this round.

I will start reviewing the Python code in the next round. Thank you for your patience, this is mostly taking this many rounds because the scope of this PR is fairly large. We can try to address that in future PRs by making them smaller. Happy to discuss the best approach, one option is to send separate PRs per each api / module / package. Reviewing and addressing comments on large PRs can sometimes feel like running a marathon ;)

ci/do_ci.sh

salvo/README.md

salvo/BUILD

salvo/src/lib/BUILD

salvo/src/lib/api/source.proto

mum4k · 2020-11-04T03:21:25Z

salvo/tools/format_python_tools.sh

+# E126 Continuation line over-indented for hanging indent
+
+# We ignore false positives because of what look like pytest peculiarities 
+# F401 Module imported but unused


We didn't get to reviewing Python code yet, but these two feel like they may be result of how we are importing code / setting up the build system. It may be valuable for us to try and address them before we silence them which can result in accumulation of technical debt. Can you help me out by pointing to the locations where these warnings come up? I might be able to suggest a simple fix.

I copied these scripts from nighthawk and they are largely unmodified.

Can we circle back to this because there are a few games we have to play with adding directories to the path because of the source layout and 'bazel'? We can certainly tighten up the style and formatting checks.

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k

Thank you for cleaning up the build system setup. Looks like this PR still has a number of comments that weren't addressed yet (they are marked as "Outdated" due to the file moves, but their content still applies). Can you please look at them in the conmversation history and address them before we move forward?

Most of the comments that are still in flight are related to the API. We have already finished reviewing the documentation and helper scripts (format tools, download dependencies, repo setup). I have a proposal that may help us decrease the scope of this PR while moving forward. How would you feel about removing all the Python code and tests from this PR and merging what we have reviewed so far (after addressing the existing comments)? We could then immediately follow up with a set of smaller PRs, one per each Python library and its tests. The benefit would be that each of the smaller PRs would take less time in review and would make it easier for us to keep track of the conversation and changes.

Please let me know what you think.

mum4k · 2020-11-06T00:54:15Z

ci/do_ci.sh

+function build_salvo() {
+  echo "Building Salvo"
+  pushd salvo
+  bazel build //:salvo


(optional) If the intention here is to build all of our code, we should also change this to:

bazel build //src/...

mum4k · 2020-11-06T00:54:42Z

salvo/README.md

+
+## Example Control Documents
+
+The control document defines the data needed to excute a benchmark. At the moment, the fully dockerized benchmark is the only one supported. This benchmark discoveres user supplied tests for execution and uses docker images to run the tests. In the exampple below, the user supplied tests files are located in `/home/ubuntu/nighthawk_tests` and are mapped to a volume in the docker container.


typo: execute

mum4k · 2020-11-06T00:55:22Z

salvo/README.md

+  "remote": false,
+  "dockerizedBenchmark": true,
+  "images": {
+    "reuseNhImages": true,


Note: we may need to update these examples after we perform some of the suggested proto field renames.

Right. Certainly doable.

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k

Thank you for the offline discussion @abaptiste. As we agreed, we will keep the Python code in this PR and achieve greater velocity by instead switching the focus of this review to architecture. This will help us move faster while giving us the opportunity to iterate on the code in future changes.

With that said, this review round represents the last set of comments I have at that level. Thank you for your patience with this process and looking forward to the next PRs. I will tag one of the maintainers here for merging after these comments are addressed.

mum4k · 2020-11-06T17:59:40Z

ci/do_ci.sh

+  pushd salvo
+  ./install_deps.sh
+
+  bazel test //src/lib/...


(nit) This can now also be bazel test //....

mum4k · 2020-11-06T18:00:28Z

salvo/BUILD

+
+py_binary(
+    name = "salvo",
+    srcs = ["src/salvo.py"],


As discussed offline, the move of salvo.py into the src/ directory was likely caused by a mistake I made in one of my comments. If we can, let's move it back to the salvo/ directory.

I had a merge conflict and I didn't catch this. I'll fix and update

mum4k · 2020-11-06T18:10:38Z

salvo/src/lib/docker_volume.py

+
+class DockerVolume():
+
+  @staticmethod


Instead of marking this method static, can we define it as a module level function (i.e. remove the class DockerVolume).

We should prefer standalone functions unless having a class is really necessary. We can test for that by identifying if the class either has interesting state that changes during its lifetime or if the code needs to maintain multiple instances of the class. If we don't have good reasons for either of these two, having a standalone method will result in simpler code.

The style guide also recommends avoiding staticmethod:
https://github.com/google/styleguide/blob/gh-pages/pyguide.md#217-function-and-method-decorators

mum4k · 2020-11-06T18:15:00Z

salvo/src/salvo.py

+
+# Run in the actual bazel directory so that the sys.path
+# is setup correctly
+if os.path.islink(sys.argv[0]):


Can you help me understand this path manipulation and the site.addsitedir("src") call below?

I wouldn't expect this to be necessary in a bazel environment. We should be able to delete all these manipulations from here and the other files and just go with an import rooted at the beginning of the WORKSPACE:

from src.lib.job_control_loader import load_control_doc

I have tested this locally by removing all the manipulation and executing the salvo target which seems to have worked at least to a point that it imported all libraries correctly. Do we need to keep this or can we do without it?

This is an artifact of us getting salvo to run in a cloud service. We noticed that bazel would symlink files to the original location so, the current working directory would be the local source location and not bazel-bin where we expect. This was true for the python generated code for the api. We needed these files packaged in a library and were not able to accomplish that without the working directory manipulation.

I can remove it here, but there are other instance where we need it to remain.

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste · 2020-11-06T18:43:25Z

@mum4k I pushed the last set of changes. Thanks for your reviews!

mum4k

Thank you for addressing the comments @abaptiste.

@htuch this PR is good to go in, please take a look.

htuch

Since @mum4k is good with this, I think it would be great to ship and land. I had a quick scan and noticed a few nits that would be good to sort out, happy to merge when they are fixed.

htuch · 2020-11-06T19:29:55Z

salvo/api/docker_volume.proto

+
+  // Define whether the mount point is read-write or read-only
+  // eg: 'rw' or 'ro'
+  string mode = 2;


Probably should be an enum, but don't feel super strong.

Enum it is.

htuch · 2020-11-06T19:30:22Z

salvo/api/env.proto

+  oneof envoy_ip_test_versions {
+    bool v4only = 1;
+    bool v6only = 2;
+    bool all = 3;


Probably should an enum.

htuch · 2020-11-06T19:30:57Z

salvo/api/source.proto

+  // Specify whether this source location is Envoy or NightHawk
+  oneof identity {
+    bool envoy = 1;
+    bool nighthawk = 2;


Enum please.

htuch · 2020-11-06T19:32:23Z

salvo/src/lib/docker_volume.py

+  # Setup the test directory
+  if test_dir:
+    properties = VolumeProperties()
+    properties.bind = '/usr/local/bin/benchmarks/benchmarks.runfiles/nighthawk/benchmarks/external_tests/'


What is this hardcoded path?

This is the location in the container that the external tests are mapped to. The path is used from this example.

htuch · 2020-11-06T19:32:51Z

salvo/src/lib/job_control_loader.py

+  contents = None
+
+  # Try loading the contents based on the file extension
+  if re.match(r'.*\.json', filename):


filename.endswith('.json') etc. below

Certainly. Will update.

htuch · 2020-11-06T19:33:25Z

salvo/src/lib/test_docker_image.py

+
+  env = ['key1=val1', 'key2=val2']
+  cmd = ['uname', '-r']
+  image_name = 'oschaaf/benchmark-dev:latest'


How come Otto's image is hardcoded?

It was available in dockerhub and I used it to validate that the docker module could actually pull an image and execute a command. The actual image here is moot and could be any valid docker image.

htuch · 2020-11-06T19:34:14Z

salvo/src/lib/test_job_control_loader.py

+      assert source.source_path == "/home/ubuntu/envoy"
+      assert not source.source_url
+      assert source.branch == "master"
+      assert source.commit_hash == "e744a103756e9242342662442ddb308382e26c8b"


What's the plan to deal with these hardcoded hashes? Should they be in some configuration file?

These are just string literals with no meaning. This could be "foo" and work the same.

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste · 2020-11-06T20:52:44Z

@htuch This is ready for you again. Thanks!

htuch · 2020-11-06T21:00:48Z

@abaptiste looks good, but what about the enums?

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste · 2020-11-06T22:06:13Z

@htuch Sorry about the enums. No excuses. Updated the PR.

htuch

Thanks, excited to see this land!

oschaaf · 2020-11-09T21:53:02Z

Exciting indeed, awesome!

abaptiste · 2020-11-10T17:11:46Z

Thanks folks!

[salvo] Initial commit for benchmark abstraction framework

dadcf0c

Signed-off-by: Alvin Baptiste <[email protected]>

oschaaf suggested changes Oct 13, 2020

View reviewed changes

abaptiste added 5 commits October 13, 2020 19:02

[salvo] Fix formatting and remove vim format metadata

6bc1487

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Add JobControl message comment

f5d5f8d

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Update do_ci.sh to build and test salvo

75e98e3

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Update README.md

d0c7a5b

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Fix formatting and remove vim format metadata

d5558a8

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste force-pushed the master branch 2 times, most recently from 7dd9c93 to 2401dda Compare October 13, 2020 19:36

[salvo] Skip running tests in CI

8dfffa1

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste force-pushed the master branch from 2401dda to 8dfffa1 Compare October 13, 2020 19:36

abaptiste added 4 commits October 13, 2020 20:04

[salvo] Run tests and collect log output on script exit

e42a89c

Signed-off-by: Alvin Baptiste <[email protected]>

Revert "[salvo] Run tests and collect log output on script exit"

8a62d02

This reverts commit e42a89c. Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] fix docstring comments

3d5b455

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Support referencing images using a tag

2820352

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste force-pushed the master branch 2 times, most recently from a062d76 to 2820352 Compare October 15, 2020 20:20

abaptiste added 7 commits October 19, 2020 22:22

[salvo] install dependecies and use a newer container for CI

8ea0e61

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Trim PR

00e4202

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Trim PR

5a4dce3

Signed-off-by: Alvin Baptiste <[email protected]>

Kick CI

3f34d06

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Specify a tag for the build container image

822c0b1

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] fix the CI config formatting

05bea79

Signed-off-by: Alvin Baptiste <[email protected]>

[salvo] Skip docker tests if the socket is missing

ba882b6

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste requested review from oschaaf and htuch November 3, 2020 16:59

abaptiste added 2 commits November 4, 2020 00:00

Cleanup and fomatting fixes

5129766

Signed-off-by: Alvin Baptiste <[email protected]>

More formatting fixes

35a26a0

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste dismissed oschaaf’s stale review via 35a26a0 November 4, 2020 00:07

Fix build failure

22baac5

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k requested changes Nov 4, 2020

View reviewed changes

abaptiste added 7 commits November 4, 2020 17:01

Address PR Feedback

81cd018

Signed-off-by: Alvin Baptiste <[email protected]>

Move api to top level directory

79b2cbe

Signed-off-by: Alvin Baptiste <[email protected]>

Remove patch field from source.proto

4291ade

Signed-off-by: Alvin Baptiste <[email protected]>

Make the lib target private

c5f8f2c

Signed-off-by: Alvin Baptiste <[email protected]>

Remove glob in api/BUILD

2392ae3

Signed-off-by: Alvin Baptiste <[email protected]>

BUILD file swizzling

5c26812

Signed-off-by: Alvin Baptiste <[email protected]>

Use ellipses for tests

73d0dc0

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k requested changes Nov 6, 2020

View reviewed changes

abaptiste added 2 commits November 6, 2020 16:21

Address PR Feedback

30d2d5d

Signed-off-by: Alvin Baptiste <[email protected]>

Address PR Feedback

4f072a9

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k requested changes Nov 6, 2020

View reviewed changes

Address PR Feedback

84bd1af

Signed-off-by: Alvin Baptiste <[email protected]>

mum4k previously approved these changes Nov 6, 2020

View reviewed changes

htuch reviewed Nov 6, 2020

View reviewed changes

Address PR Feedback

975cf35

Signed-off-by: Alvin Baptiste <[email protected]>

abaptiste dismissed mum4k’s stale review via 975cf35 November 6, 2020 19:51

Update protos to use enums

1f207ab

Signed-off-by: Alvin Baptiste <[email protected]>

htuch approved these changes Nov 9, 2020

View reviewed changes

htuch merged commit 5c434c8 into envoyproxy:master Nov 9, 2020


		## Example Control Documents

		The control document defines the data needed to excute a benchmark. At the moment, the fully dockerized benchmark is the only one supported. This benchmark discoveres user supplied tests for execution and uses docker images to run the tests. In the exampple below, the user supplied tests files are located in `/home/ubuntu/nighthawk_tests` and are mapped to a volume in the docker container.

[salvo] Initial commit for benchmark abstraction framework #73

[salvo] Initial commit for benchmark abstraction framework #73

Conversation

abaptiste commented Oct 12, 2020

mattklein123 commented Oct 12, 2020

oschaaf commented Oct 13, 2020

oschaaf left a comment

Choose a reason for hiding this comment

abaptiste commented Oct 19, 2020 • edited Loading

mum4k commented Nov 3, 2020

mum4k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mum4k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mum4k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abaptiste commented Nov 6, 2020

mum4k left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abaptiste commented Nov 6, 2020

htuch commented Nov 6, 2020

abaptiste commented Nov 6, 2020 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

oschaaf commented Nov 9, 2020

abaptiste commented Nov 10, 2020

abaptiste commented Oct 19, 2020 •

edited

Loading

abaptiste commented Nov 6, 2020 •

edited

Loading