introduce common operations directory for OSB workloads #514

OVI3D0 · 2024-11-20T20:05:36Z

Description

This change consolidates all of the common operations in OSB workloads into one shared directory. Now, rather than repeating these operations multiple times in each workload, they can simply all be pulled in with one line:
{{ benchmark.collect(parts="../../shared_operations/common_workload_setup.json") }},

Issues Resolved

#489

Testing

New functionality includes testing

Tested by importing the common ops file into other workloads and executing tests

Backport to Branches:

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 · 2024-11-22T19:23:42Z

Had some discussions offline with @OVI3D0. Michael proposed that instead of creating a single shared_operations file that contains the common setup steps in each workload, we should go forward with a modular approach. However, although most workloads have the same chronological steps, some workloads are missing some steps.

Adding comments here for visibility:

do you think it’s maybe worth making these common operations at the beginning of each workload all exactly the same?
I went through all the original workloads (aside from semantic search workload and vectorsearch workload) and most have the same structure:

delete-index
create-index
check-cluster-health
index-append
refresh-after-index
force-merge
refresh-after-force-merge
wait-until-merge-finishes

Some workloads like eventdata or so are missing one of these steps (like a refresh after force merge or refresh after indexing) but let's go with your idea of streamlining them to be the same. Refreshes should not impact or change the behavior of the workload. I think one of the workloads has like a put-settings operation before it deletes the index. You're probably aware but just be sure to keep that operation.

To add to your idea, what do you think of doing this?

Have individual files for each shared operation. One for delete_index.json, one for create_index.json, etc.
Have one file that combines all of these with the benchmark.collect. This file will exist in the shared_operations folder and can be called common_workload_setup.json or something else that you think is appropriate. This file will be inserted into the test_procedures of the official workloads. That way, we don't need to write the following in each workload:

{{ benchmark.collect(parts="../../shared_operations/delete_index.json")}}
{{ benchmark.collect(parts="../../shared_operations/create_index.json")}}
{{ benchmark.collect(parts="../../shared_operations/check_cluster_health.json")}}

# Instead each workload will have this in test procedures at the beginning
{{ benchmark.collect(parts="../../shared_operations/common_workload_setup.json")}}

When creating shared operation files, ensure that operations are grouped together by likeness. For example, for index-append file, let's include index-append and refresh-after-index. Same for force merge, refresh after force merge, wait until merge finishes https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/main/nyc_taxis/test_procedures/default.json#L41-L54

IanHoang · 2024-12-02T22:46:52Z

common_operations/force_merge.json

