feat: integrate cache_salt with KV routing for multi-tenant isolation [DYN-3178] - #8197
Conversation
fe58035 to
012617b
Compare
Cleanup-only — no behavior change. Strips review-tracker noise that accumulated on top of PR-added text during iteration: - "Closes hhzhang16 HH-19/HH-21/HH-22/HH-23/HH-25/HH-26/HH-27" - "CR-8 / CR-9 / CR-10 closure" prefixes on serde-error / doc-attach fixes - Branch-name references: bis/parity-tokenize-tcp, bis/prime-rl-merged - Internal PR numbers: #6094, #7699, #8197, #9141 - Phase numbers from internal design docs (rl-support.md Phase 1/4/5) - "prime-rl" mentions in narrative copy and mermaid diagrams → generic "RL trainer / RL orchestrator / external client" Technical content (semantics, invariants, why-this-exists rationale) preserved everywhere; only the internal-process scaffolding is removed. Scope verification: every removed line is one this branch ADDED (diff main..HEAD shows the removed text on a `+` line). No edits land on pre-existing main-branch comments. Specifically reverted the nvext.rs cleanup attempt — its target lines (GAIE Stage 1/2, SGLang-specific) live on main, not in this PR's diff. Files touched: components/src/dynamo/vllm/handlers.py +9 -10 components/src/dynamo/vllm/worker_factory.py +6 -4 docs/dynamo-RL-api.md +19 -32 lib/llm/src/http/service/openai.rs +32 -34 lib/llm/src/protocols/openai/chat_completions/delta.rs +4 -4 lib/llm/src/protocols/openai/completions/delta.rs +3 -3 lib/llm/src/protocols/openai/validate.rs +20 -20 cargo check -p dynamo-llm: clean (1 pre-existing benign warning).
e151196 to
3e82659
Compare
3e82659 to
178ed4b
Compare
b9fa2c2 to
755794f
Compare
|
/ok to test 755794f |
|
/ok to test 2be3455 |
jthomson04
left a comment
There was a problem hiding this comment.
A few non-correctness cleanup/plumbing observations (all verified — no correctness bugs surfaced). Two are inline below; the other two are on files this PR doesn't modify, so I couldn't inline-anchor them:
components/src/dynamo/router/__main__.py (get_overlap_scores ~L174, best_worker_id ~L156) — the standalone router daemon's diagnostic endpoints call kv_router.get_overlap_scores(...) / best_worker(...) positionally and don't forward the new cache_namespace kwarg, so overlap-score queries on that path report cross-tenant overlap. The main generate path is unaffected (it forwards routing through to generate_from_request, which is salt-aware). Minor, but worth plumbing for consistency.
lib/kv-hashing/tests/request_hashing.rs::producer_block_hash_matches_router_local_block_hash_lora — this test still asserts the old LoRA seed formula and its comment claims parity with compute_block_hash_for_seq's LoRA path, which this PR changed (xxh3_64(name) → xxh3_64_with_seed(name, LORA_HASH_SEED)). It still passes (kv-hashing's salt.rs governs a separate PLH/dedup hash, not the radix-tree match key, and wasn't touched), so there's no functional bug — but the comment is now misleading and worth a quick update.
|
/ok to test 1731da0 |
|
/ok to test 3f6acac |
jthomson04
left a comment
There was a problem hiding this comment.
Ran a multi-agent review over this PR (8 finder angles, each candidate adversarially verified against the branch). The core design looks solid — unsalted hashes are preserved bit-for-bit, precedence is well documented and tested, and the standalone-indexer contract is explicit. 7 findings are attached as inline comments (most severe: the LoRA seed change and the SGLang gap). One more finding lands on a file this PR doesn't touch, so it goes here:
lib/llm/src/kv_router/push_router.rs:205-215 — fallback routing-decision recording omits with_cache_namespace. The fallback builds TokensWithHashes::new(...) and applies .with_lora_name(...) but never .with_cache_namespace(request.routing.cache_namespace), while the parallel fallback in lib/bindings/python/rust/llm/kv.rs:1784-1787 was updated in this PR. The branch appears currently unreachable (records_routing_decisions() implies routing_hashes is Some), but as written it records salted requests under unsalted hashes the moment anything makes it live — a one-line fix to match the Python-binding path.
Also noting a cross-cutting theme rather than filing it per-line: salt-resolution precedence now exists in three drifting copies (Rust extensions.rs, Python trtllm request_utils.py, vLLM _iter_nvext_sources), and the "empty string means absent" rule is re-applied ad hoc at 5+ sites. Two of the inline findings are symptoms of that; normalizing once at the ingestion boundaries would eliminate the class.
For transparency, candidates that were checked and refuted during verification: /query_by_hash rejecting "" (intentional, documented, and tested), retained Ambiguous entries leaking (requires a hash collision to matter, deliberate fail-closed), vLLM missing routing.cache_salt (frontend always mirrors the salt into extra_args.nvext), and per-block seed re-hashing (negligible vs. existing per-block work).
🤖 Generated with Claude Code
|
/ok to test a5dcb50 |
PeaBrane
left a comment
There was a problem hiding this comment.
The direct vLLM/TRT-LLM request and event plumbing looks coherent. I found three remaining cross-component concerns below: the consolidator drops the namespace, the canonical hashing helper derives a different salted key, and namespace propagation adds per-block string allocations. One minor follow-up outside the diff: the standalone router best_worker_id and get_overlap_scores diagnostic/custom-scheduling endpoints still do not accept or forward cache_namespace, so external schedulers using those endpoints cannot request namespace-aware results. Leaving this as a neutral comment rather than an approval or request-changes verdict.
|
/ok to test 40c8b0f |
|
/ok to test fa59951 |
PeaBrane
left a comment
There was a problem hiding this comment.
CI note: the vLLM amd64 failure is actionable—test_generate_text_mode_applies_nvext_cache_salt still expects tenant-a, while the implementation now intentionally forwards dynamo-cache-salt:tenant-a to vLLM for unambiguous KV-event decoding. The TRT-LLM shard completed 117 passed / 1 skipped, then its container action failed near the 30-minute boundary during teardown/reporting, which looks like a runner timeout rather than a test failure. backend-status-check is the downstream aggregate failure.
|
/ok to test 893e7d8 |
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Overview:
Adds cache-salt-aware KV routing and reuse isolation across Dynamo request ingestion, block hashing, router selection, bindings, standalone indexing, vLLM, and TensorRT-LLM KV events.
This work is tracked internally as DYN-3178. It is backward compatible for unsalted requests: when no non-empty salt is supplied, Dynamo retains the existing hash and cache-reuse behavior.
Details:
x-tenant-idgateway header.nvext.cache_salt(recommended request API).cache_saltcompatibility field.dynamo-kv-hashing.TokensPromptinputs. Dynamo uses an internal tag while the salt is inside vLLM's untypedextra_keys, then removes it in the Rust event decoder so arbitrary public values, including 64-character hex salts and salts equal to a LoRA name, round-trip without ambiguity./querycomputes salted hashes from token IDs./query_by_hashrequires hashes that were already computed with the intended salt and returns400 Bad Requestfor any non-nullcache_salt..github/workflows/pr.yamlchange; the operator-gating fix is already on main.The TensorRT-LLM dependency is satisfied: NVIDIA/TensorRT-LLM#13051 is merged, and current Dynamo main pins TensorRT-LLM
1.3.0rc19.Validation completed:
cargo fmt --all -- --checkcargo test -p dynamo-llmcargo test -p dynamo-kv-router(615 tests passed on the review-fix head)cargo test -p dynamo-kv-router --features standalone-indexer --test standalone_indexer_http query_cargo test -p dynamo-ext-proc(10 tests passed on the review-fix head)cargo test -p libdynamo_llmmake dynamo-libunderdeploy/inference-gateway/eppmake testunderdeploy/inference-gateway/epp, using the repository-pinnedgolang:1.26.3builder image because the host has no Go toolchainpytest -q components/src/dynamo/trtllm/tests/test_trtllm_request_utils.py components/src/dynamo/trtllm/tests/test_trtllm_fpm_publisher.py(48 tests passed)fern checkfern docs broken-linkspre-commit run --fileson all review-fix filespre_merge and vllm and gpu_1selectstest_unified_vllm_cache_salt_isolation[tcp].The focused vLLM unit and real-engine E2E require the vLLM CI image and GPU lane; the local environment does not have vLLM installed.
Where should the reviewer start?
lib/llm/src/protocols/common/extensions.rsfor request input precedence and empty-value handling.lib/kv-router/src/protocols.rsandlib/llm/src/kv_router.rsfor salted hashing and routing propagation.components/src/dynamo/vllm/handlers.py,components/src/dynamo/vllm/llm_engine.py, andlib/kv-router/src/zmq_wire/extra_keys.rsfor vLLM request and event round-trip handling.components/src/dynamo/vllm/llm_engine.pyplustest_unified_vllm_cache_salt_isolationfor unified vLLM E2E coverage.components/src/dynamo/trtllm/utils/request_utils.pyandcomponents/src/dynamo/trtllm/publisher.pyfor TensorRT-LLM request/event handling and malformed-event resilience.lib/kv-router/src/services/indexer/server.rspluslib/kv-router/tests/standalone_indexer_http.rsfor the standalone API behavior.lib/bindings/c/src/lib.rsanddeploy/inference-gateway/ext-proc/src/epp.rsfor the shared helper consumers.Related Issues
🚫 This PR is NOT linked to an issue:
Internal tracking: DYN-3178.