Add Sink concept plus a filesytem based implementation by oschaaf · Pull Request #624 · envoyproxy/nighthawk

oschaaf · 2021-02-06T19:30:12Z

This PR has Sink, MockSink, FileSinkImpl & tests.

Sink will serve as a backing store for ExecutionResponse proto representations.

Context:

Later on a new service will expose storage and lookup methods.
This in turn can be used in a horizontally scaled system as a means to
realize a central point to transport results to and from.

Part of the horizontal scaling effort.
Split out from https://github.com/oschaaf/nighthawk/tree/horizontal-scaling

Signed-off-by: Otto van der Schaaf ovanders@redhat.com

This PR has Sink, MockSink, FileSinkImpl & tests. Sink will serve as a backing store for ExecutionResponse proto representations. Context: - Later on a new service will expose storage and lookup methods. - This in turn can be used in a horizontally scaled system as a means to realize a central point to transport results to and from. Part of the horizontal scaling effort. Split out from https://github.com/oschaaf/nighthawk/tree/horizontal-scaling Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

mum4k · 2021-02-15T04:24:03Z

@eric846 please review and assign back to me once done.

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

eric846

LGTM with some minor points that are all optional.

eric846 · 2021-02-18T13:52:30Z

source/common/sink_impl.cc

+namespace {
+
+absl::Status verifyStringIsGuid(absl::string_view s) {
+  Envoy::Random::RandomGeneratorImpl random;


Is there a security or performance reason not to put a specific uuid format explicitly in the API contract and replace this logic with a regular expression?

Well, my idea here is that the sink implementation will be the constraining factor when it comes to determining what is ok and not ok to specify for an execution_id. When initiating a load test, and not specifying an execution id, it will be auto-generated, and that will be a unique identifier.

In this file backed implementation I'm being a bit overly cautious, as a guid will have a reasonable length and contain no characters that would be illegal in a filename. I hope to find a way to easily validate a filename and make sure there's no weird stuff in it.

Later on it will also be possible to manually specify an execution id when initiating an execution, and that might be useful when you want an id that is more easily remembered. As it stands, this implementation will reject that.

eric846 · 2021-02-18T14:10:48Z

test/common/sink_test.cc

+  nighthawk::client::ExecutionResponse result_to_store;
+  *(result_to_store.mutable_execution_id()) = this->executionIdForTest();
+  absl::Status status = sink.StoreExecutionResultPiece(result_to_store);
+  ASSERT_EQ(status.ok(), true);


nit: If ASSERT_TRUE / ASSERT_FALSE are available, it seems more standard (here and below).

done: 00ebf87

eric846 · 2021-02-18T14:34:27Z

source/common/sink_impl.cc

+    std::ifstream ifs(it.path(), std::ios_base::binary);
+    if (!response.ParseFromIstream(&ifs)) {
+      return absl::Status(absl::StatusCode::kInternal,
+                          fmt::format("Failure reading/parsing '{}'.", it.path()));


nit: It could be helpful for debugging to mention ExecutionResponse in this error message.

done: 94f7299

eric846 · 2021-02-18T14:34:45Z

source/common/sink_impl.cc

+FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const {
+  absl::Status status = verifyStringIsGuid(execution_id);
+  if (!status.ok()) {
+    return status; //


nit: Extraneous // or forgotten comment?

removed in 94f7299

eric846 · 2021-02-18T14:36:06Z

source/common/sink_impl.cc

+  {
+    std::ofstream ofs(name_buffer.data(), std::ios_base::out | std::ios_base::binary);
+    if (!response.SerializeToOstream(&ofs)) {
+      return absl::Status(absl::StatusCode::kNotFound, "Failure writing to temp file");


Not sure if this is status code distinction is intentional.

No it wasn't, good catch. Changed in 94f7299

eric846 · 2021-02-18T14:46:36Z

test/common/sink_test.cc

+
+TEST(FileSinkTest, CorruptedFile) {
+  FileSinkImpl sink;
+  const std::string execution_id = "14e75b2a-3e31-4162-9279-add1e54091f9";


Any reason not to use this->executionIdForTest() here?

Well, this test is different from the others as it doesn't have a fixture. (it's not part of the TEST_P ones)

Ohh I see. Do we need to clean up the directory separately here?

eric846 · 2021-02-18T14:47:39Z

test/common/sink_test.cc

+
+template <typename T> class TypedSinkTest : public ::testing::Test {
+public:
+  void SetUp() override { uuid_ = random_.uuid(); }


Could we automatically clean up "/tmp/nh/" + executionIdForTest() + "/" in TearDown? (if it makes sense)

Done in 94f7299. It's a little odd to do it here, as this may execute for other sink implementations when the TEST_P suites iterate over their parameterization, and will not write files to that location. But cleaning up is rather nice, and this won't cause trouble.
So it seems pragmatic just add the two lines to clean up here.

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf · 2021-02-18T21:48:50Z

Thanks for the review @eric846

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

api/client/service.proto

include/nighthawk/common/sink.h

source/common/sink_impl.cc

source/common/sink_impl.h

include/nighthawk/common/sink.h

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

mum4k · 2021-03-01T20:50:59Z

Thank you for amending the PR @oschaaf. Just one remaining question in the thread about validating UUIDs.

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Otto van der Schaaf added 7 commits February 6, 2021 20:22

Clean up

ca2b61c

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Constrain filenames to guids, enhance testing.

01e7f95

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

10b0970

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Add comment

7c320d8

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Solidify filesystem operations

20b6ab3

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Test loading of corrupted file

bbba6e6

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf marked this pull request as ready for review February 13, 2021 22:17

oschaaf added the waiting-for-review A PR waiting for a review. label Feb 13, 2021

mum4k requested a review from eric846 February 15, 2021 04:23

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

4c0dff1

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

eric846 previously approved these changes Feb 18, 2021

View reviewed changes

eric846 assigned mum4k Feb 18, 2021

Otto van der Schaaf added 3 commits February 18, 2021 20:54

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

d632b5c

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

use ASSERT_TRUE/ASSERT_FALSE

00ebf87

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Review feedback

94f7299

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf dismissed eric846’s stale review via 94f7299 February 18, 2021 21:43

Amend broken text expectation

65e8e49

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

mum4k requested changes Feb 19, 2021

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Feb 19, 2021

Otto van der Schaaf added 5 commits February 19, 2021 09:57

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

630af47

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Fix format

9e00ea8

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

5dd2aae

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

5629e55

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Review feedback

8f8a5da

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Feb 27, 2021

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Mar 1, 2021

Otto van der Schaaf added 2 commits March 2, 2021 10:36

review feedback

6a31b48

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

a5008c3

…sink Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Mar 2, 2021

mum4k approved these changes Mar 2, 2021

View reviewed changes

mum4k merged commit 261abb6 into envoyproxy:main Mar 2, 2021

Conversation

oschaaf commented Feb 6, 2021

Uh oh!

mum4k commented Feb 15, 2021

Uh oh!

eric846 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Feb 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mum4k commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants