Skip to content

fix(sidecar): honor worker namespace suffix in shared argument parsing - #14906

Closed
tmonty12 wants to merge 2 commits into
mainfrom
tmonty12/dyn-sidecar-namespace-suffix
Closed

tmonty12 wants to merge 2 commits into
mainfrom
tmonty12/dyn-sidecar-namespace-suffix

Conversation

@tmonty12

@tmonty12 tmonty12 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

DYN_NAMESPACE_WORKER_SUFFIX identifies a worker generation during Kubernetes rolling updates. Appending it to the Dynamo namespace isolates worker generations and prevents cross-generation communication.

The Python entrypoint already applies this suffix in the integrated runtime/engine path. Sidecars previously ignored it. This change adds the same behavior to the shared CommonArgs namespace parser used by the vLLM, SGLang, and TensorRT-LLM sidecars:

  • Unset or empty suffix: namespace unchanged.
  • Non-empty suffix: <namespace>-<suffix>.

This applies to environment-provided, default, and explicit --namespace values.

Validation

  • Two unit tests pass, covering unset/empty/populated suffixes, the default namespace, and CLI precedence.
  • Formatting, changed-file pre-commit checks, and git diff --check pass.

Summary by CodeRabbit

  • New Features

    • Namespace configuration now supports an optional worker suffix from the environment, appending it when provided.
  • Bug Fixes

    • Empty or absent suffix values preserve the configured namespace.
    • Explicit namespace settings continue to take precedence as expected.

Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
@github-actions github-actions Bot added the fix label Sep 16, 2026
@tmonty12
tmonty12 marked this pull request as ready for review September 16, 2026 00:51
@tmonty12
tmonty12 requested a review from a team as a code owner September 16, 2026 00:51
@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b0c17ed1-7924-40ce-9145-e49dcacf6b3c

📥 Commits

Reviewing files that changed from the base of the PR and between 395fb8e and 37c0fd1.

📒 Files selected for processing (1)
  • lib/backend-common/src/args.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The namespace argument parser now reads DYN_NAMESPACE_WORKER_SUFFIX. It appends a non-empty suffix to the namespace and preserves the namespace when the suffix is absent or empty. Tests cover environment, default, and CLI values.

Changes

Worker namespace suffix

Layer / File(s) Summary
Parse and validate worker namespace
lib/backend-common/src/args.rs
The namespace parser appends non-empty DYN_NAMESPACE_WORKER_SUFFIX values. Tests verify unchanged namespaces and suffixed namespaces from environment, default, and CLI inputs.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 37c0f

The namespace suffix behavior is scoped to the shared parser and no actionable merge risk was identified.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the change and validation, but it does not follow the required template. It omits the Overview, Details, Where should the reviewer start?, and required Related Issues sections… Add the required template sections. Include reviewer guidance, and complete the Related Issues section by linking the relevant issue or confirming that no related issue exists.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: applying the worker namespace suffix in shared sidecar argument parsing.
Full details: Description check

Explanation

The description explains the change and validation, but it does not follow the required template. It omits the Overview, Details, Where should the reviewer start?, and required Related Issues sections.

  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

Comment thread lib/backend-common/src/args.rs Outdated
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
@tmonty12
tmonty12 enabled auto-merge (squash) September 16, 2026 01:24
long,
default_value = "dynamo",
env = "DYN_NAMESPACE",
value_parser = parse_worker_namespace

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Operator upgrade workloads can supply an already-suffixed DYN_NAMESPACE together with DYN_NAMESPACE_WORKER_SUFFIX (for example, default-qwen-decode-<hash> and <hash> in deploy/operator/internal/controller/upgrade_test.go). Applying this value parser unconditionally turns that into default-qwen-decode-<hash>-<hash>, while the controller's published runtime namespace remains suffixed once. Rust sidecars then register outside the namespace used for the active worker generation. Preserve an already-applied worker suffix, or have the caller provide an unambiguous base namespace before appending.

🤖 AI Fix

Append DYN_NAMESPACE_WORKER_SUFFIX only when the parsed namespace does not already end with the exact hyphen-delimited suffix, and add coverage for an already-suffixed DYN_NAMESPACE.

@tmonty12

Copy link
Copy Markdown
Contributor Author

Already implemented in: #14955

@tmonty12 tmonty12 closed this Sep 16, 2026
auto-merge was automatically disabled September 16, 2026 23:29

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants