Skip to content

[Bugfix] Expose LMCache controller reply/heartbeat ports on the router Service - #1029

Open
bazakeliad wants to merge 2 commits into
vllm-project:mainfrom
bazakeliad:fix/router-service-lmcache-controller-ports
Open

[Bugfix] Expose LMCache controller reply/heartbeat ports on the router Service#1029
bazakeliad wants to merge 2 commits into
vllm-project:mainfrom
bazakeliad:fix/router-service-lmcache-controller-ports

Conversation

@bazakeliad

Copy link
Copy Markdown
Contributor

Description

The chart points the engines at a router Service port that the chart never opens, so KV-aware
routing fails silently.

Three templates disagree on main today:

Template What it does
helm/templates/deployment-router.yaml launches the router with --lmcache-controller-reply-port / --lmcache-controller-heartbeat-port from routerSpec.* — so the router listens on them — but declares only a hardcoded containerPort: 9000
helm/templates/deployment-vllm-multi.yaml sets LMCACHE_CONTROLLER_REPLY_URL={{ .Release.Name }}-router-service:{{ controllerReplyPort }} — so every engine is told to connect there
helm/templates/service-router.yaml exposes port 9000 only

Worker registration then blocks inside ZMQ with no error on either side: no exception, no log
line, and lookup() simply returns nothing — so kvaware degenerates to QPS routing while the
deployment looks healthy. It took us a while to find, because every surface reports success.

This also fixes the pull port itself: both the containerPort and the Service port were
hardcoded to 9000 and ignored routerSpec.lmcacheControllerPort, so any non-default value
pointed the Service at a port the router was not listening on.

Finally, lmcacheControllerReplyPort and lmcacheControllerHeartbeatPort were already read by
deployment-router.yaml but were undocumented in values.yaml. Both are now documented, and
values.schema.json is updated to match.

Backward compatibility

Each port renders only when its value is set, so charts that do not configure the controller
are untouched. Verified:

$ helm template test ./helm > before.yaml   # on main
$ helm template test ./helm > after.yaml    # with this PR
$ diff before.yaml after.yaml && echo IDENTICAL
IDENTICAL

With the ports configured:

routerSpec:
  routingLogic: "kvaware"
  lmcacheControllerPort: 9000
  lmcacheControllerReplyPort: 9001
  lmcacheControllerHeartbeatPort: 9002

the router Service now renders:

  ports:
    - name: "router-sport"
      port: 80
      targetPort: 8000
      protocol: TCP
    - name: "lmcache-port"
      port: 9000
      targetPort: lmcache-port
      protocol: TCP
    - name: "lmcache-reply"
      port: 9001
      targetPort: lmcache-reply
      protocol: TCP
    - name: "lmcache-hbeat"
      port: 9002
      targetPort: lmcache-hbeat
      protocol: TCP

with matching named containerPorts on the router Deployment. (lmcache-hbeat rather than
lmcache-heartbeat because Kubernetes port names are limited to 15 characters.)

How this was found

Deploying the KV-aware setup on a 2×A10 OpenShift cluster. The symptom was that kvaware
routing appeared to work but never showed cache affinity. We worked around it with
oc patch svc ... --type=json adding the two ports, which confirmed the diagnosis, and are
sending the proper fix upstream.

helm lint passes; values.schema.json regenerated to match values.yaml.


  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].

…r Service

The chart tells the engines to reach the controller on a Service port the chart
never opens, so KV-aware routing fails silently.

Three templates disagree today:

- deployment-router.yaml launches the router with --lmcache-controller-reply-port
  and --lmcache-controller-heartbeat-port from routerSpec, so the router listens
  on them, but declares only a hardcoded containerPort 9000.
- deployment-vllm-multi.yaml sets
  LMCACHE_CONTROLLER_REPLY_URL={release}-router-service:{controllerReplyPort},
  so every engine is told to connect there.
- service-router.yaml exposes 9000 only.

Registration then blocks inside ZMQ with no error on either side: no exception,
no log line, and lookups simply return nothing, so kvaware degenerates to QPS
routing while appearing healthy.

Also fixes the pull port itself: containerPort and the Service port were both
hardcoded to 9000 and ignored routerSpec.lmcacheControllerPort, so any non-default
value pointed the Service at a port the router was not listening on.

lmcacheControllerReplyPort and lmcacheControllerHeartbeatPort were already read by
deployment-router.yaml but undocumented in values.yaml; both are now documented and
present in values.schema.json.

Ports are rendered only when the corresponding value is set, so charts that do not
configure the controller are unaffected: helm template with default values produces
byte-identical output before and after this change.

Signed-off-by: Eliad Bazak <bazakeliad@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces configuration options for the LMCache controller reply and heartbeat ports in the Helm chart, enabling worker registration and re-registration. The reviewer suggests adding fail-fast template validations to ensure these ports are provided when routingLogic is set to kvaware, and updating the documentation in values.yaml and values.schema.json to clearly reflect these requirements.

Comment thread helm/templates/deployment-router.yaml
Comment thread helm/templates/deployment-router.yaml
Comment thread helm/values.yaml Outdated
Comment thread helm/values.yaml Outdated
Comment thread helm/values.schema.json
Comment thread helm/values.schema.json
Review feedback: make it explicit in values.yaml, and mirror it in
values.schema.json, that these ports matter when routingLogic is kvaware.

The reply port is documented as required - registration blocks silently without
it, which is the bug this PR fixes.

The heartbeat port is documented as strongly recommended rather than required,
because that is what it is: kvaware works without it until the router restarts,
at which point workers cannot re-register and routing degrades to QPS until the
engines restart too.

Signed-off-by: Eliad Bazak <bazakeliad@gmail.com>
@bazakeliad

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I've taken the documentation suggestions and pushed them in 24981d5values.yaml and values.schema.json now both state the kvaware requirement explicitly.

One wording change from the suggestion: I documented the heartbeat port as strongly recommended rather than required, because that is what we measured. Without it, kvaware works normally until the router restarts — at that point workers cannot re-register and routing silently degrades to QPS until the engines are restarted too. Calling it required would overstate it.

On the fail-fast validations

I'd rather not add them in this PR, for two reasons:

1. It would break existing deployments. No one sets these ports today — that is precisely the bug this PR fixes, since the chart never exposed them. A fail on routingLogic: kvaware would turn every currently-running KV-aware deployment into a failed helm upgrade. It would also invalidate the compatibility guarantee this PR is built on: with no controller ports configured, helm template output is byte-identical before and after.

2. For the heartbeat port the premise doesn't hold. It genuinely is optional, per the measurement above, so a hard requirement would be incorrect.

That said, I think the underlying instinct is right, and it is the same one behind this PR: a silent degradation is worse than a loud failure. Turning that into a validation is a breaking change and deserves to be the maintainers' decision rather than a rider on a bugfix. Happy to send it as a follow-up PR if you'd like it — possibly as a warning path, or gated behind a chart-major bump.

Current state: defaults render byte-identically, helm lint passes, and the schema is in sync with values.yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant