feat: dynamo namespace isolation for backend component #2475
Conversation
biswapanda
requested review from
nnshah1,
tanmayv25,
piotrm-nvidia,
ptarasiewiczNV,
ryanolson,
grahamking,
paulhendricks,
tmonty12,
GuanLuo,
rmccorm4,
tedzhouhk,
alec-flowers,
kkranen,
jthomson04,
ishandhanani,
PeaBrane and
a team
as code owners
WalkthroughIntroduces namespace-aware discovery and filtering across backend and frontend, removes the vLLM --endpoint CLI flag, centralizes default namespace (“global”) via DYN_NAMESPACE, adjusts watcher APIs to accept an optional target namespace, refactors HTTP discovery to a config-driven watcher with endpoint enable/disable updates, adds a new namespace module, and updates docs/tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Env/DYN_NAMESPACE
participant Frontend as Frontend CLI/Main
participant Etcd as etcd Watch
participant Watcher as ModelWatcher
participant Router as HTTP Router
participant Services as HTTP Endpoints
Env->>Frontend: Read DYN_NAMESPACE / --namespace
Frontend->>Frontend: Build endpoint_id = "<ns>.frontend.http" (default global)
Frontend->>Etcd: Start watch on MODEL_ROOT_PATH (prefix)
Frontend->>Watcher: watch(receiver, target_namespace)
Etcd-->>Watcher: Put/Delete(ModelEntry w/ namespace)
Watcher->>Watcher: Filter by target_namespace (if non-global)
Watcher-->>Frontend: ModelUpdate(Add/Remove, scoped)
Frontend->>Router: update_http_endpoints(ModelUpdate)
Router->>Services: Enable/Disable Chat/Completion/Embedding...
sequenceDiagram
participant vLLM as vLLM Worker
participant Args as args.py
participant Config as Worker Config
vLLM->>Args: Parse CLI (no --endpoint)
Args->>Args: namespace = env DYN_NAMESPACE or "global"
Args->>Args: component = prefill ? "prefill" : "backend"
Args->>Config: endpoint = "generate"
Config-->>vLLM: Initialized without endpoint parsing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
components/backends/vllm/src/dynamo/vllm/args.py (1)
108-110: Guard empty/whitespace DYN_NAMESPACE and improve startup observabilityNormalize the environment variable, defaulting to
"global"if it’s empty or only whitespace, and emit an info-level log of the resolved settings:--- a/components/backends/vllm/src/dynamo/vllm/args.py +++ b/components/backends/vllm/src/dynamo/vllm/args.py @@ -108,3 +108,7 @@ - config.namespace = os.environ.get("DYN_NAMESPACE", "global") + ns = os.environ.get("DYN_NAMESPACE", "").strip() + config.namespace = ns if ns else "global" config.component = "prefill" if args.is_prefill_worker else "backend" config.endpoint = "generate" + +# Log resolved namespace/component/endpoint for easier debugging +logger.info( + "Dynamo namespace=%s (component=%s, endpoint=%s)", + config.namespace, + config.component, + config.endpoint, +)• Confirmed that this module no longer references any
--endpointflags orDEFAULT_ENDPOINTconstants.
• Other parts of the repo continue to manage their own endpoint flags independently.[optional_refactors_recommended]
components/frontend/src/dynamo/frontend/main.py (3)
41-45: Avoid cross-language drift on the global namespace literal.Python and Rust both define "global" separately now. Consider consolidating Python’s namespace constants in a single module (e.g., dynamo.frontend.namespace or a shared Python util) to reduce duplication and drift, even if Rust remains authoritative.
125-130: Update help text to reflect env-var fallback.The help says “If not specified, discovers models from all namespaces,” but parse_args will use DYN_NAMESPACE if set. Align the help text to prevent confusion.
Apply:
- help="Dynamo namespace for model discovery scoping. If specified, models will only be discovered from this namespace. If not specified, discovers models from all namespaces (global discovery).", + help="Dynamo namespace for model discovery scoping. If set, discovery is scoped to this namespace. If omitted, uses the DYN_NAMESPACE env var when present; otherwise uses global discovery.",
165-167: Guard against empty/whitespace env var values.Strip the env var and only apply it when non-empty. Keeps semantics clear and avoids accidental empty namespace selection.
Apply:
- if not flags.namespace: - flags.namespace = os.environ.get(DYNAMO_NAMESPACE_ENV_VAR) + ns_env = os.environ.get(DYNAMO_NAMESPACE_ENV_VAR, "").strip() + if not flags.namespace and ns_env: + flags.namespace = ns_envlib/llm/src/discovery/watcher.rs (2)
161-166: Prefer structured fields over string interpolation in tracing::errorStructured fields improve log querying and avoid format overhead when the level is disabled.
Apply this diff:
- tracing::error!( - error = format!("{err:#}"), - "error adding model {} from namespace {}", - model_entry.name, - model_entry.endpoint.namespace, - ); + tracing::error!( + error = %err, + model_name = %model_entry.name, + namespace = %model_entry.endpoint.namespace, + "error adding model" + );
189-194: Don’t treat missing entries on Delete as errors when filtering is activeWhen a non-target namespace key is deleted, it was never stored due to Put filtering; handle_delete will currently bail and emit error logs. Downgrade to a debug and return Ok(None) to avoid log noise.
Apply this diff:
- let model_entry = match self.manager.remove_model_entry(key) { - Some(entry) => entry, - None => { - anyhow::bail!("Missing ModelEntry for {key}"); - } - }; + let model_entry = match self.manager.remove_model_entry(key) { + Some(entry) => entry, + None => { + tracing::debug!( + "Delete for unknown model entry key {key}; likely out-of-scope namespace" + ); + return Ok(None); + } + };lib/llm/src/entrypoint/input/http.rs (1)
199-200: Include target_namespace in watch startup logThis improves observability when diagnosing discovery scope issues.
Apply this diff:
- tracing::info!("Watching for remote model at {}", config.network_prefix); + tracing::info!( + target_namespace = ?config.target_namespace, + "Watching for remote model at {}", + config.network_prefix + );lib/llm/tests/namespace.rs (1)
49-67: DRY opportunity for create_test_model_entry across test modulesBoth this file and http_namespace_integration.rs implement the same helper. Consider moving it into a small tests/common module to reduce duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
components/backends/vllm/README.md(0 hunks)components/backends/vllm/deploy/README.md(0 hunks)components/backends/vllm/src/dynamo/vllm/args.py(1 hunks)components/frontend/src/dynamo/frontend/main.py(6 hunks)lib/llm/src/discovery/watcher.rs(4 hunks)lib/llm/src/entrypoint/input/common.rs(1 hunks)lib/llm/src/entrypoint/input/http.rs(5 hunks)lib/llm/src/lib.rs(1 hunks)lib/llm/src/namespace.rs(1 hunks)lib/llm/tests/http_namespace_integration.rs(1 hunks)lib/llm/tests/namespace.rs(1 hunks)
💤 Files with no reviewable changes (2)
- components/backends/vllm/README.md
- components/backends/vllm/deploy/README.md
🧰 Additional context used
🧬 Code Graph Analysis (5)
lib/llm/src/lib.rs (5)
lib/runtime/src/distributed.rs (1)
namespace(185-187)lib/runtime/src/component.rs (2)
namespace(218-220)namespace(602-609)lib/bindings/python/rust/lib.rs (1)
namespace(286-291)lib/bindings/python/src/dynamo/_core.pyi (1)
namespace(37-41)lib/llm/src/kv_router/publisher.rs (1)
namespace(988-989)
components/frontend/src/dynamo/frontend/main.py (1)
lib/runtime/src/component/client.rs (1)
is_static(129-131)
lib/llm/tests/http_namespace_integration.rs (1)
lib/llm/src/namespace.rs (1)
is_global_namespace(7-9)
lib/llm/src/discovery/watcher.rs (1)
lib/llm/src/namespace.rs (1)
is_global_namespace(7-9)
lib/llm/src/entrypoint/input/http.rs (3)
lib/llm/src/namespace.rs (1)
is_global_namespace(7-9)lib/llm/src/http/service/service_v2.rs (2)
etcd_client(111-113)new(71-83)lib/llm/src/discovery/watcher.rs (1)
new(60-74)
🔇 Additional comments (17)
components/frontend/src/dynamo/frontend/main.py (3)
22-22: LGTM on logging import.Importing logging at module scope matches the new startup logs.
206-215: Dynamic endpoint construction is consistent with watcher semantics.Using f"{namespace}.frontend.http" and falling back to f"{GLOBAL_NAMESPACE}.frontend.http" aligns with is_global_namespace behavior on the Rust side.
229-241: Startup logs are useful and well-scoped.The logs clearly indicate static vs. dynamic discovery and the namespace scope. This will help triage namespace issues in multi-tenant deployments.
lib/llm/src/lib.rs (1)
30-30: Exposenamespacemodule public API.The new
pub mod namespace;is correctly placed and expands the crate’s public surface for shared namespace semantics.lib/llm/src/entrypoint/input/common.rs (1)
78-81: It looks like only thecommon.rsentrypoint is spawning a watcher with global scope today. The otherDynamicbranches inhttp.rsandendpoint.rsdo not callwatch(...), so they aren’t affected by the namespace scoping change.
- We only need to update
lib/llm/src/entrypoint/input/common.rs.- The proposed diff is safe: it captures
DYN_NAMESPACEonce, converts it toOption<&str>, and passes that intowatch(...).Unless you intend this watcher to remain unscoped (e.g. strictly for dev/debug), go ahead and merge. If you do need a global-only watcher there, please call out that exception explicitly in a comment or confirm here.
lib/llm/src/namespace.rs (1)
1-9: LGTM: clear, minimal shared namespace semantics.The constant and helper match the intended global discovery behavior and are easy to consume across modules.
lib/llm/src/discovery/watcher.rs (2)
113-125: Namespace-aware filtering on Put events looks correctFiltering before persisting entries avoids cross-namespace bleed-through. The debug log with model/namespace context is helpful for observability.
91-95: No compatibility issue with Option::is_none_orThe project’s rust-toolchain is set to Rust 1.87.0, and
Option::is_none_orwas stabilized in Rust 1.82.0, so it’s fully supported. No changes are required here—please disregard the suggestion to replace it withmap_or.Likely an incorrect or invalid review comment.
lib/llm/src/entrypoint/input/http.rs (3)
42-48: Deriving target_namespace from the local model’s namespace is soundUsing is_global_namespace to decide whether to scope discovery is consistent with the watcher’s semantics and the tests. Good.
229-267: Endpoint enable/disable logic aligns with ModelUpdate semanticsMapping Backend to both Chat and Completion and deferring others to as_endpoint_type keeps behavior consistent as backends front multiple routes. Looks good.
48-57: No namespace segment in etcd keys — skip narrowing the watched prefixAfter inspecting how model entries are written, their etcd keys are generated with
ModelNetworkName::new()asformat!("{MODEL_ROOT_PATH}/{}", uuid::Uuid::new_v4())i.e.
“models/”
The namespace lives only inside the JSON value, not in the key path. The watcher already filters by namespace in code (model_entry.endpoint.namespace != target_ns), so narrowing the etcd prefix to “models/{namespace}” wouldn’t match anything. You can ignore the suggested prefix change.Likely an incorrect or invalid review comment.
lib/llm/tests/namespace.rs (2)
11-47: Solid coverage of global namespace semanticsGood assertions around GLOBAL constant, empty string handling, whitespace non-global, and case sensitivity.
102-165: Namespace filtering test mirrors watcher semanticsDeriving a global flag via is_global_namespace and filtering accordingly matches the production code path. This helps prevent regressions.
lib/llm/tests/http_namespace_integration.rs (4)
34-102: Namespace filtering behavior tests are on pointScenarios for specific namespace, global, and empty target are correctly validated against is_global_namespace. This aligns with the watcher’s filtering.
104-153: Endpoint parsing + global classification assertions add good coverageExercising both parse() and classification makes failures actionable if parsing or namespace utilities change.
155-222: Discovery scoping scenarios reflect expected visibilityThe three scenarios cover the expected visible set under specific, global, and non-existent namespaces. Nice.
224-275: Boundary conditions are well-coveredEmpty and “global” treated as global; uppercase “GLOBAL” intentionally not. This matches the current is_global_namespace contract.
biswapanda
force-pushed
the
bis/dyn-837-dynamo-namespace-scoping-for-frontend-component
branch
from
ad534ec to
5b719a0
Compare
biswapanda
force-pushed
the
bis/dyn-838-dynamo-namespace-scoping-for-backend-component
branch
from
9ddc517 to
cfd26d6
Compare
biswapanda
force-pushed
the
bis/dyn-838-dynamo-namespace-scoping-for-backend-component
branch
from
cfd26d6 to
c4200e4
Compare
biswapanda
force-pushed
the
bis/dyn-837-dynamo-namespace-scoping-for-frontend-component
branch
from
843912e to
75882f6
Compare
biswapanda
requested review from
hutm,
julienmancuso,
mohammedabdulwahhab and
atchernych
as code owners
biswapanda
force-pushed
the
bis/dyn-837-dynamo-namespace-scoping-for-frontend-component
branch
3 times, most recently
from
16629b6 to
bca639c
Compare
biswapanda
force-pushed
the
bis/dyn-838-dynamo-namespace-scoping-for-backend-component
branch
from
c4200e4 to
1abfbc0
Compare
Signed-off-by: Biswa Panda <[email protected]>
biswapanda
force-pushed
the
bis/dyn-838-dynamo-namespace-scoping-for-backend-component
branch
from
1abfbc0 to
29c7d05
Compare
biswapanda
merged commit 1212436
into
bis/dyn-837-dynamo-namespace-scoping-for-frontend-component
biswapanda
deleted the
bis/dyn-838-dynamo-namespace-scoping-for-backend-component
branch
Signed-off-by: Biswa Panda <[email protected]>
Signed-off-by: Biswa Panda <[email protected]>
Signed-off-by: Biswa Panda <[email protected]>
Signed-off-by: Biswa Panda <[email protected]>
Signed-off-by: Biswa Panda <[email protected]>
Signed-off-by: Biswa Panda <[email protected]>
Overview:
dynamo namespace scoping for backend components
closes: DYN-838
Supports scoping backend to a specific dynamo namespace based on env variable
DYN_NAMESPACE[NOTE] Merge after related PR for frontend: #2394
Summary by CodeRabbit
New Features
Documentation
Tests