Repository navigation
feat(helm): add mesh HA support with K8s service discovery - #972
Conversation
📝 WalkthroughWalkthroughAdds optional router mesh support to the SMG Helm chart: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant HeadlessSvc as Headless_Service
participant PeerPod as Router_Peer_Pod
participant Worker
User->>Router: Send inference request
Router->>PeerPod: (mesh enabled) establish mesh connection using POD_IP/POD_NAME and mesh port
Router->>HeadlessSvc: Resolve peers (DNS) or use configured peerUrls / service-discovery
Router->>Worker: Forward request to worker (http connectionMode)
Worker-->>Router: Response
Router-->>User: Return inference result
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Mesh High Availability (HA) support for the SMG gateway, enabling multi-gateway synchronization of routing state. Key changes include the conditional conversion of the router Deployment to a StatefulSet, the addition of a headless service, and the implementation of mesh-specific CLI arguments and RBAC rules. Feedback highlights the need to scope the router selector to the specific Helm release to prevent cross-release interference, avoid duplicate CLI flags when multiple discovery modes are active, and clarify documentation regarding peer URL configuration.
![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: e9e1dc0d14
ℹ️ 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".
| in --mesh-peer-urls. */}} | ||
| - "--service-discovery" | ||
| - "--selector" | ||
| - "app.kubernetes.io/instance={{ .Release.Name }},app.kubernetes.io/component=worker" |
There was a problem hiding this comment.
When mesh is enabled without peerUrls, this template emits --selector as a single comma-joined string (app.kubernetes.io/instance=...,app.kubernetes.io/component=worker). Both selector parsers (model_gateway/src/main.rs::parse_selector and bindings/python/src/smg/router_args.py::_parse_selector) split each argument only on the first =, so this becomes one malformed key/value pair and no worker pods match; in that mode the chart then relies on service discovery, so routers can come up with zero discovered backends.
Useful? React with 👍 / 👎.
| @@ -1,5 +1,9 @@ | |||
| apiVersion: apps/v1 | |||
| {{- if .Values.router.mesh.enabled }} | |||
| kind: StatefulSet | |||
There was a problem hiding this comment.
This change allows the router workload to become a StatefulSet in mesh mode, but the HPA template still targets kind: Deployment (deploy/helm/smg/templates/hpa-router.yaml). If users enable both router.mesh.enabled and router.autoscaling.enabled, the autoscaler points to a non-existent scale target and scaling will not function.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/examples/with-mesh-ha.yaml`:
- Around line 27-28: Update the inline comment next to the healthCheck.enabled
setting to clarify this is a temporary, deployment-time workaround and provide
guidance to re-enable health checks after workers are healthy; specifically edit
the line with the YAML key "healthCheck.enabled" (and its comment "disable until
workers are ready") to state whether this example is for initial bootstrap only,
how to detect when to turn it back on (e.g., when worker pods reach Ready), or
point to a doc or helm value (e.g., health checks should be enabled for
production) so users copying the example won't leave health checks permanently
disabled.
In `@deploy/helm/smg/templates/_helpers.tpl`:
- Line 116: Add a brief explanatory comment above the complex conditional that
starts with "{{- else if and .Values.workers (not
.Values.router.serviceDiscovery.enabled) (not (and .Values.router.mesh.enabled
(not .Values.router.mesh.peerUrls))) }}" describing why auto-generated
"--worker-urls" is excluded (i.e., only include workers when serviceDiscovery is
disabled and mesh is either disabled or has peerUrls configured), and reference
the relevant values (.Values.workers, .Values.router.serviceDiscovery.enabled,
.Values.router.mesh.enabled, .Values.router.mesh.peerUrls) to make the intent
clear to future readers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5facda4a-af80-49df-aa17-047a2e3e5ce7
📒 Files selected for processing (7)
deploy/helm/smg/examples/with-mesh-ha.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/role.yamldeploy/helm/smg/templates/rolebinding.yamldeploy/helm/smg/templates/service-router-headless.yamldeploy/helm/smg/values.yaml
Add mesh synchronization support to the helm chart for multi-gateway HA deployments. When router.mesh.enabled=true: - Router Deployment converts to StatefulSet (stable pod names) - Headless Service created for mesh gossip port - POD_NAME and POD_IP injected as env vars - Mesh server binds to POD_IP (not 0.0.0.0) since --mesh-advertise-host is not available in the 1.3.3 image - Service discovery auto-discovers peer router pods via --router-selector matching app.kubernetes.io/component=router - Pod annotation sglang.ai/mesh-port set for peer port discovery - RBAC auto-created for pod list/watch - Workers discovered via --selector (service discovery mode) Note: --mesh-peer-urls requires raw IP:port (SocketAddr), not DNS hostnames. This makes static peer URLs unusable in K8s where pod IPs change. Filed #970 for DNS hostname support. The workaround uses K8s service discovery instead of static peer URLs. Tested on K8s cluster (moirai-eu-frankfurt-1-dev): - 2 router replicas with mesh enabled - Both routers discovered each other via service discovery - Gossip protocol running, nodes in Alive state - Worker discovered and registered via service discovery Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
e9e1dc0 to
c72ea6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c72ea6e7d3
ℹ️ 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".
| - "app.kubernetes.io/instance={{ .Release.Name }}" | ||
| - "app.kubernetes.io/component=worker" | ||
| - "--router-selector" | ||
| - "app.kubernetes.io/component=router" |
There was a problem hiding this comment.
When router.mesh.enabled=true and peerUrls is unset, the chart sets --router-selector to only app.kubernetes.io/component=router. In namespaces with multiple SMG releases, this makes each router discover and join pods from other releases, so mesh state is mixed across independent deployments. Add the release label (for example app.kubernetes.io/instance={{ .Release.Name }}) to the router selector so mesh membership stays isolated per install.
Useful? React with 👍 / 👎.
| {{- $firstWorker := index .Values.workers 0 }} | ||
| {{- $workerDefaults := index .Values.engineDefaults $firstWorker.engine }} | ||
| - "--service-discovery-port" | ||
| - {{ $firstWorker.port | default $workerDefaults.port | quote }} |
There was a problem hiding this comment.
In mesh auto-discovery mode, --service-discovery-port is taken from workers[0] and applied to every discovered pod. If the release contains workers on different ports (for example mixed engines or explicit per-worker port overrides), routers will generate wrong URLs for non-first workers and fail to reach them. This should either enforce a single shared port or avoid collapsing all workers to the first worker’s port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
deploy/helm/smg/examples/with-mesh-ha.yaml (1)
27-28:⚠️ Potential issue | 🟡 MinorClarify that disabling health checks is bootstrap-only.
Line 28 reads like a recommended steady-state setting. Users copying this example may leave router health checks disabled in production after worker pods are Ready.
📝 Suggested wording
healthCheck: - enabled: false # disable until workers are ready + enabled: false # optional bootstrap workaround; re-enable once worker pods are Ready🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/helm/smg/examples/with-mesh-ha.yaml` around lines 27 - 28, The example currently sets healthCheck.enabled: false with a comment that can be misread as a recommended steady-state setting; update the inline comment next to healthCheck.enabled (the healthCheck: enabled: false block) to clearly state this is bootstrap-only and must be re-enabled in production once worker pods are Ready (e.g., "disable during bootstrap until workers are Ready — re-enable for production/steady-state"); optionally add a brief one-line follow-up comment suggesting how to re-enable (set healthCheck.enabled: true) after rollout.
🤖 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 110-114: The template currently assumes a single port/scheme by
using $firstWorker and emitting a lone "--service-discovery-port", which breaks
mixed-worker/GRPC configs; update the template to either (A) iterate
.Values.workers and emit per-worker flags (preserve generation of
"--worker-urls" for each entry using each worker's port, scheme and engine via
.Values.engineDefaults) so each pod is probed with its own URL, or (B) add a
guard/validation that checks all entries in .Values.workers are homogeneous
(same engine, port, scheme) and fail-fast with a clear message if not; reference
$firstWorker, $workerDefaults, .Values.workers, "--service-discovery-port" and
"--worker-urls" when making the change.
- Around line 102-107: The --router-selector currently only matches
app.kubernetes.io/component=router which lets routers from different releases
join the same mesh; update the --router-selector value in the template to
include the release-scoped label app.kubernetes.io/instance={{ .Release.Name }}
(or the full set of labels used for the worker selector) so the router discovery
is limited to this release's pods; modify the entries around "--router-selector"
in templates/_helpers.tpl to use the same instance-scoped selector format as the
worker selector.
In `@deploy/helm/smg/templates/deployment-router.yaml`:
- Around line 2-6: The HPA's scaleTargetRef in
deploy/helm/smg/templates/hpa-router.yaml must match the workload kind chosen by
router.mesh.enabled; update hpa-router.yaml so the scaleTargetRef.kind is
rendered conditionally (use the same condition as in deployment-router.yaml: if
.Values.router.mesh.enabled then "StatefulSet" else "Deployment") and ensure the
scaleTargetRef.name and selector still target the router release; also ensure
this conditional is only applied when .Values.router.autoscaling.enabled is true
so HPA is only created when autoscaling is enabled.
In `@deploy/helm/smg/values.yaml`:
- Around line 26-32: Update the mesh.peerUrls comment to accurately describe
behavior: note that when mesh.peerUrls is empty the helper in
deploy/helm/smg/templates/_helpers.tpl falls back to service-discovery via
--router-selector (because --mesh-peer-urls cannot accept DNS hostnames), not
auto-generating peerUrls from StatefulSet DNS; change the documentation line to
explain that to use explicit peer URLs set mesh.peerUrls, otherwise the chart
will use --router-selector service discovery.
---
Duplicate comments:
In `@deploy/helm/smg/examples/with-mesh-ha.yaml`:
- Around line 27-28: The example currently sets healthCheck.enabled: false with
a comment that can be misread as a recommended steady-state setting; update the
inline comment next to healthCheck.enabled (the healthCheck: enabled: false
block) to clearly state this is bootstrap-only and must be re-enabled in
production once worker pods are Ready (e.g., "disable during bootstrap until
workers are Ready — re-enable for production/steady-state"); optionally add a
brief one-line follow-up comment suggesting how to re-enable (set
healthCheck.enabled: true) after rollout.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67c2c3d5-af1d-4ae7-b506-7d3103c91b15
📒 Files selected for processing (7)
deploy/helm/smg/examples/with-mesh-ha.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/role.yamldeploy/helm/smg/templates/rolebinding.yamldeploy/helm/smg/templates/service-router-headless.yamldeploy/helm/smg/values.yaml
| {{- if .Values.router.mesh.enabled }} | ||
| kind: StatefulSet | ||
| {{- else }} | ||
| kind: Deployment | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check deployment-router.yaml to verify the conditional kind logic
cat -n deploy/helm/smg/templates/deployment-router.yaml | head -25Repository: lightseekorg/smg
Length of output: 914
🏁 Script executed:
# Check hpa-router.yaml to verify the hardcoded kind
cat -n deploy/helm/smg/templates/hpa-router.yamlRepository: lightseekorg/smg
Length of output: 879
Keep autoscaling aligned with the mesh StatefulSet.
When router.mesh.enabled switches the workload to StatefulSet (lines 2-6), deploy/helm/smg/templates/hpa-router.yaml still hardcodes kind: Deployment in its scaleTargetRef. Enabling both router.mesh.enabled and router.autoscaling.enabled will render an HPA targeting a nonexistent Deployment resource. Update hpa-router.yaml to conditionally match the workload kind:
Fix in deploy/helm/smg/templates/hpa-router.yaml
spec:
scaleTargetRef:
apiVersion: apps/v1
- kind: Deployment
+ kind: {{ ternary "StatefulSet" "Deployment" .Values.router.mesh.enabled }}
name: {{ include "smg.fullname" . }}-router🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 3-3: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/smg/templates/deployment-router.yaml` around lines 2 - 6, The
HPA's scaleTargetRef in deploy/helm/smg/templates/hpa-router.yaml must match the
workload kind chosen by router.mesh.enabled; update hpa-router.yaml so the
scaleTargetRef.kind is rendered conditionally (use the same condition as in
deployment-router.yaml: if .Values.router.mesh.enabled then "StatefulSet" else
"Deployment") and ensure the scaleTargetRef.name and selector still target the
router release; also ensure this conditional is only applied when
.Values.router.autoscaling.enabled is true so HPA is only created when
autoscaling is enabled.
| # -- Mesh HA (multi-gateway synchronization) | ||
| # Requires replicas > 1. Converts Deployment to StatefulSet for | ||
| # stable pod names used in mesh peer discovery. | ||
| mesh: | ||
| enabled: false | ||
| port: 39527 # mesh gossip port | ||
| # peerUrls: [] # auto-generated from StatefulSet pod DNS |
There was a problem hiding this comment.
Fix the peerUrls documentation.
Line 32 says peerUrls is auto-generated from StatefulSet DNS, but deploy/helm/smg/templates/_helpers.tpl does the opposite: when peerUrls is empty it falls back to --router-selector service discovery because --mesh-peer-urls cannot use DNS hostnames. The current comment will mislead users configuring mesh overrides.
📝 Suggested doc update
# -- Mesh HA (multi-gateway synchronization)
- # Requires replicas > 1. Converts Deployment to StatefulSet for
- # stable pod names used in mesh peer discovery.
+ # Requires replicas > 1. Routers discover peers via Kubernetes
+ # service discovery unless explicit peerUrls are provided.
mesh:
enabled: false
port: 39527 # mesh gossip port
- # peerUrls: [] # auto-generated from StatefulSet pod DNS
+ # peerUrls: [] # optional explicit peer IP:port list🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/smg/values.yaml` around lines 26 - 32, Update the mesh.peerUrls
comment to accurately describe behavior: note that when mesh.peerUrls is empty
the helper in deploy/helm/smg/templates/_helpers.tpl falls back to
service-discovery via --router-selector (because --mesh-peer-urls cannot accept
DNS hostnames), not auto-generating peerUrls from StatefulSet DNS; change the
documentation line to explain that to use explicit peer URLs set mesh.peerUrls,
otherwise the chart will use --router-selector service discovery.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com> Signed-off-by: key4ng <rukeyang@gmail.com>
…ct#972) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
Add mesh HA synchronization support to the helm chart. When
router.mesh.enabled=true, the chart deploys a StatefulSet with gossip-based state sync between gateway replicas.What Changed
values.yaml: Addedrouter.meshsection (enabled, port, peerUrls)deployment-router.yaml: Converts to StatefulSet when mesh enabled. Adds mesh port, POD_NAME/POD_IP env vars, mesh-port annotation._helpers.tpl: Mesh args (--enable-mesh, --mesh-host, --mesh-port, --mesh-server-name). Uses K8s service discovery (--router-selector) for peer discovery instead of static --mesh-peer-urls (which requires raw IP:port, see bug(mesh): --mesh-peer-urls rejects DNS hostnames (requires raw IP:port) #970).service-router-headless.yaml: New headless Service for mesh gossiprole.yaml,rolebinding.yaml: Auto-create RBAC when mesh enabledexamples/with-mesh-ha.yaml: Example with 2 router replicas + meshDesign Decision: Service Discovery vs Static Peer URLs
--mesh-peer-urlsrequiresIP:portformat (parsed asSocketAddr), not DNS hostnames. This makes StatefulSet pod DNS names unusable (#970). The workaround uses K8s service discovery:--router-selector app.kubernetes.io/component=routerdiscovers peer router podssglang.ai/mesh-port: 39527tells peers which port to connect to--selectoralso set for worker discovery (required by validation)Test Plan
Tested on K8s cluster (moirai-eu-frankfurt-1-dev):
Router node smg-test-router-1 added/updated in mesh clusterChecklist
Summary by CodeRabbit
New Features
Chores