Repository navigation
test(e2e): add tokenizer service tests for sidecar and standalone modes - #1932
SoumaCloud wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
This is a lightweight replacement for the renderer subprocess used specifically in E2E tests.
The tokenizer service launches its renderer through VLLM_RENDER_COMMAND. Using the actual vLLM renderer in these tests would introduce unnecessary runtime dependencies and potentially require model downloads.
This stub exposes the HTTP endpoints expected by the tokenizer service (/health, /tokenize, and /stats), allowing us to test the real tokenizer service lifecycle and request handling without running an actual inference engine.
If there is an existing renderer mock that provides the same interface, I'm happy to reuse it. I added this because I couldn't identify an existing implementation that provides this specific renderer subprocess contract.
There was a problem hiding this comment.
i dont think we need to introduce new image
There was a problem hiding this comment.
this will see as the original image did'nt had those endpoint i made that let me check if i can clean that up
There was a problem hiding this comment.
I checked the existing mocks in the repository. The external-provider-mock doesn't expose a /tokenize endpoint, and the existing /tokenize mock in tokenization_test.go is an in-process httptest.Server, so it cannot be deployed as a Kubernetes backend.
This E2E test needs a deployable mock inference engine that exposes POST /tokenize and provides a request counter to verify that the router actually uses engine fallback. That's why I introduced the separate mock image. I've kept its Dockerfile under test/e2e/ since it's only used for testing.
| .PHONY: test-e2e-tokenizer | ||
| test-e2e-tokenizer: ## Run tokenizer service e2e tests. | ||
| @command -v kind >/dev/null 2>&1 || { echo "Kind is not installed."; exit 1; } | ||
| @IMG_TOKENIZER=${IMG_TOKENIZER} IMG_TOKENIZER_ENGINE_MOCK=${IMG_TOKENIZER_ENGINE_MOCK} TOKENIZER_DOCKERFILE=e2e TEST_CATEGORY=tokenizer ./test/e2e/setup.sh | ||
| @IMG_TOKENIZER=${IMG_TOKENIZER} IMG_TOKENIZER_ENGINE_MOCK=${IMG_TOKENIZER_ENGINE_MOCK} KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=20m ./test/e2e/router/tokenizer/... |
There was a problem hiding this comment.
This target is not added to the category matrix in .github/workflows/e2e-tests.yml, and test/e2e/README.md (category table, CI table, image notes) is not updated, so none of these tests run in CI. They will only be run by hand and are likely to rot. #1911 also asks for the new category to be documented.
Could you add tokenizer to the workflow matrix and the README? When doing so, please check the total runtime against the job's timeout-minutes: 50: the suite does two extra full Helm uninstall/install cycles and about eight router or tokenizer rollouts on top of image builds.
| RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o kthena-tokenizer-stub cmd/kthena-tokenizer-stub/main.go | ||
|
|
||
| # The tokenizer service delegates tokenization and chat-template rendering to | ||
| # `vllm launch render` subprocesses, so the runtime image is the CPU-only vLLM | ||
| # image (no GPU required) plus the Go control-plane binary. | ||
| FROM vllm/vllm-openai-cpu:latest | ||
|
|
||
| COPY --from=builder /workspace/kthena-tokenizer /usr/local/bin/kthena-tokenizer | ||
| COPY --from=builder /workspace/kthena-tokenizer-stub /usr/local/bin/kthena-tokenizer-stub |
There was a problem hiding this comment.
This puts a test-only binary into the release kthena-tokenizer image. The stub returns [1,2,3] for any prompt, so if VLLM_RENDER_COMMAND ever points at it in a real deployment, kvcache-aware silently hashes every prompt to the same blocks instead of failing. make test-e2e-tokenizer always sets TOKENIZER_DOCKERFILE=e2e, so the E2E suite doesn't need this change.
Suggest:
- Revert this file.
- Move the stub out of
cmd/, which holds the shipped binaries, to something liketest/e2e/tokenizer-renderer-stub/, matchingtest/e2e/external-provider-mock/. - Build it only in the E2E image.
The non-e2e branch in setup.sh can then be dropped too, since every test hard-codes the stub path and can't pass with the production image anyway.
| fmt.Println("Reinstalling kthena in sidecar mode for sidecar tests...") | ||
| if err := framework.UninstallKthena(kthenaNamespace); err != nil { | ||
| t.Fatalf("uninstall before sidecar test: %v", err) | ||
| } | ||
| if err := framework.InstallKthena(sidecarConfig); err != nil { | ||
| t.Fatalf("install kthena in sidecar mode: %v", err) | ||
| } | ||
| t.Cleanup(func() { | ||
| // Restore standalone mode so subsequent tests in this run are unaffected. | ||
| fmt.Println("Restoring kthena to standalone tokenizer mode...") | ||
| if err := framework.UninstallKthena(kthenaNamespace); err != nil { | ||
| t.Errorf("failed to uninstall kthena during sidecar cleanup: %v", err) | ||
| } | ||
| standaloneCfg := framework.NewDefaultConfig() | ||
| standaloneCfg.TokenizerEnabled = true | ||
| standaloneCfg.TokenizerMode = "standalone" | ||
| standaloneCfg.TokenizerRenderCommand = "/usr/local/bin/kthena-tokenizer-stub" | ||
| standaloneCfg.TokenizerModels = map[string]string{ | ||
| "tokenizer-test-model": "stub-source", | ||
| } | ||
| if err := framework.InstallKthena(standaloneCfg); err != nil { | ||
| t.Errorf("failed to restore standalone tokenizer mode after sidecar test: %v", err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Uninstalling and reinstalling the whole chart in the middle of the package makes the other tests depend on file order. tokenize_test.go has to RefreshPortForward because sidecar_test.go sorts before it. If the reinstall in t.Cleanup fails, every later test fails for an unrelated reason. And with -run, the tests see a different install depending on which ones are selected.
Every other E2E package installs once in TestMain and never reinstalls (router/, gateway-api/, gateway-inference-extension/). Could the two modes be split into two packages, e.g. tokenizer/standalone and tokenizer/sidecar, each with its own TestMain and InstallKthena config? That also removes the port-forward refresh workaround.
| // WaitForTokenizerModelReady polls the tokenizer pod logs until it logs that the model is ready. | ||
| func WaitForTokenizerModelReady(t *testing.T, kubeClient kubernetes.Interface, namespace, podName, model string, timeout time.Duration) { | ||
| t.Helper() | ||
| WaitForPodLogsContain(t, kubeClient, namespace, podName, | ||
| timeout, // use the full timeout as the "since" window | ||
| []string{fmt.Sprintf("Tokenizer for model %s ready at", model)}, | ||
| timeout, defaultPollingInterval, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This uses timeout (5m) as the SinceSeconds window, but Tokenizer for model ... ready at is logged only once, when the renderer starts during TestMain.
TestTokenizerModelLoadLog, readyz_ready and TestTokenizerModelsEndpoint run after TestRouterKVCacheAwareWithTokenizerService, which does four router rollouts plus webhook waits. Once more than 5 minutes have passed since that line, it falls outside the window and these tests fail even though the model is ready. So the outcome depends on how fast the cluster is.
GET /models already reports status: ready per model, and rendererPortForModel uses it. Could readiness poll that instead, so it doesn't depend on log text and timing?
| assert.Equal(t, []int{1, 2, 3}, resp.Tokens, "stub response should have tokens=[1,2,3]") | ||
|
|
||
| // Determinism: a second call with different prompt text must return the same token IDs. | ||
| _, resp2, err2 := utils.SendTokenizeRequest(t, testCtx.TokenizerBaseURL, | ||
| tokenizercontext.ModelServerName, "A completely different prompt") | ||
| require.NoError(t, err2, "second tokenize request should not error") | ||
| assert.Equal(t, resp.Tokens, resp2.Tokens, "stub must return deterministic tokens across calls") | ||
| } | ||
|
|
||
| // TestTokenizeUnknownModelReturns503 verifies that a model not configured in the tokenizer | ||
| // service causes the service to return HTTP 503. | ||
| func TestTokenizeUnknownModelReturns503(t *testing.T) { | ||
| require.NoError(t, testCtx.RefreshPortForward(), "refresh tokenizer port-forward") | ||
| utils.WaitForTokenizerHealthy(t, testCtx.TokenizerBaseURL, 2*time.Minute) | ||
|
|
||
| status, _, err := utils.SendTokenizeRequest(t, testCtx.TokenizerBaseURL, "no-such-model", "prompt") | ||
| // SendTokenizeRequest returns an error on non-200; check the status code. | ||
| if err == nil { | ||
| t.Fatal("expected a non-200 response for unknown model") | ||
| } | ||
| assert.Equal(t, http.StatusServiceUnavailable, status, | ||
| "tokenizer service should return 503 for unconfigured model") | ||
| } |
There was a problem hiding this comment.
Two things here:
- Duplicate and stub-only checks. The determinism check only asserts the stub's hard-coded output.
TestTokenizeUnknownModelReturns503is the same asTestTokenizerServiceDirectUnknownModelinfallback_test.go. Neither adds coverage of the service, so I'd drop both. - Real renderer path is untested. With the stub, nothing exercises the real
vllm launch rendercontract: CLI args, the/tokenizerequest and response schema, and chat-template rendering. That is the riskiest part of the service and is item 3 of test(e2e): add E2E tests for the tokenizer service (sidecar and standalone modes) #1911 (tokens consistent with the engine). Using the stub in CI is a reasonable trade-off given the size of the vLLM CPU image. But then the PR should sayPart of #1911rather thanFixes #1911, or leave a follow-up for a real-renderer case, so the gap stays tracked.
There was a problem hiding this comment.
🟡 Changes recommended
The new tokenizer tests are missing from existing E2E CI and contain setup and assertion defects that can cause image-pull failures or let crash and fallback checks pass without verifying the intended behavior.
6 open issues
What changed in this PR
Adds end-to-end coverage for Kthena’s tokenizer service in standalone and sidecar modes, including router integration, fallback behavior, and renderer lifecycle checks.
Changes:
- Adds tokenizer health, tokenization, lifecycle, and routing tests.
- Introduces a renderer stub, mock inference engine, and dedicated test images.
- Extends Helm test configuration, Kind setup, and port-forward utilities.
| File | Description |
|---|---|
test/e2e/utils/tokenizer.go |
Adds tokenizer HTTP, pod, and readiness helpers. |
test/e2e/utils/portforward.go |
Adds port allocation without a testing context. |
test/e2e/tokenizer-engine-mock/main.go |
Provides mock inference and tokenization endpoints with request counts. |
test/e2e/setup.sh |
Builds and loads tokenizer test images into Kind. |
test/e2e/router/tokenizer/tokenize_test.go |
Tests stub responses and unknown-model handling. |
test/e2e/router/tokenizer/testdata/ModelServer-tokenizer-test.yaml |
Defines the mock model backend. |
test/e2e/router/tokenizer/testdata/ModelRoute-tokenizer-test.yaml |
Routes requests to the test model. |
test/e2e/router/tokenizer/testdata/LLM-Mock-tokenizer.yaml |
Deploys the mock inference engine. |
test/e2e/router/tokenizer/sidecar_test.go |
Tests sidecar deployment, tokenization, and router integration. |
test/e2e/router/tokenizer/lifecycle_test.go |
Checks model load/unload logs and failed-startup deletion. |
test/e2e/router/tokenizer/health_test.go |
Checks health, readiness, and model listing. |
test/e2e/router/tokenizer/fallback_test.go |
Tests router tokenization paths and engine fallback. |
test/e2e/router/tokenizer/e2e_test.go |
Coordinates suite installation and cleanup. |
test/e2e/router/tokenizer/context/context.go |
Manages shared fixtures, clients, and port-forwarding. |
test/e2e/framework/framework.go |
Adds tokenizer Helm settings and deployment waits. |
Makefile |
Adds tokenizer test and image-build targets. |
docker/Dockerfile.kthena-tokenizer-engine-mock |
Builds the minimal mock engine image. |
docker/Dockerfile.kthena-tokenizer-e2e |
Builds a lightweight tokenizer image with the renderer stub. |
docker/Dockerfile.kthena-tokenizer |
Includes the renderer stub in the existing tokenizer image. |
cmd/kthena-tokenizer-stub/main.go |
Implements fixed token responses and request statistics. |
Review effort: Balanced
| @KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=10m ./test/e2e/router/gateway-api/... | ||
|
|
||
| .PHONY: test-e2e-tokenizer | ||
| test-e2e-tokenizer: ## Run tokenizer service e2e tests. |
| t.Cleanup(restore) | ||
| // Router just restarted: counters are 0. | ||
|
|
||
| engineBefore := engineMockStats(t) |
| require.Eventually(t, func() bool { | ||
| p, err := testCtx.KubeClient.CoreV1().Pods(kthenaNamespace).Get( | ||
| context.Background(), pod.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| for _, cs := range p.Status.ContainerStatuses { | ||
| if cs.Name == "tokenizer" || cs.Name == "kthena-tokenizer" { | ||
| return cs.RestartCount == restartsBefore && cs.State.Running != nil | ||
| } | ||
| } | ||
| return false | ||
| }, 30*time.Second, 2*time.Second, | ||
| "tokenizer container must remain running with no new restarts after nil-cancel delete (regression #1885)") |
| echo "Loading external provider mock image into Kind cluster" | ||
| kind load docker-image "${EXTERNAL_PROVIDER_MOCK_IMAGE}" --name "${CLUSTER_NAME}" | ||
| ;; | ||
| tokenizer) |
| fi | ||
| echo "Loading tokenizer image into Kind cluster" | ||
| kind load docker-image "${HUB}/kthena-tokenizer:${TAG}" --name "${CLUSTER_NAME}" | ||
| TOKENIZER_ENGINE_MOCK_IMAGE="${TOKENIZER_ENGINE_MOCK_IMAGE:-${HUB}/kthena-tokenizer-engine-mock:${TAG}}" |
| WaitForPodLogsContain(t, kubeClient, namespace, podName, | ||
| timeout, // use the full timeout as the "since" window | ||
| []string{fmt.Sprintf("Tokenizer for model %s ready at", model)}, | ||
| timeout, defaultPollingInterval, | ||
| ) |
|
@YaoZengzeng thanks for the review i will update the code |
Signed-off-by: Soumadip Patra <soum23dip@gmail.com>
Signed-off-by: Soumadip Patra <soum23dip@gmail.com>
Signed-off-by: Soumadip Patra <soum23dip@gmail.com>
601349b to
79f3fbe
Compare
Split the controller-manager suite into two separate go test processes (run + skip '^TestModelCR') so each gets a full 20m budget, preventing TestModelServingRoleRollingUpdateMaxUnavailable from being starved by TestModelCR* long vLLM termination grace periods. Signed-off-by: Soumadip Patra <soum23dip@gmail.com>
|
hi @YaoZengzeng 1. Tokenizer E2E tests These files directly implement the requested standalone and sidecar test coverage:
2. Test infrastructure and CI support
3. Additional infrastructure fix — Port-forwarder reliability While implementing the tests, I found a separate problem in The existing implementation did not reconnect after pod restarts, had an unreliable shutdown lifecycle that could cause goroutine leaks, and had a race during local port allocation. The tokenizer tests require long-lived port-forwards, including across router pod restarts. Without fixing this, the tests could fail because of the port-forwarding infrastructure rather than tokenizer functionality. I therefore rewrote Regarding the additional scope: I recognise that the port-forwarder rewrite is a separate infrastructure improvement and increases the size of this PR. I included it because it was necessary for reliable execution of the tokenizer E2E tests. However, if you prefer to keep this PR focused strictly on tokenizer E2E coverage, I am happy to split the port-forwarder changes into a separate PR and address that infrastructure issue independently. The intention is to keep the tokenizer tests reliable rather than introduce additional test coverage that depends on an unreliable existing utility. |
|
@YaoZengzeng you can review it |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
This PR adds end-to-end test coverage for the Kthena Tokenizer Service in both standalone and sidecar deployment modes.
It introduces the required test infrastructure, mock components, fixtures, and utilities to validate tokenizer service lifecycle, health endpoints, tokenization requests, and integration with the Kthena router.
The tests cover:
This improves confidence in tokenizer service behavior and helps prevent regressions across deployment modes.
Which issue(s) this PR fixes:
Fixes #1911
Bug evidence (required for bug-related PRs):
N/A — This is a test coverage and infrastructure enhancement, not a bug fix.
Special notes for your reviewer:
git commit -s).Does this PR introduce a user-facing change?:
No.