xDS: add flow control fields to ext_proc protocol and filter config#45509
xDS: add flow control fields to ext_proc protocol and filter config#45509markdroth wants to merge 5 commits into
Conversation
Signed-off-by: Mark D. Roth <roth@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Mark D. Roth <roth@google.com>
| // which governs how much data can be sent. | ||
| // | ||
| // [#not-implemented-hide:] | ||
| message ClientWindowUpdate { |
There was a problem hiding this comment.
Can the values also be zero? I assume one of them could and would be zero at a time incase of updating only one side window. But can both be zero? The HTTP RFC says that 0 size window update is considered protocol error, is it the same here?
There was a problem hiding this comment.
Setting both values to 0 would be a no-op, but I don't see any value in going out of our way to fail if it happens. I think we should be a little flexible here to avoid introducing edge cases in ext_proc server implementations.
|
@markdroth I understand you added this protocol change to address a possible deadlock issue you identified. Do you want to first reproduce that deadlock issue with an integration test? |
|
/assign @yanavlasov |
Co-authored-by: eshitachandwani <59800922+eshitachandwani@users.noreply.github.com> Signed-off-by: Mark D. Roth <roth@google.com>
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
I understand you added this protocol change to address a possible deadlock issue you identified. Do you want to first reproduce that deadlock issue with an integration test?
I don't feel the need to wait for a reproduction case in any particular ext_proc implementation. This is a protocol problem and needs to be addressed, independent of any individual implementation.
| // which governs how much data can be sent. | ||
| // | ||
| // [#not-implemented-hide:] | ||
| message ClientWindowUpdate { |
There was a problem hiding this comment.
Setting both values to 0 would be a no-op, but I don't see any value in going out of our way to fail if it happens. I think we should be a little flexible here to avoid introducing edge cases in ext_proc server implementations.
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
is there any reason why cannot we reuse the HTTP2 self's flow control?
|
@wbpcode Depending only on HTTP/2 flow control can introduce deadlocks. For details, see the description in https://github.com/markdroth/proposal/blob/xds_ext_proc/A93-xds-ext-proc.md#flow-control (from grpc/proposal#484). |
|
Please review, @yanavlasov, @yanjunxiang-google, @tyxia |
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
Sorry for the delay with the review. This all makes sense. Asking some clarifying questions.
/wait-any
| // send a message containing only this field. | ||
| // | ||
| // [#not-implemented-hide:] | ||
| ServerWindowUpdate server_window_update = 13; |
There was a problem hiding this comment.
If I understand correctly, the server can send window_update without needing to send anything in the oneof response above?
There was a problem hiding this comment.
Yes. That's documented on lines 365-368 above.
| // [#not-implemented-hide:] | ||
| message FlowControlInit { | ||
| // Downstream-to-sidestream initial window size. | ||
| int64 initial_window_downstream_to_sidestream = 1; |
There was a problem hiding this comment.
This assumes initial size can be negative. Is it correct?
There was a problem hiding this comment.
Yeah, that's possible.
In practice, I don't think it's useful for the initial window size to be negative, since that has basically the same effect as setting it to zero. However, since this flow control design requires senders to be able to handle negative values in general, I don't see any reason to go out of our way to disallow them for the initial window size.
There was a problem hiding this comment.
Did we want to say anything in the doc about the initial sizes the senders can assume? The spec proposed 64 or 32Kb. This could be useful for implementer to know what peers assume as initial values.
There was a problem hiding this comment.
I added a note about this.
| int64 window_increment_sidestream_to_upstream = 1; | ||
|
|
||
| // Window update for sidestream-to-downstream. | ||
| int64 window_increment_sidestream_to_downstream = 2; |
There was a problem hiding this comment.
Are we allowing negative values? This would mean that sender budget could go negative. If this is allowed, it needs to be clarified in the comments.
There was a problem hiding this comment.
Yes, it's totally valid for a window update to be negative. This is documented on line 135 above.
| // ones are set for a particular HTTP request/response depend on the | ||
| // processing mode. | ||
| oneof request { | ||
| option (validate.required) = true; |
There was a problem hiding this comment.
Removing this might not be backward compatible.
There was a problem hiding this comment.
For example the existing servers may always evaluating a ProcessingRequest contains oneof these message types, and not including one will cause the server break.
There was a problem hiding this comment.
In practice, I don't think it will cause any problems. The initial message from the filter to the ext_proc server will always set one of the fields in this oneof, and for FULL_DUPLEX_STREAMED mode, the filter will enable flow control only if the ext_proc server sends back a window update. If the ext_proc server does send back a window update, then we know it supports flow control, so there won't be a problem; if it doesn't, then we assume it doesn't, in which case we will never send a message that doesn't populate this field.
Also, note that the PGV annotations aren't really that strict anyway:
- The API versioning policy says that it's not safe to increase the strictness of these annotations. Decreasing the strictness (which is what we're doing here) is generally fine.
- As per the xDS specification, the PGV annotations aren't really a formal part of the spec. They're essentially an implementation detail of Envoy when validating config resources. (gRPC does not support PGV, and none of our internal xDS-capable data planes do either.)
- Even Envoy does not actually implement these validations for things other than xDS resources themselves. Since this is part of the ext_proc wire protocol, not part of an xDS resource, I don't think Envoy actually enforces this.
There was a problem hiding this comment.
It will be much easier for security reason that Envoy state machine expecting a finite number of message types:
That way, Envoy can easily deems certain message types are valid, and other types are spurious:
There was a problem hiding this comment.
I don't understand what benefit we get by requiring these fields to be mutually exclusive. Keep in mind that there is overhead involved in each message sent on the stream, and I think there are a lot of cases where it would make sense to combine multiple events into a single message. For example, in the case where the ext_proc server needs to see the body before responding to headers, it would be advantageous to send both headers and body in the same message. The oneof here prevents that from working.
It's not clear to me what benefit we're getting in exchange for that restriction. It seems like the Envoy code could easily handle that: instead of having a switch block, it could just check for each event being present individually.
| // has sent a window update, thus indicating that it supports flow control. | ||
| // | ||
| // [#not-implemented-hide:] | ||
| ClientWindowUpdate client_window_update = 13; |
There was a problem hiding this comment.
I am thinking we should add this ClientWindowUpdate into the oneof so the WINDOW_UPDATE frames can be sent as a standalone message. And we do not need to support piggy-back WINDOW_UPDATE after other types.
There was a problem hiding this comment.
I think it's useful to be able to include a window update in the same message with header or body payload. I suspect that there will be situations where that will make a difference to performance.
Also, in general, oneof can cause all sorts of backward compatibility problems. That's why the xDS style guide explicitly says not to use it these days. I think we should try to avoid putting things in oneofs whenever possible.
There was a problem hiding this comment.
Envoy recently added kStreamedImmediateResponse message type (#42424) into this oneof.
|
|
||
| // The response type that is sent by the server. | ||
| oneof response { | ||
| option (validate.required) = true; |
There was a problem hiding this comment.
See my other replies. I think this is not going to cause any real problems, and I think it's important for our backward-compatibility story for FULL_DUPLEX_STREAMED mode.
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
The change looks good to me. Just one small question about initial window sizes. LGTM otherwise.
/wait-any
| // [#not-implemented-hide:] | ||
| message FlowControlInit { | ||
| // Downstream-to-sidestream initial window size. | ||
| int64 initial_window_downstream_to_sidestream = 1; |
There was a problem hiding this comment.
Did we want to say anything in the doc about the initial sizes the senders can assume? The spec proposed 64 or 32Kb. This could be useful for implementer to know what peers assume as initial values.
| // servers. | ||
| // | ||
| // [#not-implemented-hide:] | ||
| google.protobuf.Duration flow_control_init_timeout = 27; |
There was a problem hiding this comment.
I think we do not need to maintain a separate timer for this to keep it simpler. We can just use existing STREAM_IDLE_TIMEOUT to recover if the server response does not arrive in time.
The logic is this:
If the response does not come back in time,
Envoy continue sends ProcessingRequest carries data until the initial window size exhausted and stop sending any further, and eventually the stream idle timeout kicks in and destroyes the connections.
If the response does come back, then Envoy data flows through.
There was a problem hiding this comment.
If I'm understanding correctly, it seems like you're saying that we would simply fail the request instead of proceeding without flow control? That doesn't seem like a good failure mode.
Consider the case where the ext_proc server needs to see the body before it responds to the headers. Without flow control push-back, that works fine, because the filter can send the full payload, and then the ext_proc server can respond to the headers. However, if the body is larger than the initial window size and the ext_proc server does not support flow control, then the filter will not be able to send the full body, but the ext_proc server will not send the headers response until it sees the full body.
In that case, we want the filter to, after some timeout, assume that the ext_proc server does not support flow control, so that it can continue sending the rest of the body. We don't want the request to just fail.
There was a problem hiding this comment.
Oh, right, sounds good!
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
|
/retest |
7 participants
Commit Message: xDS: add flow control fields to ext_proc protocol and filter config
Additional Description: Add flow control fields to ext_proc protocol and filter config.
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
CC @penguingao @tyxia @yanjunxiang-google @ejona86 @easwars @dfawley