feat(containers): add NativeBackend for bare-metal worker deployment#1691
feat(containers): add NativeBackend for bare-metal worker deployment#1691hognek wants to merge 1 commit into
Conversation
Add a bare-metal container backend that runs agent workloads directly on the host without container overhead. Selected when container_runtime is explicitly set to 'native' in taOS config. Changes: - New NativeBackend in containers/native.py — implements the full ContainerBackend API using systemd service units and direct subprocess calls. Falls back gracefully when systemd is absent. - Registered in configure_container_runtime() as a config-only runtime (never auto-detected — bare metal is always available). - Exported from containers/__init__.py. - Added 'native' to the allowed runtime list in the settings API. - 24 new tests in test_container_native.py covering creation, lifecycle, env management, snapshots, and fallback paths. - 2 new tests in test_container_runtime_config.py for native config override and non-auto-detection guarantee. Fixes jaylfc#892
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ 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 |
|
I have read the CLA Document and I hereby sign the CLA |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| name = unit[:-len(".service")] if unit.endswith(".service") else unit | ||
| if not name.startswith(prefix): | ||
| continue | ||
| status = parts[3] # ACTIVE: active, inactive, failed, etc. |
There was a problem hiding this comment.
CRITICAL: Wrong column used for ACTIVE state.
systemctl list-units --no-legend emits columns in the order UNIT LOAD ACTIVE SUB JOB DESCRIPTION. parts[3] is the SUB column (sub-state like running, dead, exited), not the ACTIVE column. The intent here is the top-level ACTIVE state (active, inactive, failed), which lives at parts[2].
Effect: a service that is active (running) (most normal agents) will still map to Running because both ACTIVE and SUB are running/active — but a service in any other SUB state while ACTIVE (e.g. active (exited) for a Type=oneshot, active (waiting) for a Type=notify, active (start-pre) during a delayed start) will be misreported as the SUB value capitalized, and failed/inactive agents with non-trivial SUB will also be misclassified.
| status = parts[3] # ACTIVE: active, inactive, failed, etc. | |
| status = parts[2] # ACTIVE: active, inactive, failed, etc. |
| os.close(self._master_fd) | ||
| except OSError: | ||
| pass | ||
| self._proc.wait(timeout=5) |
There was a problem hiding this comment.
CRITICAL: Blocking subprocess.Popen.wait(timeout=5) inside an async context.
_NativePtyHandle.close is reached from the async event loop, but proc.wait(timeout=5) is a synchronous blocking call. If the child ignores SIGTERM (or a 5-second timeout is too short for graceful shutdown), the loop stalls for up to 5 seconds per PTY close. With multiple concurrent PTYs this serialises and can wedge interactive shells.
Use the asyncio subprocess variant (await loop.run_in_executor(None, proc.wait)) or wait with asyncio.wait_for on proc.wait() after switching to asyncio.create_subprocess_exec.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=/bin/true |
There was a problem hiding this comment.
WARNING: The generated unit is inert — start_container will report success but does nothing.
ExecStart=/bin/true plus Restart=no means systemctl start <unit>.service returns success immediately and the process exits 0 within milliseconds. From the caller's point of view create_container + start_container both returned success: True, so deployment code that polls these to wait for "running" will believe the agent is up when nothing is actually running.
The docstring acknowledges this is a "stub", but the public API surface does not. Consider either:
- leaving
success: Trueonly when the unit file is written AND ExecStart is replaced, returningsuccess: False(with a clearnote) when the unit is still the/bin/truestub; or - adding a
wait_for_execparameter / areload_payload(name, exec_start, ...)method that callers must invoke beforestart_container.
At minimum, the start_container success output should warn when ExecStart=/bin/true, so misconfigured deploys fail loudly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # rename_container | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| async def rename_container(self, old_name: str, new_name: str) -> dict: |
There was a problem hiding this comment.
WARNING: rename_container does not stop a running service before renaming on disk.
The ABC docstring at backend.py:153-154 explicitly says rename_container is for a stopped container. The LXC backend relies on incus rename to enforce that. This implementation renames the unit file on disk regardless of state; if the service is active, systemd keeps the old unit name live in memory, the new file is loaded only after the next daemon-reload (which is run), and any process still bound to the old name via /run/systemd ends up in a half-renamed state. There's no systemctl stop first and no check of is-active.
Either reject the rename when the unit is active (return success=False, output='unit is active; stop first') or stop+disable, rename, daemon-reload, and document the behaviour.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| """ | ||
|
|
||
| try: | ||
| _SYSTEMD_DIR.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
WARNING: _SYSTEMD_DIR.mkdir(parents=True, exist_ok=True) and unit_path.write_text(...) are attempted unconditionally under the systemd branch, with no privilege check.
On any systemd-enabled host where taOS is not running as root (which is the common case — systemd-managed services typically run as a dedicated user), this will raise PermissionError: [Errno 13] Permission denied: '/etc/systemd/system'. The except branch returns {"success": False, "error": "...Permission denied..."}, which is fine, but:
- There is no upfront detection (e.g.,
os.access(_SYSTEMD_DIR, os.W_OK)) so the failure surfaces only after the call. - The PR claims to support a bare-metal workflow, but doesn't say anything about needing root or a dedicated unit directory; without a
sudo/root setup it always fails on first run. The README/PR description should call this out.
Consider supporting a configurable unit directory via env var or config (TAOS_SYSTEMD_DIR=~/.local/share/taos/systemd for non-root), or returning a clearer note field that tells the operator exactly which privilege is missing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # exec_in_container | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| async def exec_in_container( |
There was a problem hiding this comment.
WARNING: name is decorative on the native backend — security boundary must be enforced by the caller.
exec_in_container, push_file, and spawn_pty all ignore name and operate directly on the host filesystem / process tree / network with full root-style privileges (modulo the actual uid of the taOS process). Any caller that has per-name authorisation checks (the obvious mental model from the LXC/Docker backends, where name IS the isolation boundary) silently gets nothing here.
This is an intentional design choice given the bare-metal intent, but it MUST be called out at the class docstring level (not just the per-method docstrings) so anyone plugging a new caller into the ContainerBackend interface understands that selecting the native backend changes the security model. Something like a # SECURITY: block at the top of NativeBackend stating: "Selecting this backend removes the per-name isolation that LXC/Docker provide. All operations execute with the taOS process's full privileges. Callers must treat name as a label, not a security boundary, and must authenticate/authorise separately."
Also consider raising (not silently ignoring) if name contains path-traversal sequences like ../ for push_file/remote_path — the LXC backend implicitly protects against this because the container rootfs is sandboxed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| )) | ||
| return results | ||
|
|
||
| async def _list_containers_fallback( |
There was a problem hiding this comment.
SUGGESTION: _list_containers_fallback is effectively dead code.
It's only invoked when _has_systemd() returns False (i.e. systemctl isn't on PATH). On any such host /etc/systemd/system typically doesn't exist either, so the first check at line 151 short-circuits and returns []. The only case this path is useful is a CI runner or container-in-container that has /etc/systemd/system populated as a mount but no systemctl binary — extremely rare.
Either drop it (and have list_containers return [] early on no-systemd) or document the niche case it covers in the docstring.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| stderr=asyncio.subprocess.STDOUT, | ||
| ) | ||
| stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) | ||
| return proc.returncode or 0, stdout.decode() if stdout else "" |
There was a problem hiding this comment.
SUGGESTION: proc.returncode or 0 collapses negative return codes (kill-by-signal) to 0.
When a child is terminated by SIGKILL/SIGTERM, asyncio.subprocess.Process.returncode is set to -SIGTERM / -SIGKILL (negative). x or 0 evaluates truthy for any non-zero integer including negatives, so -15 or 0 == -15 — fine here. But this is fragile to refactoring: a future contributor switching to returncode or 0 style elsewhere may lose signal info. Use proc.returncode if proc.returncode is not None else 0 (also handles the None case during timeout).
Also, communicate() raises TimeoutError on timeout — _run doesn't catch it, so a hung command raises into the caller without cleanup of the subprocess. Consider try/except (TimeoutError, asyncio.TimeoutError) and proc.kill() + await proc.wait() in a finally.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 8 Issues Found | Recommendation: Address before merge Overview
The new The Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 48.6K · Output: 8.1K · Cached: 352.4K |
|
Did a deep pass on this. As dormant opt-in code it is safe (no auto-detect, existing backends untouched), but a couple of things before it should be selectable:
Also worth confirming scope: issue #892 describes a cluster worker capability contract (register/advertise/drain/apply_update/health-check for model-serving backends), whereas this implements a ContainerBackend ABC used by the agent deployer. Related but a different abstraction, so I would hold off marking #892 fixed. Happy to merge once the isolation posture and supervision are sorted. |
|
Makes sense — will add the path-traversal guard + writable-dir precheck + the SECURITY note that name is decorative here, and either wire real supervision or return success:False while it is a stub rather than report deployed-and-running. Will also fix the SUB-vs-ACTIVE state read and the blocking proc.wait in the async PTY close. And agreed on scope: leaving #892 open — this is the ContainerBackend ABC, not the cluster-worker capability contract. |
|
Superseded by #1713, which contains this same NativeBackend implementation PLUS your requested review fixes [path-traversal guard on push_file/remote_path + writable-dir precheck + SECURITY note that name is decorative; real supervision or success:False for the stub; SUB->ACTIVE state read; non-blocking PTY close]. Please review #1713 instead — closing this to avoid two PRs for the same work. Scope note still stands: this is the ContainerBackend ABC, not the #892 cluster-worker capability contract, so #892 stays open. |
2 participants
Summary
Adds a bare-metal deployment option for TinyAgentOS workers — no containers, no Incus, no Docker. The worker agent runs directly on the host, avoiding container overhead on constrained nodes (e.g., CPU-only Qwen3-Embedding-8B on a low-power node).
Fixes #892.
What's new
tinyagentos/containers/native.py—NativeBackendimplements the fullContainerBackendABC using systemd service units and direct subprocess calls.create_container()writes a systemd unit,exec_in_container()runs commands directly on the host,push_file()copies files natively, etc.containers/backend.py— registered"native"inconfigure_container_runtime(). Native is config-only — never auto-detected (every host is bare-metal-capable, so auto-detect would shadow LXC/Docker).containers/__init__.py— exportedNativeBackend.routes/settings.py— added"native"to the allowed runtime list and backend-switching logic.Tests
test_container_native.py— 24 new tests covering:_service_unit_path,_has_systemdtest_container_runtime_config.py— 2 new tests: config override to native, non-auto-detection guaranteeUsage
Set in
config.yaml:Or via the settings API: