Workflow to run performance tests using opensearch-benchmark #3415

rishabh6788 · 2023-04-18T02:05:03Z

Description

This PR adds support to run performance tests against OpenSearch cluster using opensearch-benchmark. The workflow is very similar to perf-tests with below mentioned updates and improvements:

Uses public cdk infra package opensearch-cluster-cdk instead of private opensearch-infra repo.
Uses opensearch-benchmark for running and analyzing performance run tests instead of custom internal tool.
Supports single and as well as multi-node clusters.
Supports adding additional OS config from the command line.

Issues Resolved

opensearch-project/opensearch-benchmark#102

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Rishabh Singh <[email protected]>

codecov-commenter · 2023-04-18T02:11:03Z

Codecov Report

Merging #3415 (babffab) into main (8028342) will increase coverage by 0.09%.
The diff coverage is 93.59%.

❗ Current head babffab differs from pull request most recent head 9a7f6cb. Consider uploading reports for the commit 9a7f6cb to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3415      +/-   ##
==========================================
+ Coverage   91.74%   91.84%   +0.09%     
==========================================
  Files         172      181       +9     
  Lines        4991     5272     +281     
==========================================
+ Hits         4579     4842     +263     
- Misses        412      430      +18

Impacted Files	Coverage Δ
...k_test/benchmark_test_runner_opensearch_plugins.py	`83.33% <83.33%> (ø)`
..._workflow/benchmark_test/benchmark_test_cluster.py	`85.22% <85.22%> (ø)`
src/run_benchmark_test.py	`92.30% <92.30%> (ø)`
...t_workflow/benchmark_test/benchmark_test_runner.py	`95.00% <95.00%> (ø)`
...benchmark_test/benchmark_test_runner_opensearch.py	`96.55% <96.55%> (ø)`
src/test_workflow/benchmark_test/benchmark_args.py	`100.00% <100.00%> (ø)`
..._workflow/benchmark_test/benchmark_test_runners.py	`100.00% <100.00%> (ø)`
...st_workflow/benchmark_test/benchmark_test_suite.py	`100.00% <100.00%> (ø)`
src/test_workflow/test_jsonargs.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dblock

Looks pretty clean! Some nits.

Please document this in .md as part of this PR?

src/test_workflow/benchmark_test/benchmark_test_cluster.py

src/test_workflow/benchmark_test/benchmark_args.py

src/test_workflow/benchmark_test/benchmark_test_cluster.py

dblock · 2023-04-18T14:22:34Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

+        subprocess.check_call(command, cwd=os.getcwd(), shell=True)
+        with open(self.output_file, "r") as read_file:
+            load_output = json.load(read_file)
+            print(load_output[self.stack_name])


Use logging if you need to print something.

Not required, removed.

dblock · 2023-04-18T14:24:51Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

+            "minDistribution": "true" if self.args.minDistribution else "false",
+            "serverAccessType": config["Constants"]["serverAccessType"],
+            "restrictServerAccessTo": config["Constants"]["restrictServerAccessTo"],
+            "additionalConfig": self.args.additionalConfig,


There's a lot of pass through params. I wonder whether we're better off not exposing all these command line options and only accepting a config file and passing it through here?

Could you please give an example here, not sure if I understood the comment properly.
@dblock

I think I'm saying that almost all Count arguments come from args. I don't know whether that's the right answer, but a config file that contained all these options could be passed as 1 single argument --config foobar.yml. Then this code doesn't need to know about dataNodeCount for example, that would be contained inside foobar.yml.

The idea to keep them as args is that there may be cases where a user wants to try different cluster configurations for running benchmark in parallel with different node counts for each cluster. Another is the ML use case where a user wants to have ML node enabled.
@dblock

That's fine. I think we're just arguing command line params vs. a config file.

src/run_benchmark_test.py

src/test_workflow/benchmark_test/benchmark_args.py

src/test_workflow/benchmark_test/benchmark_test_cluster.py

dblock · 2023-04-19T22:50:48Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

+            "minDistribution": "true" if self.args.minDistribution else "false",
+            "serverAccessType": config["Constants"]["serverAccessType"],
+            "restrictServerAccessTo": config["Constants"]["restrictServerAccessTo"],
+            "additionalConfig": self.args.additionalConfig,


I think I'm saying that almost all Count arguments come from args. I don't know whether that's the right answer, but a config file that contained all these options could be passed as 1 single argument --config foobar.yml. Then this code doesn't need to know about dataNodeCount for example, that would be contained inside foobar.yml.

dblock

You still have the camelCase variables all over the place, that's not the convention in the rest of the project. Example: https://github.com/opensearch-project/opensearch-build/blob/main/src/test_workflow/perf_test/perf_args.py#L21

src/test_workflow/benchmark_test/benchmark_test_cluster.py

dblock · 2023-04-25T17:52:24Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

+        self.stack_name = f"opensearch-infra-stack-{self.args.stack_suffix}-{self.manifest.build.id}-{self.manifest.build.architecture}"
+
+    def start(self) -> None:
+        # os.chdir(self.work_dir)


remove commented code

dblock · 2023-04-25T17:53:19Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

+            "minDistribution": "true" if self.args.minDistribution else "false",
+            "serverAccessType": config["Constants"]["serverAccessType"],
+            "restrictServerAccessTo": config["Constants"]["restrictServerAccessTo"],
+            "additionalConfig": self.args.additionalConfig,


That's fine. I think we're just arguing command line params vs. a config file.

rishabh6788 · 2023-04-26T17:42:01Z

You still have the camelCase variables all over the place, that's not the convention in the rest of the project. Example: https://github.com/opensearch-project/opensearch-build/blob/main/src/test_workflow/perf_test/perf_args.py#L21

Got it! let me refactor everything and update accordingly. Too much coding in TS got me into this habit :-|

rishabh6788 · 2023-04-26T22:18:21Z

You still have the camelCase variables all over the place, that's not the convention in the rest of the project. Example: https://github.com/opensearch-project/opensearch-build/blob/main/src/test_workflow/perf_test/perf_args.py#L21

Fixed it. Also added the TODO to get rid of hack I put to quote the json input argument.
@dblock

dblock

Needs documentation in .md files for these new tests.

Code: I'm down to nits. Address those and it's ready to merge.

dblock · 2023-05-01T15:20:04Z

src/test_workflow/benchmark_test/benchmark_args.py

