A router-benchmark implement can be integrated - #1285
Conversation
…scenarios - Use ai-dynamo/aiperf for Load Gen and dynamo mocker for Mock Backend - Deploy mocker with kthena resource definitions - Provide s2 scenario: routing latency under different QPS; provide s7 scenario: latency variance on heterogeneous backends; provide s8 scenario: schedule strategy comparison Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive performance benchmarking framework for the Kthena Router based on a sandwich isolation model, utilizing AIPerf as a load generator and Dynamo Mocker as a mock backend. It includes Kubernetes manifests, smoke test scenarios, orchestrator scripts, and unit tests. The review feedback highlights several critical areas for improvement to ensure robustness and cloud compatibility: preventing deadlocks in port-forwarding subprocesses, cleaning up stale port-forwards between A/B runs, resolving LoadBalancer port and hostname resolution issues in cloud environments, wrapping metrics collection in try-except blocks to prevent orchestrator crashes, and handling integer duration inputs robustly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR adds a WIP “router-benchmark” prototype under benchmark/ to A/B test Kthena Router scheduling configs using AIPerf for load generation and a Dynamo mocker backend, plus a GitHub Actions workflow to run the benchmark in CI.
Changes:
- Introduce an A/B test runner (
scripts/ab_test.py+scripts/router_ab_test/) that applies router configs, warms up routes, runs AIPerf, collects Prometheus/pprof artifacts, and emits a JSON report with regression detection. - Add benchmark scenarios and K8s manifests for mock backends and router scheduler ConfigMaps.
- Add a dedicated
Router BenchmarkGitHub Actions workflow and unit tests covering the Python orchestration/reporting utilities.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmark/router-benchmark/TODO.md | Tracks remaining prototype gaps and follow-ups. |
| benchmark/router-benchmark/tests/test_ab_test.py | Unit tests for CLI orchestration, metric comparison, artifact collection, and exit code behavior. |
| benchmark/router-benchmark/scripts/router_ab_test/reporter.py | Builds comparison results, writes JSON reports, and prints summaries. |
| benchmark/router-benchmark/scripts/router_ab_test/orchestrator.py | Coordinates applying configs and running A/B benchmark passes. |
| benchmark/router-benchmark/scripts/router_ab_test/models.py | Scenario and result dataclasses + YAML loading. |
| benchmark/router-benchmark/scripts/router_ab_test/metrics_collector.py | Collects router /metrics and /debug/pprof/* artifacts per run. |
| benchmark/router-benchmark/scripts/router_ab_test/load_generator.py | Maps scenario YAML into aiperf profile CLI arguments and parses output metrics. |
| benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py | Kubectl-based deployment orchestration, readiness checks, and endpoint access (port-forward/LB). |
| benchmark/router-benchmark/scripts/router_ab_test/init.py | Re-exports the benchmark framework modules for easier importing. |
| benchmark/router-benchmark/scripts/ab_test.py | CLI entrypoint wiring together orchestrator and exit-code regression gating. |
| benchmark/router-benchmark/scenarios/smoke-test-s2.yaml | Scenario definition for latency-vs-QPS benchmark. |
| benchmark/router-benchmark/scenarios/smoke-test-s7.yaml | Scenario definition for heterogeneous backend variance benchmark. |
| benchmark/router-benchmark/scenarios/smoke-test-s8.yaml | Scenario definition for routing strategy comparison benchmark. |
| benchmark/router-benchmark/README.md | Documents architecture, module layout, usage, and scenarios. |
| benchmark/router-benchmark/k8s/router-config-random.yaml | Router scheduler ConfigMap for random scoring. |
| benchmark/router-benchmark/k8s/router-config-least-request.yaml | Router scheduler ConfigMap for least-request plugin. |
| benchmark/router-benchmark/k8s/router-config-least-latency.yaml | Router scheduler ConfigMap for least-latency plugin. |
| benchmark/router-benchmark/k8s/modelserver.yaml | ModelServer resource pointing at the mock backend workload. |
| benchmark/router-benchmark/k8s/modelroute.yaml | ModelRoute wiring router traffic to the mock ModelServer. |
| benchmark/router-benchmark/k8s/mocker-deployment.yaml | Dynamo mocker deployment/service for CI/local benchmarking. |
| benchmark/router-benchmark/k8s/mocker-deployment-local-hf.yaml | Variant mocker deployment for offline/local HF cache setups. |
| .github/workflows/benchmark.yml | CI workflow to provision Kind + Kthena, deploy mockers, and run the A/B test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
YaoZengzeng
left a comment
There was a problem hiding this comment.
Thanks for the prototype — the sandwich model (AIPerf → Router → Dynamo Mocker) is a clean way to isolate the router, and the module split (models / kubernetes / load_generator / metrics_collector / orchestrator / reporter) plus unit tests make it easy to review. A few things to address before this leaves WIP/Draft:
- Docs language: The README is written in Chinese. Per AGENTS.md / CONTRIBUTING conventions the repo docs are in English — please translate so all reviewers/maintainers can follow it.
- CI trigger:
benchmark.ymlonly runs on a personalpushtofeat/router-benchmark; thepull_requesttrigger is commented out, so the benchmark won't run for real PRs. Restore it (or gate behind a label) before merge. - Load model semantics: every AIPerf run gets both
--concurrencyand--request-rate. These are two different load-control modes; please confirm precedence and pick one based onschedule.mode. - Housekeeping: temp manifest files leak (
delete=Falsewith no cleanup), a few unconditionaltime.sleep(5), and some dead code (build_router_debug_patch, redundantfrom_yamlcoercion). Inline notes below.
Overall direction looks good; keeping it as Draft until the CI-on-kind failure noted in the description is understood makes sense. Left specific comments inline.
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
7ed56f3 to
861e95f
Compare
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (5)
benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py:507
_start_port_forward()always targetsROUTER_DEPLOYMENTeven whentarget_typeissvc. This works only because the Service and Deployment names currently match; if they ever diverge, port-forwarding the Service will break.
local_endpoint = f"localhost:{local_port}"
print(f" Starting port-forward ({local_endpoint} → {description})")
target = f"{target_type}/{self.ROUTER_DEPLOYMENT}"
proc = subprocess.Popen(
benchmark/router-benchmark/scripts/ab_test.py:122
- If AIPerf fails to write any comparable metrics (e.g.
profile_export_aiperf.jsonmissing),ResultReporter.compare()returns an empty dict andmain()exits 0 (no regression). That can make CI pass despite producing no benchmark signal; treat an empty comparison as a failed run (non-zero exit).
report = orchestrator.run()
comparison = report.get("comparison", {})
if comparison.get("_skipped"):
# At least one run was invalid/framework_error — not a regression
# signal, but the run itself failed the validity gate (issue #1271).
raise SystemExit(2)
has_regression = any(metric.get("regression", False) for metric in comparison.values())
raise SystemExit(1 if has_regression else 0)
benchmark/router-benchmark/scripts/router_ab_test/reporter.py:491
compare_router()treatsavg_a=0.0as missing because it usesif not avg_a, which will incorrectly skip comparisons when a plugin reports a legitimate 0ms average. It should only skip when values areNone(and also guard against divide-by-zero explicitly).
for plugin_key in sorted(set(plugins_a) & set(plugins_b)):
avg_a = plugins_a[plugin_key].get("avg_ms")
avg_b = plugins_b[plugin_key].get("avg_ms")
if not avg_a or avg_b is None:
continue
benchmark/router-benchmark/scripts/router_ab_test/load_generator.py:137
_append_load_mode_args()falls back to concurrency mode for any unrecognizedload.schedule.modevalue (including missing/typos). That can silently run the wrong load model; it’s safer to validate the mode and raise for invalid values.
mode = str(schedule.get("mode", "")).lower()
if mode in {"rate", "constant_rate"}:
cmd.extend(["--request-rate", str(schedule.get("rate", 10))])
self._append_traffic_args(cmd, traffic, benchmark_secs)
else:
cmd.extend(["--concurrency", str(concurrency.get("connections", 10))])
self._append_concurrency_args(cmd, concurrency)
benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py:292
_build_model_crds_docs()usesconfig.profiles[0]to pick the ModelServermodelandinferenceEngine, butBackendsConfigexplicitly allows per-profile overrides. If profiles differ, the CRDs can be created for the wrong model/engine relative to some deployed mocker pods. Consider validating that all profiles share a single model/engine (or extend CRD generation to support multiple).
def _build_model_crds_docs(self, config: BackendsConfig) -> list[dict[str, Any]]:
"""Return ModelServer and ModelRoute dicts for the mocker model."""
model = config.profiles[0].model
engine = self._ENGINE_CRD_MAP.get(config.profiles[0].engine_type, config.profiles[0].engine_type)
|
I think this A/B benchmark for router scheduling can be merged. Here is a demo result "comparison": {
"ttft_avg_ms": {
"config_a": 1122.6853564159999,
"config_b": 1021.061779176,
"delta_pct": 9.05,
"regression": false
},
"latency_avg_ms": {
"config_a": 1129.1931873039998,
"config_b": 1027.4200536239998,
"delta_pct": 9.01,
"regression": false
},
"throughput_rps": {
"config_a": 2.8548999248217695,
"config_b": 2.8597083356111392,
"delta_pct": 0.17,
"regression": false
}
},
"router_comparison": {
"request_success_rate_pct": {
"config_a": 100.0,
"config_b": 100.0,
"delta_pp": 0.0,
"regression": false
},
"request_duration_avg_ms": {
"config_a": 1116.798,
"config_b": 1017.239,
"delta_pct": 8.91,
"regression": false
}
} |
|
/retest |
| │ │ • QPS control│ │ • Routing │ │ • TTFT simulation│ │ | ||
| │ │ • Concurrency│ │ • Conn. pooling │ │ • TPOT simulation│ │ | ||
| │ │ • Arrival │ │ • Load balancing │ │ • KV Cache sim. │ │ | ||
| │ │ distribution │ • Failover │ │ • Prom. metrics │ │ |
|
Overall, switching the programming language from python to Go would be better (AI could be helpful). The community maintainers are more familiar with Go and Go is more native for operations on Kubernetes objects. It also helps maintain consistency with the most of the project's code, unless it's tightly coupled with other language. |
| include: | ||
| - scenario: smoke-test-s2 | ||
| router-config-a: router-config-random | ||
| router-config-b: router-config-least-latency |
There was a problem hiding this comment.
We should enable all 8 scenarios?
I thought this benchmark tool is loose coupled with router itself, so Go is not primary. Python version is for POC, change to Go version if necessary. |
|
There are still many TODOes around #942
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
benchmark/router-benchmark/scripts/router_ab_test/reporter.py:491
compare_router()skips plugin comparisons whenavg_ais 0.0 because it uses a truthiness check (if not avg_a). That can silently drop data and also conflates 0 with None; the intent here should be to skip only when values are missing (or whenavg_a == 0to avoid division-by-zero).
avg_a = plugins_a[plugin_key].get("avg_ms")
avg_b = plugins_b[plugin_key].get("avg_ms")
if not avg_a or avg_b is None:
continue
benchmark/router-benchmark/scripts/router_ab_test/load_generator.py:137
- Unknown or missing
load.schedule.modecurrently falls into the concurrency branch, which can hide typos/misconfigurations and run a different load model than intended. Explicitly validate supported modes and raise for anything else.
if mode in {"rate", "constant_rate"}:
cmd.extend(["--request-rate", str(schedule.get("rate", 10))])
self._append_traffic_args(cmd, traffic, benchmark_secs)
else:
cmd.extend(["--concurrency", str(concurrency.get("connections", 10))])
benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py:291
_build_model_crds_docs()indexesconfig.profiles[0]without validating the scenario config. If a scenario YAML omitsbackends.profiles(or it's empty), this will raise anIndexErrorwith a confusing stack trace; fail fast with a clear error message.
def _build_model_crds_docs(self, config: BackendsConfig) -> list[dict[str, Any]]:
"""Return ModelServer and ModelRoute dicts for the mocker model."""
model = config.profiles[0].model
engine = self._ENGINE_CRD_MAP.get(config.profiles[0].engine_type, config.profiles[0].engine_type)
| - name: Install AIPerf | ||
| run: | | ||
| pip install 'aiperf>=0.9,<0.11' | ||
|
|
Signed-off-by: Leo Xie <stleox@qq.com>
6b78de8 to
53ddfae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (3)
benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py:507
- _start_port_forward() always port-forwards to ROUTER_DEPLOYMENT, even when target_type is the default "svc". That means get_router_endpoint() will run
kubectl port-forward svc/kthena-router ...only if ROUTER_DEPLOYMENT happens to match the Service name; otherwise it will port-forward the wrong resource and can fail in CI.
local_endpoint = f"localhost:{local_port}"
print(f" Starting port-forward ({local_endpoint} → {description})")
target = f"{target_type}/{self.ROUTER_DEPLOYMENT}"
proc = subprocess.Popen(
benchmark/router-benchmark/scripts/router_ab_test/kubernetes.py:291
- _build_model_crds_docs() derives the ModelServer/ModelRoute model + inferenceEngine from config.profiles[0]. That can diverge from ScenarioConfig.backends.default_model (used by AIPerfRunner and wait_for_router_ready), and it also makes heterogeneous profile configs behave inconsistently. Prefer using BackendsConfig defaults for the CRDs (or validate all profiles match) so the route being probed matches the route being registered.
def _build_model_crds_docs(self, config: BackendsConfig) -> list[dict[str, Any]]:
"""Return ModelServer and ModelRoute dicts for the mocker model."""
model = config.profiles[0].model
engine = self._ENGINE_CRD_MAP.get(config.profiles[0].engine_type, config.profiles[0].engine_type)
benchmark/router-benchmark/scripts/router_ab_test/load_generator.py:137
- _append_load_mode_args() treats any schedule.mode other than {rate, constant_rate} as concurrency mode. This can silently misconfigure a scenario when mode is missing or misspelled, producing very different load than intended. It would be safer to explicitly handle "concurrency" and raise on unknown modes.
mode = str(schedule.get("mode", "")).lower()
if mode in {"rate", "constant_rate"}:
cmd.extend(["--request-rate", str(schedule.get("rate", 10))])
self._append_traffic_args(cmd, traffic, benchmark_secs)
else:
/kind feature
Summary
This prototype response to proposal #1284
Result
Successed
I can run tests on a well-resourced cluster (3 nodes, 64G memory).
Failed
While in Github CI, using kind setup cluster, benchmark failed.
Also aiperf warns server restarting.