Skip to content

doc: add doc best-practice/configuring-rolling-updates#388

Open
diw-zw wants to merge 2 commits into
sgl-project:mainfrom
diw-zw:0710-doc-03
Open

doc: add doc best-practice/configuring-rolling-updates#388
diw-zw wants to merge 2 commits into
sgl-project:mainfrom
diw-zw:0710-doc-03

Conversation

@diw-zw

@diw-zw diw-zw commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@cheyang

cheyang commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

/copilot review

@cheyang

cheyang commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

@gemini-code-assist /review

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@cheyang

cheyang commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Solid documentation PR adding rolling update best-practice guides in both English and Chinese. Each language gets a concept doc (explaining parameters, rationale, and architecture) and a hands-on operations guide (step-by-step kubectl workflows with expected outputs). Content is technically accurate and well-structured.

Two minor nits:

  1. Typo in doc/best-practice/zh/03-configuring-rolling-updates-guide.md: In the CoordinatedPolicy verification output, 50mm should be 50m.
  2. Dangling links: The "Related Documents" sections in both concept docs reference 01-deploy-inference-service.md, 02-using-role-templates.md, and 08-configuring-autoscaling.md which do not exist yet.

Neither issue is blocking. LGTM overall.

@cheyang

cheyang commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

The documentation examples all use lmsysorg/sglang:v0.5.9 (10GB+) but the actual command is just sleep 3600 for control-plane validation. This makes it impractical for users who want to quickly follow along — pulling a multi-GB inference image just to run sleep is heavy.

Could you swap the demo image to something lightweight like busybox:1.36 or nginx:1.25-alpine? The rolling update behavior being demonstrated doesn't depend on the inference runtime, so a small image would make the guide much more accessible for testing.

@cheyang

cheyang commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

One more user-experience note (separate from the image-size one above):

CoordinatedPolicy provenance is unclear. Operation 4 introduces a CoordinatedPolicy CRD but doesn't say whether it comes with the RBG Controller install or requires a separate step. First-time users who follow the earlier docs and then hit this one may get "no matches for kind CoordinatedPolicy" and not know how to recover. A one-line "This CRD is installed together with the RBG Controller via the same Helm chart / kustomize" (or the opposite) would fully unblock this.

Also: the kubectl get pods -w output in Operation 4 shows some Pods entering Error state briefly. That's actually normal — sleep receives SIGTERM and exits non-zero — but the doc doesn't say so, and a user watching this stream will assume something is broken. A short annotation like # Error is normal: sleep exits non-zero on SIGTERM would prevent panic.

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.

2 participants