+                            help="Do not delete the working temporary directory.")
+        parser.add_argument("--single-node", dest="single_node", action="store_true",
+                            help="Is this a single node cluster")
+        parser.add_argument("--min-distribution", dest="min_distribution", action="store_true", help="Is it the "


Put each argument such as help on its own line and it won't need to be wrapped like this.

dblock · 2023-05-01T15:22:49Z

src/test_workflow/test_jsonargs.py

+            getattr(namespace, self.dest)[key] = value
+
+
+JsonArgs.__test__ = False  # type:ignore


Call this file json_args.py and this won't be needed. It's also the convention.

Makes sense, done!

dblock · 2023-05-01T15:22:59Z

src/test_workflow/test_jsonargs.py

+class JsonArgs(argparse.Action):
+    def __call__(self, parser: Any, namespace: argparse.Namespace, values: Union[str, Sequence[Any], None], option_string: str = None) -> None:
+        setattr(namespace, self.dest, dict())
+        print(f"values are {values}")


Stray printf, remove.

dblock · 2023-05-01T15:26:00Z

src/test_workflow/benchmark_test/benchmark_test_suite.py

+        self.command = (
+            f"{base_command} opensearchproject/opensearch-benchmark:latest execute_test "
+            f"--workload={self.args.workload} --test-mode --pipeline=benchmark-only --target-hosts={endpoint}"
+        )


Looks messy, and uses an unnecessary local base_command variable.

self.command = "docker run" if args.benchmark_config: self.command += " -v ..." self.command += "opensearchproject/opensearch-benchmark:latest execute_test: ...

Thanks, done!

dblock · 2023-05-01T15:26:16Z

src/test_workflow/benchmark_test/benchmark_test_suite.py

+
+        if args.user_tag:
+            user_tag = f"--user-tag=\"{args.user_tag}\""
+            self.command = f"{self.command} {user_tag}"


self.command += ...

rishabh6788 · 2023-05-01T22:52:23Z

Needs documentation in .md files for these new tests.

Code: I'm down to nits. Address those and it's ready to merge.

Added README for benchmarking test. Have a TODO for adding jenkins job details once it is ready. It is still WIP. Will be updating the README as and when i get more clarity on requirements and different use-cases.
Addressed the comments.
@dblock

dblock

Minor stuff for the markdown please.

dblock · 2023-05-02T18:43:51Z

src/test_workflow/README.md

@@ -174,6 +174,17 @@ Internal tools provide dashboards for monitoring cluster behavior during these t
 |Indexing Latency|Consistent during each test iteration|upward trends|
 |Query Latency|Varies based on the query being issued|upward trends|

+### Benchmarking Tests


Sorry about that, updated!

dblock · 2023-05-02T18:44:17Z

src/test_workflow/README.md

@@ -174,6 +174,17 @@ Internal tools provide dashboards for monitoring cluster behavior during these t
 |Indexing Latency|Consistent during each test iteration|upward trends|
 |Query Latency|Varies based on the query being issued|upward trends|

+### Benchmarking Tests
+Runs benchmarking test on a remote opensource OpenSearch cluster. Uses [OpenSearch Benchmark](https://github.com/opensearch-project/OpenSearch-Benchmark) to run benchmark tests.
+At a high-level the benchmarking test workflow uses [opensearch-cluster-cdk](https://github.com/opensearch-project/opensearch-cluster-cdk.git) to first set-up an OpenSearch 


Don't line wrap, https://code.dblock.org/2021/06/07/to-wrap-or-not-to-wrap-in-markdown.html

Ack! fixed!

dblock · 2023-05-02T18:44:44Z

src/test_workflow/README.md

@@ -174,6 +174,17 @@ Internal tools provide dashboards for monitoring cluster behavior during these t
 |Indexing Latency|Consistent during each test iteration|upward trends|
 |Query Latency|Varies based on the query being issued|upward trends|

+### Benchmarking Tests
+Runs benchmarking test on a remote opensource OpenSearch cluster. Uses [OpenSearch Benchmark](https://github.com/opensearch-project/OpenSearch-Benchmark) to run benchmark tests.


test or tests?

you can remove "to run benchmark tests", that's what it does

dblock · 2023-05-02T18:45:13Z

src/test_workflow/README.md

+cluster (single/multi-node) and then executes `opensearch-benchmark` to run benchmark test against that cluster. The performance metric that opensearch-benchmark generates 
+during the run are ingested into another OS cluster for further analysis and dashboarding purpose.
+
+The benchmarking tests will be run nightly and if you have a feature in any released/un-released OS version that you want to benchmark periodically please create an issue


I believe we don't abbreviate OS, spell OpenSearch.

dblock · 2023-05-02T18:45:40Z

src/test_workflow/README.md

@@ -237,6 +248,9 @@ After the performance test completes, it will report back the test results as we

 The development is tracked by meta issue [#126](https://github.com/opensearch-project/opensearch-build/issues/126)

+#### benchmarkTest job


Capitalize Like Other Titles

You can remove this for now since we don't have content.

Removed for now. Will add once we have the job ready to be merged.

Signed-off-by: Rishabh Singh <[email protected]>

rishabh6788 and others added 7 commits March 6, 2023 09:48

Bump snakeyaml to version 2.0

15dd61c

Signed-off-by: Rishabh Singh <[email protected]>

Merge branch 'opensearch-project:main' into main

a652aea

Merge branch 'opensearch-project:main' into main

f9ac46f

Changes to use OSB with opensearch-cluster-cdk

98953df

Signed-off-by: Rishabh Singh <[email protected]>

Incremental updates on benchmarking

1b1e335

Signed-off-by: Rishabh Singh <[email protected]>

Merge branch 'opensearch-project:main' into main

2472bcd

updated OSB docker image

0c35065

Signed-off-by: Rishabh Singh <[email protected]>

rishabh6788 requested review from dblock, peterzhuamazon, bbarani, gaiksaya, zelinh, jordarlu, prudhvigodithi, Divyaasm and tianleh as code owners April 18, 2023 02:05

github-actions bot added the all-star-contributor label Apr 18, 2023

dblock reviewed Apr 18, 2023

View reviewed changes

dblock reviewed Apr 19, 2023

View reviewed changes

rishabh6788 requested a review from dblock April 25, 2023 00:03

dblock requested changes Apr 25, 2023

View reviewed changes

dblock reviewed Apr 25, 2023

View reviewed changes

rishabh6788 requested a review from dblock April 27, 2023 17:49

dblock requested changes May 1, 2023

View reviewed changes

rishabh6788 requested a review from dblock May 2, 2023 06:15

dblock requested changes May 2, 2023

View reviewed changes

Code clean up and formatting

7798f89

Signed-off-by: Rishabh Singh <[email protected]>

rishabh6788 requested a review from dblock May 2, 2023 19:02

dblock approved these changes May 3, 2023

View reviewed changes

dblock merged commit 1c6da01 into opensearch-project:main May 3, 2023

rishabh6788 mentioned this pull request May 10, 2023

Use OSB to run performance benchmarking against OpenSearch opensearch-project/opensearch-benchmark#235

Closed

		getattr(namespace, self.dest)[key] = value


		JsonArgs.__test__ = False # type:ignore

		@@ -237,6 +248,9 @@ After the performance test completes, it will report back the test results as we

		The development is tracked by meta issue [#126](https://github.com/opensearch-project/opensearch-build/issues/126)

		#### benchmarkTest job

Workflow to run performance tests using opensearch-benchmark #3415

Workflow to run performance tests using opensearch-benchmark #3415

Conversation

rishabh6788 commented Apr 18, 2023

Description

Issues Resolved

codecov-commenter commented Apr 18, 2023 • edited Loading

Codecov Report

dblock left a comment • edited Loading

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

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh6788 commented Apr 26, 2023

rishabh6788 commented Apr 26, 2023

dblock 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

rishabh6788 commented May 1, 2023 • edited Loading

dblock 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

codecov-commenter commented Apr 18, 2023 •

edited

Loading

dblock left a comment •

edited

Loading

rishabh6788 commented May 1, 2023 •

edited

Loading