Skip to content

fix(examples): wire online kv-events ZMQ subscriber#674

Open
Iceber wants to merge 1 commit into
llm-d:mainfrom
Iceber:fix-online-example
Open

fix(examples): wire online kv-events ZMQ subscriber#674
Iceber wants to merge 1 commit into
llm-d:mainfrom
Iceber:fix-online-example

Conversation

@Iceber

@Iceber Iceber commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the online KV events example used by the Helm kv-cache-manager deployment

Previously, the online manager started the KV events pool but never created a ZMQ subscriber, so vLLM KV cache events published to ZMQ_ENDPOINT were not ingested. The manager could still serve HTTP scoring endpoints, but its KV block index would not be updated from runtime events.

The fix wires SubscriberManager into the online startup path:

  • reads the KV events config once in run
  • starts the events pool
  • creates a static ZMQ subscriber for ZMQ_ENDPOINT / ZMQ_TOPIC
  • shuts down the subscriber manager and pool during graceful shutdown

Copilot AI review requested due to automatic review settings June 17, 2026 02:08
@github-actions

Copy link
Copy Markdown

Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

@github-actions github-actions Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2026

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds explicit setup/teardown for a static ZMQ subscriber alongside the existing KV events pool, making subscription management more explicit in the online KV events example.

Changes:

  • Pass a shared kvevents.Config into setupEventsPool rather than creating it inside the helper.
  • Introduce setupEventsSubscriber to create/manage a static ZMQ subscriber via kvevents.SubscriberManager.
  • Ensure shutdown now includes subscriber manager shutdown before shutting down the events pool.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread examples/kv_events/online/main.go Outdated
Comment on lines +253 to +257
subscriberManager := kvevents.NewSubscriberManager(pool)
if cfg.ZMQEndpoint != "" && !cfg.DiscoverPods {
if err := subscriberManager.EnsureSubscriber(ctx, staticSubscriberID, cfg.ZMQEndpoint, cfg.TopicFilter, false); err != nil {
return nil, fmt.Errorf("failed to create static ZMQ subscriber: %w", err)
}
Comment thread examples/kv_events/online/main.go Outdated
@Iceber Iceber force-pushed the fix-online-example branch 2 times, most recently from 86fe4e7 to b0364be Compare June 17, 2026 02:10
Signed-off-by: Iceber Gu <caiwei95@hotmail.com>
@Iceber Iceber force-pushed the fix-online-example branch from b0364be to da60b37 Compare June 17, 2026 02:21
@yankay

yankay commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Thanks for fixing this. The subscriber wiring matches the Helm kv-cache-manager path, and CI is green.
/lgtm

@github-actions github-actions Bot added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Looks good to me, indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants