-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: gateway timeouts #78
base: main
Are you sure you want to change the base?
Feature: gateway timeouts #78
Conversation
Signed-off-by: Nick Beenham <[email protected]>
Signed-off-by: Nick Beenham <[email protected]>
we plan to do in the future, but are out of scope of this design, list | ||
them here. Provide a brief explanation on why this is a non-goal. | ||
--> | ||
None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it out of scope to provide more httpproxy punch-through options? Is this pattern extensible to other routing configuration users may want to provide in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add note to doc to limit scope to requirements
team. Do not provide the design details in this, section - there is a | ||
dedicated section for that later in the document. | ||
--> | ||
The purpose of this feature is to allow the user to configure a timeout on the gateway for an application. This will allow the user to specify how long the gateway should wait for a response from the application before timing out. This will be useful in scenarios where the application may take a long time to respond, or where the user wants to ensure that the application responds within a certain time frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the main difference between health/readiness probes and this? Is this for an endpoint that would take a long time to process something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my explicit use case is for LLM responses
destination: 'http://${ui.name}:3000' | ||
} | ||
{ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax is kind of confusing. if I'm reading it right, this timeoutPolicy
is applied to the /api
route. why is timeoutPolicy
not present at the top level along with path
and destination
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a typo that I will fix
team. Do not provide the design details in this, section - there is a | ||
dedicated section for that later in the document. | ||
--> | ||
The purpose of this feature is to allow the user to configure a timeout on the gateway for an application. This will allow the user to specify how long the gateway should wait for a response from the application before timing out. This will be useful in scenarios where the application may take a long time to respond, or where the user wants to ensure that the application responds within a certain time frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the timeouts configurable per route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
} | ||
{ | ||
{ | ||
timeoutPolicy: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way (or a need) to apply global timeout policies for all routes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary for this PR but worth some further investigation
**idle:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. | ||
**idleConnection:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. | ||
|
||
This maps to the contour timeoutPolicy - https://github.com/projectcontour/contour/blob/main/apis/projectcontour/v1/httpproxy.go#L1126 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to consider how this design will evolve if we support other ingress controllers options in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 as removing Contour as a hard dependency is on the roadmap: radius-project/roadmap#58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need refactoring, will check with k8s gateway API for similarities
**idle:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. | ||
**idleConnection:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Definitions for both idle and idleConnection are same..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update and correct definitions
The solution would update the specification for the gateway resource in the bicep file to include a timeoutPolicy object. This object would contain the following properties: | ||
**response:** The amount of time the gateway should wait for a response from the application before timing out. | ||
**idle:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. | ||
**idleConnection:** The amount of time the gateway should wait for a response from the application before timing out when the connection is idle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idle and idleConnection seem to have same meaning, should we clarify it better when we write the user docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update
Any thoughts on the future maintenance of the spec with the possible gateway implementation changes? The suggested contract change is tied to Contour specific implementation of the gateway. If there is a plan to switch from Contour specific API to more generic GatewayAPI, then the suggested contact will be hard to migrate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 This is good - agree with checking on the K8s gateway api, as well.
Adding a new comment for timeouts comparison with the gateway api specification. Route Timeouts have 2 optional fields:
Request specifies the maximum duration for a gateway to respond to an HTTP request. If the gateway has not been able to respond before this deadline is met, the gateway MUST return a timeout error. and BackendRequest specifies a timeout for an individual request from the gateway to a backend. This covers the time from when the request first starts being sent from the gateway to when the full response has been received from the backend. Is the consensus to update to match the gateway specification? |
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Can I get a review please @kachawla , @zachcasper @sylvainsf |
Signed-off-by: Nick Beenham <[email protected]>
Signed-off-by: Nick Beenham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great doc!
Signed-off-by: Nick Beenham <[email protected]>
@willtsai is anything else needed here to approve and merge? |
New design note for gateway timeouts