Support full duplex streaming by kfswain · Pull Request #450 · kubernetes-sigs/gateway-api-inference-extension

kfswain · 2025-03-05T02:15:48Z

This PR supports the FULL_DUPLEX_STREAMED mode for ext-proc.

Fixes #388

This feature is currently only enabled via an env_var as it is still experimental.

Follow ups:

support full streaming, we are currently buffering the body onto the EPP. We can instead use decoder.Token() to read JSON tokens and stream back the body as it comes in.
Merge both server implementations together
- If not feasible, refactor to share logic between extproc server implementations
Test coverage (this pr woefully has none, we need to fix this ASAP)

k8s-ci-robot · 2025-03-05T02:15:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfswain

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kfswain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2025-03-05T02:18:03Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`7c6c599`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67c8e278285ad400084c6ce5
😎 Deploy Preview	https://deploy-preview-450--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

yanjunxiang-google · 2025-03-05T18:29:58Z

+    #   operation:
+    #     op: replace
+    #     path: "/default_filter_chain/filters/0/typed_config/http_filters/0/typed_config/processing_mode/response_header_mode"
+    #     value: SEND


Should you set the request_header_mode to SEND as well?

It actually already is via:

gateway-api-inference-extension/config/manifests/gateway/extension_policy.yaml

Line 15 in 5b82374

body: Buffered

We just need to populate the request field for it to be included as per: https://gateway.envoyproxy.io/latest/api/extension_types/#extprocprocessingmode

I suppose we could include it here for completeness though. Open to either

But I would like to do away with this specific patch policy stuff and use EnvoyExtensionPolicy for this. I have PRs out to Envoy to support envoyproxy/gateway#5349 & envoyproxy/envoy#38578. I just need to follow up on those and get them unstuck.

yanjunxiang-google · 2025-03-05T18:42:08Z

