Add task to generate input json file to run multibench #825
Conversation
📝 WalkthroughWalkthroughAdds a new Jinja2 JSON template and an Ansible task to render it, renames Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
roles/multibench_run/tasks/main.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.9)
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 49s |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
roles/multibench_run/defaults/main.yml(1 hunks)roles/multibench_run/templates/all-in-one.json.j2(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.9)
🔇 Additional comments (1)
roles/multibench_run/defaults/main.yml (1)
4-4: LGTM!The path update to include the
/dailysubdirectory aligns with the new template-driven workflow.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 57s |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 55s |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
roles/multibench_run/defaults/main.yml(1 hunks)roles/multibench_run/templates/all-in-one.json.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- roles/multibench_run/templates/all-in-one.json.j2
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.9)
🔇 Additional comments (1)
roles/multibench_run/defaults/main.yml (1)
1-1: No references to the old variable name found in the codebase.The search for
nb_iterationsreturns no results across the repository. This indicates that either:
- All references to the old variable name have been properly updated, or
- The variable name is new and was never used elsewhere
Similarly,
multibench_script_diris only defined in this defaults file and not referenced elsewhere, suggesting the path change does not impact other parts of the codebase that depend on hardcoded paths.Without evidence of remaining references to
nb_iterationsor hardcoded paths that need updating, the original review comment's concern cannot be substantiated.
|
from change #825: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
roles/multibench_run/templates/all-in-one.json.j2 (1)
764-766: Consider using themultibench_tagsvariable here.The
defaults/main.ymldefinesmultibench_tags, but this template hardcodes the tag. If you'd like to allow custom tags, you could template this section:"tags": { - "automation": "ansible" +{% for key, value in multibench_tags.items() %} + "{{ key }}": "{{ value }}"{% if not loop.last %},{% endif %} + +{% endfor %} },Or use the
to_jsonfilter for simpler output:"tags": {{ multibench_tags | to_json }},
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
roles/multibench_run/templates/all-in-one.json.j2(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-15T23:08:36.322Z
Learnt from: tonyskapunk
Repo: redhatci/ansible-collection-redhatci-ocp PR: 725
File: roles/deploy_cr/tasks/main.yml:7-10
Timestamp: 2025-07-15T23:08:36.322Z
Learning: In Ansible, the `default(omit)` filter only works when a variable is undefined, not when it's set to the string "omit". For the deploy_cr role in ansible-collection-redhatci-ocp, cluster-scoped resources should not define the dc_namespace variable at all, rather than setting it to the string "omit".
Applied to files:
roles/multibench_run/templates/all-in-one.json.j2
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
🔇 Additional comments (4)
roles/multibench_run/templates/all-in-one.json.j2 (4)
1-739: Benchmark configurations look solid.The iperf and uperf benchmark definitions are correctly structured. The repetitive per-ID param blocks are verbose but clear. If this template grows, you might consider Jinja2 loops to reduce duplication, but for now it's perfectly acceptable.
741-758: Endpoints configuration uses template variables correctly.The
ocp_hostandmultibench_script_dirplaceholders are well placed. Just ensure the referencednodeSelector-worker*.jsonfiles exist in themultibench_script_diror are created as part of the setup.
760-763: Nice fix onnum-samples.Using
{{ multibench_sample | default(3) }}aligns with the pattern inconfig.ini.j2and provides a sensible fallback.
767-782: Tool parameters look good.The sysstat and procstat configurations are fine. Minor nit: the
paramsarray items (lines 771-776) have slightly different indentation than the rest of the file, but it's purely cosmetic.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 56s |
|
from change #825: |
ramperher
left a comment
ramperher
left a comment
There was a problem hiding this comment.
LGTM. Is this JSON file directly taken by run.sh script? Don't you need to pass it as argument? I assume so.
If that's correct, I'd propose to update the README, at least this paragraph:
If you want to customize the scenarios, you can edit the run.sh file which defines the previous layout of test. You can edit the resources requested by the pods (clients or servers), in order to have a Guaranteed QoS. You can change the variable multibench_script to keep several scenarios ready and easily switch from one to another.
Because, now, the source to define the scenario would not be run.sh, but all-in-one.json.
Yes, this json file will be passed as argument to run-daily.sh script.Please see below [root@multibench-vm daily]# tail -2 run-daily.sh
crucible run $SCRIPT_DIR/all-in-one.json
[root@multibench-vm daily]# pwd
/root/crucible-examples/multibench/openshift/example-A/daily
[root@multibench-vm daily]# |
|
from change #825: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
roles/multibench_run/README.md (3)
31-33: Fix code fence language identifier.The example command is bash/shell, not YAML. Update the code fence to match the content type.
-```yaml +```bash /root/regulus/bin/reg-c2j-config.py --name iperf,uperf ... -``` +```
24-29: Clarify the workflow scope for run.sh vs. all-in-one.json.The section transitions from describing run.sh customization to the daily workflow with all-in-one.json, but doesn't clearly delineate which workflow each approach applies to. This could confuse readers about which path to take.
Consider restructuring to explicitly state: (1) run.sh is for the default/example-A workflow, and (2) all-in-one.json is the new approach for daily jobs. This aligns better with the PR's goal of documenting the JSON-based workflow as the primary input source.
25-25: Minor stylistic improvement."In order to" can be more concise. Consider rewording to simply "to":
- You can edit the resources requested by the pods (clients or servers), in order to have a Guaranteed QoS. + You can edit the resources requested by the pods (clients or servers), to have a Guaranteed QoS.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
roles/multibench_run/README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
roles/multibench_run/README.md
[style] ~25-~25: Consider a more concise word here.
Context: ...ested by the pods (clients or servers), in order to have a Guaranteed QoS. You can change t...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
rajchiluveru
force-pushed
the
input_json
branch
from
3780dca to
6e19ffd
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 52s |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
roles/multibench_run/README.md (1)
16-16: Update README table to match the new defaultmultibench_script_dir.The documentation on line 16 still shows the old path without the
/dailysubdirectory, but the default indefaults/main.ymlhas been updated to/root/crucible-examples/multibench/openshift/example-A/daily. Update the README table to reflect this change.
♻️ Duplicate comments (1)
roles/multibench_run/defaults/main.yml (1)
4-4: Path mismatch between setup and runtime confirmed—must be corrected.The setup playbook clones the repository to
/root/crucible-example(singular) and manipulates paths like/root/crucible-example/multibench/openshift/example-A/(lines 55, 62, 71, 85 ofplaybooks/multibench_setup_host.yml). However, the default inroles/multibench_run/defaults/main.ymlline 4 specifies/root/crucible-examples/multibench/openshift/example-A/daily(plural +/daily).Additionally, the README documents the default path as
/root/crucible-examples/multibench/openshift/example-A(plural, without/daily), reserving the/dailysubdirectory specifically for daily jobs. The current default incorrectly commits to the daily-only path for all runs.This will cause runtime failures when the role attempts to write config files to a non-existent path. Either:
- Update the setup playbook to clone to the plural path
/root/crucible-examples, or- Revert the default to the non-daily path and make
/dailyconfigurable for daily-specific jobs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
roles/multibench_run/README.md(2 hunks)roles/multibench_run/defaults/main.yml(1 hunks)roles/multibench_run/tasks/main.yml(1 hunks)roles/multibench_run/templates/all-in-one.json.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- roles/multibench_run/templates/all-in-one.json.j2
- roles/multibench_run/tasks/main.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-24T21:40:17.028Z
Learnt from: tonyskapunk
Repo: redhatci/ansible-collection-redhatci-ocp PR: 715
File: roles/copy_and_render/tasks/main.yml:18-24
Timestamp: 2025-06-24T21:40:17.028Z
Learning: The copy_and_render role in the ansible-collection-redhatci-ocp repository is designed to work only with relative paths for car_source_dir, not absolute paths. This is an intentional design constraint.
Applied to files:
roles/multibench_run/defaults/main.yml
🪛 LanguageTool
roles/multibench_run/README.md
[style] ~25-~25: Consider a more concise word here.
Context: ...ested by the pods (clients or servers), in order to have a Guaranteed QoS. You can change t...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Ansible-lint Check
🔇 Additional comments (1)
roles/multibench_run/defaults/main.yml (1)
1-1: Variable rename is consistent. ✓The change from
nb_iterationstonum_samplesaligns with the template and task updates.
Added task to generate input JSON file for multibench run.
SUMMARY
Crucible command accepts single input json file, hence updating role to generate the input json file with required benchmarks and other key:value pair data.
ISSUE TYPE
Enhanced Feature