-
Notifications
You must be signed in to change notification settings - Fork 772
feat: dynamo namespace isolation #2394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: dynamo namespace isolation #2394
Conversation
WalkthroughAdds namespace-scoped model discovery. Frontend exposes a --namespace flag and computes endpoint_id accordingly. LLM input HTTP path introduces a namespace-aware watcher using ModelWatcher::watch_namespace_filtered, with logging clarifying global vs namespace discovery. A new watcher method filters events by namespace; existing global discovery remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Discovery
User->>Frontend: Start with optional --namespace
Frontend->>Frontend: Compute endpoint_id (namespace.frontend.http or NS.frontend.http)
Frontend->>Discovery: Initiate discovery (static or dynamic per namespace/global)
sequenceDiagram
participant HTTPEntrypoint
participant Etcd
participant ModelWatcher
HTTPEntrypoint->>HTTPEntrypoint: Check local model namespace
alt Namespace specified and not "NS"
HTTPEntrypoint->>Etcd: Watch prefix (namespace-targeted)
HTTPEntrypoint->>ModelWatcher: watch_namespace_filtered(events, target_namespace)
else Global or "NS"
HTTPEntrypoint->>Etcd: Watch prefix (global)
HTTPEntrypoint->>ModelWatcher: watch(events)
end
ModelWatcher->>ModelWatcher: Handle Put/Delete (filtered when namespaced)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
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 comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/frontend/src/dynamo/frontend/main.py (1)
116-121: Consider adding validation for namespace argumentThe
--namespaceargument accepts any string value without validation. Consider adding a validation function to ensure the namespace follows expected naming conventions (e.g., alphanumeric with underscores, no special characters that might cause issues in etcd keys or endpoint IDs).Example validation function:
def validate_namespace(value): """Validate that namespace follows naming conventions.""" if value and not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', value): raise argparse.ArgumentTypeError( f"namespace must contain only alphanumeric characters and underscores, got: {value}" ) return valueThen update the argument definition:
parser.add_argument( "--namespace", - type=str, + type=validate_namespace, default=None, 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).", )lib/llm/src/discovery/watcher.rs (1)
333-428: Consider refactoring to reduce code duplicationThe
watch_namespace_filteredmethod has significant code duplication with thewatchmethod. Consider refactoring to share common logic while maintaining the namespace filtering capability.One approach would be to have a single internal method that accepts an optional namespace filter:
async fn watch_internal(&self, mut events_rx: Receiver<WatchEvent>, target_namespace: Option<&str>) { let debug_msg = match target_namespace { Some(ns) => format!("model watcher started for namespace: {}", ns), None => "model watcher started".to_string(), }; tracing::debug!("{}", debug_msg); while let Some(event) = events_rx.recv().await { match event { WatchEvent::Put(kv) => { // Parse model entry (common logic) let model_entry = match serde_json::from_slice::<ModelEntry>(kv.value()) { // ... error handling ... }; // Apply namespace filter if specified if let Some(ns) = target_namespace { if model_entry.endpoint.namespace != ns { tracing::trace!( model_namespace = model_entry.endpoint.namespace, target_namespace = ns, model_name = model_entry.name, "Skipping model from different namespace" ); continue; } } // Rest of the common logic... } // ... Delete handling ... } } } pub async fn watch(&self, events_rx: Receiver<WatchEvent>) { self.watch_internal(events_rx, None).await } pub async fn watch_namespace_filtered(&self, events_rx: Receiver<WatchEvent>, target_namespace: &str) { self.watch_internal(events_rx, Some(target_namespace)).await }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/frontend/src/dynamo/frontend/main.py(4 hunks)lib/llm/src/discovery/watcher.rs(1 hunks)lib/llm/src/entrypoint/input/http.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/llm/src/entrypoint/input/http.rs (2)
lib/llm/src/http/service/service_v2.rs (4)
etcd_client(58-60)model_manager(122-124)new(29-35)spawn(126-129)lib/llm/src/discovery/watcher.rs (1)
new(48-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (4)
components/frontend/src/dynamo/frontend/main.py (1)
182-191: LGTM!The endpoint_id construction logic correctly handles both namespace-scoped and global discovery modes, using "NS" as the sentinel value for global discovery.
lib/llm/src/entrypoint/input/http.rs (3)
41-71: LGTM!The namespace filtering logic correctly determines whether to use namespace-specific or global discovery based on the endpoint's namespace. The condition properly handles the "NS" sentinel value and empty strings, with clear logging for both modes.
172-172: Good documentation improvementThe added comment clearly indicates that
run_watcherperforms global discovery across all namespaces.
191-211: Well-implemented namespace watcher functionThe
run_namespace_watcherfunction is properly implemented with clear documentation, appropriate logging, and correct delegation to thewatch_namespace_filteredmethod.
5c94dd5 to
ac61df1
Compare
7888ba2 to
d6288ca
Compare
94f409e to
11fb3ac
Compare
components/backends/sglang/src/dynamo/sglang/utils/clear_namespace.py
Outdated
Show resolved
Hide resolved
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]>
Signed-off-by: Biswa Panda <[email protected]>
3ad1136 to
a16f27b
Compare
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]> Signed-off-by: nnshah1 <[email protected]>
Overview:
dynamo namespace scoping for frontend/backend component
closes: DYN-837
closes: DYN-838
when
DYN_NAMESPACEis not specified, Frontend will discover model backends from all dynamo namespaces.Supports scoping frontend to a specific dynamo_namespace based on one of 2 syntax:
Now operator auto-injects this env var as part of feat: inject DGD id in planner env variables #2460
Summary by CodeRabbit