Repository navigation
Conversation
Add design document for the SMG Helm chart at deploy/helm/smg/. What changed: - docs/plans/2026-03-09-helm-chart-design.md: new design doc covering chart architecture, values.yaml schema, template logic, and documentation plan Why: Planning document for adding a user-friendly Helm chart that supports three deployment modes: router-only, router+worker, and router+PD disaggregation. How: Single chart with a top-level mode toggle. Conditional templates render workers/PD components based on mode. Values designed flat and self-documenting with JSON Schema validation and escape hatches (extraArgs/extraEnv) on every component. Signed-off-by: Simon Olin <simon@simolin.dev> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
What changed: - docs/plans/2026-03-09-helm-chart-plan.md: 13-task implementation plan covering chart scaffolding, values.yaml, template helpers, router/worker/PD templates, RBAC, optional resources, JSON Schema, examples, and README Why: Step-by-step plan for implementing the Helm chart designed in 2026-03-09-helm-chart-design.md, with exact file paths, code, and verification commands for each task. Signed-off-by: Simon Olin <simon@simolin.dev> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add deploy/helm/smg/templates/_helpers.tpl containing all reusable template functions for the SMG Helm chart: - smg.name: chart name with nameOverride support, truncated to 63 chars - smg.fullname: fully qualified app name with fullnameOverride support - smg.chart: chart name and version for chart labels - smg.labels: common Kubernetes labels (helm.sh/chart, app version, etc.) - smg.selectorLabels: selector labels for app name and instance - smg.routerImage: constructs router container image reference from global.image.registry/repository/tag values - smg.workerImage: constructs worker container image with fallback defaults for sglang and vllm backends - smg.serviceAccountName: resolves service account name based on whether creation is enabled - smg.routerArgs: the core CLI argument builder that translates structured values.yaml configuration into the exact CLI flags expected by the smg binary, covering: - Host/port/policy and worker URL wiring - Service discovery configuration - PD disaggregation (prefill/decode) mode - Model and tokenizer paths - Cache tuning parameters (threshold, eviction, tree size, block size) - Request handling (payload size, timeouts, concurrency, queue) - Retry policy with backoff configuration - Circuit breaker settings - Health check configuration - Observability (Prometheus metrics, logging, OTLP tracing) - History backend (postgres, redis, oracle) - Rate limiting, WASM hooks, MCP config - Reasoning and tool call parsers - Extra args escape hatch for unlisted flags All CLI flag names have been verified against the Rust source in model_gateway/src/main.rs. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…t templates
Add the core Kubernetes templates that are always deployed regardless of
mode:
- deployment-router.yaml: Router Deployment with liveness/readiness
probes, metrics port, env vars for secrets (API key, Postgres URL,
Redis URL), autoscaling support, and full pod scheduling controls
(nodeSelector, affinity, tolerations)
- service-router.yaml: ClusterIP Service exposing HTTP and metrics
ports with configurable annotations and service type
- serviceaccount.yaml: Conditional ServiceAccount creation with
custom annotations support
- secret.yaml: Opaque Secret created only when inline credentials are
provided (apiKey, postgres url, redis url, oracle dsn); skipped when
users reference existingSecret
- NOTES.txt: Post-install notes showing endpoint URLs, metrics,
policy, and mode-specific information (workers, PD disaggregation,
ingress)
Also fix _helpers.tpl: remove inline Go template comments that used
{{- /* ... */ -}} trim markers, which caused adjacent args to merge
onto the same line in rendered output. Replace em-dashes with ASCII
dashes in block comments for portability.
Verified with helm template: renders Deployment, Service, and
ServiceAccount correctly. Secret renders conditionally when
auth.apiKey is set.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
- Remove *.md and !README.md negation pattern from .helmignore that caused Helm v3.19 to incorrectly skip Chart.yaml - Quote image value in deployment-router.yaml to handle empty tags - Wrap env block in conditional to avoid empty env: when no secrets are configured - Use int conversion for maxTreeSize and maxPayloadSize in routerArgs helper to prevent scientific notation in rendered args - Restore Chart.yaml to full content with commented-out sub-chart dependencies Signed-off-by: Si <simonmolinaroli@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add Role and RoleBinding templates that are conditionally created when either rbac.create=true or router.serviceDiscovery.enabled=true. The Role grants list/watch permissions on pods, which is required for the router's Kubernetes service discovery feature to discover worker pods. - templates/role.yaml: Role with pods list/watch permissions - templates/rolebinding.yaml: RoleBinding linking the Role to the chart's ServiceAccount Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ard templates Add five optional router-related templates, all disabled by default and gated behind their respective values toggles: - templates/ingress.yaml: Kubernetes Ingress with configurable hosts, TLS, className, and annotations (router.ingress.enabled) - templates/hpa-router.yaml: HorizontalPodAutoscaler targeting the router Deployment with CPU-based scaling (router.autoscaling.enabled) - templates/pdb.yaml: PodDisruptionBudget for the router pods with configurable minAvailable (router.podDisruptionBudget.enabled) - templates/servicemonitor.yaml: Prometheus ServiceMonitor for the router metrics endpoint (router.metrics.serviceMonitor.enabled) - templates/grafana-dashboard.yaml: ConfigMap containing a Grafana dashboard JSON with panels for request rate, p99 latency, active workers, and queue depth (grafana.dashboard.enabled) All templates use the standard smg.fullname and smg.labels helpers for consistent naming and labeling across the chart. Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add templates for the inference worker component, conditionally rendered when mode=router-worker. The worker runs an LLM inference backend (sglang, vllm, or trtllm) as a sidecar deployment alongside the router. - templates/worker/deployment.yaml: Worker Deployment with configurable model, parallelism (tensor/data parallel), GPU resources, /dev/shm shared memory volume, and optional model storage (emptyDir, hostPath, or PVC). Image is auto-selected based on backend type when not explicitly set. Extra args and env vars are supported. - templates/worker/service.yaml: ClusterIP Service exposing the worker HTTP port, used by the router to forward inference requests. The router deployment automatically includes the worker URL in its --worker-urls argument when mode=router-worker via the smg.routerArgs helper. Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add templates for prefill/decode disaggregated inference, conditionally rendered when mode=router-pd. This enables separating the prefill (prompt processing) and decode (token generation) phases onto different GPU pools for improved throughput and resource utilization. - templates/pd/deployment-prefill.yaml: Prefill worker Deployment with configurable model, tensor parallelism, GPU resources, /dev/shm shared memory, bootstrap port for KV-cache transfer, and optional model storage (emptyDir, hostPath, or PVC). - templates/pd/deployment-decode.yaml: Decode worker Deployment with the same configuration options as prefill but without the bootstrap port (decode receives KV-cache, does not initiate transfer). - templates/pd/service-prefill.yaml: ClusterIP Service exposing the prefill HTTP port and optional bootstrap port for KV-cache transfer. - templates/pd/service-decode.yaml: ClusterIP Service exposing the decode HTTP port. The router deployment automatically includes --pd-disaggregation, --prefill, --decode, and per-phase routing policy flags via the smg.routerArgs helper when mode=router-pd. Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add a test pod that uses curl to verify the router's /health endpoint is reachable after deployment. The test runs as a Helm test hook using the curlimages/curl image. - Creates tests/test-connection.yaml with helm.sh/hook: test annotation - Curls http://<fullname>-router:<service-port>/health - Uses router.service.port from values.yaml (default 80) Refs: deploy/helm/smg/tests/test-connection.yaml Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add values.schema.json to validate Helm values at lint/install time. The schema enforces enum constraints on key configuration fields. Validated fields: - mode: must be router, router-worker, or router-pd (required) - router.policy: must be a valid routing policy enum - router.port/replicas: integer with min/max bounds - worker.backend: must be sglang, vllm, or trtllm - worker.replicas/port: integer with min/max bounds - history.backend: must be none, memory, postgres, redis, or oracle Invalid values now produce clear errors at helm lint/install time. Refs: deploy/helm/smg/values.schema.json Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add 10 example values files covering all supported deployment modes and optional features. Each file is a minimal, commented override that can be used directly with helm install -f. Example files added: - router-only.yaml: minimal router pointing at existing workers - router-worker-sglang.yaml: router + sglang workers with 4 GPUs each - router-worker-vllm.yaml: router + vLLM workers - router-worker-trtllm.yaml: router + TensorRT-LLM workers - router-pd.yaml: PD disaggregation with prefill and decode workers - with-postgres.yaml: PostgreSQL history backend configuration - with-service-discovery.yaml: Kubernetes auto-discovery of worker pods - with-ingress.yaml: Ingress with TLS termination - with-monitoring.yaml: Prometheus ServiceMonitor + Grafana dashboard - production.yaml: production-hardened with HPA, PDB, monitoring, GPU node selectors, tolerations, and resource limits All examples validated with helm template rendering. Refs: deploy/helm/smg/examples/ Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add README.md documenting the SMG Helm chart with: - Prerequisites (Kubernetes, Helm, NVIDIA GPU Operator) - Quick start commands for all three deployment modes - Deployment modes table (router, router-worker, router-pd) - Configuration reference covering all values.yaml sections - Routing policies and inference backend details - Links to all 10 example values files with descriptions - Helm test instructions - Upgrade notes for v0.1.0 - Troubleshooting section for common issues (GPU scheduling, worker connectivity, service discovery RBAC) Refs: deploy/helm/smg/README.md Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Remove worker and PD disaggregation templates, values, examples, and documentation. These will be added in a future release once the worker CLI interface stabilizes. What was removed: - mode toggle (router-worker, router-pd) - worker/ and pd/ template directories - worker and pd sections from values.yaml - worker-related example files (sglang, vllm, trtllm, pd, production) - smg.workerImage helper from _helpers.tpl - PD disaggregation args from routerArgs helper - Worker/PD references from README and NOTES.txt - Worker/PD properties from values.schema.json - PD-specific selectors (prefillSelector, decodeSelector) from values.yaml Signed-off-by: Simo Lin <simo@simolin.dev> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Helm chart at deploy/helm/smg/ with Chart metadata, values and schema, many templated Kubernetes manifests (router Deployment, Service, HPA, Ingress, RBAC, ServiceMonitor, Grafana dashboard, etc.), example values and tests, README and planning docs, and a pre-commit YAML exclusion for Helm templates. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (helm install)
participant Helm as Helm
participant K8s as Kubernetes API
participant Router as Router Pod
participant Worker as Worker Pod
participant DB as Postgres/Redis
participant Prom as Prometheus
rect rgba(90,150,255,0.5)
Dev->>Helm: helm install smg ./deploy/helm/smg -f values.yaml
Helm->>K8s: Render templates -> POST manifests
end
rect rgba(120,200,80,0.5)
K8s->>Router: Create Deployment/Service/Config
K8s->>Worker: (optional) Create worker resources
K8s->>DB: (optional) Create DB resources / Secrets
K8s->>Prom: (optional) Create ServiceMonitor / ConfigMap
end
rect rgba(255,200,80,0.5)
Router->>Worker: Forward inference requests (workerUrls / service discovery)
Worker->>DB: Read/write history or model artifacts (when enabled)
Router->>Prom: Expose /metrics scraped by Prometheus
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a production-ready Helm chart for deploying the SMG router in a Kubernetes environment. It focuses on providing a flexible and configurable solution for router-only deployments, connecting to existing inference workers. The chart incorporates best practices for Kubernetes deployments, including RBAC, autoscaling, and monitoring, and offers comprehensive documentation and examples to facilitate adoption. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Helm chart for deploying the SMG router. The chart is well-structured with good defaults, extensive configuration options, and helpful examples. However, there are significant security concerns regarding the handling of sensitive database credentials. Connection strings for PostgreSQL, Redis, and Oracle are currently passed as command-line arguments, which can leak passwords to logs and other users on the host. Additionally, the Deployment template has incomplete logic for mounting these secrets as environment variables, particularly when provided directly in the chart's values. I've also included a couple of medium-severity suggestions to improve maintainability and robustness by enhancing the .helmignore file and expanding the values.schema.json for better validation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dfecd8124
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
What changed: - _helpers.tpl: fix --postgres-url to --postgres-db-url, --postgres-pool-max to --postgres-pool-max-size, --redis-pool-max to --redis-pool-max-size; remove non-existent --postgres-retention-days flag; add --oracle-user and --oracle-password CLI args; add --api-key CLI arg for auth - deployment-router.yaml: remove broken SMG_API_KEY, POSTGRES_URL, REDIS_URL env var mounts (router does not read these env vars); simplify env block to only handle extraEnv - secret.yaml: remove api-key, postgres-url, redis-url entries; keep only oracle secrets since oracle is the only backend with env var bindings - values.yaml: rename poolMax to poolMaxSize for postgres and redis; remove retentionDays (no such CLI flag); remove existingSecret for postgres/redis (no env var bindings); remove apiKeySecret/apiKeySecretKey (no env var binding for api-key) - examples/with-postgres.yaml: update poolMax to poolMaxSize, remove retentionDays - .pre-commit-config.yaml: exclude Helm template and test directories from check-yaml hook (Go template syntax is not valid YAML) Why: - CLI flag names must match the actual flags in model_gateway/src/main.rs - The router reads --api-key from CLI, not from SMG_API_KEY env var - The router has no env var bindings for postgres/redis connection URLs - Oracle backend has ATP_* env var bindings so secrets work for oracle only Refs: #699 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/Chart.yaml`:
- Around line 1-6: Chart.yaml is missing kubeVersion which lets Helm install on
unsupported clusters; add a kubeVersion field to Chart.yaml (e.g., kubeVersion:
">=1.26.0") to enforce the documented Kubernetes minimum version and prevent
installs on older clusters.
In `@deploy/helm/smg/examples/router-only.yaml`:
- Line 2: Update the example usage header so the -f flag points to the file path
from the repo root: replace "helm install smg deploy/helm/smg -f
examples/router-only.yaml" with "helm install smg deploy/helm/smg -f
deploy/helm/smg/examples/router-only.yaml", and apply the same correction to any
sibling example headers that use "-f examples/..." so copy-paste from the repo
root works.
In `@deploy/helm/smg/examples/with-postgres.yaml`:
- Around line 8-10: The example currently hard-codes DB credentials in the
postgres.url value; change the example to avoid inline secrets by replacing the
password portion with an obvious placeholder (e.g. "<DB_PASSWORD>") or by
showing the existing-secret flow instead of embedding credentials; update the
postgres.url value and/or add a comment indicating that production credentials
must come from a Kubernetes Secret (do not commit real passwords) and keep
poolMax as-is (keys to edit: postgres, url, poolMax).
In `@deploy/helm/smg/templates/_helpers.tpl`:
- Around line 198-200: Remove the sensitive CLI flags and instead read the
connection strings from environment variables populated from Secrets: stop
emitting the "--postgres-url", "--redis-url", and "--oracle-dsn" args in the
template (references: the conditional blocks checking
.Values.history.postgres.url, .Values.history.redis.url,
.Values.history.oracle.dsn and the literal flag strings) and replace that logic
with environment variable references that are sourced from the chart's Secret
support (use the existingSecret/generated secret keys used by the chart, e.g.,
HISTORY_POSTGRES_URL, HISTORY_REDIS_URL, HISTORY_ORACLE_DSN), ensuring the
template only wires in env vars from .Values.history.existingSecret or the
created Secret and no longer injects the values as command arguments.
In `@deploy/helm/smg/templates/deployment-router.yaml`:
- Around line 59-60: The resources: block currently always renders even when
.Values.router.resources is empty; wrap the block in a conditional using Helm's
with (e.g., {{- with .Values.router.resources }}) so that the resources: key and
its toYaml rendering (toYaml .Values.router.resources | nindent 12) are only
emitted when router.resources is non-empty, and close the block with {{- end }}
to avoid producing an empty resources: entry.
- Around line 47-58: The livenessProbe and readinessProbe in the deployment
template are hardcoded (see livenessProbe, readinessProbe, httpGet path /health
and port http); make their initialDelaySeconds and periodSeconds configurable
via values.yaml (e.g., router.livenessProbe.initialDelaySeconds,
router.livenessProbe.periodSeconds, router.readinessProbe.initialDelaySeconds,
router.readinessProbe.periodSeconds) and update the template to use Helm
template lookups with sensible defaults (use the values with default fallback)
so operators can tune probe timings without editing the chart.
In `@deploy/helm/smg/templates/grafana-dashboard.yaml`:
- Around line 53-54: The dashboard UID is hard-coded as "smg-overview" which
causes collisions across releases; update the UID to be release-scoped or
configurable by replacing the literal "smg-overview" used for the dashboard "SMG
Overview" with a templated value (e.g., include the release fullname via the
"smg.fullname" helper) or read from a values entry so each release gets a unique
UID.
In `@deploy/helm/smg/templates/hpa-router.yaml`:
- Around line 13-21: The HPA template (hpa-router.yaml) scales on CPU but may
fail if .Values.router.resources.requests.cpu is not set; add a validation or
documentation change: either update the Helm chart schema to require
.Values.router.resources.requests.cpu when .Values.router.autoscaling.enabled is
true (add a conditional required rule referencing router.autoscaling.enabled and
router.resources.requests.cpu) or update values.yaml and README to clearly state
that .Values.router.resources.requests.cpu must be provided for autoscaling to
work; ensure references to .Values.router.autoscaling.minReplicas,
.Values.router.autoscaling.maxReplicas and
.Values.router.autoscaling.targetCPUUtilizationPercentage remain unchanged.
In `@deploy/helm/smg/templates/ingress.yaml`:
- Around line 20-34: The ingress template emits an empty rules array when
.Values.router.ingress.hosts is empty; wrap the entire rules block in a guard
like {{- if .Values.router.ingress.hosts }} ... {{- end }} so that rules is
omitted when there are no hosts (refer to the symbols
.Values.router.ingress.hosts and the block that calls include "smg.fullname" $
for the router backend). Also update values.schema.json to add a
conditional/oneOf that requires router.ingress.hosts to be a non-empty array
(and validate that each host has at least one path) when router.ingress.enabled
is true so Helm will validate non-empty hosts/paths before rendering.
In `@deploy/helm/smg/templates/secret.yaml`:
- Line 1: The Oracle branch currently only checks .Values.history.oracle.dsn so
it will create oracle-user and oracle-password with empty values; update the
top-level predicate and the Oracle-specific predicate (and the branch handling
lines 19–22) to require all three keys .Values.history.oracle.dsn,
.Values.history.oracle.user, and .Values.history.oracle.password when
.Values.history.oracle.existingSecret is not set, or alternatively make the
template fail-fast by erroring when existingSecret is unset but any of
dsn/user/password are missing; change the conditional logic to gate creation on
all three being present (or emit a clear error) so empty credentials are never
written.
In `@deploy/helm/smg/templates/service-router.yaml`:
- Line 4: The fullname helper (smg.fullname) is truncated to 63 chars before
resource-specific suffixes like -router are appended, causing DNS names to
exceed 63 chars; add resource-specific helpers (e.g., smg.routerName,
smg.secretsName) that build the base name plus the suffix and perform truncation
as a single unit (ensuring final name <=63 chars), then update templates that
currently call include "smg.fullname" (such as service-router,
deployment-router, hpa-router, ingress, pdb, servicemonitor) to call the new
helpers (e.g., include "smg.routerName") or alternatively reduce smg.fullname’s
truncation length to reserve space for the longest suffix — implement one
consistent approach and replace all usages to avoid future overlong names.
In `@deploy/helm/smg/tests/test-connection.yaml`:
- Line 12: The test Job uses a mutable image tag ("image:
curlimages/curl:latest") which makes helm tests non-reproducible and may be
blocked by policies; update the image field in this Helm test manifest to use an
immutable reference by either a specific version tag (e.g., 8.18.0) or a
content-addressable digest (sha256), or expose the test image via chart values
so operators can control it—locate the image line in the Helm test manifest (the
Job/container spec that currently sets image: curlimages/curl:latest) and
replace it with a fixed tag or digest or wire it to a values.yaml property
(e.g., test.image) so it’s configurable.
In `@docs/plans/2026-03-09-helm-chart-design.md`:
- Around line 20-37: The fenced code blocks in the doc contain unlabeled fences
that trigger markdownlint MD040; add a language identifier to each fence: change
the ASCII diagram block (the fence that begins with the SMG Router / ASCII art)
to use ```text, and change the helper/template snippet fences to use
```gotemplate (or ```text if you prefer plain text); apply the same change to
the other occurrences referenced in the comment (the similar diagram and helper
blocks in the document) so all triple-backtick fences are labeled.
- Around line 5-10: The doc still lists three Helm deployment modes (`router`,
`router-worker`, `router-pd`) and templates/examples that no longer exist;
update this design note to either (a) reflect the current chart which is
router-only by removing or rewriting the `router-worker`/`router-pd` sections
and examples and adjusting any references to worker templates, or (b) mark the
entire design doc as superseded with a clear header and a short summary that the
PR implements router-only behavior; ensure you update or remove any references
to the now-missing templates/examples and keep the `router` mode description
accurate.
In `@docs/plans/2026-03-09-helm-chart-plan.md`:
- Line 15: The "### Task 1: Chart Scaffolding" heading jumps from the implicit
document title (H1) to H3; change that heading to H2 by replacing "### Task 1:
Chart Scaffolding" with "## Task 1: Chart Scaffolding" (or insert an H2 section
above the task block) so heading levels increment properly and follow markdown
best practices.
- Around line 1022-1057: The NOTES.txt code block lacks a language specifier;
update the opening triple-backtick for the block that contains the Helm template
(the block referencing $fullname, .Values.mode, .Values.router.*,
.Values.worker.*, .Values.pd.*, and the final helm test {{ .Release.Name }}) to
include an appropriate language identifier such as gotmpl or text (for example
change ``` to ```gotmpl) so syntax highlighting and tooling recognize the Helm
template content.
- Around line 1-11: The plan document titled "SMG Helm Chart Implementation
Plan" still describes three deployment modes via the top-level value "mode" and
includes "router", "router-worker", and "router-pd" as well as Tasks 7 and 8 for
worker/PD templates; update the document to reflect the actual router-only
implementation by removing or marking as deprecated the "router-worker" and
"router-pd" references, delete or annotate Tasks 7 and 8 as obsolete, and add a
brief note stating the chart was simplified to router-only (or alternatively
keep the original design as historical context and clearly label it as
archived/original-design) so future maintainers are not confused.
- Around line 66-72: The code block showing .helmignore contents lacks a
language specifier; update the fenced code block in the .helmignore example (the
block containing ".git .gitignore *.md !README.md examples/") to include a
language tag such as "text" (e.g., replace ``` with ```text) so syntax
highlighting and documentation consistency are applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01b344c8-a1ce-489e-a7c0-57060032ec75
📒 Files selected for processing (26)
deploy/helm/smg/.helmignoredeploy/helm/smg/Chart.yamldeploy/helm/smg/README.mddeploy/helm/smg/examples/router-only.yamldeploy/helm/smg/examples/with-ingress.yamldeploy/helm/smg/examples/with-monitoring.yamldeploy/helm/smg/examples/with-postgres.yamldeploy/helm/smg/examples/with-service-discovery.yamldeploy/helm/smg/templates/NOTES.txtdeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/grafana-dashboard.yamldeploy/helm/smg/templates/hpa-router.yamldeploy/helm/smg/templates/ingress.yamldeploy/helm/smg/templates/pdb.yamldeploy/helm/smg/templates/role.yamldeploy/helm/smg/templates/rolebinding.yamldeploy/helm/smg/templates/secret.yamldeploy/helm/smg/templates/service-router.yamldeploy/helm/smg/templates/serviceaccount.yamldeploy/helm/smg/templates/servicemonitor.yamldeploy/helm/smg/tests/test-connection.yamldeploy/helm/smg/values.schema.jsondeploy/helm/smg/values.yamldocs/plans/2026-03-09-helm-chart-design.mddocs/plans/2026-03-09-helm-chart-plan.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e5c0d06f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/templates/_helpers.tpl`:
- Around line 252-255: The Helm template currently renders the "--api-key" CLI
flag using .Values.auth.apiKey which leaks secrets via process args; change to
support a Secret-backed path by adding auth.existingSecret to values (expected
Secret key "api-key"), update the deployment template to remove the "--api-key"
entry from the smg.routerArgs block and instead inject an env var (e.g.,
ROUTER_API_KEY) sourced from the Secret named by auth.existingSecret, and keep a
fallback to using .Values.auth.apiKey only when auth.existingSecret is empty so
existing installs remain supported.
In `@deploy/helm/smg/templates/deployment-router.yaml`:
- Around line 34-64: The container template for the router is missing a
securityContext; add support for .Values.router.securityContext in the
deployment-router.yaml template by injecting the securityContext block after the
imagePullPolicy (use the Helm with/ toYaml pattern) so that values under
router.securityContext (e.g., runAsNonRoot, runAsUser, readOnlyRootFilesystem,
allowPrivilegeEscalation, capabilities.drop) are rendered into the container
spec; update values.yaml to include a recommended router.securityContext object
and ensure the template uses {{- with .Values.router.securityContext }}
securityContext: {{- toYaml . | nindent 12 }} {{- end }} so the container runs
non-root with a read-only filesystem and dropped capabilities.
In `@deploy/helm/smg/values.yaml`:
- Around line 166-177: Add an auth.existingSecret option alongside auth.apiKey
(e.g., auth.existingSecret.name and auth.existingSecret.key) and update Helm
templates to prefer the secret value when present: in the deployment/args where
--api-key is passed, read the API key from a Kubernetes Secret (envFrom/env or
valueFrom.secretKeyRef) using auth.existingSecret if set, otherwise fall back to
auth.apiKey; ensure templates handle both empty and present cases and add chart
values documentation for the new auth.existingSecret keys.
- Around line 186-196: Postgres and Redis value blocks lack an existingSecret
option like the Oracle block (oracle.existingSecret); add existingSecret: ""
entries under the postgres and redis sections (alongside url, poolMaxSize,
deploy) and update Helm templates that create DB/Redis credentials
(deployment/statefulset/secret templates) to prefer
.Values.postgres.existingSecret and .Values.redis.existingSecret when present
(fallback to inline url/credentials when empty), ensuring the chart loads
credentials from an existing Kubernetes Secret the same way
oracle.existingSecret is handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2f3a3e7-2ed1-41e1-ba86-2c5cd643c179
📒 Files selected for processing (6)
.pre-commit-config.yamldeploy/helm/smg/examples/with-postgres.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/secret.yamldeploy/helm/smg/values.yaml
- Add kubeVersion constraint (>=1.26.0-0) to Chart.yaml
- Fix example usage paths to work from repo root
- Replace hardcoded credentials in with-postgres example
- Pin test-connection curl image to 8.12.1 (was mutable :latest)
- Wrap resources block in {{- with }} to avoid empty rendering
- Make Grafana dashboard UID release-scoped to prevent collisions
- Require all Oracle fields (dsn, user, password) in secret template
- Truncate fullname to 46 chars to prevent DNS name overflow with suffixes
- Add note that CPU requests are required when autoscaling is enabled
- Mark design and plan docs as partially superseded (router-only scope)
- Add language identifiers to fenced code blocks in design doc
Signed-off-by: Simon Lin <simonlin.code@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c65c89508f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
docs/plans/2026-03-09-helm-chart-plan.md (3)
11-11:⚠️ Potential issue | 🟡 MinorArchive the worker/PD tasks explicitly.
The preamble says worker and PD work was deferred, but these sections still instruct maintainers to build and validate those modes as part of the plan. Please move them under an archived/future-work heading or clearly strike them as out-of-scope for v0.1.0.
Also applies to: 1332-1764, 2077-2084
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-helm-chart-plan.md` at line 11, The plan mentions worker and PD modes as if they are in-scope but the preamble says they were deferred; update the document (sections referencing the Helm chart at deploy/helm/smg/, the top-level mode value behavior, and template descriptions for Router, worker and PD, plus CLI assembly in _helpers.tpl) by moving the worker and PD build/validation instructions into an "Archived / Future work" or "Out of scope for v0.1.0" heading (or clearly strike them), and apply the same change to the duplicate sections referenced (around the other ranges) so maintainers will not treat worker/PD steps as required for v0.1.0.
70-76:⚠️ Potential issue | 🟡 MinorAdd language identifiers to these fenced blocks.
These two fences are still unlabeled, which keeps MD040 noisy.
textworks for.helmignore;gotemplate/gotmplortextworks for theNOTES.txtsnippet.Also applies to: 1026-1061
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-helm-chart-plan.md` around lines 70 - 76, The two unlabeled fenced code blocks containing the .git/.gitignore/*.md/README.md/examples list and the NOTES.txt snippet need explicit language identifiers to satisfy MD040; add "text" (or "gotemplate"/"gotmpl" for template content) after the opening backticks for the block showing the .helmignore contents and for the NOTES.txt snippet so the fences become, for example, ```text (or ```gotemplate) to silence the lint warnings and clarify the block types.
19-19:⚠️ Potential issue | 🟡 MinorDrop
Task 1to H2.The document jumps from H1 to H3 here, so markdownlint MD001 is still failing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-helm-chart-plan.md` at line 19, Replace the H3 header "### Task 1: Chart Scaffolding" with an H2 header "## Task 1: Chart Scaffolding" so the document goes H1 → H2 and satisfies markdownlint MD001; locate the line containing the string "### Task 1: Chart Scaffolding" and change the leading hashes from three to two.docs/plans/2026-03-09-helm-chart-design.md (1)
84-128:⚠️ Potential issue | 🟡 MinorMark these inventories as future-state, not shipped content.
Even with the superseded note, these sections still read like the current v0.1.0 chart layout/examples/dependencies, but several worker/PD paths listed here are not part of the released router-only chart. Please either relabel these sections as planned future layout or trim the non-existent items so readers do not chase missing files.
Also applies to: 287-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-helm-chart-design.md` around lines 84 - 128, Update the "File Layout" inventory under the File Layout heading for deploy/helm/smg/ so it does not present non-shipped files as current: either clearly prefix the block (and the similar block at lines 287-321) with a "Future-state / Planned layout" note and leave the full tree (including templates/worker/, templates/pd/, and example files like router-worker-sglang.yaml, router-pd.yaml, with-postgres.yaml), or prune the tree to only list files actually present in the released router-only chart (remove templates/worker/*, templates/pd/* and the non-existent examples). Ensure you update the README.md reference text near the inventory so readers aren’t directed to missing paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/templates/grafana-dashboard.yaml`:
- Line 5: The ConfigMap metadata.name can exceed the 63‑char DNS label limit
because you append "-grafana-dashboard" to {{ include "smg.fullname" . }};
change the template to truncate the base name so the final string is <=63 chars
and still DNS-safe (lowercase, alphanum, hyphen). For example, replace the
current name line with one that truncates the fullname to (63 - length of
"-grafana-dashboard") characters using the Helm/Sprig trunc filter (e.g.
(include "smg.fullname" .) | trunc 45) and then append "-grafana-dashboard", and
ensure you trim any trailing hyphens so the resulting metadata.name remains a
valid DNS label.
- Around line 53-54: The UID currently builds as "{{ include "smg.fullname" .
}}-overview" which can exceed Grafana's 40-char limit; update the Helm template
to use a dedicated helper (e.g., create a template helper like "smg.uid" or
"smg.hashName") that either truncates the result of include "smg.fullname" . to
a safe length before appending "-overview" or returns a short hash (sha1/md5) of
the fullname and append "-overview", then reference that helper in
grafana-dashboard.yaml's "uid" instead of include "smg.fullname" . to guarantee
<=40 chars.
In `@deploy/helm/smg/values.yaml`:
- Around line 172-176: The values.yaml exposes an unused OIDC block
(auth.oidc.enabled, issuer, audience) that deployment-router.yaml never reads;
either remove this no-op oidc section or wire its fields into smg.routerArgs so
the router actually receives them. Update the values schema by deleting the oidc
keys or map them under smg.routerArgs (e.g., ensure smg.routerArgs includes
oidc.enabled, oidc.issuer, oidc.audience) and adjust any template references in
deployment-router.yaml to consume smg.routerArgs instead of the removed
top-level auth.oidc keys; verify the router template (deployment-router.yaml)
reads the mapped values.
In `@docs/plans/2026-03-09-helm-chart-design.md`:
- Around line 236-241: Update the "Existing secret" pattern description to stop
saying it uses envFrom and instead explain that the chart should reference
specific secret keys via SecretKeyRef and pass values to the router/history
processes as CLI arguments; explicitly mention using secretKeyRef to target e.g.
auth.apiKeySecret (for API key) and that connection strings for the router must
be supplied via CLI flags --postgres-db-url, --redis-url, and --oracle-dsn
rather than environment variables so the Helm templates should read the Secret's
key and inject it into the container args.
In `@docs/plans/2026-03-09-helm-chart-plan.md`:
- Around line 917-930: The current snippets expose secrets as env vars
(POSTGRES_URL, REDIS_URL) but the router only accepts connection strings via CLI
args (--postgres-db-url, --redis-url, --oracle-dsn), so update the Helm
Deployment examples to (1) keep mounting the secret values into env vars using
the existing secretKeyRef blocks for .Values.history.postgres.existingSecret,
.Values.history.redis.existingSecret and the oracle secret, and (2) add
container args that pass those env vars into the router CLI like
--postgres-db-url=$(POSTGRES_URL), --redis-url=$(REDIS_URL) and
--oracle-dsn=$(ORACLE_DSN); replace the broken direct-secret-to-env-only
examples with this env->arg wiring wherever the history backend snippets appear
(including the other occurrence called out) so the router receives the
connection strings via the named CLI flags.
---
Duplicate comments:
In `@docs/plans/2026-03-09-helm-chart-design.md`:
- Around line 84-128: Update the "File Layout" inventory under the File Layout
heading for deploy/helm/smg/ so it does not present non-shipped files as
current: either clearly prefix the block (and the similar block at lines
287-321) with a "Future-state / Planned layout" note and leave the full tree
(including templates/worker/, templates/pd/, and example files like
router-worker-sglang.yaml, router-pd.yaml, with-postgres.yaml), or prune the
tree to only list files actually present in the released router-only chart
(remove templates/worker/*, templates/pd/* and the non-existent examples).
Ensure you update the README.md reference text near the inventory so readers
aren’t directed to missing paths.
In `@docs/plans/2026-03-09-helm-chart-plan.md`:
- Line 11: The plan mentions worker and PD modes as if they are in-scope but the
preamble says they were deferred; update the document (sections referencing the
Helm chart at deploy/helm/smg/, the top-level mode value behavior, and template
descriptions for Router, worker and PD, plus CLI assembly in _helpers.tpl) by
moving the worker and PD build/validation instructions into an "Archived /
Future work" or "Out of scope for v0.1.0" heading (or clearly strike them), and
apply the same change to the duplicate sections referenced (around the other
ranges) so maintainers will not treat worker/PD steps as required for v0.1.0.
- Around line 70-76: The two unlabeled fenced code blocks containing the
.git/.gitignore/*.md/README.md/examples list and the NOTES.txt snippet need
explicit language identifiers to satisfy MD040; add "text" (or
"gotemplate"/"gotmpl" for template content) after the opening backticks for the
block showing the .helmignore contents and for the NOTES.txt snippet so the
fences become, for example, ```text (or ```gotemplate) to silence the lint
warnings and clarify the block types.
- Line 19: Replace the H3 header "### Task 1: Chart Scaffolding" with an H2
header "## Task 1: Chart Scaffolding" so the document goes H1 → H2 and satisfies
markdownlint MD001; locate the line containing the string "### Task 1: Chart
Scaffolding" and change the leading hashes from three to two.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7ad71be-8374-4743-a9d3-0b49226e5a10
📒 Files selected for processing (14)
deploy/helm/smg/Chart.yamldeploy/helm/smg/examples/router-only.yamldeploy/helm/smg/examples/with-ingress.yamldeploy/helm/smg/examples/with-monitoring.yamldeploy/helm/smg/examples/with-postgres.yamldeploy/helm/smg/examples/with-service-discovery.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/grafana-dashboard.yamldeploy/helm/smg/templates/secret.yamldeploy/helm/smg/tests/test-connection.yamldeploy/helm/smg/values.yamldocs/plans/2026-03-09-helm-chart-design.mddocs/plans/2026-03-09-helm-chart-plan.md
…iew feedback The Docker image uses the Python entrypoint (smg.launch_router) which accepts different flag names than the Rust CLI. Fix the flags to match what the published image actually accepts: - --postgres-pool-max-size → --postgres-pool-max - --redis-pool-max-size → --redis-pool-max - --oracle-dsn → --oracle-connect-descriptor - --oracle-user → --oracle-username Also addresses remaining review comments: - Reduce fullname truncation from 46 to 45 chars (longest suffix -grafana-dashboard is 18 chars, 63-18=45) - Truncate Grafana dashboard UID to max 40 chars (Grafana API limit) - Remove non-functional OIDC config (routerArgs never consumes it) - Remove non-functional existingSecret from oracle config - Revert poolMaxSize back to poolMax (matches Python CLI flag name) - Fix design doc Secret Management section (envFrom → CLI args) - Add note in plan doc about env var wiring being from original design Signed-off-by: Simon Lin <simonlin.code@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
commented
Mar 10, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/plans/2026-03-09-helm-chart-plan.md (1)
70-76: 🧹 Nitpick | 🔵 TrivialAdd language specifier to
.helmignorecode block.The fenced code block lacks a language specifier, which triggers markdownlint MD040. This was flagged in a previous review as addressed, but the current code still shows an unspecified fence.
📝 Suggested fix
-``` +```text .git .gitignore *.md !README.md examples/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-helm-chart-plan.md` around lines 70 - 76, The fenced code block showing the .helmignore contents is missing a language specifier (triggers markdownlint MD040); update the fenced block around the .git/.gitignore/*.md/!README.md/examples/ lines in docs/plans/2026-03-09-helm-chart-plan.md to include a language token (e.g., use ```text) so the block is explicitly marked and MD040 is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-09-helm-chart-plan.md`:
- Around line 1030-1065: The NOTE block's fenced code block lacks a language
specifier which triggers markdownlint MD040; update the opening fence for the
template that starts with {{- $fullname := include "smg.fullname" . -}} (the
NOTES.txt/Helm template content showing SMG deployed in "{{ .Values.mode }}"
mode and the Router/Workers/PD/Ingress sections) to include a language
identifier such as gotmpl or text (e.g., change ``` to ```gotmpl) so the block
is properly recognized by the linter.
---
Duplicate comments:
In `@docs/plans/2026-03-09-helm-chart-plan.md`:
- Around line 70-76: The fenced code block showing the .helmignore contents is
missing a language specifier (triggers markdownlint MD040); update the fenced
block around the .git/.gitignore/*.md/!README.md/examples/ lines in
docs/plans/2026-03-09-helm-chart-plan.md to include a language token (e.g., use
```text) so the block is explicitly marked and MD040 is satisfied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18386eb3-ebd8-4c91-a65b-291cae0e2db9
📒 Files selected for processing (7)
deploy/helm/smg/examples/with-postgres.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/grafana-dashboard.yamldeploy/helm/smg/templates/secret.yamldeploy/helm/smg/values.yamldocs/plans/2026-03-09-helm-chart-design.mddocs/plans/2026-03-09-helm-chart-plan.md
- Add missing --enable-trace flag when router.tracing.enabled=true (Python CLI requires this alongside --otlp-traces-endpoint) - Fix history.backend condition to only skip "memory" (the default), so "none" is properly forwarded to the router CLI Signed-off-by: Simon Lin <simon@lightseek.org> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aed0e84161
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/templates/_helpers.tpl`:
- Around line 196-210: When history.backend is "postgres" or "redis" the
template must fail render if the corresponding .Values.history.postgres.url or
.Values.history.redis.url is empty; update the blocks in templates/_helpers.tpl
that emit "--postgres-db-url"/"--redis-url" and the pool flags to use Helm's
required check (e.g., wrap or replace the current .Values access with required
so rendering errors when the URL is missing) and only emit the pool-max flags
when the URL is present; reference the keys .Values.history.backend,
.Values.history.postgres.url, .Values.history.postgres.poolMax,
.Values.history.redis.url, .Values.history.redis.poolMax and the flags
"--postgres-db-url"/"--redis-url" when making the change.
In `@deploy/helm/smg/values.yaml`:
- Around line 140-147: Add validation and template guards to enforce the HPA
preconditions: when autoscaling.enabled is true, verify
router.resources.requests.cpu is set (non-empty) and ensure maxReplicas >=
minReplicas; update the chart's schema (values schema) to require
router.resources.requests.cpu when autoscaling.enabled, and add conditional
checks in the HPA template rendering logic that error out or fail rendering when
autoscaling.enabled is true but router.resources.requests.cpu is missing, or
when autoscaling.maxReplicas < autoscaling.minReplicas. Reference the
autoscaling object (autoscaling.enabled, autoscaling.minReplicas,
autoscaling.maxReplicas) and router.resources.requests.cpu in both the schema
and template guards.
- Around line 23-35: Add a Helm render-time validation that errors when no
worker discovery source is configured: create a template (e.g.,
templates/validate-worker-discovery.yaml) that checks .Values.workerUrls (empty
list) and .Values.serviceDiscovery.enabled (false) and calls the Helm fail
function with a clear message if both indicate “no workers” (i.e., workerUrls is
empty AND serviceDiscovery.enabled is false); reference .Values.workerUrls and
.Values.serviceDiscovery.enabled in the conditional so the chart render/install
fails fast unless one of them is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7bc4ff3-55da-4bc6-b5d1-45b2022fd670
📒 Files selected for processing (4)
deploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/values.schema.jsondeploy/helm/smg/values.yaml
…ble probes - Rename --eviction-interval to --eviction-interval-secs (Python CLI name) - Default service discovery namespace to .Release.Namespace to match the namespaced Role RBAC (avoids cluster-wide watch permission errors) - Remove --enable-wasm flag (Python CLI only supports --storage-hook-wasm-path) - Restrict policy enum in schema to Python CLI choices: cache_aware, round_robin, power_of_two, manual, random - Add router.securityContext with secure defaults (readOnlyRootFilesystem, runAsNonRoot, allowPrivilegeEscalation: false) - Make liveness/readiness probe timing configurable via values - Expand values.schema.json with port ranges, autoscaling, pool sizes Signed-off-by: Simon Lin <simon@lightseek.org> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
This was referenced Mar 21, 2026
This was referenced Mar 30, 2026
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.
Summary
Adds a production-ready Helm chart for deploying the SMG router. This initial release supports router-only mode — connecting to pre-existing inference workers via explicit URLs or Kubernetes service discovery. Worker and PD disaggregation templates will be added in a future release.
Closes #XXX
What changed
New files (
deploy/helm/smg/)smg.routerArgswhich assembles the full CLI args from valuesserviceDiscovery.enabled=true)Design docs
docs/plans/2026-03-09-helm-chart-design.md— architecture and values designdocs/plans/2026-03-09-helm-chart-plan.md— implementation planWhy
SMG currently has no official deployment packaging. A Helm chart provides:
helm install smg deploy/helm/smg --set ...)How
_helpers.tplfrom structured values, keeping the Deployment template clean*.enabledflags and render nothing by defaultintconversion to prevent Go template scientific notationhelm lint/helm installtimeTest plan
helm lint deploy/helm/smg/passes (0 failures)helm templaterenders correctly with default valuesmodevaluesserviceDiscovery.enabled=trueSummary by CodeRabbit
New Features
Documentation
Tests
Chores