test: add E2E tests for payload processor with Kind cluster CI - #31
Conversation
|
Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
9da84d3 to
1b2b395
Compare
aradhalevy
left a comment
There was a problem hiding this comment.
Looks good, some minor comments (and we will need the new added check to run and pass first)
| name: llama-adapters | ||
| namespace: $E2E_NS | ||
| labels: | ||
| inference.llm-d.io/ipp-managed: "true" |
There was a problem hiding this comment.
I believe it supposed to be llm-d.ai instead of llm-d.io as per #28.
After this fix the test pass for me locally
| name: deepseek-adapters | ||
| namespace: $E2E_NS | ||
| labels: | ||
| inference.llm-d.io/ipp-managed: "true" |
There was a problem hiding this comment.
same llm-d.ai instead of llm-d.io
| | Streaming routing | SSE chunks returned | | ||
| | Metrics | `bbr_info`, `bbr_success_total` | | ||
|
|
||
| ## Troubleshooting |
There was a problem hiding this comment.
When I had a couple of other clusters set up in kind, Envoy tried to route requests to them. Please add a suggestion / troubleshooting to use kind delete clusters --all first to clean your kind environment first.
| containers: | ||
| - name: payload-processor | ||
| image: $E2E_IMAGE | ||
| imagePullPolicy: Never |
There was a problem hiding this comment.
This should be IfNotPresent if we want to test on a different cluster other than kind. But that requires pushing an image to ghcr.io and might require some more changes, and can be dealt with in another issue / PR if you prefer to keep this PR for kind only
There was a problem hiding this comment.
Keeping it for now, will address it in a follow-up pr
|
|
||
| - name: Run E2E tests | ||
| run: | | ||
| E2E_IMAGE=ghcr.io/llm-d/llm-d-inference-payload-processor:e2e \ |
There was a problem hiding this comment.
You don't use the Makefile / script here, I think it would be better to use them to have a single source of truth.
1b2b395 to
9de9955
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
aradhalevy
left a comment
There was a problem hiding this comment.
LGTM.
I think this is fine even tough it is a large PR as all the code is needed and relevant to this minimal e2e test suite.
|
|
||
| var ( | ||
| testConfig *testutils.TestConfig | ||
| ppImage string |
There was a problem hiding this comment.
nit: it would be better to align the name on ipp rather than pp (that was the agreed acronym).
| | Base model routing | Pool routing via header | | ||
| | LoRA adapter routing | ConfigMap adapter lookup | | ||
| | Streaming routing | SSE chunks returned | | ||
| | Metrics | `bbr_info`, `bbr_success_total` | |
There was a problem hiding this comment.
as a follow up, we should update all metrics to be named ipp instead of bbr.
not a blocker
| | Streaming routing | SSE chunks returned | | ||
| | Metrics | `bbr_info`, `bbr_success_total` | | ||
|
|
||
| ## Troubleshooting |
There was a problem hiding this comment.
this should probably go to a separate troubleshot guide.
quickstart guide should be quick, and simple :)
in other words, the simplest explanation of the green path.
| @@ -0,0 +1,164 @@ | |||
| # Llama model server simulator | |||
| apiVersion: apps/v1 | |||
There was a problem hiding this comment.
can you explain the separation between e2e-deployment and deepseek-model-server?
I see deepseek has deployment + svc.
here I see deployment + svc for a llama plus adapter of deepseek + llama + many other CRs.
not sure I understand the separation.
| - '!**/*.md' | ||
| - '!LICENSE' | ||
| - '!OWNERS' | ||
|
|
There was a problem hiding this comment.
can you move this logic to the file "ci-pr-checks.yaml" (and on the way to clean from it the lint python and build at the end)?
9de9955 to
3c561d1
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
|
@shmuelk can you please review this PR when you have time? |
|
@nirrozenbaum I took a very quick look at this PR. I don't like it's structure. This E2E test looks a lot more like the old IGW E2E test and not like the scheduler's E2E test. @roytman restructured the End to End test and the development environment on Kind to use the same K8S YAML and config YAML files where possible. Following that idea here will make it easier to put together a development environment on Kind. |
@asaadbalum can you please take a look on @shmuelk's feedback and work towards setting the e2e to work like they do in llm-d scheduler? (or the new name llm-d router). |
3c561d1 to
2a0668c
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
|
cc for another pair of eyes: @noyitz |
2a0668c to
1732c7b
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
|
Your PR is large. Please consider breaking it into multiple PRs. The |
7fbc4bd to
6942517
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
6942517 to
573a25e
Compare
|
@shmuelk Ready for review again, alll of the previous comments addressed and pr rebased. |
| - role: control-plane | ||
| extraPortMappings: | ||
| - containerPort: 30080 | ||
| hostPort: 30080 |
There was a problem hiding this comment.
This port here is hard coded. The environment variable E2E_PORT is useless.
Change this to use E2E_PORT when creating the cluster
| hostPort: 30080 | ||
| protocol: TCP | ||
| - containerPort: 30090 | ||
| hostPort: 30090 |
There was a problem hiding this comment.
This port is also hard coded. The environment variable E2E_METRICS_PORT is useless.
Change this to use E2E_METRICS_PORT when creating the cluster
| ## Architecture | ||
|
|
||
| ```text | ||
| Go test (OpenAI SDK) ──► Envoy NodePort:30080 (ext_proc) ──► Payload Processor |
There was a problem hiding this comment.
The port numbers are the default values...
shmuelk
left a comment
There was a problem hiding this comment.
Looks much better. I left a few comments
573a25e to
0975629
Compare
Addressed |
| nodes: | ||
| - role: control-plane | ||
| extraPortMappings: | ||
| - containerPort: ${E2E_PORT} |
There was a problem hiding this comment.
As you didn't change Envoy's Service definition, this line shouldn't have been changed.
| - containerPort: ${E2E_PORT} | |
| - containerPort: 30080 |
| - containerPort: ${E2E_PORT} | ||
| hostPort: ${E2E_PORT} | ||
| protocol: TCP | ||
| - containerPort: ${E2E_METRICS_PORT} |
There was a problem hiding this comment.
As you didn't change the IPPs Service definition this line shouldn't have been changed.
| - containerPort: ${E2E_METRICS_PORT} | |
| - containerPort: 30090 |
|
Getting close. I left two more comments. Please run the end to end test locally, with E2E_PORT=31080 and E2E_METRICS_PORT=31090 (i.e. non-default values) |
Adds end-to-end tests that deploy a complete stack on a Kind cluster:
Envoy proxy (distroless-v1.33.2, FULL_DUPLEX_STREAMED ext_proc),
Payload Processor, Llama and DeepSeek model-server simulators, and
adapter ConfigMaps.
Kubernetes manifests live under deploy/ following the llm-d-router
pattern: shared components (deploy/components/) and environment-specific
infrastructure (deploy/environments/dev/e2e-infra/). Test code references
these manifests via relative paths with ${VAR} substitution.
Tests use the OpenAI Go SDK to send requests via Kind NodePort, with
net/http for adapter routing and streaming where raw headers are needed.
CI delegates all orchestration to hack/test-e2e.sh via make test-e2e.
Tests cover base-model routing, LoRA adapter resolution, streaming
requests, and ipp_* metrics exposure.
Signed-off-by: Asaad Balum <asaad.balum@gmail.com>
0975629 to
d507609
Compare
Done |
|
/lgtm |
Summary
Add end-to-end tests that deploy a complete Envoy + Payload Processor + model-server-simulator stack on a Kind cluster and validate core functionality through the actual ext_proc gRPC pipeline.
modelfield extraction from/v1/chat/completionsand/v1/completionsbodies routes Llama and DeepSeek requests to the correct pools viaX-Gateway-Base-Model-Nameheader."stream": true) return SSEtext/event-streamchunks through the full Envoy → Payload Processor → model-server pipeline.ipp_infoandipp_success_totalPrometheus metrics are populated after traffic flows.ci-pr-checks.yaml— delegates all orchestration tomake test-e2e/hack/test-e2e.sh.Manifest structure
Kubernetes manifests live under
deploy/following the llm-d-router pattern: shared components (deploy/components/) and environment-specific infrastructure (deploy/environments/dev/e2e-infra/). Test code references these manifests via relative paths with${VAR}substitution, enabling reuse for both E2E tests and local Kind development.The E2E Envoy configuration mirrors production:
distroless-v1.33.2withrequest_body_mode: FULL_DUPLEX_STREAMEDallow_mode_override: true,failure_mode_allow: falserequest_trailer_mode: SEND,response_trailer_mode: SKIPModel server simulators use fake model names (
ipp-test-llama,ipp-test-deepseek) with--mode=echo, matching the llm-d-router's standard E2E pattern. The sim image is pinned tov0.9.0for reproducibility.New files
deploy/components/ipp/deployment.yamlimagePullPolicy: IfNotPresent)deploy/components/ipp/service.yamldeploy/components/ipp/rbac.yamldeploy/components/model-server/llama/deployment.yamldeploy/components/model-server/deepseek/deployment.yamldeploy/environments/dev/e2e-infra/envoy.yamltest/e2e/e2e_suite_test.gotest/e2e/e2e_test.gotest/e2e/README.mdtest/e2e/TROUBLESHOOTING.mdhack/test-e2e.shModified files
.github/workflows/ci-pr-checks.yamle2ejob (callsmake test-e2e)Makefiletest-e2e,image-build-local,image-kindtargets + E2E config varsgo.mod/go.sumgithub.com/onsi/ginkgo/v2,github.com/onsi/gomega,github.com/openai/openai-go+ transitive depsWhat's NOT included
pkg/,internal/,cmd/)Test plan
7 Passed | 0 Failed)