add automatic testing and aggregation to OSB

[OVI3D0]

Description

This change enhances OpenSearch Benchmark by allowing users to run multiple iterations of benchmark tests and automatically aggregate the results, all within a single command. New flags have been added to the existing execute command to support this functionality. Users can now specify the number of test iterations with --test-iterations, control result aggregation with --aggregate, set a sleep timer between iterations with --sleep-timer, and choose to cancel on error with --cancel-on-error. For example, users can run:

opensearch-benchmark execute --test-iterations=2 --aggregate=true --workload=geonames --target-hosts=127.0.0.1:9200

This command will run the geonames workload twice and aggregate the results. All existing arguments and flags for test execution remain compatible with these new options, allowing users to customize their benchmark runs as needed. For instance:

opensearch-benchmark execute --test-iterations=2 --aggregate=true --workload=geonames --target-hosts=127.0.0.1:9200 --test-mode --kill-running-processes

This enhancement provides a more streamlined way to conduct multiple test runs and obtain aggregated results, improving the benchmarking workflow in OpenSearch Benchmark.

Issues Resolved

#631

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.

[IanHoang]

Some considerations for running multiple tests

• When users run multiple tests, we should ensure that they automatically have --kill-runninng-processes by default. If it isn't present in the parsed args, we need to add it by default, or else, we'll run into a wall of errors for every test after the first successful test because existing processes might be still alive.
• I tested what happens when users cancel (ctrl + c) the test and they'll have to cancel the test for each iteration (e.g. if a user has 100 test iterations, they'll be spamming ctrl + c for some time 😆 ). Alternatively, they can open a separate window and kill all running processes. We should find a way to make it easier to cancel all tests with a single invocation of ctrl + c if possible
• When users run into errors across all tests, we should reduce the help text and errors. If we can't avoid that, we should include an additional parameter called "--cancel-upon-error" or something like that
• There are additional parameters that I think would be useful for running multiple tests:
  • --time-between-tests / --sleep-time (you can improve the name) = this would allow users to specify how much time there should be between tests. It's a nice-to-have as I've seen some users incorporate sleeps between OSB runs because they want to change something on the host's end between tests.
  • --disable-aggregate = by default we should have this set to False. But some users might not care about aggregating the final results. We should give them the option to disable it.

Some considerations for aggregating results after all tests

• Can we add the relative standard deviation (RSD) in aggregate feature? This would be useful so that users can see the spread of their data and how much each test deviates from one another
• Maybe in user tag it would be include to runs and their ids in the user tags by default? That way users have a way to track down which test execution ids relate to the aggregated one once the console is wiped.

[IanHoang]

Could auto-aggregate be renamed to execute-tests and auto_aggregate_parser be renamed to multiple_test_executions_parser? Those names would be more suitable since the main intention behind this subcommand is to allow users to run multiple tests.

But before renaming them, please consider the following:
If all the parameters between the test_execution_parser and auto_aggregate_parser are the same except --test-iterations, what are your thoughts on omitting a separate subcommand and just incorporating --test-iterations into test_execution_parser and have the multiple tests logic performed in elif subcommand == "execute-test"?

There doesn't seem to be a large enough difference between running a single test and running multiple tests that justifies creating another subcommand. But let me know if there's another reason why a separate subcommand might be better.

[OVI3D0]

Discussed offline and added in the revision 👍

[IanHoang]

Is semaphore really needed? Wondering if there might be some conflicts with preexisting threading / Thespianpy within OSB. Might be overkilil. I think a simple for-loop should suffice?

[IanHoang]

We should avoid using dispatch_sub_command and manipulating the args in line 880.

[IanHoang]

Semaphore might be impacting the performance of each run. I ran a command that sets clients and target throughput to 10. This should ensure that final results have throughput of 10 ops/s. When I ran the command with these auto-aggregate, I see that target throughput is not being achieved across all runs and the aggregated results.

Command: $ opensearch-benchmark auto-aggregate --test-iterations=3 --workload=geonames --target-hosts=XXXXXXX --client-options=basic_auth_user:'XXXXX',basic_auth_password:'XXXXXXX' --include-tasks="term" --test-mode --kill-running-processes --workload-params="search_clients:10,target_throughput:10"

# Each test has the following results
|                                                  Segment count |        |         374 |        |
|                                                 Min Throughput |   term |       33.13 |  ops/s |
|                                                Mean Throughput |   term |       35.22 |  ops/s |
|                                              Median Throughput |   term |       35.69 |  ops/s |
|                                                 Max Throughput |   term |       36.39 |  ops/s |

