Skip to content

Conversation

nwangfw
Copy link
Collaborator

@nwangfw nwangfw commented Aug 1, 2025

Pull Request Description

Add regression testing yamls in the following test cases

  1. sglang random router vs slgang cache_aware router vs aibrix-sglang router vs no pd-router
  2. vllm router vs aibrix vllm router
  3. dynamo

Related Issues

Resolves: (#1336)

Important: Before submitting, please complete the description above and review the checklist below.


Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

  • [Bug]: Corrections to existing functionality
  • [CI]: Changes to build process or CI pipeline
  • [Docs]: Updates or additions to documentation
  • [API]: Modifications to aibrix's API or interface
  • [CLI]: Changes or additions to the Command Line Interface
  • [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

  • PR title includes appropriate prefix(es)
  • Changes are clearly explained in the PR description
  • New and existing tests pass successfully
  • Code adheres to project style and best practices
  • Documentation updated to reflect changes (if applicable)
  • Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @nwangfw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've added a significant number of new YAML configurations to establish a robust regression testing suite for the upcoming 0.4 release. These additions focus on various router and disaggregation scenarios across SGLang, VLLM, and Dynamo Cloud environments. The goal is to ensure the stability and performance of different model serving architectures, including those leveraging Aibrix-specific routers and disaggregated prefill/decode roles. I've also made a minor correction and enhancement to an existing SGLang configuration for consistency and improved monitoring.

Highlights

  • New Dynamo Cloud Regression Test Configurations: I've introduced new regression testing YAML configurations for Dynamo Cloud, covering disaggregation setups for LLMs, including both standard and router-enabled deployments. These YAMLs facilitate testing of Qwen3-8B models within a Kubernetes environment, with specific configurations for image and model fetching.
  • Expanded SGLang Regression Test Configurations: I've added a comprehensive suite of SGLang testing YAMLs, exploring various router policies such as random and cache-aware, and different replica configurations (e.g., 1 prefill/1 decode, 2 prefill/2 decode, 4 prefill/3 decode). These configurations support both Qwen3-8B and Qwen3-32B models and include specific Aibrix router setups.
  • Refinement of Existing SGLang Test Configuration: I've updated an existing SGLang YAML (sglang-router-1p1d.yaml) to standardize labels and align the router policy to 'random'. Additionally, I've enabled metrics for the SGLang server in this configuration to provide better observability during testing.
  • SGLang Router RBAC Configuration: I've included a new RBAC YAML for the SGLang router, granting necessary pod-read permissions within Kubernetes. This ensures the router can properly discover and interact with pods in the testing environment.
  • New VLLM Regression Test Configurations: I've introduced new VLLM testing YAMLs, covering various disaggregation setups (1 prefill/1 decode, 2 prefill/2 decode, 4 prefill/3 decode) for Qwen3-8B and Qwen3-32B models. These include configurations for both base VLLM servers and Aibrix router integrations, along with a minimal proxy router setup.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a comprehensive set of YAML files for regression testing of Dynamo, SGLANG, and VLLM router configurations. The changes are extensive and cover various scenarios. My review focuses on improving security, maintainability, and consistency across these new test files. Key findings include critical security vulnerabilities due to placeholder credentials in configuration files, use of insecure patterns like hostPath volumes and binding cluster roles to default service accounts, and several opportunities to improve maintainability by removing code duplication and inconsistencies.

apt update && apt install wget -y
wget https://tos-tools.tos-cn-beijing.volces.com/linux/amd64/tosutil
chmod +x tosutil
./tosutil config -i <YOUR_ACCESS_KEY_ID> -k <YOUR_SECRET_ACCESS_KEY> -e tos-cn-beijing.ivolces.com -re cn-beijing
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

Storing secret placeholders like <YOUR_ACCESS_KEY_ID> and <YOUR_SECRET_ACCESS_KEY> directly in the YAML is a major security risk, as real credentials could be accidentally committed. These should be managed using Kubernetes Secrets and injected into the container as environment variables.

For example, you can define environment variables in your container spec that pull from a secret:

env:
- name: YOUR_ACCESS_KEY_ID
  valueFrom:
    secretKeyRef:
      name: my-tos-secret
      key: accessKeyId
- name: YOUR_SECRET_ACCESS_KEY
  valueFrom:
    secretKeyRef:
      name: my-tos-secret
      key: secretAccessKey

This issue is repeated for the VllmWorker and PrefillWorker services in this file, and in other files in this PR.

              ./tosutil config -i $YOUR_ACCESS_KEY_ID -k $YOUR_SECRET_ACCESS_KEY -e tos-cn-beijing.ivolces.com -re cn-beijing

Comment on lines 4 to 17
labels:
model.aibrix.ai/name: qwen3-8b
model.aibrix.ai/port: "8000"
name: qwen3-8b
namespace: default
spec:
replicas: 1
selector:
matchLabels:
model.aibrix.ai/name: qwen3-8b
template:
metadata:
labels:
model.aibrix.ai/name: qwen3-8b
Copy link
Contributor

Choose a reason for hiding this comment

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

high

There's an inconsistency in the model name. The metadata labels and name refer to qwen3-8b, but the container arguments specify --model-path /models/Qwen3-32B and --served-model-name qwen3-32b. This is confusing and could lead to incorrect test execution. Please ensure the metadata, selector, and template labels are consistent with the model being served.

  labels:
    model.aibrix.ai/name: qwen3-32b
    model.aibrix.ai/port: "8000"
  name: qwen3-32b
  namespace: default
spec:
  replicas: 1
  selector:
    matchLabels:
      model.aibrix.ai/name: qwen3-32b
  template:
    metadata:
      labels:
        model.aibrix.ai/name: qwen3-32b

Comment on lines +16 to +27
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: pod-read-binding
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: pod-read
subjects:
- kind: ServiceAccount
name: default
namespace: default
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Binding a ClusterRole to the default service account in the default namespace is a security risk. It grants permissions to any pod in that namespace that doesn't specify a serviceAccountName. It's better to create a dedicated service account for the router pods and bind the role to that specific service account to follow the principle of least privilege.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: pod-read-binding
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: pod-read
subjects:
  - kind: ServiceAccount
    name: sglang-router-sa # A dedicated service account
    namespace: default

Comment on lines +104 to +110
# apt update && apt install wget -y
# wget https://tos-tools.tos-cn-beijing.volces.com/linux/amd64/tosutil
# chmod +x tosutil
# ./tosutil config -i <YOUR_ACCESS_KEY_ID> -k <YOUR_SECRET_ACCESS_KEY> -e tos-cn-beijing.ivolces.com -re cn-beijing
# ./tosutil cp tos://aibrix-artifact-testing/models/Qwen3-8B ./models -r -p 8 -j 32

# echo "model downloaded, start serving"
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This block of code is commented out. If it's not needed, it should be removed to improve readability and reduce clutter.

# Dynamo Installation Instruction


We follow the instruction in [dynamo](https://github.com/ai-dynamo/dynamo) to deploy the Dynamo Cloud in Kubernetes. The detailed instrunction can be found in Section 1. `1. Installing Dynamo Cloud from Published Artifacts` from dynamo's [quickstart guide](https://github.com/ai-dynamo/dynamo/blob/main/docs/guides/dynamo_deploy/quickstart.md). We use the most recent release images (version: 0.3.2) published by Dynamo team.
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There's a typo in "instrunction". It should be "instruction".

Suggested change
We follow the instruction in [dynamo](https://github.com/ai-dynamo/dynamo) to deploy the Dynamo Cloud in Kubernetes. The detailed instrunction can be found in Section 1. `1. Installing Dynamo Cloud from Published Artifacts` from dynamo's [quickstart guide](https://github.com/ai-dynamo/dynamo/blob/main/docs/guides/dynamo_deploy/quickstart.md). We use the most recent release images (version: 0.3.2) published by Dynamo team.
We follow the instruction in [dynamo](https://github.com/ai-dynamo/dynamo) to deploy the Dynamo Cloud in Kubernetes. The detailed instruction can be found in Section 1. `1. Installing Dynamo Cloud from Published Artifacts` from dynamo's [quickstart guide](https://github.com/ai-dynamo/dynamo/blob/main/docs/guides/dynamo_deploy/quickstart.md). We use the most recent release images (version: 0.3.2) published by Dynamo team.

Comment on lines +91 to +92
nodeSelector:
kubernetes.io/hostname: 192.168.0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The nodeSelector is hardcoded to a specific node hostname (192.168.0.6). This makes the deployment brittle and not portable. For better flexibility, consider using node labels for selection or removing the selector if not strictly necessary for this test. This is also present in the decode role.

apiVersion: orchestration.aibrix.ai/v1alpha1
kind: StormService
metadata:
name: aibirx-vllm-1p1d
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There seems to be a typo in the service name: "aibirx" should probably be "aibrix". This typo is also present in the selector and template labels.

  name: aibrix-vllm-1p1d

apiVersion: orchestration.aibrix.ai/v1alpha1
kind: StormService
metadata:
name: aibirx-vllm-2p2d-tp2
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There seems to be a typo in the service name: "aibirx" should probably be "aibrix". This typo is also present in the selector and template labels.

  name: aibrix-vllm-2p2d-tp2

apiVersion: orchestration.aibrix.ai/v1alpha1
kind: StormService
metadata:
name: aibirx-vllm-4p3d
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There seems to be a typo in the service name: "aibirx" should probably be "aibrix". This typo is also present in the selector and template labels.

  name: aibrix-vllm-4p3d

Comment on lines +7 to +12
- name: disagg-proxy
image: kvcache-container-image-hb2-cn-beijing.cr.volces.com/aibrix/vllm-openai:v0.9.2-cu128-nixl-v0.4.1-lmcache-0.3.1.post1
command: ["sh", "-c"]
args:
- |
sleep 6000
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The pod uses a large vllm-openai server image just to run sleep 6000. This is inefficient. For a placeholder or utility pod like this, a much smaller base image such as busybox or alpine should be used to save resources and reduce pull times.

    - name: disagg-proxy
      image: busybox:1.36
      command: ["sh", "-c"]
      args:
        - |
          sleep 6000

@Jeffwan
Copy link
Collaborator

Jeffwan commented Aug 1, 2025

em. seems there're some files out of expectation. let's hold the PR for a while. Please focus on testing and let's revisit this later

Signed-off-by: Ning Wang <[email protected]>
@nwangfw nwangfw force-pushed the ning/release-benchmarking branch from 9904495 to bc1d830 Compare August 1, 2025 04:03
Signed-off-by: Ning Wang <[email protected]>
@nwangfw nwangfw force-pushed the ning/release-benchmarking branch from bc1d830 to 2065f3c Compare August 1, 2025 04:04
Signed-off-by: Ning Wang <[email protected]>
@nwangfw nwangfw force-pushed the ning/release-benchmarking branch from d144570 to 7e27762 Compare August 1, 2025 18:55
@nwangfw nwangfw force-pushed the ning/release-benchmarking branch from 1b8ba9b to 6600b29 Compare August 2, 2025 04:19
@Jeffwan
Copy link
Collaborator

Jeffwan commented Aug 2, 2025

we discuss not to check in all yamls. we can close this one at this moment.

@Jeffwan Jeffwan closed this Aug 2, 2025
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