Skip to content

refactor: consistent config flags for transport implementations - #378

Merged
shimib merged 5 commits into
llm-d:mainfrom
evacchi:refactor-config
Aug 3, 2026
Merged

refactor: consistent config flags for transport implementations#378
shimib merged 5 commits into
llm-d:mainfrom
evacchi:refactor-config

Conversation

@evacchi

@evacchi evacchi commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Reorganizes backend/transport configuration around a single transport abstraction: each backend is now selected with --transport and configured from one JSON document supplied via --transport-config (inline) or --transport-config-file (file). This replaces the previous sprawl of per-backend CLI flags (--redis.*, --redis.ss.*, --pubsub.*, --message-queue-impl).

Key changes:

  • New transport-config surface
    • --transportredis-pubsub | redis-sortedset | gcp-pubsub (default redis-pubsub).
    • --transport-config / --transport-config-file — inline or file JSON, mutually exclusive, one required when using the new path.
    • Backends parse their config via redis.LoadPubSubConfig, redis.LoadSortedSetConfig, pubsub.LoadConfig (each with ApplyDefaults/Validate/env overrides), and expose new constructor signatures:
      • NewRedisMQFlow(cfg, workerPools)
      • NewRedisSortedSetFlow(cfg, workerPools, gateFactory)
      • NewGCPPubSubMQFlow(cfg, workerPools, gateFactory)
    • runner.go's loadFlow selects by transport type and passes config bytes.
  • Backwards compatibility (pkg/server/compat.go)
    • Every legacy flag stays registered and functional; a logr deprecation warning names the replacement when one is used.
    • Legacy flags are translated into the equivalent transport JSON (synthesizeTransportConfig), including single-queue fallback and --redis-tracingenable_tracing.
    • New flags win when both are set (the legacy ones are then reported as ignored).
    • The retired gcp-pubsub-gated implementation is accepted via --message-queue-impl and normalized to gcp-pubsub with per-topic gate_type.
  • Flag naming consistency
    • Added --request-merge-policy-config-file (matching --transport-config-file); the older --request-merge-policy-config is kept as a deprecated alias. Precedence resolved by Options.mergePolicyConfigFile().
  • Docs
    • README.md: new Transport Configuration section with per-backend JSON schemas; per-backend flag sections marked deprecated with alias→field mappings; merge-policy and redis-tracing references updated.

