Add new optional parameters for OS cluster and benchmark #3481

rishabh6788 · 2023-05-05T22:59:04Z

Description

This PR adds 3 new parameters (2 for opensearch-cluster-cdk and 1 for opensearch-benchmark) that are required to roll out P0 benchmark-test workflow. The new parameters added are:

use_50_percent_heap: Optional parameter to enable 50% heap usage on OpenSearch cluster.
isInteral: Immutable isInternal parameter passed from config file. The will create internal network load balancer that can be accessed in a VPC peered environment between jenkins and cluster accounts.
workload_params: Optional parameter to pass additional parameters to opensearch-benchmark.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

src/test_workflow/benchmark_test/benchmark_args.py

peterzhuamazon · 2023-05-05T23:32:40Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

@@ -141,7 +141,9 @@ def setup_cdk_params(self, config: dict) -> dict:
            "mlNodeCount": self.args.ml_node_count,
            "dataNodeStorage": self.args.data_node_storage,
            "mlNodeStorage": self.args.ml_node_storage,
-            "jvmSysProps": self.args.jvm_sys_props
+            "jvmSysProps": self.args.jvm_sys_props,
+            "use50PercentHeap": str(self.args.use_50_percent_heap).lower(),


This does not seem to be used anywhere?

Just realize this is just a setting for part of the clustercdk then I have no issues.

gaiksaya · 2023-05-05T23:36:46Z

LGTM! Needs passing python tests.

peterzhuamazon

Thanks @rishabh6788 please fix the tests when you can. Approved. Thanks.

codecov-commenter · 2023-05-06T00:24:11Z

Codecov Report

Merging #3481 (07883be) into main (a140cfd) will decrease coverage by 0.66%.
The diff coverage is 100.00%.

📣 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    #3481      +/-   ##
==========================================
- Coverage   91.81%   91.16%   -0.66%     
==========================================
  Files         181      181              
  Lines        5268     5340      +72     
==========================================
+ Hits         4837     4868      +31     
- Misses        431      472      +41

Impacted Files	Coverage Δ
..._workflow/benchmark_test/benchmark_test_cluster.py	`84.88% <ø> (ø)`
src/test_workflow/benchmark_test/benchmark_args.py	`100.00% <100.00%> (ø)`
...st_workflow/benchmark_test/benchmark_test_suite.py	`100.00% <100.00%> (ø)`

... and 7 files with indirect coverage changes

tests/tests_test_workflow/test_benchmark_workflow/benchmark_test/test_benchmark_test_suite.py

gkamat · 2023-05-06T21:46:07Z

src/test_workflow/benchmark_test/benchmark_args.py

+        parser.add_argument("--workload", dest="workload", required=True,
+                            help="workload type for the OpenSearch benchmarking")


Technically, this is not the workload "type", perhaps you can rephrase this as something like:
"name of the workload that OpenSearch Benchmark should run"

Ack! Updated.

gkamat · 2023-05-06T21:50:39Z

src/test_workflow/benchmark_test/benchmark_args.py

+        parser.add_argument("--workload-params", dest="workload_params",
+                            help="With this parameter you can inject variables into workloads. Parameters differs "
+                                 "for each workload type. e.g., --workload-params \"number_of_replicas:1,number_of_shards:5\"")


nit: Parameters differ

You might change this to: "Parameters are listed in the README file provided with each workload."

Also, since you are adding a new parameter, please consider adding --client-options as well. If you do that, you will have to merge fragments you are probably already specifying like basic_auth_user, etc. But making this option configurable will allow the user to override items like the client-timeout value.

Thanks! will take this up in the next iteration.

gkamat · 2023-05-06T21:51:57Z

src/test_workflow/benchmark_test/benchmark_args.py

@@ -102,7 +106,9 @@ def __init__(self) -> None:
        self.data_node_storage = args.data_node_storage if args.data_node_storage else None
        self.ml_node_storage = args.ml_node_storage if args.ml_node_storage else None
        self.workload = args.workload
+        self.workload_params = args.workload_params if args.workload_params else None


Will this work here?

