feat: implemented e2e test for router config-update #921
feat: implemented e2e test for router config-update #921volcano-sh-bot merged 7 commits intovolcano-sh:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests to verify that updating the router's configuration via a ConfigMap and restarting the deployment correctly applies the new settings. The tests cover both standard and Gateway API scenarios, validating routing functionality and scheduler plugin metrics after the update. Feedback includes simplifying label selector construction using standard library functions, addressing a potential resource leak with unclosed port-forwards in cleanup blocks, and ensuring ConfigMap updates use fresh objects to avoid concurrency conflicts.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an end-to-end test that validates router ConfigMap updates take effect after restarting the router, shared across the “router” and “gateway-api” e2e suites.
Changes:
- Introduces
TestRouterConfigUpdateSharedto update the router ConfigMap, restart router pods, and verify behavior + metrics. - Adds suite-specific wrapper tests in both router and gateway-api packages to run the shared test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/e2e/router/shared.go | Adds shared e2e test logic for updating router config, restarting pods, and validating metrics. |
| test/e2e/router/gateway-api/e2e_test.go | Adds a Gateway API wrapper test calling the shared test helper. |
| test/e2e/router/e2e_test.go | Adds a non-Gateway API wrapper test calling the shared test helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/cc @LiZhenCheng9527 @YaoZengzeng @hzxuzhonghu @FAUST-BENCHOU e2e are passing! feel free to review this! |
|
/assign @YaoZengzeng |
|
This test should also be added to |
f9d6c19 to
9b90e91
Compare
|
|
||
| // Register cleanup to restore original ConfigMap, restart router, and | ||
| // re-establish port-forward for subsequent tests. | ||
| t.Cleanup(func() { |
There was a problem hiding this comment.
i actually removed port-forward from cleanup to avoid the resource leak. The test now uses its own dynamic port with defer pf.Close(), just like TestHTTPRouteNotSkippedAfterRouterRestart test
| defer pf.Close() | ||
|
|
||
| restartedRouterURL := fmt.Sprintf("http://127.0.0.1:%s/v1/chat/completions", restartedRouterPort) | ||
| restartedMetricsURL := fmt.Sprintf("http://127.0.0.1:%s/metrics", restartedRouterPort) | ||
|
|
||
| // Verify routing works after config update and restart. | ||
| t.Run("VerifyUpdatedConfig", func(t *testing.T) { | ||
| resp := utils.CheckChatCompletionsWithURL(t, restartedRouterURL, modelRoute.Spec.ModelName, messages) | ||
| assert.Equal(t, 200, resp.StatusCode, "Routing should work after config update and restart") | ||
| }) | ||
|
|
||
| // Verify the updated config took effect by checking scheduler plugin metrics. | ||
| // After restart, only the configured score plugins should appear in metrics. |
| preRestartPods := getRouterPods(t, testCtx.KubeClient, kthenaNamespace) | ||
| preRestartPodNames := make(map[string]bool, len(preRestartPods)) | ||
| for _, pod := range preRestartPods { | ||
| preRestartPodNames[pod.Name] = true | ||
| } |
9b90e91 to
f9d6c19
Compare
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
f9d6c19 to
0bd609a
Compare
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
|
/cc @YaoZengzeng @hzxuzhonghu @LiZhenCheng9527 test are passing and everything is fixed now can we merge it now! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YaoZengzeng The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description:
Adds an E2E test that verifies updating the router's ConfigMap and restarting the pods picks up the new configuration.
What the test does:
Fix: #853