However, when I run the command with execute-test, it reaches the target throughput.

Command: $ opensearch-benchmark execute-test --workload=geonames --target-hosts=XXXXXXX --client-options=basic_auth_user:'XXXXXXX',basic_auth_password:'XXXXXXX--include-tasks="term" --kill-running-processes --workload-params="search_clients:10,target_throughput:10"

|                                                  Segment count |        |         374 |        |
|                                                 Min Throughput |   term |        9.99 |  ops/s |
|                                                Mean Throughput |   term |          10 |  ops/s |
|                                              Median Throughput |   term |          10 |  ops/s |
|                                                 Max Throughput |   term |          10 |  ops/s |

Both commands were run on the same machine. We should reconsider using semaphore and go for a simpler approach. We can discuss this offline.

[OVI3D0]

Is semaphore really needed? Wondering if there might be some conflicts with preexisting threading / Thespianpy within OSB. Might be overkilil. I think a simple for-loop should suffice?

I tried a simple for loop but i was having issues with already running processes 😕 let's discuss more offline like you mentioned

[OVI3D0]

Discussed more offline and used an alternate approach. I tried a few test runs on the same machine with and without the changes, and the results were very similar. Let me know if you find any differences

[IanHoang]

@OVI3D0 Overall, great work. This feature has been long overdue in OSB. Let's sync offline if you have any questions.

[OVI3D0]

• I tested what happens when users cancel (ctrl + c) the test and they'll have to cancel the test for each iteration (e.g. if a user has 100 test iterations, they'll be spamming ctrl + c for some time 😆 ). Alternatively, they can open a separate window and kill all running processes. We should find a way to make it easier to cancel all tests with a single invocation of ctrl + c if possible

This is fixed!

• When users run into errors across all tests, we should reduce the help text and errors. If we can't avoid that, we should include an additional parameter called "--cancel-upon-error" or something like that

Good idea! added this.

• --time-between-tests / --sleep-time (you can improve the name) = this would allow users to specify how much time there should be between tests. It's a nice-to-have as I've seen some users incorporate sleeps between OSB runs because they want to change something on the host's end between tests.

Added this as well!

• --disable-aggregate = by default we should have this set to False. But some users might not care about aggregating the final results. We should give them the option to disable it.

I instead added --aggregate and defaulted it to True. But if you think it makes more sense the other way around then I can fix this

Some considerations for aggregating results after all tests

• Can we add the relative standard deviation (RSD) in aggregate feature? This would be useful so that users can see the spread of their data and how much each test deviates from one another

I think this would be a great idea for a future PR

• Maybe in user tag it would be include to runs and their ids in the user tags by default? That way users have a way to track down which test execution ids relate to the aggregated one once the console is wiped.

Yeah great idea, I added individual test ID's to the user tags of each aggregated test result.

[IanHoang]

Will we ever reach this point since argparse by default sets the value to 1 if --test-iterations is not provided? If not, let's remove else statement

[OVI3D0]

True, I removed this. Although users can potentially enter invalid values

[IanHoang]

Oops, you're right. Let's add it back

[IanHoang]

If this exception block gets reached, test_exeswill still include the failed test_execution_id since it was appended in line 990 and we'd be including the failed test execution id in the aggregation, which might result in a failure when OSB attempts to aggregate the successful results.

Maybe move line 990 to be after line 992, assuming execute_test succeeds, or we can pop from the list in the exception block.

On a side note, based on the way execute_test() function is constructed, I don't think we reach this exception block and aggregate the successful tests in cases where the user cancels the test. We'll need to look into this route but can keep it for now.

[OVI3D0]

Nice catch, I moved like 990 so it only adds the test ID after the execute_test is called.

[IanHoang]

Alternatively, we could assign the user_tag to a k,v pair something like

 test_execution.user_tags = {
    "aggregation-of-runs": list(self.test_executions.keys())
}

[IanHoang]

Would help users who are trying to filtering for specific aggregated files in the MDS

[OVI3D0]

I didn't consider this, I fixed it to look like your suggestion above!

[IanHoang]

No need to explicitly set default=False when using store_true or store_false actions in argparse. The default value is automatically set to False for store_true and True for store_false.

[OVI3D0]

Haha oops. Fixed now :)

[IanHoang]

LGTM