Preserved from main (not regressed): module path github.com/llm-d/llm-d-async; Valkey support; EPP budget cascade / gate factory; plugin-based merge policy; redis tracing; backlog polling; and all newer sorted-set/pubsub runtime behavior (cancellation, retry-reservation release #311, gate-closed accounting #368, GateOwner stamping #369, structured results, pubsub health probing). Only the config/constructor surface was swapped; runtime logic is main's.

Why is this change needed?

The per-backend flag surface had grown large and duplicative, with many mutually-exclusive flags per backend. A single JSON config per transport is easier to document, template in Helm, and extend, and it unifies single-queue and multi-queue configuration. All previous flags are retained and still work, so existing deployments keep functioning while migrating.

How was this tested?

  • Unit tests added/updated
  • Integration/e2e tests added/updated
  • Manual testing performed

Details:

  • make build and make test pass across all modules (root, api/, pipeline/, producer/); go fmt/go vet clean.
  • Ported the four backend flow tests to the new constructors (keeping main's added tests) and added pkg/server/compat_test.go covering config synthesis, new-vs-legacy precedence, gated-alias normalization, deprecation warnings, and the merge-policy resolver.
  • go vet -tags integration ./test/integration/ compiles.
  • Manual smoke: --help shows both surfaces; validation paths verified for new-path (--transport-config required), legacy-path (--pubsub.project-id required), and invalid --transport.

Checklist

  • Commits are signed off (git commit -s) per DCO
  • Code follows project contributing guidelines
  • Tests pass locally (make test)
  • Linters pass (make lint)
  • Documentation updated (if applicable)

Related Issues

Follow up to #283, #287, #295
Related #284

Release note

Breaking: Add a unified transport-config surface: select a backend with `--transport`
(`redis-pubsub` | `redis-sortedset` | `gcp-pubsub`) and configure it from one
JSON document supplied via `--transport-config` (inline) or
`--transport-config-file` (file). The previous per-backend flags
(`--message-queue-impl`, `--redis.*`, `--redis.ss.*`, `--pubsub.*`,
`--redis-tracing`) are translated internally and still work, logging a
deprecation warning that names the replacement; `--request-merge-policy-config`
is likewise superseded by `--request-merge-policy-config-file`. **Deprecated:**
these legacy flags will be removed no earlier than `v0.10.0`.

Behavior notes for the legacy path:

- **Breaking (legacy path):** for the Redis transports, `REDIS_URL` is now a
  fallback default for `url` rather than an override — an explicit `--redis.url`
  (or inline `url` in `--transport-config`) wins when both are set. The Helm
  chart, which injects `REDIS_URL` and never sets `url`, is unaffected.
- `--message-queue-impl=gcp-pubsub` (the plain, ungated alias) stays ungated: a
  `gate_type` in its topics-config file remains inert, as before. Per-topic
  gating is available on the new `--transport gcp-pubsub` surface via
  `gate_type`/`gate_params`.
- The `redis-sortedset` single-queue retry fallback now uses the first
  configured queue's `queue_name` (previously `--redis.ss.request-queue-name`).
  In multi-queue configs the fallback is order-dependent on the `queues` array.


Deprecation window: 2 releases from now.

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 30, 2026 17:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors backend/transport configuration around a single --transport selection plus a single JSON transport config provided via --transport-config or --transport-config-file, while preserving legacy per-backend flags via a compatibility shim.

Changes:

  • Introduces transport-scoped CLI flags and config resolution (--transport, --transport-config, --transport-config-file) and updates runner flow selection to load typed transport configs.
  • Adds a backwards-compat translation layer that keeps legacy flags working while emitting deprecation warnings.
  • Updates Redis/PubSub flow constructors and tests to accept parsed transport config structs instead of per-backend flag option structs, and documents the new config surface.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/integration/redisimpl_test.go Updates integration tests to construct flows via new typed Redis transport config structs.
README.md Documents the new transport config JSON surface and marks legacy flags as deprecated.
pkg/server/runner.go Switches flow selection to --transport + transport config loaders; adds deprecated-flag warnings.
pkg/server/options.go Adds TransportOptions, new flags, and resolver helpers for new-vs-legacy precedence.
pkg/server/config.go Adds helper to load transport config bytes from inline JSON or file.
pkg/server/compat.go Implements legacy-flag deprecation warnings and translation into transport config JSON.
pkg/server/compat_test.go Adds tests for translation, precedence, gated-alias normalization, and deprecation warnings.
pkg/redis/sortedset_impl.go Refactors sorted-set flow constructor to accept SortedSetConfig and per-queue config entries.
pkg/redis/sortedset_impl_test.go Ports sorted-set tests to the new config-driven constructors and validation paths.
pkg/redis/redisimpl.go Refactors Redis pub/sub flow constructor to accept PubSubConfig and queue config entries.
pkg/redis/redisimpl_test.go Ports Redis pub/sub tests to the new config-driven constructors.
pkg/redis/options.go Adds JSON transport config structs/loaders/defaults/validation for Redis transports; retains legacy flag structs.
pkg/pubsub/pubsubimpl.go Refactors Pub/Sub flow constructor to accept parsed pubsub.Config and workerPools/gateFactory args.
pkg/pubsub/pubsubimpl_test.go Ports Pub/Sub tests to the new config-driven constructor.
pkg/pubsub/options.go Adds JSON transport config struct/loader/defaults/validation for gcp-pubsub transport.
Comments suppressed due to low confidence (1)

pkg/redis/options.go:131

  • SortedSetConfig.Validate currently doesn't enforce url and allows negative/zero poll_interval_ms and batch_size if explicitly provided. These values feed directly into durations and loop bounds in the sorted-set flow and can result in stalled polling or other unintended behavior. Tighten validation to reject missing URL and non-positive poll interval / batch size.
func (c *SortedSetConfig) Validate() error {
	if len(c.Queues) == 0 {
		return fmt.Errorf("at least one queue must be configured")
	}
	seenID := make(map[string]bool, len(c.Queues))

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

Comment thread pkg/redis/sortedset_impl.go
Comment thread pkg/redis/options.go
Comment thread pkg/pubsub/options.go
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@shimib

shimib commented Jul 30, 2026

Copy link
Copy Markdown
Member

Nice refactor — the compat shim is thorough, tests carried over 1:1 rather than dropped, and adding --transport-config to sensitiveFlags is a good catch now that credentials live in the JSON. Built and ran unit + integration tests and golangci-lint on the branch, all green.

Two things I'd want addressed before merge, both on the legacy path:

1. REDIS_URL now beats an explicit --redis.url. ApplyEnvOverrides() runs on every load,
including the shim's synthesized config, and overwrites unconditionally. On main the env only
seeds the flag's default, so the flag wins. Confirmed locally:

synthesized JSON: {"url":"redis://from-flag:6379", ...}
effective URL: redis://from-env:6379

The chart is unaffected today (it injects REDIS_URL and never passes --redis.url), but the
same inversion means an inline url in --transport-config is silently ignored whenever REDIS_URL is set — which is exactly the chart's environment, so it's a trap for the chart's own migration to the new surface. Suggest if c.URL == "" { c.URL = os.Getenv("REDIS_URL") } so it's a default rather than an override. README:150 documents it as an override, so if that's deliberate it at least wants a test.

2. Legacy --message-queue-impl=gcp-pubsub (ungated) now gates. loadFlow passes
gateFactory unconditionally, and pubsubimpl.go:145 builds a gate when gateFactory != nil && cfg.GateType != "". On main the plain impl passed no factory, so gate_type in a topics file was inert and every topic got ConstOpenGate(). Same config now starts throttling. (The gated alias itself is fine — its single-topic fallback never set gate_type.) TestResolveTransport_NormalizesGatedAlias only asserts the type string, so this isn't covered either way. Either pass a nil factory for the plain legacy impl, or call the change out explicitly in the release note.

Missing release-note fragment. The body has a non-NONE release-note block but there's no
release-notes.d/unreleased/378.mdrelease-notes-lint only validates fragments that exist, so
CI won't catch it. Related: what's the removal window for the legacy flags? Neither the PR nor the
README commits to a release, and the fragment should say.

Chart follow-up (not blocking). charts/llm-d-async/templates/ap-deployments.yaml:48+ still
emits every deprecated flag, so a default install logs ~8 deprecation warnings per pod start about
flags the operator doesn't control. Worth an issue — tests/deployment_test.yaml asserts those
args, so the two move together.

Nits:

  • sortedset_impl.go:137: defaultRequestQueueName moves from --redis.ss.request-queue-name to
    Queues[0].QueueName. Reasonable fix (the old default rarely matched a real queue in multi-queue
    setups) and it's tested, but it's order-dependent — reordering the array moves the retry
    fallback. Worth a release-note line.
  • WarnDeprecatedFlags is exported but only called from runner.go in the same package; same for
    ApplyDefaults/ApplyEnvOverrides/Validate, which only their own LoadX calls. Per
    CLAUDE.md's YAGNI rule these can be unexported.

@evacchi

evacchi commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Related: what's the removal window for the legacy flags? Neither the PR nor the
README commits to a release, and the fragment should say.

IIRC the time frame usually is +2 releases 🤔

evacchi added 2 commits July 31, 2026 19:13
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi

evacchi commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Chart follow-up (not blocking).

yes let's create an issue for that

EDIT: PR #388

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@shimib
shimib merged commit 450943e into llm-d:main Aug 3, 2026
8 checks passed
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.

3 participants