[container_benchmark] MacOS and Linux Experiments #870
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded@Honny1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughConfiguration file updated with inline documentation for Docker Desktop remote-host behavior and a new field ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
|
Skipping CI for Draft Pull Request. |
Honny1
force-pushed
the
run-others
branch
3 times, most recently
from
18ad5c0 to
31fd1dd
Compare
|
/test cont_bench-jump-ci |
|
/test cont_bench-jump-ci |
|
🟢 Test of 'container_bench test test_ci' succeeded after 05 hours 42 minutes 07 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test cont_bench-jump-ci |
|
🟢 Test of 'container_bench test test_ci' succeeded after 04 hours 30 minutes 49 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test cont_bench-jump-ci |
|
🔴 Test of 'container_bench test prepare_ci' failed after 00 hours 00 minutes 01 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: |
|
🔴 Test of 'container_bench test prepare_ci' failed after 00 hours 00 minutes 06 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'container_bench test test_ci' failed after 03 hours 57 minutes 34 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: |
|
🟢 Test of 'container_bench test test_ci' succeeded after 04 hours 31 minutes 45 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test cont_bench-jump-ci |
|
🟢 Test of 'container_bench test test_ci' succeeded after 12 hours 29 minutes 34 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: 2
♻️ Duplicate comments (2)
projects/container_bench/toolbox/container_bench_iperf_host_to_container_benchmark/tasks/main.yml (1)
13-29: Consistent with other iperf tasks; same optional concerns applyThe
sleep_cmdand switch todocker.io/networkstatic/iperf3:latestmirror the net‑bridge benchmark and look consistent. The same optional considerations apply here: pinning a concrete image tag and using a readiness check instead of a fixed 5s sleep if you later want more deterministic runs.projects/container_bench/toolbox/container_bench_iperf_net_host_benchmark/tasks/main.yml (1)
13-29: Net‑host iperf changes are consistent; image/tag and sleep handling match other tasksThe cross‑platform
sleep_cmdand unified use ofdocker.io/networkstatic/iperf3:latestare consistent with the other iperf benchmarks. As with those files, you might later consider pinning a specific tag and replacing the fixed 5s sleep with a readiness check if you hit flakiness or need strict reproducibility.
🧹 Nitpick comments (4)
projects/container_bench/toolbox/container_bench_iperf_net_bridge_benchmark/tasks/main.yml (1)
13-32: Cross‑platform sleep and image switch look fine; consider making them more deterministicThe
sleep_cmdfact and switch todocker.io/networkstatic/iperf3:latestare consistent and will work across OSes. Two optional refinements you might consider later:
- Avoid
:latestfor reproducibility (pin a known-good tag and reference it via a single variable).- Replace the fixed
sleep 5/Start-Sleep -Seconds 5with something like a readiness check (wait_foron the TCP port, or a loop retry) to reduce flakiness on slow or loaded hosts.projects/container_bench/testing/config.yaml (3)
111-118: iperf3 image in prepare container_images now usesnetworkstatic/iperf3:latestThis aligns with the task files and keeps a single image reference across configuration and playbooks. As elsewhere, if you later need stricter reproducibility, consider pinning a specific tag instead of
:latest.
153-162: Increasing podman machine CPUs to 12—just ensure it matches host capacityBumping
prepare.podman.machine.configuration.cpusto 12 is reasonable for heavier benchmarks, but on hosts with fewer physical cores it may either over‑commit or cause podman‑machine configuration/start failures. Worth double‑checking that all CI machines backing this config have at least that many cores.
195-235: Container benchmark runs increased to 10—verify CI runtime and noiseRaising all container benchmark
runsfrom 1 to 10 will materially increase total test time, which is great for smoothing variance but may stress CI timeouts or resource usage. Make sure your CI presets and time budgets are adjusted so this doesn’t introduce flakes from timeouts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
projects/container_bench/testing/config.yaml(7 hunks)projects/container_bench/testing/container_engine.py(2 hunks)projects/container_bench/toolbox/container_bench_capture_system_state/tasks/windows.yml(1 hunks)projects/container_bench/toolbox/container_bench_iperf_host_to_container_benchmark/tasks/main.yml(1 hunks)projects/container_bench/toolbox/container_bench_iperf_net_bridge_benchmark/tasks/main.yml(1 hunks)projects/container_bench/toolbox/container_bench_iperf_net_host_benchmark/tasks/main.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/container_bench/testing/container_engine.py (1)
projects/container_bench/testing/config_manager.py (3)
ConfigManager(82-263)get_podman_machine_config(154-166)is_windows(92-93)
🔇 Additional comments (4)
projects/container_bench/toolbox/container_bench_capture_system_state/tasks/windows.yml (1)
18-22: Windows system version formatting looks goodUsing
OsNameandOsVersionfromGet-ComputerInfowithWindowsBuildLabExis a clear, stable way to capture the Windows OS string and keeps the format aligned with the macOS-style summary.projects/container_bench/testing/config.yaml (3)
59-62: Docker Desktop remote‑host comment is a useful clarificationThe added note about
base_work_dirvshome_is_base_work_dirfor Docker Desktop remote runs clarifies an easy‑to‑miss requirement; no changes needed.
300-321: Podman repo_version matrix and matbenchmarking wiring look consistent; confirm versionsAdding
test.podman.repo_versionvalues (v5.4.2,v5.7.0) and including them initerable_test_fields, mapped toprepare.podman.repo.version, is structurally consistent with the existing matbenchmarking setup.Please just confirm that:
- These versions exist in the referenced podman repo/artifacts, and
- The templated path
" {@prepare.podman.repo.version}.zip"matches the actual filenames.
341-350:generate_plots_from_pr_args: true—ensure downstream tooling expects thisEnabling
generate_plots_from_pr_argshere is fine; just verify the visualization/CI entrypoint reads this flag and behaves correctly when it’s true (especially for non‑PR or local runs).
| def stop(self): | ||
| cmd = f"{self.get_cmd_env()} {get_podman_binary(self.base_work_dir)} machine stop {self.machine_name}" | ||
| if ConfigManager.is_windows(): | ||
| # Immediate end of ssh session may lead to machine not stopping properly on Windows. | ||
| cmd = f"{cmd}; Start-Sleep -Seconds 30" | ||
| machine_config = ConfigManager.get_podman_machine_config() | ||
| is_wsl = machine_config['env_containers_machine_provider'] == "wsl" | ||
| if ConfigManager.is_windows() and is_wsl: | ||
| # There is a bug in WSL that doesn't unlock ports after stopping usage of that port. | ||
| # https://github.com/microsoft/WSL/issues/10601 | ||
| cmd = f"{cmd}; wsl --shutdown" | ||
| remote_access.run_with_ansible_ssh_conf(self.base_work_dir, cmd) |
There was a problem hiding this comment.
🧩 Analysis chain
PodmanMachine.stop now shuts down all WSL; confirm that this side effect is acceptable
The new logic:
- Detects
env_containers_machine_provider == "wsl"and, on Windows, appendswsl --shutdownto the stop command.
This will fix the WSL port‑unlock bug on dedicated CI machines, but it also shuts down every running WSL distro on that host, which can impact any other workloads using WSL.
Consider gating this behind a dedicated config flag or at least logging very prominently that a full WSL shutdown is occurring, so it’s an explicit opt‑in behaviour for environments where WSL is shared.
I'll verify the review comment by examining the codebase context and checking the related code changes.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect container_engine.py around the PodmanMachine.stop() method
fd -n "container_engine.py" --type f -x cat {} | head -300
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for DockerDesktopMachine class and its stop() method
rg -n "class DockerDesktopMachine" -A 100 | head -150Length of output: 4980
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of wsl --shutdown in the codebase
rg -n "wsl --shutdown" -B 3 -A 3Length of output: 1602
🏁 Script executed:
#!/bin/bash
# Check for ConfigManager usage and wsl-related config flags
rg -n "env_containers_machine_provider\|wsl" -C 2 --type pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the stop() method implementations in machine-related classes
rg -n "def stop\(" -B 2 -A 10 --type py | head -200Length of output: 6354
I need to dig deeper into the configuration and deployment context to fully validate the review comment.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ConfigManager and get_podman_machine_config implementation
rg -n "class ConfigManager\|def get_podman_machine_config" -A 20 --type py | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any environment variables or flags controlling wsl shutdown behavior
rg -n "wsl\|WSL" --type py -C 3 | grep -i "env\|flag\|config\|disable\|opt" | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check where PodmanMachine and DockerDesktopMachine are instantiated/used
rg -n "PodmanMachine\|DockerDesktopMachine" --type py | grep -v "class " | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about deployment context
rg -n "CI\|shared\|dedicated\|host" projects/container_bench/testing/container_engine.py -C 2 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a way to disable or control the wsl shutdown behavior
rg -n "shutdown\|SHUTDOWN" --type py -C 2 | head -50Length of output: 0
Based on the code verification, I found the review comment is partially inaccurate. Let me provide the corrected version:
DockerDesktopMachine.stop() unconditionally shuts down all WSL distros on Windows; PodmanMachine.stop() is properly gated
The implementations diverge significantly:
-
PodmanMachine.stop() (lines 259–267): Properly gated—appends
wsl --shutdownonly whenConfigManager.is_windows() AND is_wsl(machine_config provider check). Safer than the review comment suggested. -
DockerDesktopMachine.stop() (lines 341–347): Unconditionally appends
wsl --shutdownon any Windows host (if ConfigManager.is_windows()only), with no provider validation. This assumes Docker Desktop always uses WSL on Windows and will shut down all distros regardless.
The core risk remains valid—wsl --shutdown affects every running WSL distro on the host. However:
- PodmanMachine is correctly scoped to its WSL provider only.
- DockerDesktopMachine lacks this safeguard and poses the actual cross-workload risk. Consider adding a provider check or explicit config flag to gate this behavior on Windows.
Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Jan Rodák <[email protected]>
2 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.