+		}
+
+		switch v := req.Request.(type) {
+		case *extProcPb.ProcessingRequest_RequestHeaders:


A few questions:

Does the streaming server buffer the entire body, or buffer just a portion of the body, then send the response?

Do you have some integration tests with Envoy <-> StreamingServer to test the end-to-end functionalities?

Currently we are buffering the whole body as it was faster to implement, but a follow up is to stream the response back.

Not yet. I've been testing this all manually on my own cluster. I also have that as a follow up

linux-foundation-easycla · 2025-03-05T20:29:42Z

The committers listed above are authorized under a signed CLA.

✅ login: kfswain / name: Kellen Swain (d94680a, ff5ab61, 4e10f9e, b4e1e14, c1ee3ac, 5d859d9, 01da659, 47f8428, b2c7b4b, a331092, 08925b2, cd14149, a59449d, 0030d5d, 99bcfdc, dba0d27, 7c6c599, 7c9eb76)

kfswain · 2025-03-05T21:40:23Z

EasyCLA started happy hour a little early today: linuxfoundation/easycla#4605

robscott · 2025-03-05T21:58:28Z

/check-cla

ahg-g

Thanks Kellen, mostly asking questions

ahg-g · 2025-03-05T20:50:28Z

        - "9003"
+        env:
+        - name: USE_STREAMING
+          value: "true"


can we use_streaming even if the ext-proc is setup in buffered mode?

I tried it, and it didn't work out of the box. I think because we have to use the streaming response body.

We can probably do some generalizing (i.e. if we recieve a stream in, we can assume stream out)

ok, then we probably want to set this to false then since we are commenting out the streaming part in the patch.

ahg-g · 2025-03-05T21:04:02Z

+type StreamRequestState int
+
+const (
+	RequestReceived                  StreamRequestState = 0


can you comment if those are states that we defined or states part of the ext-proc streaming protocol?

These are states I defined. But based on the protocol we need to follow. I used the states because simple nil checking of the response object we need to send back is not enough. (Imagine the case where we receive a stream chunk, but that chunk doesnt have the model field. So even though the bodyResponse is non-nil, we cant send it yet b/c we havent sent the header yet)

ahg-g · 2025-03-05T21:41:11Z

+		case *extProcPb.ProcessingRequest_RequestBody:
+			loggerVerbose.Info("Incoming body chunk", "body", string(v.RequestBody.Body), "EoS", v.RequestBody.EndOfStream)
+			go func() {
+				_, err := writer.Write(v.RequestBody.Body)


is this were we block to get the whole stream?

Yeah, each stream chunk that comes in is passed to the writer, the writer then blocks until it's read. Once we get an EndofStream we let the decoder use the reader to read them all, and close the reader (which closes the corresponding writer(s)).

For a true stream, we would need to leverage decode.Token() and stream back what we can. Although since we modify the body it may be challenging to not buffer.

ahg-g · 2025-03-05T22:53:58Z

        - "9003"
+        env:
+        - name: USE_STREAMING
+          value: "true"


ok, then we probably want to set this to false then since we are commenting out the streaming part in the patch.

ahg-g · 2025-03-05T22:56:53Z

+			// To buffer the full message, we create a goroutine with a writer.Write()
+			// call, which will block until the corresponding reader reads from it.
+			// We do not read until we receive the EndofStream signal, and then
+			// decode the entire JSON body.


is there a sequence id on the chunks that we need to adhere to when writing to the pipe? is it possible that we receive the chunks out of order?

I think all the web protocol portion is handled before the Process() func is called, as I didn't see any race condition issues related to this with my testing.

Additionally, the only fields exposed in the RequestBody are Body and EndofStream: https://github.com/envoyproxy/go-control-plane/blob/66fc0a3b55b04be5eeb362fb9639a51845218b38/envoy/service/ext_proc/v3/external_processor.pb.go#L605

Request body definition: https://github.com/envoyproxy/go-control-plane/blob/66fc0a3b55b04be5eeb362fb9639a51845218b38/envoy/service/ext_proc/v3/external_processor.pb.go#L255

… request and response

…ementing on both servers

ahg-g · 2025-03-05T23:48:49Z

This looks good to me, but we need to add proper test coverage as a followup. Let me know when ready and I can tag it.

BenTheElder · 2025-03-05T23:51:45Z

/check-cla

BenTheElder · 2025-03-05T23:52:37Z

Maybe linuxfoundation/easycla#4605

ahg-g · 2025-03-06T00:01:10Z

/lgtm

kfswain · 2025-03-06T00:10:47Z

I forced merged this due to EasyCLA having an outage: linuxfoundation/easycla#4605

That was the only blocker holding this PR.
Apologies for any issues/alarms that go off.

jarias-lfx · 2025-03-06T13:47:34Z

/easycla

This PR supports the FULL_DUPLEX_STREAMED mode for ext-proc.

…extension#450) This PR supports the FULL_DUPLEX_STREAMED mode for ext-proc.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2025

k8s-ci-robot requested review from danehans and robscott March 5, 2025 02:15

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 5, 2025

kfswain requested a review from ahg-g March 5, 2025 02:15

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2025

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2025

kfswain force-pushed the duplex-stream branch from cc93b56 to ba70331 Compare March 5, 2025 17:58

yanjunxiang-google reviewed Mar 5, 2025

View reviewed changes

robscott mentioned this pull request Mar 5, 2025

Testing CLA #452

Closed

ahg-g reviewed Mar 5, 2025

View reviewed changes

kfswain force-pushed the duplex-stream branch 2 times, most recently from 4fe978e to f65f43b Compare March 5, 2025 23:45

kfswain added 4 commits March 5, 2025 23:46

manifest modifications to enable duplex streaming

ff5ab61

Initial working hard code of duplex streaming

d94680a

Enabling full duplex streaming for request & response flows.

5d859d9

Cleanup of local dev work

4e10f9e

kfswain added 14 commits March 5, 2025 23:46

Removing WG as it is unneeded, close the reader after decode for both…

b2c7b4b

… request and response

Remove empty body check, simplify code

b4e1e14

build fix after rebase

c1ee3ac

fixing comment scope

a331092

Extracting immediate response error handling logic to a func and impl…

99bcfdc

…ementing on both servers

Simplifying cyclometric complexity

a59449d

Removing dev logs

cd14149

Fixing err handling of writers

01da659

white space change

47f8428

CLA test commit

7c9eb76

Review comments

dba0d27

update extproc config

08925b2

CLA test

0030d5d

verification test

7c6c599

kfswain force-pushed the duplex-stream branch from f65f43b to 7c6c599 Compare March 5, 2025 23:47

kfswain changed the title ~~[WIP] Support full duplex streaming~~ Support full duplex streaming Mar 5, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2025

k8s-ci-robot assigned ahg-g Mar 6, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2025

kfswain merged commit 70965a0 into kubernetes-sigs:main Mar 6, 2025

nirrozenbaum mentioned this pull request Mar 9, 2025

error when following quickstart guide #465

Closed

kfswain deleted the duplex-stream branch March 27, 2025 19:39

kfswain added a commit to kfswain/llm-instance-gateway that referenced this pull request Apr 29, 2025

Support full duplex streaming (kubernetes-sigs#450)

75413d5

This PR supports the FULL_DUPLEX_STREAMED mode for ext-proc.

elevran pushed a commit to llm-d/llm-d-inference-scheduler that referenced this pull request Apr 23, 2026

Support full duplex streaming (kubernetes-sigs/gateway-api-inference-…

d3708ad

…extension#450) This PR supports the FULL_DUPLEX_STREAMED mode for ext-proc.

Conversation

kfswain commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 5, 2025

Uh oh!

netlify Bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla Bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfswain commented Mar 5, 2025

Uh oh!

robscott commented Mar 5, 2025

Uh oh!

ahg-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahg-g commented Mar 5, 2025

Uh oh!

BenTheElder commented Mar 5, 2025

Uh oh!

BenTheElder commented Mar 5, 2025

Uh oh!

ahg-g commented Mar 6, 2025

Uh oh!

kfswain commented Mar 6, 2025

Uh oh!

jarias-lfx commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kfswain commented Mar 5, 2025 •

edited

Loading

netlify Bot commented Mar 5, 2025 •

edited

Loading

linux-foundation-easycla Bot commented Mar 5, 2025 •

edited

Loading