+{
+    "operation": {
+        "operation-type": "force-merge",
+        "request-timeout": 7200


Let's also include these params https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/main/big5/test_procedures/common/big5-schedule.json#L35-L37

@OVI3D0 Could you add these params to allow users to manage segments?

Sorry, didn't see this. Added now👍

OVI3D0 · 2024-12-04T21:30:38Z

After some more discussions offline with @IanHoang, we came up with a new configuration for the common operations.

create-index typically takes an index_settings param, with different default settings depending on the test procedure.

with this latest revision, the default index_settings value has been renamed to a new default_index_settings value; and after a change to the create_index operation, if no index_settings value is passed, OSB should fall back to the default_index_settings value. After testing this and running a curl for my cluster settings, I can see the default settings are still being passed through.

This will allow us to reduce these common operations down to something like this:

        {% with default_index_settings={
          "index.sort.field": ["country_code.raw", "admin1_code.raw"],
          "index.sort.order": ["asc", "asc"]
        } %}
        {{ benchmark.collect(parts="../../common_operations/common_workload_setup.json") }}
        {% endwith %}

In the case of the geonames workload, this will reduce the file size by half!

IanHoang · 2024-12-06T17:16:11Z

common_operations/create_index.json

+{
+    "operation": {
+        "operation-type": "create-index",
+        "settings": {{index_settings | default(default_index_settings | default({})) | tojson}}


Just curious, is default({})) necessary if the workload has nothing specified?

{% with default_index_settings={} %}

I found that when I removed that final default({}) I was getting jinja2 templating errors, even if I specified default_settings={} in the geonames workload

It's necessary because when default_index_settings is not specified, the {} is needed for the JSON to be valid.

IanHoang

Left two comments but the rest looks good to me

Signed-off-by: Michael Oviedo <[email protected]>

IanHoang

LGTM

gkamat

This change will really improve the workloads, thanks for working on it @OVI3D0. Will this model be applied to all the workloads in a separate check-in? I notice that only geonames is enhanced here.

gkamat · 2024-12-11T17:30:17Z

common_operations/create_index.json

+{
+    "operation": {
+        "operation-type": "create-index",
+        "settings": {{index_settings | default(default_index_settings | default({})) | tojson}}


It's necessary because when default_index_settings is not specified, the {} is needed for the JSON to be valid.

common_operations/common_workload_setup.json

gkamat · 2024-12-11T17:33:36Z

common_operations/force_merge.json

+{
+    "operation": {
+        "operation-type": "force-merge",
+        "request-timeout": 7200{%- if max_num_segments is defined %},


Do you feel request_timeout should be parameterized here, so someone who wants to override it can do it? It's unlikely, but you never know.

I'm of the opinion that there's no harm in parameterizing most of the workload values. I added a param for request-timeout in the latest revision. Let me know what you think!

gkamat · 2024-12-11T17:34:52Z

common_operations/index_append.json

@@ -0,0 +1,10 @@
+{
+    "operation": "index-append",
+    "warmup-time-period": 120,


Similar suggestion as the force merge request timeout.. If you do add parameters, the README's will need to be updated.

Similar to the request-timeout comment, I added a param for this as well.

OVI3D0 · 2024-12-11T18:08:07Z

This change will really improve the workloads, thanks for working on it @OVI3D0. Will this model be applied to all the workloads in a separate check-in? I notice that only geonames is enhanced here.

Yeah I will change the other workloads in separate PR's, I wanted to include geonames here as an example of the changes I plan to apply.

Signed-off-by: Michael Oviedo <[email protected]>

gkamat · 2024-12-12T03:28:22Z

common_operations/force_merge.json

+{
+    "operation": {
+        "operation-type": "force-merge",
+        "request-timeout": {{ request_timeout | default(60) | tojson }}{%- if max_num_segments is defined %},


It might be better to make this explicit, since it applies only to the force-merge operation. So calling it something like force_merge_request_timeout will make it clear it does not apply to all operations in general.

I'd do this in the next set of changes, just so this PR can go in as is.

* restructure common operations folder based on discussion with ian Signed-off-by: Michael Oviedo <[email protected]> * refactor common ops changes based on discussion with ian Signed-off-by: Michael Oviedo <[email protected]> * rework common ops configuration Signed-off-by: Michael Oviedo <[email protected]> * add missing params and update geonames workload Signed-off-by: Michael Oviedo <[email protected]> * parameterize request-timeout and warmup-time-period Signed-off-by: Michael Oviedo <[email protected]> --------- Signed-off-by: Michael Oviedo <[email protected]> (cherry picked from commit 5119393) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* restructure common operations folder based on discussion with ian * refactor common ops changes based on discussion with ian * rework common ops configuration * add missing params and update geonames workload * parameterize request-timeout and warmup-time-period --------- (cherry picked from commit 5119393) Signed-off-by: Michael Oviedo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…roject#514) * restructure common operations folder based on discussion with ian Signed-off-by: Michael Oviedo <[email protected]> * refactor common ops changes based on discussion with ian Signed-off-by: Michael Oviedo <[email protected]> * rework common ops configuration Signed-off-by: Michael Oviedo <[email protected]> * add missing params and update geonames workload Signed-off-by: Michael Oviedo <[email protected]> * parameterize request-timeout and warmup-time-period Signed-off-by: Michael Oviedo <[email protected]> --------- Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 requested review from IanHoang, gkamat, beaioun, cgchinmay, rishabh6788 and VijayanB as code owners November 20, 2024 20:05

OVI3D0 force-pushed the main branch 2 times, most recently from 80c8492 to f7d5cfa Compare November 22, 2024 21:53

IanHoang reviewed Dec 2, 2024

View reviewed changes

OVI3D0 requested a review from IanHoang December 4, 2024 21:45

IanHoang reviewed Dec 6, 2024

View reviewed changes

IanHoang requested changes Dec 6, 2024

View reviewed changes

OVI3D0 added 3 commits December 9, 2024 22:50

restructure common operations folder based on discussion with ian

7fe6c56

Signed-off-by: Michael Oviedo <[email protected]>

refactor common ops changes based on discussion with ian

e735d34

Signed-off-by: Michael Oviedo <[email protected]>

rework common ops configuration

e6ed9ec

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 force-pushed the main branch from cc6b254 to e6ed9ec Compare December 9, 2024 22:50

add missing params and update geonames workload

81b43e1

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 requested a review from IanHoang December 9, 2024 23:00

IanHoang approved these changes Dec 10, 2024

View reviewed changes

gkamat requested changes Dec 11, 2024

View reviewed changes

parameterize request-timeout and warmup-time-period

aefcaab

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 requested a review from gkamat December 11, 2024 22:30

gkamat approved these changes Dec 12, 2024

View reviewed changes

OVI3D0 added backport 2 Backport to the "2" branch backport 1 backport 3 Backport to the "3" branch backport 7 Backport to the "7" branch labels Dec 12, 2024

OVI3D0 merged commit 5119393 into opensearch-project:main Dec 12, 2024
6 checks passed

opensearch-trigger-bot bot mentioned this pull request Dec 12, 2024

[Backport 2] introduce common operations directory for OSB workloads #526

Merged

opensearch-trigger-bot bot mentioned this pull request Dec 12, 2024

[Backport 1] introduce common operations directory for OSB workloads #527

Merged

opensearch-trigger-bot bot mentioned this pull request Dec 12, 2024

[Backport 3] introduce common operations directory for OSB workloads #528

Merged

opensearch-trigger-bot bot mentioned this pull request Dec 12, 2024

[Backport 7] introduce common operations directory for OSB workloads #529

Merged

AndreKurait mentioned this pull request Dec 13, 2024

Fix OSB on specific working workload commit opensearch-project/opensearch-migrations#1202

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce common operations directory for OSB workloads #514

introduce common operations directory for OSB workloads #514

OVI3D0 commented Nov 20, 2024 •

edited

Loading

IanHoang commented Nov 22, 2024

IanHoang Dec 2, 2024

IanHoang Dec 6, 2024

OVI3D0 Dec 9, 2024

OVI3D0 commented Dec 4, 2024

IanHoang Dec 6, 2024

OVI3D0 Dec 9, 2024

gkamat Dec 11, 2024

IanHoang left a comment

IanHoang left a comment

gkamat left a comment

gkamat Dec 11, 2024

gkamat Dec 11, 2024

OVI3D0 Dec 11, 2024

gkamat Dec 11, 2024

OVI3D0 Dec 11, 2024

OVI3D0 commented Dec 11, 2024

gkamat Dec 12, 2024

introduce common operations directory for OSB workloads #514

introduce common operations directory for OSB workloads #514

Conversation

OVI3D0 commented Nov 20, 2024 • edited Loading

Description

Issues Resolved

Testing

Backport to Branches:

IanHoang commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OVI3D0 commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanHoang left a comment

Choose a reason for hiding this comment

IanHoang left a comment

Choose a reason for hiding this comment

gkamat 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

OVI3D0 commented Dec 11, 2024

Choose a reason for hiding this comment

OVI3D0 commented Nov 20, 2024 •

edited

Loading