Add full support for running in toolbox with containers - #2871
Add full support for running in toolbox with containers#2871olliewalsh wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Fedora Toolbox support by detecting toolbox environments, routing container-engine commands through ChangesToolbox support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RamaLama
participant Toolbox
participant HostEngine
participant Container
RamaLama->>Toolbox: detect toolbox environment
RamaLama->>Toolbox: invoke flatpak-spawn --host
Toolbox->>HostEngine: run container-engine command
HostEngine->>Container: execute container operation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
Instead of disabling container mode entirely when running inside a toolbox, use flatpak-spawn --host to proxy engine commands to the host. This allows full container-based model serving from within Fedora toolbox containers when podman or docker is available on the host. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Oliver Walsh <owalsh@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 665-670: The toolbox E2E job and registry fixture must use a
host-shared temporary directory. In .github/workflows/ci.yml lines 665-670,
create and export TMPDIR="$HOME/.cache/ramalama/tmp" inside the toolbox before
make e2e-tests; in test/e2e/conftest.py lines 75-88, update the registry
fixture’s TemporaryDirectory usage to explicitly use that shared directory so
local toolbox runs do not depend on workflow setup.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd97b77b-6d07-4945-b336-509fcce0e588
📒 Files selected for processing (23)
.github/workflows/ci.ymldocs/ramalama-info.1.mddocs/ramalama.1.mdpyproject.tomlramalama/cli.pyramalama/common.pyramalama/config.pyramalama/engine.pyramalama/oci_tools.pyramalama/stack.pyramalama/transports/oci/oci.pyramalama/transports/oci/resolver.pyramalama/transports/oci/strategies.pyramalama/transports/rlcr.pytest/conftest.pytest/e2e/conftest.pytest/e2e/test_pull.pytest/e2e/test_rag.pytest/e2e/test_run.pytest/e2e/test_serve.pytest/unit/conftest.pytest/unit/test_common.pytest/unit/test_config.py
🚧 Files skipped from review as they are similar to previous changes (16)
- test/conftest.py
- test/e2e/test_pull.py
- test/unit/conftest.py
- ramalama/cli.py
- ramalama/transports/oci/resolver.py
- docs/ramalama-info.1.md
- test/e2e/test_rag.py
- pyproject.toml
- test/unit/test_config.py
- docs/ramalama.1.md
- ramalama/stack.py
- test/unit/test_common.py
- ramalama/oci_tools.py
- ramalama/transports/rlcr.py
- ramalama/common.py
- ramalama/engine.py
|
Cool! Ill give it a shot on my fedora system soon! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 599-611: Add a job-level least-privilege permissions block to
e2e-tests-toolbox, granting only contents read access, positioned before its
steps while preserving the existing job condition, dependencies, and test
execution.
- Around line 647-651: Update the “Install dependencies in toolbox” workflow
step to run uv venv inside the project directory before uv pip install '.[dev]'.
Keep pip install uv and uv tool install tox --with tox-uv before creating the
project environment, as requested.
- Around line 620-651: Install Ollama inside the ramalama-test Toolbox during
the “Install dependencies in toolbox” workflow step, using the existing toolbox
run mechanism, and ensure the installed ollama executable is available on PATH
for subsequent make e2e-tests execution. Keep the installation scoped to the
toolbox environment alongside the other E2E dependencies.
- Line 611: Update the actions/checkout step to disable credential persistence
by configuring its persist-credentials option to false, while preserving the
existing checkout behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df1138af-64fb-4930-aa69-b88075826d5e
📒 Files selected for processing (9)
.github/workflows/ci.ymlpyproject.tomltest/conftest.pytest/e2e/conftest.pytest/e2e/test_pull.pytest/e2e/test_rag.pytest/e2e/test_run.pytest/e2e/test_serve.pytest/unit/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- test/conftest.py
- test/unit/conftest.py
- test/e2e/test_pull.py
- pyproject.toml
- test/e2e/test_rag.py
If you want to use CUDA then need to run all of the CDI etc... setup on both the host and inside the toolbox. I expect this can be improved to just require the host to be configured, but will leave it for a follow-up PR as this PR is already big enough. |
Exercises the flatpak-spawn --host code path by running unit and e2e tests inside a real toolbox container on the Ubuntu runner. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Oliver Walsh <owalsh@redhat.com>
|
@debarshiray PTAL |
Previously running in a toolbox environment set the container engine to
None, effectively forcingnocontainermode. This adds full support for running in a toolbox environment by using flatpak-spawn to access the host podman/docker.Add engine_cmd() helper that wraps the container engine binary with
flatpak-spawn --hostwhen running inside a toolbox.Use
~/.cache/ramalama/tmpas the default temp directory in toolbox environments since/var/tmpis not shared with the host (required for image builds etc...).Report toolbox status in ramalama info output via a new Toolbox field
Add e2e-tests-toolbox CI job that creates a Fedora 44 toolbox on Ubuntu 24.04
Tested on fedora44 silverblue host with fedora44 toolbox for AMD ROCm/Vukan and NVIDIA CUDA llama.cpp backends.
Fixes: #2798
Fixes: #1095