[container_bench] Podman machine benchmarks#904
Open
Honny1 wants to merge 3 commits intoopenshift-psap:mainfrom
Open
[container_bench] Podman machine benchmarks#904Honny1 wants to merge 3 commits intoopenshift-psap:mainfrom
Honny1 wants to merge 3 commits intoopenshift-psap:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
Honny1
force-pushed
the
podman-machine-benchmarks
branch
from
d8293f1 to
1812832
Compare
📝 WalkthroughWalkthroughThis pull request introduces Podman machine lifecycle benchmarking support to the container_bench testing framework. It adds two new benchmark types (machine first-start and subsequent start timing), platform-agnostic Ansible task definitions for Unix/Windows environments, configuration management for machine hardware parameters (CPU, memory, rootful mode), test orchestration logic to route machine benchmarks to a dedicated platform, and visualization components for benchmark comparison reporting. Changes
Sequence DiagramsequenceDiagram
participant TestOrch as Test Orchestrator
participant Toolbox as Container_Bench<br/>(Toolbox Method)
participant AnsibleExec as Ansible Executor
participant PodmanSys as Podman/System
participant Artifacts as Artifacts
TestOrch->>Toolbox: machine_[first_]start_benchmark(exec_props)
activate Toolbox
Toolbox->>AnsibleExec: RunAnsibleRole(machine_[first_]start_benchmark)
deactivate Toolbox
activate AnsibleExec
AnsibleExec->>AnsibleExec: Extract exec_props parameters
AnsibleExec->>AnsibleExec: Build prepare_commands (init, config machine)
AnsibleExec->>AnsibleExec: Build benchmark_command (run podman machine start)
AnsibleExec->>AnsibleExec: Build cleanup_commands (stop, remove machine)
AnsibleExec->>PodmanSys: Execute prepare_commands
activate PodmanSys
PodmanSys->>PodmanSys: podman machine stop/rm (cleanup)
PodmanSys->>PodmanSys: podman machine init benchmark-machine
PodmanSys->>PodmanSys: podman machine set --cpus/--memory/--rootful
deactivate PodmanSys
AnsibleExec->>Artifacts: Create artifacts directory
AnsibleExec->>PodmanSys: Execute benchmark_command (measure start time)
activate PodmanSys
PodmanSys->>PodmanSys: podman machine start (time measurement)
deactivate PodmanSys
AnsibleExec->>PodmanSys: Execute cleanup_commands
activate PodmanSys
PodmanSys->>PodmanSys: podman machine stop/rm
deactivate PodmanSys
deactivate AnsibleExec
TestOrch->>TestOrch: Collect benchmark results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Honny1
changed the title
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Honny1
force-pushed
the
podman-machine-benchmarks
branch
from
1812832 to
96b09c2
Compare
Collaborator
Author
|
/test cont_bench-jump-ci |
|
🟢 Test of 'container_bench test test_ci' succeeded after 04 hours 54 minutes 23 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/container_bench/testing/test_container_bench.py (1)
352-368:⚠️ Potential issue | 🟡 MinorMachine stop() is outside try-finally block - consider error handling.
If
machine.stop()(line 355) raises an exception, thefinallyblock won't execute andmachine.start()won't be called to restore the original state. This could leave the development machine without a running Podman machine.Consider wrapping the entire machine lifecycle in the try-finally:
🛡️ Suggested restructuring for safer lifecycle handling
exit_code = 1 + machine = PodmanMachine() if platform.container_engine == "podman" else None try: + if benchmark.is_machine_benchmark and machine: + machine.stop() - - machine = PodmanMachine() if platform.container_engine == "podman" else None - if benchmark.is_machine_benchmark and machine: - machine.stop() - - exit_code = 1 - try: for _ in range(benchmark.runs):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/container_bench/testing/test_container_bench.py` around lines 352 - 368, The call to machine.stop() occurs before the try/finally so if machine.stop() raises an exception machine.start() in the finally will never run; wrap the PodmanMachine lifecycle around the benchmark run by moving the stop/start inside a try/finally: after creating machine when benchmark.is_machine_benchmark is true call machine.stop() inside the try block (or call stop() immediately but ensure a surrounding try/finally that always calls machine.start() in its finally), keep the loop that calls run.run_toolbox(...) inside that same try, and ensure machine.start() is invoked in the finally so PodmanMachine.start() always restores state even if stop() or run.run_toolbox() throws.
🧹 Nitpick comments (3)
projects/container_bench/testing/config_manager.py (1)
188-191: Normalizemachine.rootfulat config boundary to avoidNonepropagation.Line 191 currently forwards raw config and may pass
Nonedownstream; defaulting toFalsehere keeps behavior aligned with documented defaults and avoids ambiguous Ansible inputs.♻️ Suggested patch
def get_benchmark_machine_config(benchmark_name): @@ return { 'provider': config.project.get_config(f"{benchmark_name}.machine.provider", print=False), 'cpus': config.project.get_config(f"{benchmark_name}.machine.cpus", print=False), 'memory': config.project.get_config(f"{benchmark_name}.machine.memory", print=False), - 'rootful': config.project.get_config(f"{benchmark_name}.machine.rootful", print=False), + 'rootful': config.project.get_config( + f"{benchmark_name}.machine.rootful", default_value=False, print=False + ) or False, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/container_bench/testing/config_manager.py` around lines 188 - 191, The machine dict construction forwards the raw value of config.project.get_config(f"{benchmark_name}.machine.rootful", ...) which can be None; change the code that builds the machine dict (the block using benchmark_name and config.project.get_config) to normalize rootful to a boolean by treating None as False (i.e., coalesce the retrieved value to False before placing it into the dict) so downstream code/Ansible never receives None.projects/container_bench/toolbox/shared_tasks/windows_machine_benchmark_body.yml (1)
28-30: Consider adding error resilience to cleanup commands.The cleanup task may fail if the machine is already stopped or removed. In the main.yml files,
cleanup_commandsincludes2>/dev/null || truefor error suppression in the shell command itself, which handles this at the command level. This is acceptable, but consider addingignore_errors: trueat the task level for additional resilience during teardown.♻️ Optional: Add ignore_errors for cleanup resilience
- name: Execute benchmark cleanup commands ansible.windows.win_shell: "{{ cleanup_commands }}" when: cleanup_commands | length > 0 + ignore_errors: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/container_bench/toolbox/shared_tasks/windows_machine_benchmark_body.yml` around lines 28 - 30, The cleanup task "Execute benchmark cleanup commands" using ansible.windows.win_shell with the variable cleanup_commands should be made resilient to failures by adding ignore_errors: true at the task level; locate the task that uses ansible.windows.win_shell and cleanup_commands and add ignore_errors: true so the playbook continues even if cleanup commands fail (the existing shell-level suppression can remain).projects/container_bench/toolbox/shared_tasks/unix_machine_benchmark_body.yml (1)
29-32: Consider adding error resilience to cleanup commands.Same observation as the Windows version - while the shell commands themselves include
|| truefor error suppression, addingignore_errors: trueat the task level provides additional resilience.♻️ Optional: Add ignore_errors for cleanup resilience
- name: Execute benchmark cleanup commands become: "{{ rootfull | default(false) }}" shell: "{{ cleanup_commands }}" when: cleanup_commands | length > 0 + ignore_errors: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/container_bench/toolbox/shared_tasks/unix_machine_benchmark_body.yml` around lines 29 - 32, The "Execute benchmark cleanup commands" Ansible task currently runs shell: "{{ cleanup_commands }}" with become: "{{ rootfull | default(false) }}" and only guards on when: cleanup_commands | length > 0; add task-level error resilience by setting ignore_errors: true on that task so any nonzero exit status doesn't fail the playbook even if a command slips past the inline "|| true" in cleanup_commands—locate the task by its name "Execute benchmark cleanup commands" and add ignore_errors: true alongside the existing become, shell and when keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/container_bench/testing/config.yaml`:
- Around line 386-389: The CI matrix never includes the
benchmark_machine.rootful axis because the matrix entry
"test.benchmark_machine.rootful" is commented out; uncomment that line and
enable the corresponding value mappings for rootful in the matrix so the test
matrix includes both rootful:true/false cases. Specifically, restore
"test.benchmark_machine.rootful" alongside "test.benchmark_machine.provider",
"test.benchmark_machine.cpus", and "test.benchmark_machine.memory" and re-enable
the value mapping block that defines the rootful options so runs no longer
always use the default rootful:false.
In `@projects/container_bench/toolbox/container_bench.py`:
- Around line 443-445: The docstrings that list exec_props entries (the blocks
containing "machine_cpus", "machine_memory", and "machine_rootful") are missing
the "machine_provider" field; update each of those docstring sections (the ones
at the shown locations and the other occurrences) to include a short line
describing "machine_provider" (e.g., "machine_provider: provider name for the
machine benchmark, populated by the test harness") so generated docs correctly
reflect the expected exec_props shape.
In
`@projects/container_bench/visualizations/benchmark/plotting/comparison_plots.py`:
- Around line 197-204: The performance plot function _create_exec_time_plot
currently coerces missing execution_time_95th_percentile to 0 which causes
synthetic-only comparisons to produce a misleading zero chart; instead, update
_create_exec_time_plot to first filter configurations to those that have a
non-null execution_time_95th_percentile (e.g., using
c.get("execution_time_95th_percentile") is not None), and if that filtered list
is empty return None so the "No performance data available" fallback runs; then
sort the filtered list by the actual execution_time_95th_percentile value
(remove the "or 0" coercion) and proceed.
In
`@projects/container_bench/visualizations/benchmark/plotting/comparison_report.py`:
- Around line 328-339: The code builds a content list starting with html.H4(...)
then tests `if content:` which is always true; change the condition to only
append the section when there is actual data by checking `synthetic_table` or
`synthetic_plot` (e.g., use `if synthetic_table or synthetic_plot:`) before
calling `tables.append(html.Div(content))`; apply the same change to the other
identical blocks that initialize a content list with a header and then later
test `if content:` so they also check the presence of the data variables (like
`synthetic_table`, `synthetic_plot`, `real_table`, `real_plot`, etc.) instead of
the `content` list.
---
Outside diff comments:
In `@projects/container_bench/testing/test_container_bench.py`:
- Around line 352-368: The call to machine.stop() occurs before the try/finally
so if machine.stop() raises an exception machine.start() in the finally will
never run; wrap the PodmanMachine lifecycle around the benchmark run by moving
the stop/start inside a try/finally: after creating machine when
benchmark.is_machine_benchmark is true call machine.stop() inside the try block
(or call stop() immediately but ensure a surrounding try/finally that always
calls machine.start() in its finally), keep the loop that calls
run.run_toolbox(...) inside that same try, and ensure machine.start() is invoked
in the finally so PodmanMachine.start() always restores state even if stop() or
run.run_toolbox() throws.
---
Nitpick comments:
In `@projects/container_bench/testing/config_manager.py`:
- Around line 188-191: The machine dict construction forwards the raw value of
config.project.get_config(f"{benchmark_name}.machine.rootful", ...) which can be
None; change the code that builds the machine dict (the block using
benchmark_name and config.project.get_config) to normalize rootful to a boolean
by treating None as False (i.e., coalesce the retrieved value to False before
placing it into the dict) so downstream code/Ansible never receives None.
In
`@projects/container_bench/toolbox/shared_tasks/unix_machine_benchmark_body.yml`:
- Around line 29-32: The "Execute benchmark cleanup commands" Ansible task
currently runs shell: "{{ cleanup_commands }}" with become: "{{ rootfull |
default(false) }}" and only guards on when: cleanup_commands | length > 0; add
task-level error resilience by setting ignore_errors: true on that task so any
nonzero exit status doesn't fail the playbook even if a command slips past the
inline "|| true" in cleanup_commands—locate the task by its name "Execute
benchmark cleanup commands" and add ignore_errors: true alongside the existing
become, shell and when keys.
In
`@projects/container_bench/toolbox/shared_tasks/windows_machine_benchmark_body.yml`:
- Around line 28-30: The cleanup task "Execute benchmark cleanup commands" using
ansible.windows.win_shell with the variable cleanup_commands should be made
resilient to failures by adding ignore_errors: true at the task level; locate
the task that uses ansible.windows.win_shell and cleanup_commands and add
ignore_errors: true so the playbook continues even if cleanup commands fail (the
existing shell-level suppression can remain).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da2f988c-5000-40e4-8d5e-50ed918c0a34
📒 Files selected for processing (18)
docs/toolbox.generated/Container_Bench.machine_first_start_benchmark.rstdocs/toolbox.generated/Container_Bench.machine_start_benchmark.rstdocs/toolbox.generated/index.rstprojects/container_bench/testing/config.yamlprojects/container_bench/testing/config_manager.pyprojects/container_bench/testing/test_container_bench.pyprojects/container_bench/testing/utils.pyprojects/container_bench/toolbox/container_bench.pyprojects/container_bench/toolbox/container_bench_machine_first_start_benchmark/defaults/main/config.ymlprojects/container_bench/toolbox/container_bench_machine_first_start_benchmark/tasks/main.ymlprojects/container_bench/toolbox/container_bench_machine_start_benchmark/defaults/main/config.ymlprojects/container_bench/toolbox/container_bench_machine_start_benchmark/tasks/main.ymlprojects/container_bench/toolbox/shared_tasks/unix_machine_benchmark_body.ymlprojects/container_bench/toolbox/shared_tasks/windows_machine_benchmark_body.ymlprojects/container_bench/visualizations/benchmark/plotting/__init__.pyprojects/container_bench/visualizations/benchmark/plotting/comparison_plots.pyprojects/container_bench/visualizations/benchmark/plotting/comparison_report.pyprojects/container_bench/visualizations/benchmark/plotting/utils/config.py
Comment on lines
+443
to
+445
| machine_cpus: number of CPUs to allocate to the benchmark machine (optional) | ||
| machine_memory: memory in MB to allocate to the benchmark machine (optional) | ||
| machine_rootful: whether the podman machine runs in rootful mode (default: false) |
There was a problem hiding this comment.
Document machine_provider in both machine benchmark exec_props docs.
machine_provider is populated by the test harness for machine benchmarks, but it is missing from these method docstrings, which causes generated docs to under-specify the expected exec_props shape.
📝 Suggested docstring update
properties of exec_props:
binary_path: path to the podman binary
rootfull: whether to run Ansible tasks as root
additional_args: additional arguments to pass to the podman binary
exec_time_path: path to the exec_time.py script
+ machine_provider: podman machine provider (optional)
machine_cpus: number of CPUs to allocate to the benchmark machine (optional)
machine_memory: memory in MB to allocate to the benchmark machine (optional)
machine_rootful: whether the podman machine runs in rootful mode (default: false)
@@
Args:
exec_props: dict containing execution properties (binary_path, rootfull, additional_args,
- exec_time_path, machine_cpus, machine_memory, machine_rootful)
+ exec_time_path, machine_provider, machine_cpus, machine_memory, machine_rootful)
@@
properties of exec_props:
binary_path: path to the podman binary
rootfull: whether to run Ansible tasks as root
additional_args: additional arguments to pass to the podman binary
exec_time_path: path to the exec_time.py script
+ machine_provider: podman machine provider (optional)
machine_cpus: number of CPUs to allocate to the benchmark machine (optional)
machine_memory: memory in MB to allocate to the benchmark machine (optional)
machine_rootful: whether the podman machine runs in rootful mode (default: false)
@@
Args:
exec_props: dict containing execution properties (binary_path, rootfull, additional_args,
- exec_time_path, machine_cpus, machine_memory, machine_rootful)
+ exec_time_path, machine_provider, machine_cpus, machine_memory, machine_rootful)Also applies to: 452-454, 471-473, 481-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/container_bench/toolbox/container_bench.py` around lines 443 - 445,
The docstrings that list exec_props entries (the blocks containing
"machine_cpus", "machine_memory", and "machine_rootful") are missing the
"machine_provider" field; update each of those docstring sections (the ones at
the shown locations and the other occurrences) to include a short line
describing "machine_provider" (e.g., "machine_provider: provider name for the
machine benchmark, populated by the test harness") so generated docs correctly
reflect the expected exec_props shape.
Comment on lines
+197
to
+204
| def _create_exec_time_plot(configurations): | ||
| if not configurations: | ||
| return None | ||
|
|
||
| sorted_configurations = sorted( | ||
| configurations, | ||
| key=lambda c: c.get("execution_time_95th_percentile", 0) or 0 | ||
| ) |
There was a problem hiding this comment.
Skip the performance chart when P95 data is absent.
Line 203 coerces missing execution_time_95th_percentile to 0, so synthetic-only comparisons render a misleading all-zero performance plot instead of taking the existing "No performance data available" fallback.
📉 Proposed fix
def _create_exec_time_plot(configurations):
if not configurations:
return None
+
+ configurations = [
+ config for config in configurations
+ if config.get("execution_time_95th_percentile") is not None
+ ]
+ if not configurations:
+ return None
sorted_configurations = sorted(
configurations,
key=lambda c: c.get("execution_time_95th_percentile", 0) or 0
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _create_exec_time_plot(configurations): | |
| if not configurations: | |
| return None | |
| sorted_configurations = sorted( | |
| configurations, | |
| key=lambda c: c.get("execution_time_95th_percentile", 0) or 0 | |
| ) | |
| def _create_exec_time_plot(configurations): | |
| if not configurations: | |
| return None | |
| configurations = [ | |
| config for config in configurations | |
| if config.get("execution_time_95th_percentile") is not None | |
| ] | |
| if not configurations: | |
| return None | |
| sorted_configurations = sorted( | |
| configurations, | |
| key=lambda c: c.get("execution_time_95th_percentile", 0) or 0 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@projects/container_bench/visualizations/benchmark/plotting/comparison_plots.py`
around lines 197 - 204, The performance plot function _create_exec_time_plot
currently coerces missing execution_time_95th_percentile to 0 which causes
synthetic-only comparisons to produce a misleading zero chart; instead, update
_create_exec_time_plot to first filter configurations to those that have a
non-null execution_time_95th_percentile (e.g., using
c.get("execution_time_95th_percentile") is not None), and if that filtered list
is empty return None so the "No performance data available" fallback runs; then
sort the filtered list by the actual execution_time_95th_percentile value
(remove the "or 0" coercion) and proceed.
Comment on lines
+328
to
+339
| content = [html.H4("Benchmark Results", style=css.STYLE_H4)] | ||
| if synthetic_table: | ||
| tables.append(html.Div([ | ||
| html.H4("Benchmark Results", style=css.STYLE_H4), | ||
| synthetic_table | ||
| ])) | ||
| content.append(synthetic_table) | ||
| if synthetic_plot: | ||
| content.append(html.Div( | ||
| synthetic_plot, | ||
| style=css.STYLE_PLOTS_GRID, | ||
| className='plots-grid-responsive' | ||
| )) | ||
|
|
||
| if content: | ||
| tables.append(html.Div(content)) |
There was a problem hiding this comment.
Condition if content: is always true.
The content list is initialized with html.H4(...) on line 328 before checking if content: on line 338. This means the condition always evaluates to true. The same pattern appears on lines 348/358.
If the intent is to only append a section when there's actual data (table or plot), the check should verify those elements exist.
🔧 Suggested fix
- content = [html.H4("Benchmark Results", style=css.STYLE_H4)]
if synthetic_table:
- content.append(synthetic_table)
- if synthetic_plot:
- content.append(html.Div(
+ content = [html.H4("Benchmark Results", style=css.STYLE_H4)]
+ content.append(synthetic_table)
+ if synthetic_plot:
+ content.append(html.Div(
synthetic_plot,
style=css.STYLE_PLOTS_GRID,
className='plots-grid-responsive'
))
-
- if content:
tables.append(html.Div(content))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@projects/container_bench/visualizations/benchmark/plotting/comparison_report.py`
around lines 328 - 339, The code builds a content list starting with
html.H4(...) then tests `if content:` which is always true; change the condition
to only append the section when there is actual data by checking
`synthetic_table` or `synthetic_plot` (e.g., use `if synthetic_table or
synthetic_plot:`) before calling `tables.append(html.Div(content))`; apply the
same change to the other identical blocks that initialize a content list with a
header and then later test `if content:` so they also check the presence of the
data variables (like `synthetic_table`, `synthetic_plot`, `real_table`,
`real_plot`, etc.) instead of the `content` list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
machine_first_start_benchmarkandmachine_start_benchmarkfor measuring machine initialization and startup performance.Documentation