self.workload_params = args.workload_params

If the params are passed then it will be considered else ignored.

gkamat · 2023-05-06T22:02:49Z

src/test_workflow/benchmark_test/benchmark_test_cluster.py

-            "jvmSysProps": self.args.jvm_sys_props
+            "jvmSysProps": self.args.jvm_sys_props,
+            "use50PercentHeap": str(self.args.use_50_percent_heap).lower(),
+            "isInternal": config["Constants"]["isInternal"]


Perhaps add a comment indicating what this flag does?

Since these go into the cluster-cdk command, the details of all the flags are present in the repo README if someone wants more details.

IanHoang · 2023-05-08T13:40:05Z

src/test_workflow/benchmark_test/benchmark_args.py

-        parser.add_argument("--workload", dest="workload", help="workload type for the OpenSearch benchmarking",
-                            required=True)
+        parser.add_argument("--workload", dest="workload", required=True,
+                            help="workload type for the OpenSearch benchmarking")
        parser.add_argument("--benchmark-config", dest="benchmark_config",
                            help="absolute filepath to custom opensearch-benchmark.ini config")


The help section says opensearch-benchmark.ini but it should be benchmark.ini.

hoangia@3c22fbd0d988 Desktop % ls -lrt ~/.benchmark drwxr-xr-x 6 hoangia staff 192 Jun 10 2022 benchmarks drwxr-xr-x 3 hoangia staff 96 Aug 9 2022 logs -rw-r--r-- 1 hoangia staff 654 Mar 14 11:10 benchmark-in-memory-it.ini -rw-r--r-- 1 hoangia staff 768 Mar 14 11:10 benchmark-os-it.ini -rw-r--r-- 1 hoangia staff 757 Mar 27 11:54 benchmark.ini

Thanks, will correct this.

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

rishabh6788 marked this pull request as ready for review May 5, 2023 22:59

github-actions bot added the all-star-contributor label May 5, 2023

rishabh6788 requested review from dblock, peterzhuamazon, bbarani, gaiksaya, zelinh, jordarlu, prudhvigodithi, Divyaasm and tianleh as code owners May 5, 2023 22:59

peterzhuamazon reviewed May 5, 2023

View reviewed changes

peterzhuamazon approved these changes May 5, 2023

View reviewed changes

gaiksaya reviewed May 6, 2023

View reviewed changes

tests/tests_test_workflow/test_benchmark_workflow/benchmark_test/test_benchmark_test_suite.py Show resolved Hide resolved

gaiksaya approved these changes May 6, 2023

View reviewed changes

gkamat approved these changes May 6, 2023

View reviewed changes

IanHoang reviewed May 8, 2023

View reviewed changes

IanHoang approved these changes May 8, 2023

View reviewed changes

Add new optional parameters for OS cluster and benchmark

07883be

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

rishabh6788 merged commit 4d3f191 into opensearch-project:main May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new optional parameters for OS cluster and benchmark #3481

Add new optional parameters for OS cluster and benchmark #3481

rishabh6788 commented May 5, 2023

peterzhuamazon May 5, 2023

peterzhuamazon May 5, 2023

gaiksaya commented May 5, 2023

peterzhuamazon left a comment

codecov-commenter commented May 6, 2023 •

edited

Loading

gkamat May 6, 2023

rishabh6788 May 8, 2023

gkamat May 6, 2023

rishabh6788 May 8, 2023

gkamat May 6, 2023

rishabh6788 May 8, 2023

gkamat May 6, 2023

rishabh6788 May 8, 2023

IanHoang May 8, 2023

rishabh6788 May 8, 2023

		parser.add_argument("--workload", dest="workload", required=True,
		help="workload type for the OpenSearch benchmarking")

Add new optional parameters for OS cluster and benchmark #3481

Add new optional parameters for OS cluster and benchmark #3481

Conversation

rishabh6788 commented May 5, 2023

Description

Issues Resolved

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaiksaya commented May 5, 2023

peterzhuamazon left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 6, 2023 • edited Loading

Codecov Report

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 May 6, 2023 •

edited

Loading